Fix sizeless clones for sourceRef data volumes #2759

akalenyu · 2023-06-18T13:08:55Z

What this PR does / why we need it:
We were hitting a panic because we're passing the original DV object to renderPvcSpec (source is nil)
instead of the mutated DV which has sourceRef converted (source not nil)

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:

BugFix: sizeless clones cause panic for sourceRef data volumes

We were hitting a panic because we're passing the original DV object to renderPvcSpec (source is nil) instead of the mutated DV which has sourceRef converted (source not nil) Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

alromeros · 2023-06-19T10:01:44Z

pkg/controller/datavolume/util.go

@@ -292,7 +292,7 @@ func resolveVolumeSize(c client.Client, dvSpec cdiv1.DataVolumeSpec, pvcSpec *v1

 	if !found {
 		// Storage size can be empty when cloning
-		isClone := dvSpec.Source.PVC != nil
+		isClone := dvSpec.Source.PVC != nil || dvSpec.Source.Snapshot != nil


We don't do regular size-detection when cloning from a snapshot, are we sure this works? For example, what would happen if the target DV size is empty and the snapshot restoreSize is zero?

covered by

containerized-data-importer/pkg/controller/common/util.go

Lines 790 to 792 in 5f85c42

} else if isSizelessClone && !restoreSizeAvailable {

return fmt.Errorf("size not specified by user/provisioner, can't tell how much needed for restore")

}

akalenyu · 2023-06-19T10:46:13Z

/retest

alromeros · 2023-06-19T10:55:14Z

/lgtm

akalenyu · 2023-06-19T11:23:15Z

/cc @mhenriks
I skipped the smart clone controller fix since I know you're removing that,
but I think that once your PR is in the issue should not occur for snapshot pvc clones
(and the skip line can be removed)

mhenriks · 2023-06-21T14:52:41Z

/approve

kubevirt-bot · 2023-06-21T14:52:58Z

[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

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

akalenyu · 2023-06-22T08:23:56Z

/cherrypick release-v1.57

kubevirt-bot · 2023-06-22T08:24:37Z

@akalenyu: new pull request created: #2768

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.

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

* remove CSI clone bye bye Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * no more smart clone Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * PVC clone same namespace Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * cross namespace pvc clone Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * various fixes to get some functional tests to work Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * delete smart clone controller again somehow reappeared after rebase Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * mostly pvc clone functional test fixes make sure size detect pod only runs on kubevirt content type clone populator was skipping last round op applying pvc' annotations various func test fixes review comments Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * more various test fixes host clone phase should (implicitly) wait for clone source pod to exit Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * remove "smart" clone from snapshot Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * DataVolume clone from snapshot uses populator Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * improve clone populator/datavolume coordination on "running" condition For host clone, not much changes, values still comming from annotations on host clone PVC For smart/csi clone the DataVolume will be "running" if not in pending or error phase Will have the same values for terminal "completed" state regardless of clone type Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * unit tests for pvc/snapshot clone controllers Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * remove skipped test added in #2759 Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * attempt address AfterSuite and generate-verify failures Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * handle snapshot clone with no target size specified also add more validation to some snapshot clone tests Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * remove Patch calls Using the controller runtime Patch API with controller runtime cached client seems to be a pretty bad fit At least given the way the CR API is designed where an old object is compared to new. I like patch in theory though and will revisit Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Clone populator should plan and execute even if PVC is bound It was possible to miss "preallocation applied" annotation otherwise Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * add long term token to datavolume Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Rename ProgressReporter to StatusReporter Should have been done back when annotations were addded to "progress" Also, if pvc is bound do not call phase Reconcile functions only Status Signed-off-by: Michael Henriksen <mhenriks@redhat.com> --------- Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

#2783) * remove CSI clone bye bye Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * no more smart clone Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * PVC clone same namespace Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * cross namespace pvc clone Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * various fixes to get some functional tests to work Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * delete smart clone controller again somehow reappeared after rebase Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * mostly pvc clone functional test fixes make sure size detect pod only runs on kubevirt content type clone populator was skipping last round op applying pvc' annotations various func test fixes review comments Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * more various test fixes host clone phase should (implicitly) wait for clone source pod to exit Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * remove "smart" clone from snapshot Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * DataVolume clone from snapshot uses populator Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * improve clone populator/datavolume coordination on "running" condition For host clone, not much changes, values still comming from annotations on host clone PVC For smart/csi clone the DataVolume will be "running" if not in pending or error phase Will have the same values for terminal "completed" state regardless of clone type Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * unit tests for pvc/snapshot clone controllers Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * remove skipped test added in #2759 Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * attempt address AfterSuite and generate-verify failures Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * handle snapshot clone with no target size specified also add more validation to some snapshot clone tests Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * remove Patch calls Using the controller runtime Patch API with controller runtime cached client seems to be a pretty bad fit At least given the way the CR API is designed where an old object is compared to new. I like patch in theory though and will revisit Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Clone populator should plan and execute even if PVC is bound It was possible to miss "preallocation applied" annotation otherwise Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * add long term token to datavolume Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Rename ProgressReporter to StatusReporter Should have been done back when annotations were addded to "progress" Also, if pvc is bound do not call phase Reconcile functions only Status Signed-off-by: Michael Henriksen <mhenriks@redhat.com> --------- Signed-off-by: Michael Henriksen <mhenriks@redhat.com> Co-authored-by: Michael Henriksen <mhenriks@redhat.com>

Fix sizeless clones for sourceRef data volumes

7e9a053

We were hitting a panic because we're passing the original DV object to renderPvcSpec (source is nil) instead of the mutated DV which has sourceRef converted (source not nil) Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 18, 2023

kubevirt-bot requested review from awels and maya-r June 18, 2023 13:08

kubevirt-bot added the size/M label Jun 18, 2023

alromeros reviewed Jun 19, 2023

View reviewed changes

kubevirt-bot assigned alromeros Jun 19, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2023

kubevirt-bot requested a review from mhenriks June 19, 2023 11:23

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2023

kubevirt-bot merged commit 9ea73dc into kubevirt:main Jun 21, 2023
16 checks passed

mhenriks added a commit to mhenriks/containerized-data-importer that referenced this pull request Jun 22, 2023

remove skipped test added in kubevirt#2759

75e1297

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

kubevirt-bot mentioned this pull request Jun 22, 2023

[release-v1.57] Fix sizeless clones for sourceRef data volumes #2768

Merged

mhenriks added a commit to mhenriks/containerized-data-importer that referenced this pull request Jun 23, 2023

remove skipped test added in kubevirt#2759

489b504

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks added a commit to mhenriks/containerized-data-importer that referenced this pull request Jun 27, 2023

remove skipped test added in kubevirt#2759

6e49650

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

kubevirt-bot pushed a commit to kubevirt-bot/containerized-data-importer that referenced this pull request Jun 30, 2023

remove skipped test added in kubevirt#2759

f484cfa

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sizeless clones for sourceRef data volumes #2759

Fix sizeless clones for sourceRef data volumes #2759

akalenyu commented Jun 18, 2023

alromeros Jun 19, 2023

akalenyu Jun 19, 2023

akalenyu commented Jun 19, 2023

alromeros commented Jun 19, 2023

akalenyu commented Jun 19, 2023 •

edited

Loading

mhenriks commented Jun 21, 2023

kubevirt-bot commented Jun 21, 2023

akalenyu commented Jun 22, 2023

kubevirt-bot commented Jun 22, 2023

	} else if isSizelessClone && !restoreSizeAvailable {
	return fmt.Errorf("size not specified by user/provisioner, can't tell how much needed for restore")
	}

Fix sizeless clones for sourceRef data volumes #2759

Fix sizeless clones for sourceRef data volumes #2759

Conversation

akalenyu commented Jun 18, 2023

alromeros Jun 19, 2023

Choose a reason for hiding this comment

akalenyu Jun 19, 2023

Choose a reason for hiding this comment

akalenyu commented Jun 19, 2023

alromeros commented Jun 19, 2023

akalenyu commented Jun 19, 2023 • edited Loading

mhenriks commented Jun 21, 2023

kubevirt-bot commented Jun 21, 2023

akalenyu commented Jun 22, 2023

kubevirt-bot commented Jun 22, 2023

akalenyu commented Jun 19, 2023 •

edited

Loading