Cleanup DataImportCron jobs on deletion #2088

arnongilboa · 2022-01-10T13:49:59Z

Signed-off-by: Arnon Gilboa agilboa@redhat.com

What this PR does / why we need it:
Mostly for the case the job pod is stuck Pending (e.g. due to failed MountVolume.SetUp) and not terminated even when deleting its DataImportCron.
Warning FailedMount 24s (x8 over 87s) kubelet MountVolume.SetUp failed for volume "cdi-cert-vol" : configmap "no-certs" not found

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:

NONE

mhenriks · 2022-01-10T14:40:42Z

Functional test?

awels · 2022-01-10T17:51:52Z

pkg/controller/dataimportcron-controller.go

@@ -471,6 +470,34 @@ func (r *DataImportCronReconciler) cleanup(ctx context.Context, dataImportCron *
 	return nil
 }

+func (r *DataImportCronReconciler) deleteJobs(ctx context.Context, dataImportCron *cdiv1.DataImportCron) error {
+	cronJobName := GetCronJobName(dataImportCron)


Does this not work?

cronJobName := GetCronJobName(dataImportCron) deletePropagationBackground := metav1.DeletePropagationBackground cronJob := &v1beta1.CronJob{ObjectMeta: metav1.ObjectMeta{Namespace: r.cdiNamespace, Name: cronJobName}} if err := r.client.Delete(ctx, cronJob, &client.DeleteOptions{PropagationPolicy: &deletePropagationBackground}); IgnoreNotFound(err) != nil { return err }

Right, this will delete the current cronjob job, but not the initial job which is not a cronjob one. However, initial job should be deleted by name, so no need to list the jobs. Fixed. Thanks @awels .

awels

Code looks good, what about a functional test to make sure everything is gone?

awels · 2022-01-11T15:23:08Z

tests/dataimportcron_test.go

+		cron, err = f.CdiClient.CdiV1beta1().DataImportCrons(f.Namespace.Name).Create(context.TODO(), cron, metav1.CreateOptions{})
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Verify jobs were created")


Should it also create pods associated with those jobs? AFAICT there are 3 levels of objects. Top level DataImportCrons, second level CronJob and Initial Job, and third level the actual pods created by those jobs. So I would expect the test to verify all 3 get created, and that all 3 get deleted when the top level is deleted.

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

brybacki · 2022-01-12T09:06:11Z

it is interesting - on the ceph lane the DV actually succeeded, but test failed (maybe it did not finish in time)

1.641918946014968e+09	DEBUG	controller.datavolume-controller	UUID not already used, marking checkpoint completed by current pod ID.	{"checkpoint": "cirros.raw", "podId": "c40fb2b7-3899-4092-a34d-bf4ecf46dc30"}
1.641918946015955e+09	DEBUG	controller.import-controller	Updated PVC	{"PVC": "cdi-e2e-tests-dv-func-test-s68fl/dv-imageio-warm-import-test", "pvc.anno.Phase": "Succeeded", "pvc.anno.Restarts": "0"}
1.6419189460159824e+09	DEBUG	controller.import-controller	Import completed successfully	{"PVC": "cdi-e2e-tests-dv-func-test-s68fl/dv-imageio-warm-import-test"}

brybacki · 2022-01-12T09:06:33Z

/retest

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

awels

Logic is fine, just some alternative suggestions on some tests to better convey their intentions.

awels · 2022-01-12T14:23:51Z

tests/dataimportcron_test.go

+
+		By("Verify initial job created")
+		initialJobName := controller.GetInitialJobName(cron)
+		Eventually(func() bool {


I think this is more readable

Eventually(func() *corev1.Job { job, _ := f.K8sClient.BatchV1().Jobs(f.CdiInstallNs).Get(context.TODO(), initialJobName, metav1.GetOptions{}) return job }, dataImportCronTimeout, pollingInterval).ShouldNot(BeNil())

The intention is to verify the job exists, the eventually func doesn't have to return a bool, it can return anything.

awels · 2022-01-12T14:25:15Z

tests/dataimportcron_test.go

+		}, dataImportCronTimeout, pollingInterval).Should(BeTrue())
+
+		By("Verify initial job pod created")
+		Eventually(func() bool {


Alternative suggestion:

Eventually(func() *corev1.Pod { pod, _ := utils.FindPodByPrefixOnce(f.K8sClient, f.CdiInstallNs, initialJobName, "") return pod }, dataImportCronTimeout, pollingInterval).ShouldNot(BeNil())

awels · 2022-01-12T14:30:15Z

tests/dataimportcron_test.go

+		By("Verify cronjob created and has active job")
+		cronJobName := controller.GetCronJobName(cron)
+		var jobName string
+		Eventually(func() bool {


Alternative suggestion:

Eventually(func() string { jobName = "" cronjob, _ := f.K8sClient.BatchV1beta1().CronJobs(f.CdiInstallNs).Get(context.TODO(), cronJobName, metav1.GetOptions{}) if cronjob != nil && len(cronjob.Status.Active) > 0 { jobName = cronjob.Status.Active[0].Name } return jobName }, dataImportCronTimeout, pollingInterval).ShouldNot(BeEmpty())

Note we are ignoring errors here, maybe write them to the output to help debug if there are issues.

awels · 2022-01-12T14:41:35Z

tests/dataimportcron_test.go

+		}, dataImportCronTimeout, pollingInterval).Should(BeTrue())
+
+		By("Verify cronjob first job created")
+		Eventually(func() bool {


Alternative:

Eventually(func() *corev1.Job { job, _ := f.K8sClient.BatchV1().Jobs(f.CdiInstallNs).Get(context.TODO(), jobName, metav1.GetOptions{}) return job }, dataImportCronTimeout, pollingInterval).ShouldNot(BeNil())

awels · 2022-01-12T14:42:20Z

tests/dataimportcron_test.go

+		}, dataImportCronTimeout, pollingInterval).Should(BeTrue())
+
+		By("Verify cronjob first job pod created")
+		Eventually(func() bool {


Alternative:

Eventually(func() *corev1.Pod { pod, _ := utils.FindPodByPrefixOnce(f.K8sClient, f.CdiInstallNs, jobName, "") return pod }, dataImportCronTimeout, pollingInterval).ShouldNot(BeNil())

awels · 2022-01-12T15:02:11Z

tests/dataimportcron_test.go

+		}, dataImportCronTimeout, pollingInterval).Should(BeTrue())
+
+		By("Verify initial job pod deleted")
+		Eventually(func() bool {


Alternative:

Eventually(func() bool { _, err := utils.FindPodByPrefixOnce(f.K8sClient, f.CdiInstallNs, intialJobName, "") return errors.IsNotFound(err) }, dataImportCronTimeout, pollingInterval).Should(BeTrue())

awels · 2022-01-12T15:02:43Z

tests/dataimportcron_test.go

+		}, dataImportCronTimeout, pollingInterval).Should(BeTrue())
+
+		By("Verify cronjob first job pod deleted")
+		Eventually(func() bool {


Alternative:

Eventually(func() bool { _, err := utils.FindPodByPrefixOnce(f.K8sClient, f.CdiInstallNs, jobName, "") return errors.IsNotFound(err) }, dataImportCronTimeout, pollingInterval).Should(BeTrue())

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

awels

/lgtm
lets see if our fix to the findPodByPrefix func breaks anything, but looks fine otherwise.

awels · 2022-01-12T19:55:56Z

/approve

kubevirt-bot · 2022-01-12T19:56:07Z

[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

arnongilboa · 2022-01-13T16:10:52Z

/cherrypick release-v1.43

kubevirt-bot · 2022-01-13T16:11:32Z

@arnongilboa: new pull request created: #2100

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 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. labels Jan 10, 2022

kubevirt-bot requested review from aglitke and maya-r January 10, 2022 13:50

kubevirt-bot added the size/M label Jan 10, 2022

arnongilboa force-pushed the cleanup_dic_jobs_n_pods branch from c145c6c to 9922d21 Compare January 10, 2022 14:40

arnongilboa force-pushed the cleanup_dic_jobs_n_pods branch from 9922d21 to 248312d Compare January 10, 2022 16:12

awels reviewed Jan 10, 2022

View reviewed changes

awels reviewed Jan 11, 2022

View reviewed changes

arnongilboa force-pushed the cleanup_dic_jobs_n_pods branch 2 times, most recently from 898a069 to 055e08d Compare January 11, 2022 14:21

awels reviewed Jan 11, 2022

View reviewed changes

Cleanup DataImportCron jobs on deletion

fc54f59

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

CR fixes

4f1c6a3

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

arnongilboa force-pushed the cleanup_dic_jobs_n_pods branch from 055e08d to 4f1c6a3 Compare January 12, 2022 11:12

kubevirt-bot added size/L and removed size/M labels Jan 12, 2022

awels reviewed Jan 12, 2022

View reviewed changes

Func test fixes

63085ec

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

awels approved these changes Jan 12, 2022

View reviewed changes

kubevirt-bot assigned awels Jan 12, 2022

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

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

kubevirt-bot merged commit 97288e3 into kubevirt:main Jan 12, 2022

kubevirt-bot mentioned this pull request Jan 13, 2022

[release-v1.43] Cleanup DataImportCron jobs on deletion #2100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup DataImportCron jobs on deletion #2088

Cleanup DataImportCron jobs on deletion #2088

arnongilboa commented Jan 10, 2022

mhenriks commented Jan 10, 2022

awels Jan 10, 2022

arnongilboa Jan 11, 2022

awels left a comment

awels Jan 11, 2022

arnongilboa Jan 12, 2022

brybacki commented Jan 12, 2022

brybacki commented Jan 12, 2022

awels left a comment

awels Jan 12, 2022

awels Jan 12, 2022

awels Jan 12, 2022

awels Jan 12, 2022

awels Jan 12, 2022

awels Jan 12, 2022

awels Jan 12, 2022

awels left a comment

awels commented Jan 12, 2022

kubevirt-bot commented Jan 12, 2022

arnongilboa commented Jan 13, 2022

kubevirt-bot commented Jan 13, 2022

Cleanup DataImportCron jobs on deletion #2088

Cleanup DataImportCron jobs on deletion #2088

Conversation

arnongilboa commented Jan 10, 2022

mhenriks commented Jan 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brybacki commented Jan 12, 2022

brybacki commented Jan 12, 2022

awels 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

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 left a comment

Choose a reason for hiding this comment

awels commented Jan 12, 2022

kubevirt-bot commented Jan 12, 2022

arnongilboa commented Jan 13, 2022

kubevirt-bot commented Jan 13, 2022