Improve handling of prePopulated DV #2205

brybacki · 2022-03-25T14:56:21Z

Signed-off-by: Bartosz Rybacki brybacki@redhat.com

What this PR does / why we need it:

Previously, when a DataVolume was Reconciled then CDI created PVC
regardless of prePopulated annotation.

There is a possibility that DataVolume and PVC pair is being
restored or moved from other cluster. When DataVolume is marked
as prePopulated, the CDI should not try to populate it.

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 #

Special notes for your reviewer:

Release note:

Handle prepopulated DV

brybacki · 2022-03-28T09:27:36Z

Both tests fail on:

"reason": "Error",
"message": "Unable to connect to imageio data source: bad status: 403 Forbidden kubevirt.io/containerized-data-importer/pkg/importer.createImageioReader \tpkg/importer/imageio-datasource.go:589 kubevirt.io/containerized-data-importer/pkg/importer.NewImageioDataSource \tpkg/importer/imageio-datasource.go:71 main.newDataSource \tcmd/cdi-importer/importer.go:235 main.handleImport \tcmd/cdi-importer/importer.go:146 main.main \tcmd/cdi-importer/importer.go:116 runtime.main \tGOROOT/src/runtime/proc.go:255 runtime.goexit \tGOROOT/src/runtime/asm_amd64.s:1581"
}

brybacki · 2022-03-28T09:27:47Z

/test pull-containerized-data-importer-e2e-k8s-1.22-hpp

mhenriks · 2022-03-28T14:46:49Z

Can you give an example of the bug/issue addressed here? Is it simply that a "pre populated" DV may exist but the PVC may not yet? What will be the DV status in that case?

brybacki · 2022-03-28T15:48:41Z

the case is as follows (happens when restoring DV + PVC using velero but I suppose ):

DV is created (with the annotation prePopulated), PVC will be created next
CDI checks the DV and starts import ignoring the annotation

I think in such case it should mark DV as Pending and wait for PVC

tests/datavolume_test.go

Previously, when a DataVolume was Reconciled then CDI created PVC regardless of prePopulated annotation. There is a possibility that DataVolume and PVC pair is being restored or moved from other cluster. When DataVolume is marked as prePopulated, the CDI should not try to populate it. Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

brybacki · 2022-03-30T12:16:07Z

/test pull-containerized-data-importer-e2e-k8s-1.22-upg

brybacki · 2022-03-30T14:17:07Z

/test pull-containerized-data-importer-e2e-k8s-1.21-hpp

awels · 2022-03-30T17:56:52Z

/test pull-containerized-data-importer-e2e-k8s-1.21-hpp
/test pull-containerized-data-importer-e2e-k8s-1.22-upg
/test pull-containerized-data-importer-e2e-k8s-1.21-ceph

awels · 2022-03-30T17:57:06Z

/test pull-containerized-data-importer-e2e-k8s-1.22-hpp

brybacki · 2022-03-31T14:35:12Z

lets try If CI is up
/test pull-containerized-data-importer-e2e-k8s-1.22-hpp

brybacki · 2022-03-31T15:47:30Z

/test pull-containerized-data-importer-e2e-k8s-1.21-hpp

brybacki · 2022-03-31T15:47:44Z

/test pull-containerized-data-importer-e2e-k8s-1.22-upg

mhenriks · 2022-04-01T00:56:16Z

pkg/controller/datavolume-controller.go

