Don't include excluded items in ItemBlocks #8572
Conversation
sseago
force-pushed
the
exclude-pvs-from-backup
branch
from
acb6f9f to
defd3a4
Compare
|
I haven't tested this locally yet. I need to rebuild my cluster tomorrow and then I'll test. |
|
@sseago |
sseago
force-pushed
the
exclude-pvs-from-backup
branch
from
defd3a4 to
533e8fb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8572 +/- ##
=======================================
Coverage 59.17% 59.17%
=======================================
Files 370 370
Lines 39568 39608 +40
=======================================
+ Hits 23416 23440 +24
- Misses 14673 14684 +11
- Partials 1479 1484 +5 ☔ View full report in Codecov by Sentry. |
|
@Lyndon-Li I was hoping to test the PR in a cluster environment today, but I ended up having to fix some problems with the PR (updated now) as well as rebuild my aws cluster. My plan is to test the PR on Monday, in the meantime it should be ready to review (but I want to do the testing on Monday before merging anything -- I want to make sure the change actually fixes the problem reported -- with PVC labeled for exclude but no labels on PV. |
Sure, we will start to review it, and will not merge it until you fully test it. |
| } | ||
| // Don't add to ItemBlock if item is excluded | ||
| // itemInclusionChecks logs the reason | ||
| if !b.itemBackupper.itemInclusionChecks(log, false, metadata, &unstructured, item.groupResource) { |
There was a problem hiding this comment.
To me, it seems this change should already work.
It seems no need to add the same check in the pkg/backup/backup.go.
There was a problem hiding this comment.
@blackpiglet When we add related items to the ItemBlock, we first attempt to find it in the list from the item collector -- in that case we already have the item content and don't need to make an APIServer call. That's the code path that adds to the ItemBlock here, so we need the inclusion check to skip excluded items.
In cases where the item is not already in the list of items from the item collector (PVs returned from PVC plugin when includeClusterResources==nil), then we have to pull the item via the APIServer -- that's the code path that makes the check in pkg/backup/backup.go
Note that this itemInclusionChecks call happens within itemblock.addKubernetesResource -- and whenever addKubernetesResource is called from backup.go, there's a continue call afterwards (line 613) , and the second itemInclusionChecks call is after this continue, so this check won't be performed twice.
sseago
force-pushed
the
exclude-pvs-from-backup
branch
from
533e8fb to
f41927d
Compare
Signed-off-by: Scott Seago <sseago@redhat.com>
sseago
force-pushed
the
exclude-pvs-from-backup
branch
from
f41927d to
4b09b63
Compare
|
@Lyndon-Li PR updated with feedback from @kaovilai and I was able to test it in an aws cluster environment today. Labeling the PVC but not the PV with the exclude label now properly excludes both PVC and PV from backup -- without this PR, I reproduced the bug found by the issue reporter. |
Thank you for contributing to Velero!
Please add a summary of your change
Run itemExclusionChecks before adding items to ItemBlocks. Fixes a regression in 1.15 where excluding PVCs from backup no longer excludes associated PVs.
Does your change fix a particular issue?
Fixes #8510
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main.