Ignore VMIs from backup when owner VMs are excluded #273
Conversation
kubevirt-bot
added
the
dco-signoff: yes
This commit updates the backup process for VMIs so, when the VMI is owned by a VM that's excluded from the backup, we also exclude the VMI without triggering an error. Signed-off-by: Alvaro Romero <[email protected]>
alromeros
force-pushed
the
fix-vmi-backup
branch
from
4c4ff49 to
8b5dae0
Compare
| @@ -79,6 +79,15 @@ func (p *VMIBackupItemAction) Execute(item runtime.Unstructured, backup *v1.Back | |||
| return nil, nil, errors.WithStack(err) | |||
| } | |||
|
|
|||
| // There's no point in backing up a VMI when it's owned by a VM excluded from the backup | |||
There was a problem hiding this comment.
why not call this code under "isVMIOwned(vmi)" and then remove the ownership check from shouldExcludeVMI?
i.e put this in replace of the code you deleted
(then you will not need to update the irrelevant UT)
There was a problem hiding this comment.
I changed this to avoid triggering an error (for example, if launcher pod is excluded) if we eventually want to ignore the VMI. I like to have this as a self-contained check before doing anything else.
| @@ -2129,7 +2157,7 @@ var _ = Describe("Resource excludes", func() { | |||
| Expect(err).ToNot(HaveOccurred()) | |||
| }) | |||
|
|
|||
| It("[test_id:10227]Running VM excluded: backup should fail", func() { | |||
| It("[test_id:10227]Running VM excluded: empty backup without failure", func() { | |||
There was a problem hiding this comment.
I think all the tests now are pretty similar and all the other cases UT is enough. I'm in favor of keeping only this test and delete all the rest that you changed.
There was a problem hiding this comment.
I agree, I'd leave maybe two tests and delete the other two. I'm always hesitant to delete func tests though.
There was a problem hiding this comment.
Also, are we ok with deleting tests with test id?
There was a problem hiding this comment.
I think we have too many functional tests in this repo lol
And a lot of them are some edge cases which I dont think really interest anyone.. dont think it matters if it has test_id, just maybe the QE will need to take note that this tests are removed (same as to change the behavior of those you modified)
This commit updates the functional tests to match the new behavior where we ignore the VMI backup if its owner is excluded from the backup. Signed-off-by: Alvaro Romero <[email protected]>
alromeros
force-pushed
the
fix-vmi-backup
branch
from
10e9ad9 to
a2e21d0
Compare
| @@ -79,6 +79,15 @@ func (p *VMIBackupItemAction) Execute(item runtime.Unstructured, backup *v1.Back | |||
| return nil, nil, errors.WithStack(err) | |||
| } | |||
|
|
|||
| // There's no point in backing up a VMI when it's owned by a VM excluded from the backup | |||
|
@mhenriks can you take a look and approve if looks ok to you? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
|
/cherrypick release-v0.7 |
1 similar comment
|
/cherrypick release-v0.7 |
|
@alromeros: new pull request created: #314 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What this PR does / why we need it:
This Pull Request aims to update the backup process for VMIs so that when the VMI is owned by a VM excluded from the backup, we also exclude the VMI without triggering an error.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # https://issues.redhat.com/browse/CNV-45094
Special notes for your reviewer:
Release note: