Allow snapshots as format for DataImportCron created sources #2700

akalenyu · 2023-04-25T14:59:29Z

What this PR does / why we need it:
Today DataImportCron maintains up-to-date PVCs that serve as base image sources for cloning,
However, for certain storage types, we know that snapshots provide a better fit.
Snapshots are also immutable which makes them better suited to act as up-to-date OS image sources.

This PR attempts to allow DataImportCrons to manage a snapshot as the source format instead of DataVolume/PVC

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:

Allow snapshots as a format for DataImportCron created sources

kubevirt-bot · 2023-04-25T14:59:32Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

akalenyu · 2023-04-25T14:59:38Z

/test all

akalenyu · 2023-04-25T21:50:54Z

/test all

akalenyu · 2023-04-25T21:57:27Z

/test all

akalenyu · 2023-04-25T22:23:00Z

/test pull-cdi-linter

akalenyu · 2023-04-30T13:35:35Z

/test all

akalenyu · 2023-04-30T13:51:35Z

/test all

arnongilboa · 2023-05-01T12:05:56Z

pkg/controller/dataimportcron-controller.go

+	return snapshot, nil
+}
+
+func (r *DataImportCronReconciler) updateUnderlyingSourceObject(ctx context.Context, cron *cdiv1.DataImportCron, obj client.Object) error {


mmm... clumsy func name. Isn't updateSource() good enough?

yeah I didn't feel so good about this too.. updateSourceObject should be better

akalenyu · 2023-05-03T15:09:16Z

/test all

arnongilboa · 2023-06-04T15:03:56Z

/lgtm

akalenyu · 2023-06-04T18:57:05Z

/retest

awels · 2023-06-05T15:35:40Z

pkg/controller/common/util.go

@@ -1009,6 +1010,21 @@ func CreateStorageClass(name string, annotations map[string]string) *storagev1.S
 	}
 }

