Fix populators not repopulating pvc after it was deleted #3056

ShellyKa13 · 2024-01-09T14:04:16Z

What this PR does / why we need it:
This PR should fix this bug:
Jira-ticket: https://issues.redhat.com/browse/CNV-35085

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 #CNV-35085

Special notes for your reviewer:

Release note:

None

akalenyu · 2024-01-10T08:55:04Z

/retest
failures not related to PR

akalenyu · 2024-01-10T09:16:45Z

tests/cloner_test.go

@@ -138,6 +138,33 @@ var _ = Describe("all clone tests", func() {
 			Expect(uploader.DeletionTimestamp).To(BeNil())
 		})

+		It("should recreate and reclone target pvc if it was deleted", func() {


So this test will actually run Serially since it's under Describe("[rfe_id:1277][crit:high][vendor:cnv-qe@redhat.com][level:component]Cloner Test Suite", Serial, func()
Can we maybe move it around so it can run parallel, or is there a constraint I am missing?

yes sure thanks for noticing, I guess it was introduced recently

arnongilboa · 2024-01-10T13:16:46Z

tests/import_test.go

+		By("Verify PVC prime was created")
+		pvcPrime, err = utils.WaitForPVC(f.K8sClient, pvc.Namespace, populators.PVCPrimeName(pvc))
+		Expect(err).ToNot(HaveOccurred())


Just a dumb question (I guess this is done in many tests), can't PVC prime already be deleted in this point if import was quick enough to complete? and why are you verifying it's creation in this flow? I guess PVC bound and verifying the content in the end are more than enough?

yeah I agree, I did several changes in this test, since I found the deletePVCByUID then its safe enough to see that the new created PVC became bound again.. (I had a race there with the get). Ill simplify the test more.

Signed-off-by: Shelly Kagan <skagan@redhat.com>

In case someone deleted the pvc a new pvc is created and the status of the dv is still succeded. when using populators in order for the population to restart and the dv state to change accordingly we need to retrigger the population, this depends on the existing of the source CR. That CR should exist as long as the pvc pod state is not succeeded. When the pvc is recreated the succeded pod state annotation was removed. Adjusted UT accordingly Signed-off-by: Shelly Kagan <skagan@redhat.com>

Move cloner test to a parallel test suite. Simplify import test. Signed-off-by: Shelly Kagan <skagan@redhat.com>

alromeros · 2024-01-11T18:25:07Z

Looks really good. Maybe add an upload functional test?

akalenyu · 2024-01-14T13:19:59Z

Looks really good. Maybe add an upload functional test?

Random question. Did we not ever have tests that check the PVC recreated & repopulated upon deletion?
Do those exist, and for some reason not use populators?

ShellyKa13 · 2024-01-14T14:28:57Z

@akalenyu hmm from what I saw I didnt find any..
@alromeros the pipeline of upload populator is like the import populator, so I thought that UT for both and functest for import is enough..

akalenyu · 2024-01-14T19:59:45Z

hmm from what I saw I didnt find any..

There's

containerized-data-importer/tests/datavolume_test.go

Line 3023 in 4188a86

    
           It("[test_id:4962]Should create a new PVC when PVC is deleted during import", func() {

ShellyKa13 · 2024-01-15T10:49:13Z

hmm from what I saw I didnt find any..

There's

containerized-data-importer/tests/datavolume_test.go

Line 3023 in 4188a86

It("[test_id:4962]Should create a new PVC when PVC is deleted during import", func() {

Yeah this is different since we delete the pvc during the import so the dv haven't reached succeeded state yet thatss why we didnt catch the bug

ShellyKa13 · 2024-01-15T14:11:18Z

@akalenyu @alromeros @arnongilboa can you review again?

alromeros · 2024-01-15T16:54:01Z

/lgtm

akalenyu · 2024-01-15T17:03:48Z

cc @mhenriks
I am fine with the test changes I was suggesting

mhenriks · 2024-01-16T20:24:17Z

/approve

kubevirt-bot · 2024-01-16T20:24:25Z

[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:

~~OWNERS~~ [mhenriks]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ShellyKa13 · 2024-01-17T10:06:28Z

/cherrypick release-v1.57

kubevirt-bot · 2024-01-17T10:07:18Z

@ShellyKa13: #3056 failed to apply on top of branch "release-v1.57":

Applying: Add tests to verify pvc is recreated and repopulated after delete
Using index info to reconstruct a base tree...
M	tests/cloner_test.go
M	tests/import_test.go
M	tests/utils/datavolume.go
M	tests/utils/pvc.go
Falling back to patching base and 3-way merge...
Auto-merging tests/utils/pvc.go
Auto-merging tests/utils/datavolume.go
Auto-merging tests/import_test.go
CONFLICT (content): Merge conflict in tests/import_test.go
Auto-merging tests/cloner_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add tests to verify pvc is recreated and repopulated after delete
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-v1.57

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/test-infra repository.

jpeimer · 2024-01-17T11:36:28Z

Needs a backport to 1.58 as well

ShellyKa13 · 2024-01-17T11:43:27Z

/cherrypick release-v1.58

kubevirt-bot · 2024-01-17T11:44:06Z

@ShellyKa13: new pull request created: #3071

In response to this:

/cherrypick release-v1.58

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/test-infra repository.

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Jan 9, 2024

kubevirt-bot requested review from arnongilboa and awels January 9, 2024 14:05

ShellyKa13 force-pushed the fix-populators-not-repopulating-pvc branch from fd6becd to 108ba33 Compare January 9, 2024 14:25

akalenyu reviewed Jan 10, 2024

View reviewed changes

arnongilboa reviewed Jan 10, 2024

View reviewed changes

ShellyKa13 added 3 commits January 11, 2024 12:37

Add tests to verify pvc is recreated and repopulated after delete

f322cb6

Signed-off-by: Shelly Kagan <skagan@redhat.com>

Adjust tests after review

4188a86

Move cloner test to a parallel test suite. Simplify import test. Signed-off-by: Shelly Kagan <skagan@redhat.com>

ShellyKa13 force-pushed the fix-populators-not-repopulating-pvc branch from 108ba33 to 4188a86 Compare January 11, 2024 10:42

kubevirt-bot assigned alromeros Jan 15, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2024

kubevirt-bot merged commit 40a382d into kubevirt:main Jan 17, 2024
18 checks passed

kubevirt-bot mentioned this pull request Jan 17, 2024

[release-v1.58] Fix populators not repopulating pvc after it was deleted #3071

Merged

ShellyKa13 mentioned this pull request Jan 17, 2024

[release-v1.57] Fix populators not repopulating pvc #3073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix populators not repopulating pvc after it was deleted #3056

Fix populators not repopulating pvc after it was deleted #3056

ShellyKa13 commented Jan 9, 2024

akalenyu commented Jan 10, 2024

akalenyu Jan 10, 2024

ShellyKa13 Jan 11, 2024 •

edited

Loading

arnongilboa Jan 10, 2024 •

edited

Loading

ShellyKa13 Jan 11, 2024

alromeros commented Jan 11, 2024

akalenyu commented Jan 14, 2024 •

edited

Loading

ShellyKa13 commented Jan 14, 2024

akalenyu commented Jan 14, 2024

ShellyKa13 commented Jan 15, 2024

ShellyKa13 commented Jan 15, 2024

alromeros commented Jan 15, 2024

akalenyu commented Jan 15, 2024

mhenriks commented Jan 16, 2024

kubevirt-bot commented Jan 16, 2024

ShellyKa13 commented Jan 17, 2024

kubevirt-bot commented Jan 17, 2024

jpeimer commented Jan 17, 2024

ShellyKa13 commented Jan 17, 2024

kubevirt-bot commented Jan 17, 2024

Fix populators not repopulating pvc after it was deleted #3056

Fix populators not repopulating pvc after it was deleted #3056

Conversation

ShellyKa13 commented Jan 9, 2024

akalenyu commented Jan 10, 2024

akalenyu Jan 10, 2024

Choose a reason for hiding this comment

ShellyKa13 Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

arnongilboa Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

ShellyKa13 Jan 11, 2024

Choose a reason for hiding this comment

alromeros commented Jan 11, 2024

akalenyu commented Jan 14, 2024 • edited Loading

ShellyKa13 commented Jan 14, 2024

akalenyu commented Jan 14, 2024

ShellyKa13 commented Jan 15, 2024

ShellyKa13 commented Jan 15, 2024

alromeros commented Jan 15, 2024

akalenyu commented Jan 15, 2024

mhenriks commented Jan 16, 2024

kubevirt-bot commented Jan 16, 2024

ShellyKa13 commented Jan 17, 2024

kubevirt-bot commented Jan 17, 2024

jpeimer commented Jan 17, 2024

ShellyKa13 commented Jan 17, 2024

kubevirt-bot commented Jan 17, 2024

ShellyKa13 Jan 11, 2024 •

edited

Loading

arnongilboa Jan 10, 2024 •

edited

Loading

akalenyu commented Jan 14, 2024 •

edited

Loading