-			if err := r.setCloneOfOnPvc(pvc); err != nil {
+		switch selectedCloneStrategy {
+		case HostAssistedClone:
+			if err := r.ensureExtendedToken(pvc); err != nil {


I think this PR does a good job of handling the case where a DV has "pre populated" annotation and the PVC does not exist. But it appears we have to better handle the case where the PVC has "populated for".

This bug has more details: https://bugzilla.redhat.com/show_bug.cgi?id=2070366

Basically, the controller sees "populated for", takes ownership of the PVC, but still gets to this ensureExtendedToken call

do you want me to handle the problem with this PR, or create a new PR?

updated the logic, now I need to prepare a test so this can be confirmed,

looking at the tests

brybacki · 2022-04-04T13:04:01Z

I think current version will work but I suppose it can be written with a better style

akalenyu · 2022-04-05T11:04:15Z

/retest

brybacki · 2022-04-05T17:59:06Z

Updated the logic to cover DV and PVC, also added a test that failed on previous logic on host assisted clone and also on csi clone.

There is only one problem left. If I try to write a test without adding a source PVC it fails on :

                Message: "admission webhook \"datavolume-validate.cdi.kubevirt.io\" denied the request:  Source PVC cdi-e2e-tests-dv-func-test-rq7w9/sourcepvcempty not found",

brybacki · 2022-04-06T08:53:13Z

Ok, that is bad. I know what to do.
I'll go back to small change, to only handle annotation on DV in this PR, and I'll open a new PR with the changes to handle pvc correctly.

brybacki · 2022-04-06T09:19:44Z

@mhenriks I have reverted change here to only handle DV with prePopulated annotation. I would like this to be reviewed and merged. The rest of the problem will be done in a separate PR.

WIP PR here #2227. I am still trying to analyze the case and discover edge cases so it might be a mess.

mhenriks · 2022-04-06T15:53:54Z

pkg/controller/datavolume-conditions.go

@@ -122,7 +122,7 @@ func updateReadyCondition(conditions []cdiv1.DataVolumeCondition, status corev1.
 }

 func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.PersistentVolumeClaim, reason string) []cdiv1.DataVolumeCondition {
-	if pvc != nil {
+	if pvc != nil && pvc.Name != "" {


When would a PVC not have a name? PVC is used after client returns notfound?

At the beginning of main reconcile loop we do pvc := &corev1.PersistentVolumeClaim{} to get the PVC.
If it is not found we have an empty PVC with no name.

I can make it null, but I would need to carefully check if all the code is ready for null. This needs to be improved, and I'll try to clean up on the next PR.

Yeah this is pretty ugly, please handle in #2227

mhenriks · 2022-04-06T15:54:17Z

/retest

mhenriks · 2022-04-07T12:04:46Z

/retest

mhenriks · 2022-04-07T12:04:56Z

/lgtm
/approve

kubevirt-bot · 2022-04-07T12:05:13Z

[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

brybacki · 2022-04-07T15:12:50Z

/test pull-containerized-data-importer-e2e-k8s-1.22-hpp-istio

brybacki · 2022-06-14T11:51:02Z

/cherrypick release-v1.43

brybacki · 2022-06-14T11:52:36Z

/cherrypick release-v1.43

kubevirt-bot · 2022-06-14T11:52:42Z

@brybacki: new pull request created: #2322

In response to this:

/cherrypick release-v1.43

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 · 2022-06-14T11:53:31Z

@brybacki: new pull request could not be created: failed to create pull request against kubevirt/containerized-data-importer#release-v1.43 from head kubevirt-bot:cherry-pick-2205-to-release-v1.43: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for kubevirt-bot:cherry-pick-2205-to-release-v1.43."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

In response to this:

/cherrypick release-v1.43

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.

aglitke · 2022-06-17T16:01:00Z

/cherrypick release-v1.49

kubevirt-bot · 2022-06-17T16:10:49Z

@aglitke: #2205 failed to apply on top of branch "release-v1.49":

Applying: Improve handling prePopulated DV
Using index info to reconstruct a base tree...
M	pkg/controller/datavolume-controller.go
M	tests/datavolume_test.go
Falling back to patching base and 3-way merge...
Auto-merging tests/datavolume_test.go
Auto-merging pkg/controller/datavolume-controller.go
CONFLICT (content): Merge conflict in pkg/controller/datavolume-controller.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 Improve handling prePopulated DV
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.49

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.

aglitke · 2022-06-17T17:01:24Z

Oops. This is already on release-v1.49

kubevirt-bot requested a review from maya-r March 25, 2022 14:56

kubevirt-bot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Mar 25, 2022

kubevirt-bot requested a review from mhenriks March 25, 2022 14:56

brybacki changed the title ~~Improve handling prePopulated DV~~ WIP: Improve handling prePopulated DV Mar 25, 2022

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2022

mhenriks reviewed Mar 28, 2022

View reviewed changes

tests/datavolume_test.go Outdated Show resolved Hide resolved

brybacki force-pushed the bugfix-prepopulated branch from 9a17513 to 588e826 Compare March 30, 2022 09:01

brybacki changed the title ~~WIP: Improve handling prePopulated DV~~ Improve handling of prePopulated DV Mar 30, 2022

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 30, 2022

mhenriks reviewed Apr 1, 2022

View reviewed changes

brybacki force-pushed the bugfix-prepopulated branch from 20934bd to 7b4d1e2 Compare April 4, 2022 13:02

brybacki force-pushed the bugfix-prepopulated branch from 7b4d1e2 to a5b2bf4 Compare April 5, 2022 13:50

brybacki force-pushed the bugfix-prepopulated branch from a5b2bf4 to 200b5c5 Compare April 5, 2022 17:59

brybacki force-pushed the bugfix-prepopulated branch from 200b5c5 to 588e826 Compare April 6, 2022 09:12

mhenriks reviewed Apr 6, 2022

View reviewed changes

kubevirt-bot assigned mhenriks Apr 7, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2022

kubevirt-bot merged commit 2f7987d into kubevirt:main Apr 7, 2022

brybacki deleted the bugfix-prepopulated branch May 30, 2022 09:20

brybacki restored the bugfix-prepopulated branch June 14, 2022 11:52

kubevirt-bot mentioned this pull request Jun 14, 2022

[release-v1.43] Improve handling of prePopulated DV #2322

Merged

Improve handling of prePopulated DV #2205

Improve handling of prePopulated DV #2205

Conversation

brybacki commented Mar 25, 2022 • edited Loading

brybacki commented Mar 28, 2022

brybacki commented Mar 28, 2022

mhenriks commented Mar 28, 2022

brybacki commented Mar 28, 2022

brybacki commented Mar 30, 2022

brybacki commented Mar 30, 2022

awels commented Mar 30, 2022

awels commented Mar 30, 2022

brybacki commented Mar 31, 2022

brybacki commented Mar 31, 2022

brybacki commented Mar 31, 2022

mhenriks Apr 1, 2022

Choose a reason for hiding this comment

brybacki Apr 1, 2022

Choose a reason for hiding this comment

brybacki Apr 4, 2022

Choose a reason for hiding this comment

brybacki Apr 4, 2022

Choose a reason for hiding this comment

brybacki commented Apr 4, 2022 • edited Loading

akalenyu commented Apr 5, 2022

brybacki commented Apr 5, 2022

brybacki commented Apr 6, 2022

brybacki commented Apr 6, 2022

mhenriks Apr 6, 2022

Choose a reason for hiding this comment

brybacki Apr 7, 2022

Choose a reason for hiding this comment

mhenriks Apr 7, 2022

Choose a reason for hiding this comment

mhenriks commented Apr 6, 2022

mhenriks commented Apr 7, 2022

mhenriks commented Apr 7, 2022

kubevirt-bot commented Apr 7, 2022

brybacki commented Apr 7, 2022

brybacki commented Jun 14, 2022

brybacki commented Jun 14, 2022

kubevirt-bot commented Jun 14, 2022

kubevirt-bot commented Jun 14, 2022

aglitke commented Jun 17, 2022

kubevirt-bot commented Jun 17, 2022

aglitke commented Jun 17, 2022

brybacki commented Mar 25, 2022 •

edited

Loading

brybacki commented Apr 4, 2022 •

edited

Loading