+// CreateSnapshotClass creates snapshot class
+func CreateSnapshotClass(name string, annotations map[string]string, snapshotter string) *snapshotv1.VolumeSnapshotClass {


I realize you created this to have a single copy of this function. But it seems odd to have this here just for tests. I don't completely hate it because I can't think of a better location, but it seems odd.

Yeah, it gave me weird chills as well.. reverted, shouldn't be too bad

awels · 2023-06-05T15:48:51Z

pkg/controller/dataimportcron-controller.go

-	if err = r.client.Get(ctx, types.NamespacedName{Namespace: dataImportCron.Namespace, Name: dvName}, pvc); err != nil {
+	dataImportCron.Status.CurrentImports = []cdiv1.ImportStatus{{DataVolumeName: dvName, Digest: digest}}
+
+	sources := []client.Object{&corev1.PersistentVolumeClaim{}, &snapshotv1.VolumeSnapshot{}}


Could you reverse the order here, so that snapshots are checked first. If the snapshot exists, then it should have precedence over the pvc

awels · 2023-06-05T15:52:11Z

pkg/controller/dataimportcron-controller.go

I agree with Arnon here, I had the exact same thought but maybe instead of passing format, pass the function to call into the function?

awels · 2023-06-05T16:09:01Z

pkg/controller/dataimportcron-controller.go

+		snapshot := &snapshotv1.VolumeSnapshot{}
+		err := r.client.Get(ctx, types.NamespacedName{Namespace: dataImportCron.Namespace, Name: pvcName}, snapshot)
+		if err != nil {
+			if !k8serrors.IsNotFound(err) {
+				return err
+			}
+			// Snapshot creation will trigger reconcile
+			return nil
+		}


I am with Arnon on this one. If the status changed between us looking up the value earlier, and updating the status. Then that should trigger a new reconcile since we are watching the snapshotvolumes, and the next reconcile will properly update the condition to what it should be.

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

…tegy and sourceFormat Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

awels

/approve
Please create a follow up PR that replaces all the switch statements with either a map to functions or pass the functions directly into the function as an argument.

awels · 2023-06-07T16:01:38Z

/test pull-containerized-data-importer-e2e-destructive

kubevirt-bot · 2023-06-07T16:02:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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~~ [awels]

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

awels · 2023-06-07T16:15:28Z

/lgtm

akalenyu · 2023-06-07T22:30:58Z

/test pull-cdi-linter

brianmcarey · 2023-06-08T08:06:33Z

/test pull-containerized-data-importer-non-csi-hpp

…t#2700) * StorageProfile API for declaring format of resulting cron disk images Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Integrate recommended format in dataimportcron controller Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Take snapclass existence into consideration when populating cloneStrategy and sourceFormat Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --------- Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Enable empty schedule in DataImportCron (#2711) Allow disabling DataImportCron schedule and support external trigger Signed-off-by: Ido Aharon <iaharon@redhat.com> * expand upon #2721 (#2731) Need to replace requeue bool with requeue duration Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Add clone from snapshot functionalities to clone-populator (#2724) * Add clone from snapshot functionalities to the clone populator Signed-off-by: Alvaro Romero <alromero@redhat.com> * Update clone populator unit tests to cover clone from snapshot capabilities Signed-off-by: Alvaro Romero <alromero@redhat.com> * Fix storage class assignation in temp-source claim for host-assisted clone from snapshot This commit also includes other minor and styling-related fixes Signed-off-by: Alvaro Romero <alromero@redhat.com> --------- Signed-off-by: Alvaro Romero <alromero@redhat.com> * Prepare CDI testing for the upcoming non-CSI lane (#2730) * Update functional tests to skip incompatible default storage classes Signed-off-by: Alvaro Romero <alromero@redhat.com> * Enable the use of non-csi HPP in testing lanes This commit modifies several scripts to allow the usage of classic HPP as the default SC in tests. This allows us to test our non-populator flow with a non-csi provisioner. Signed-off-by: Alvaro Romero <alromero@redhat.com> --------- Signed-off-by: Alvaro Romero <alromero@redhat.com> * Allow snapshots as format for DataImportCron created sources (#2700) * StorageProfile API for declaring format of resulting cron disk images Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Integrate recommended format in dataimportcron controller Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Take snapclass existence into consideration when populating cloneStrategy and sourceFormat Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --------- Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Remove leader election test (#2745) Now that we are using the standard k8s leases from the controller runtime library, there is no need to test our implementation as it is no longer in use. This will save some testing time and random failures. Signed-off-by: Alexander Wels <awels@redhat.com> * Integration of Data volume using CDI populators (#2722) * move cleanup out of dv deletion It seemed off to call cleanup in the prepare function just because we don't call cleanup unless the dv is deleting. Instead we check in the clenup function itself if it should be done: in this 2 specific cases in case of deletion and in case the dv succeeded. The cleanup will be used in future commit also for population cleanup which we also want to happen not only on deletion. Signed-off-by: Shelly Kagan <skagan@redhat.com> * Use populator if csi storage class exists Add new datavolume phase PendingPopulation to indicate wffc when using populators, this new phase will be used in kubevirt in order to know that there is no need for dummy pod to pass wffc phase and that the population will occur once creating the vm. Signed-off-by: Shelly Kagan <skagan@redhat.com> * Update population targetPVC with pvc prime annotations The annotations will be used to update dv that uses the populators. Signed-off-by: Shelly Kagan <skagan@redhat.com> * Adjust UT with new behavior Signed-off-by: Shelly Kagan <skagan@redhat.com> * updates after review Signed-off-by: Shelly Kagan <skagan@redhat.com> * Fix import populator report progress The import pod should be taken from pvcprime Signed-off-by: Shelly Kagan <skagan@redhat.com> * Prevent requeue upload dv when failing to find progress report pod Signed-off-by: Shelly Kagan <skagan@redhat.com> * Remove size inflation in populators The populators are handling existing PVCs. The PVC already has a defined requested size, inflating the PVC' with fsoverhead will only be on the PVC' spec and will not reflect on the target PVC, this seems undesired. Instead if the populators is using by PVC that the datavolume controller created the inflation will happen there if needed. Signed-off-by: Shelly Kagan <skagan@redhat.com> * Adjust functional tests to handle dvs using populators Signed-off-by: Shelly Kagan <skagan@redhat.com> * Fix clone test Signed-off-by: Shelly Kagan <skagan@redhat.com> * add shouldUpdateProgress variable to know if need to update progress Signed-off-by: Shelly Kagan <skagan@redhat.com> * Change update of annotation from denied list to allowed list Instead if checking if the annotation on pvcPrime is not desired go over desired list and if the annotation exists add it. Signed-off-by: Shelly Kagan <skagan@redhat.com> * fix removing annotations from pv when rebinding Signed-off-by: Shelly Kagan <skagan@redhat.com> * More fixes and UT Signed-off-by: Shelly Kagan <skagan@redhat.com> * a bit more updates and UTs Signed-off-by: Shelly Kagan <skagan@redhat.com> --------- Signed-off-by: Shelly Kagan <skagan@redhat.com> * Run bazelisk run //robots/cmd/uploader:uploader -- -workspace /home/prow/go/src/github.com/kubevirt/project-infra/../containerized-data-importer/WORKSPACE -dry-run=false (#2751) Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> * Allow dynamic linked build for non bazel build (#2753) The current script always passes the static ldflag to the compiler which will result in a static binary. We would like to be able to build dynamic libraries instead. cdi-containerimage-server has to be static because we are copying it into the context of a container disk container which is most likely based on a scratch container and has no libraries for us to use. Signed-off-by: Alexander Wels <awels@redhat.com> * Disable DV GC by default (#2754) * Disable DV GC by default DataVolume garbage collection is a nice feature, but unfortunately it violates fundamental principle of Kubernetes. CR should not be auto-deleted when it completes its role (Job with TTLSecondsAfter- Finished is an exception), and once CR was created we can assume it is there until explicitly deleted. In addition, CR should keep idempotency, so the same CR manifest can be applied multiple times, as long as it is a valid update (e.g. DataVolume validation webhook does not allow updating the spec). When GC is enabled, some systems (e.g GitOps / ArgoCD) may require a workaround (DV annotation deleteAfterCompletion = "false") to prevent GC and function correctly. On the next kubevirt-bot Bump kubevirtci PR (with bump-cdi), it will fail on all kubevirtci lanes with tests referring DVs, as the tests IsDataVolumeGC() looks at CDIConfig Spec.DataVolumeTTLSeconds and assumes default is enabled. This should be fixed there. Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Fix test waiting for PVC deletion with UID Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Fix clone test assuming DV was GCed Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Fix DIC controller DV/PVC deletion when snapshot is ready Signed-off-by: Arnon Gilboa <agilboa@redhat.com> --------- Signed-off-by: Arnon Gilboa <agilboa@redhat.com> --------- Signed-off-by: Ido Aharon <iaharon@redhat.com> Signed-off-by: Michael Henriksen <mhenriks@redhat.com> Signed-off-by: Alvaro Romero <alromero@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Signed-off-by: Alexander Wels <awels@redhat.com> Signed-off-by: Shelly Kagan <skagan@redhat.com> Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> Signed-off-by: Arnon Gilboa <agilboa@redhat.com> Co-authored-by: Ido Aharon <iaharon@redhat.com> Co-authored-by: Michael Henriksen <mhenriks@redhat.com> Co-authored-by: alromeros <alromero@redhat.com> Co-authored-by: akalenyu <akalenyu@redhat.com> Co-authored-by: Shelly Kagan <skagan@redhat.com> Co-authored-by: kubevirt-bot <kubevirtbot@redhat.com> Co-authored-by: Arnon Gilboa <agilboa@redhat.com>

regression introduced in kubevirt#2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

regression introduced in #2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

regression introduced in kubevirt#2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

regression introduced in #2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com> Co-authored-by: Arnon Gilboa <agilboa@redhat.com>

kubevirt-bot requested review from aglitke and awels April 25, 2023 14:59

kubevirt-bot added the size/XL label Apr 25, 2023

akalenyu changed the title ~~[WIP] Allow snapshots as a format for DataImportCrons created sources~~ [WIP] Allow snapshots as a format for DataImportCron created sources Apr 25, 2023

akalenyu changed the title ~~[WIP] Allow snapshots as a format for DataImportCron created sources~~ [WIP] Allow snapshots as format for DataImportCron created sources Apr 25, 2023

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023

akalenyu force-pushed the golden-snapshot-cron-integration branch from 6bae9a4 to 9d76efb Compare April 25, 2023 21:50

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023

akalenyu force-pushed the golden-snapshot-cron-integration branch from 9d76efb to d54d428 Compare April 25, 2023 21:56

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2023

akalenyu force-pushed the golden-snapshot-cron-integration branch from d54d428 to 0117011 Compare April 30, 2023 13:18

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2023

akalenyu force-pushed the golden-snapshot-cron-integration branch from 0117011 to a8836e0 Compare April 30, 2023 13:51

arnongilboa reviewed May 1, 2023

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2023

akalenyu force-pushed the golden-snapshot-cron-integration branch from a8836e0 to a783d93 Compare May 3, 2023 15:08

kubevirt-bot added size/XXL and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL labels May 3, 2023

akalenyu force-pushed the golden-snapshot-cron-integration branch from ed3de0a to 2244b83 Compare June 4, 2023 12:47

kubevirt-bot assigned arnongilboa Jun 4, 2023

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

awels reviewed Jun 5, 2023

View reviewed changes

akalenyu force-pushed the golden-snapshot-cron-integration branch from 2244b83 to 657df57 Compare June 5, 2023 17:35

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

akalenyu added 2 commits June 5, 2023 20:37

Integrate recommended format in dataimportcron controller

a36f258

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

Take snapclass existence into consideration when populating cloneStra…

b0e2ff2

…tegy and sourceFormat Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

akalenyu force-pushed the golden-snapshot-cron-integration branch from 657df57 to b0e2ff2 Compare June 5, 2023 17:38

awels reviewed Jun 7, 2023

View reviewed changes

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

kubevirt-bot assigned awels Jun 7, 2023

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

kubevirt-bot merged commit 33c55a5 into kubevirt:main Jun 8, 2023
16 checks passed

mhenriks mentioned this pull request Jun 21, 2023

akalenyu nomination to approver #2766

Merged

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

Fix DIC returned reconcile.Result for ImageStream

d8c4d5f

regression introduced in kubevirt#2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

arnongilboa mentioned this pull request Jul 27, 2023

Fix DIC returned reconcile.Result for ImageStream #2823

Merged

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

Fix DIC returned reconcile.Result for ImageStream

627b1f1

regression introduced in kubevirt#2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

kubevirt-bot pushed a commit that referenced this pull request Jul 28, 2023

Fix DIC returned reconcile.Result for ImageStream (#2823)

96de865

regression introduced in #2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

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

Fix DIC returned reconcile.Result for ImageStream

c57ab57

regression introduced in kubevirt#2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

kubevirt-bot added a commit that referenced this pull request Jul 30, 2023

Fix DIC returned reconcile.Result for ImageStream (#2826)

f506f69

regression introduced in #2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com> Co-authored-by: Arnon Gilboa <agilboa@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow snapshots as format for DataImportCron created sources #2700

Allow snapshots as format for DataImportCron created sources #2700

akalenyu commented Apr 25, 2023 •

edited

Loading

kubevirt-bot commented Apr 25, 2023

akalenyu commented Apr 25, 2023

akalenyu commented Apr 25, 2023

akalenyu commented Apr 25, 2023

akalenyu commented Apr 25, 2023

akalenyu commented Apr 30, 2023

akalenyu commented Apr 30, 2023

arnongilboa May 1, 2023 •

edited

Loading

akalenyu May 1, 2023 •

edited

Loading

akalenyu commented May 3, 2023

arnongilboa commented Jun 4, 2023

akalenyu commented Jun 4, 2023

awels Jun 5, 2023

akalenyu Jun 5, 2023

awels Jun 5, 2023

akalenyu Jun 5, 2023

awels Jun 5, 2023

awels Jun 5, 2023

awels left a comment

awels commented Jun 7, 2023

kubevirt-bot commented Jun 7, 2023

awels commented Jun 7, 2023

akalenyu commented Jun 7, 2023

brianmcarey commented Jun 8, 2023

Allow snapshots as format for DataImportCron created sources #2700

Allow snapshots as format for DataImportCron created sources #2700

Conversation

akalenyu commented Apr 25, 2023 • edited Loading

kubevirt-bot commented Apr 25, 2023

akalenyu commented Apr 25, 2023

akalenyu commented Apr 25, 2023

akalenyu commented Apr 25, 2023

akalenyu commented Apr 25, 2023

akalenyu commented Apr 30, 2023

akalenyu commented Apr 30, 2023

arnongilboa May 1, 2023 • edited Loading

Choose a reason for hiding this comment

akalenyu May 1, 2023 • edited Loading

Choose a reason for hiding this comment

akalenyu commented May 3, 2023

arnongilboa commented Jun 4, 2023

akalenyu commented Jun 4, 2023

awels Jun 5, 2023

Choose a reason for hiding this comment

akalenyu Jun 5, 2023

Choose a reason for hiding this comment

awels Jun 5, 2023

Choose a reason for hiding this comment

akalenyu Jun 5, 2023

Choose a reason for hiding this comment

awels Jun 5, 2023

Choose a reason for hiding this comment

awels Jun 5, 2023

Choose a reason for hiding this comment

awels left a comment

Choose a reason for hiding this comment

awels commented Jun 7, 2023

kubevirt-bot commented Jun 7, 2023

awels commented Jun 7, 2023

akalenyu commented Jun 7, 2023

brianmcarey commented Jun 8, 2023

akalenyu commented Apr 25, 2023 •

edited

Loading

arnongilboa May 1, 2023 •

edited

Loading

akalenyu May 1, 2023 •

edited

Loading