Delete CRDs & check finalizers in operator delete #2097

arnongilboa · 2022-01-13T13:01:17Z

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

What this PR does / why we need it:
reconcileDeleteControllerDeployment deletes the CRDs of DataImportCron and DataVolume, and returns error if any DIC/DV has a finalizer. It will get retried and controller will eventually remove the finalizers.

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

reconcileDeleteControllerDeployment deletes the CRDs of DataImportCron and DataVolume, and returns error if any DIC/DV has a finalizer. It will get retried and controller will eventually remove the finalizers. Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

mhenriks · 2022-01-13T14:57:15Z

pkg/operator/controller/callbacks.go

+		return err
+	}
+	dics := &cdiv1.DataImportCronList{}
+	if err := args.Client.List(context.TODO(), dics, &client.ListOptions{}); err != nil && !errors.IsNotFound(err) {


This should be IsNoMatchError not IsNotFound

Should probably check for both no? If the CRD doesn't exist, or there are no DICs left, we are happy.

I don't think list returns notfound. Just returns an empty list in that case

mhenriks · 2022-01-13T14:58:28Z

Should be "destructive" test for this that uninstalls CDI

awels

Also agree with Michael need a destructive test here where some DICs and DVs with finalizers on them.

awels · 2022-01-13T15:36:32Z

pkg/operator/controller/callbacks.go

+		return err
+	}
+	dics := &cdiv1.DataImportCronList{}
+	if err := args.Client.List(context.TODO(), dics, &client.ListOptions{}); err != nil && !errors.IsNotFound(err) {


Should probably check for both no? If the CRD doesn't exist, or there are no DICs left, we are happy.

awels · 2022-01-13T15:37:10Z

pkg/operator/controller/callbacks.go

+		return err
+	}
+	dvs := &cdiv1.DataVolumeList{}
+	if err := args.Client.List(context.TODO(), dvs, &client.ListOptions{}); err != nil && !errors.IsNotFound(err) {


Same here, check for IsNoMatchError and NotFound

mhenriks · 2022-01-13T21:50:29Z

pkg/operator/controller/callbacks.go

+	args.Logger.Info("Deleting CRDs and verifing no finalizers")
+
+	crd := &extv1.CustomResourceDefinition{ObjectMeta: metav1.ObjectMeta{Name: "dataimportcrons.cdi.kubevirt.io"}}
+	if err := args.Client.Delete(context.TODO(), crd, &client.DeleteOptions{}); cdicontroller.IgnoreIsNoMatchError(err) != nil {


NotFound is okay here

so you say in CRD delete we need to accept both NoMatch and NotFound?

Just NotFound

mhenriks · 2022-01-13T21:51:05Z

pkg/operator/controller/callbacks.go

+		return err
+	}
+	dics := &cdiv1.DataImportCronList{}
+	if err := args.Client.List(context.TODO(), dics, &client.ListOptions{}); err != nil {


NoMatch is okay here

so in List (after we deleted the CRD) we need to accept both NoMatch and NotFound?

Just NoMatch

awels · 2022-01-14T12:58:22Z

/retest

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

awels · 2022-01-17T15:06:46Z

/retest

mhenriks · 2022-01-18T15:34:03Z

tests/operator_test.go

+				url := fmt.Sprintf(utils.TrustedRegistryURL, f.DockerPrefix)
+				cron := NewDataImportCron("cron-test", "5Gi", scheduleEveryMinute, "ds", cdiv1.DataVolumeSourceRegistry{URL: &url, PullMethod: &registryPullNode}, cdiv1.DataImportCronRetainAll)
+				cron, err := f.CdiClient.CdiV1beta1().DataImportCrons(f.Namespace.Name).Create(context.TODO(), cron, metav1.CreateOptions{})
+				Expect(err).ToNot(HaveOccurred())


can we wait for DV to be created/succeed? Or at least validate that the finalizer exists?

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

awels · 2022-01-19T13:48:16Z

/lgtm

awels · 2022-01-19T13:48:31Z

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

mhenriks · 2022-01-19T14:12:21Z

/approve

kubevirt-bot · 2022-01-19T14:12:35Z

[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

arnongilboa · 2022-01-19T15:49:07Z

/cherrypick release-v1.43

kubevirt-bot · 2022-01-19T15:49:46Z

@arnongilboa: new pull request created: #2114

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. size/S labels Jan 13, 2022

kubevirt-bot requested review from awels and mhenriks January 13, 2022 13:01

arnongilboa force-pushed the cdi_delete_dics branch from 621275b to e4a52e3 Compare January 13, 2022 14:00

arnongilboa force-pushed the cdi_delete_dics branch from e4a52e3 to 62a7566 Compare January 13, 2022 14:03

kubevirt-bot added size/M and removed size/S labels Jan 13, 2022

mhenriks reviewed Jan 13, 2022

View reviewed changes

awels reviewed Jan 13, 2022

View reviewed changes

mhenriks reviewed Jan 13, 2022

View reviewed changes

Fix error handling

f6df10f

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

arnongilboa force-pushed the cdi_delete_dics branch from 8141309 to f6df10f Compare January 17, 2022 11:24

mhenriks reviewed Jan 18, 2022

View reviewed changes

arnongilboa force-pushed the cdi_delete_dics branch from 8c0e73b to de86883 Compare January 18, 2022 22:41

Add destructive operator test

84ad9dd

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

arnongilboa force-pushed the cdi_delete_dics branch from de86883 to 84ad9dd Compare January 19, 2022 08:08

kubevirt-bot assigned awels Jan 19, 2022

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

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

kubevirt-bot merged commit d9a23e5 into kubevirt:main Jan 19, 2022

kubevirt-bot mentioned this pull request Jan 19, 2022

[release-v1.43] Delete CRDs & check finalizers in operator delete #2114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete CRDs & check finalizers in operator delete #2097

Delete CRDs & check finalizers in operator delete #2097

arnongilboa commented Jan 13, 2022

mhenriks Jan 13, 2022

awels Jan 13, 2022

mhenriks Jan 13, 2022

mhenriks commented Jan 13, 2022

awels left a comment

awels Jan 13, 2022

awels Jan 13, 2022

mhenriks Jan 13, 2022 •

edited

arnongilboa Jan 13, 2022

mhenriks Jan 14, 2022

arnongilboa Jan 17, 2022

mhenriks Jan 13, 2022

arnongilboa Jan 13, 2022

mhenriks Jan 14, 2022

arnongilboa Jan 17, 2022

awels commented Jan 14, 2022

awels commented Jan 17, 2022

mhenriks Jan 18, 2022

arnongilboa Jan 18, 2022

awels commented Jan 19, 2022

awels commented Jan 19, 2022

mhenriks commented Jan 19, 2022

kubevirt-bot commented Jan 19, 2022

arnongilboa commented Jan 19, 2022

kubevirt-bot commented Jan 19, 2022

Delete CRDs & check finalizers in operator delete #2097

Delete CRDs & check finalizers in operator delete #2097

Conversation

arnongilboa commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks commented Jan 13, 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

mhenriks Jan 13, 2022 • edited

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 commented Jan 14, 2022

awels commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels commented Jan 19, 2022

awels commented Jan 19, 2022

mhenriks commented Jan 19, 2022

kubevirt-bot commented Jan 19, 2022

arnongilboa commented Jan 19, 2022

kubevirt-bot commented Jan 19, 2022

mhenriks Jan 13, 2022 •

edited