Enable external trigger in DataImportCron #2711

ido106 · 2023-05-10T10:56:57Z

Signed-off-by: Ido Aharon iaharon@redhat.com

What this PR does / why we need it:
We would like to allow the user to disable the automatic updates in DataImportCron. In this situation, when a new update is released the user will have to trigger the import manually.

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 #2689

Special notes for your reviewer:

Release note:

Enable external trigger import in DataImportCron. When the `schedule` property is an empty string (""), import the latest update.

kubevirt-bot · 2023-05-10T10:57:06Z

Hi @ido106. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

akalenyu · 2023-05-10T11:02:52Z

Note that #2688 is about a different issue related to secrets, so we may not want to close it

arnongilboa · 2023-05-10T11:23:48Z

pkg/apiserver/webhooks/dataimportcron-validate.go

@@ -116,7 +116,7 @@ func (wh *dataImportCronValidatingWebhook) validateDataImportCronSpec(request *a
 		return causes
 	}

-	if _, err := cronexpr.Parse(spec.Schedule); err != nil {
+	if _, err := cronexpr.Parse(spec.Schedule); spec.Schedule != "" && err != nil {


no need to parse if empty schedule

arnongilboa · 2023-05-10T11:27:06Z

pkg/controller/dataimportcron-controller_test.go

@@ -310,6 +310,9 @@ var _ = Describe("All DataImportCron Tests", func() {

 			verifyCronJobContainerImage("old-image")
 			verifyCronJobContainerImage("new-image")
+


why test cronjob if it shouldn't be created? split it to another test anyway

fixed, I wrote another test

arnongilboa · 2023-05-10T11:33:00Z

pkg/controller/dataimportcron-controller.go

@@ -353,7 +357,8 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
 	}

 	// We use the poller returned reconcile.Result for RequeueAfter if needed
-	if isImageStreamSource(dataImportCron) {
+	// skip if we disabled schedule
+	if dataImportCron.Spec.Schedule != "" && isImageStreamSource(dataImportCron) {


first check isImageStreamSource()

arnongilboa · 2023-05-10T11:36:30Z

pkg/controller/dataimportcron-controller.go

@@ -132,9 +132,13 @@ func (r *DataImportCronReconciler) Reconcile(ctx context.Context, req reconcile.
 	if !shouldReconcile || err != nil {
 		return reconcile.Result{}, err
 	}
-	if err := r.initCron(ctx, dataImportCron); err != nil {
-		return reconcile.Result{}, err
+	// do not use the poller otherwise


The commend doesn't add much, so better drop it, or rewrite:
// Cron job and initial job are not initialized if no schedule, allowing external source update trigger

fixed, I changed to the comment you mentioned

awels · 2023-05-11T18:03:52Z

To get rid of the DCO failure please run git commit --amend -s on each commit in the PR the -s will sign the commit

akalenyu · 2023-05-17T07:57:32Z

/ok-to-test

akalenyu

looks good minor comments

akalenyu · 2023-05-17T11:04:25Z

pkg/controller/dataimportcron-controller_test.go

@@ -340,6 +343,12 @@ var _ = Describe("All DataImportCron Tests", func() {
 			Expect(*dv.Spec.Source.Registry.URL).To(Equal(testRegistryURL + "@" + testDigest))
 			Expect(dv.Annotations[cc.AnnImmediateBinding]).To(Equal("true"))

+			if cron.Spec.Schedule == emptySchedule {
+				exists, err := reconciler.cronJobExistsAndUpdated(context.TODO(), cron)


I think for the unit test it makes sense to write the GET request ourselves, in case cronJobExistsAndUpdated ever has an issue. Just adds to the test's integrity IMO

akalenyu · 2023-05-17T11:05:45Z

pkg/controller/dataimportcron-controller_test.go

+				Expect(err).ToNot(HaveOccurred())
+				Expect(exists).To(BeFalse())
+			}
+


We should probably also ensure the batchv1.Job{} doesn't get created

Thanks. fixed both comments

ido106 · 2023-05-17T20:19:51Z

I want to add another part to the test.
I will try to upload the poller independently and take the digest from it

arnongilboa · 2023-05-29T12:01:13Z

pkg/controller/dataimportcron-controller.go

+	if dataImportCron.Spec.Schedule != "" {
+		if err := r.initCron(ctx, dataImportCron); err != nil {
+			return reconcile.Result{}, err
+		}


The check should be inside initCron() as it may do other things except initializing CronJob and initial Job.

Allow disabling DataImportCron schedule and support external trigger Signed-off-by: Ido Aharon <iaharon@redhat.com>

arnongilboa · 2023-05-30T13:16:16Z

/lgtm

awels · 2023-05-30T18:35:02Z

/approve

kubevirt-bot · 2023-05-30T18:35:10Z

[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

Allow disabling DataImportCron schedule and support external trigger Signed-off-by: Ido Aharon <iaharon@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>

kubevirt-bot added the size/S label May 10, 2023

kubevirt-bot requested review from awels and maya-r May 10, 2023 10:57

arnongilboa reviewed May 10, 2023

View reviewed changes

ido106 force-pushed the dataimportcron_external_trigger branch from b0f8445 to 7648891 Compare May 16, 2023 12:47

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. size/S labels May 16, 2023

ido106 force-pushed the dataimportcron_external_trigger branch from 7648891 to c8549f3 Compare May 16, 2023 14:36

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2023

akalenyu reviewed May 17, 2023

View reviewed changes

ido106 force-pushed the dataimportcron_external_trigger branch from c8549f3 to 5ab0ed1 Compare May 17, 2023 19:58

ido106 requested a review from akalenyu May 17, 2023 20:13

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

ido106 force-pushed the dataimportcron_external_trigger branch from 5ab0ed1 to 029bebc Compare May 21, 2023 09:12

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

ido106 force-pushed the dataimportcron_external_trigger branch from 029bebc to babd354 Compare May 29, 2023 11:41

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

arnongilboa reviewed May 29, 2023

View reviewed changes

Enable empty schedule in DataImportCron

e51c0ca

Allow disabling DataImportCron schedule and support external trigger Signed-off-by: Ido Aharon <iaharon@redhat.com>

ido106 force-pushed the dataimportcron_external_trigger branch from babd354 to e51c0ca Compare May 29, 2023 15:33

kubevirt-bot assigned arnongilboa May 30, 2023

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

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

kubevirt-bot merged commit 6205bba into kubevirt:main May 30, 2023

awels pushed a commit to awels/containerized-data-importer that referenced this pull request Jun 21, 2023

Enable empty schedule in DataImportCron (kubevirt#2711)

ae6058a

Allow disabling DataImportCron schedule and support external trigger Signed-off-by: Ido Aharon <iaharon@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable external trigger in DataImportCron #2711

Enable external trigger in DataImportCron #2711

ido106 commented May 10, 2023 •

edited

Loading

kubevirt-bot commented May 10, 2023

akalenyu commented May 10, 2023

arnongilboa May 10, 2023

ido106 May 29, 2023

arnongilboa May 10, 2023

ido106 May 29, 2023 •

edited

Loading

arnongilboa May 10, 2023

ido106 May 29, 2023

arnongilboa May 10, 2023

ido106 May 29, 2023

awels commented May 11, 2023

akalenyu commented May 17, 2023

akalenyu left a comment

akalenyu May 17, 2023

akalenyu May 17, 2023

ido106 May 29, 2023

ido106 commented May 17, 2023

arnongilboa May 29, 2023

ido106 May 29, 2023

arnongilboa commented May 30, 2023

awels commented May 30, 2023

kubevirt-bot commented May 30, 2023

		@@ -310,6 +310,9 @@ var _ = Describe("All DataImportCron Tests", func() {

		verifyCronJobContainerImage("old-image")
		verifyCronJobContainerImage("new-image")

Enable external trigger in DataImportCron #2711

Enable external trigger in DataImportCron #2711

Conversation

ido106 commented May 10, 2023 • edited Loading

kubevirt-bot commented May 10, 2023

akalenyu commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ido106 May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels commented May 11, 2023

akalenyu commented May 17, 2023

akalenyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ido106 commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnongilboa commented May 30, 2023

awels commented May 30, 2023

kubevirt-bot commented May 30, 2023

ido106 commented May 10, 2023 •

edited

Loading

ido106 May 29, 2023 •

edited

Loading