Deprecate DV garbage collection

After several releases with GC disabled by default, we decided to
deprecate it, as unfortunately it violates fundamental principle of
Kubernetes. CR should not be auto-deleted when it completes its role
(Job with TTLSecondsAfterFinished 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).

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

api/openapi-spec/swagger.json

@@ -3938,7 +3938,7 @@
     "type": "object",
     "properties": {
      "dataVolumeTTLSeconds": {
-      "description": "DataVolumeTTLSeconds is the time in seconds after DataVolume completion it can be garbage collected. Disabled by default.",
+      "description": "DataVolumeTTLSeconds is the time in seconds after DataVolume completion it can be garbage collected. Disabled by default. Deprecated: Removed in v1.62.",
       "type": "integer",
       "format": "int32"
      },

doc/cdi-config.md

@@ -16,7 +16,6 @@ CDI configuration in specified by administrators in the `spec.config` of the `CD
 | preallocation            | nil           | Preallocation setting to use unless a per-dataVolume value is set                                                                                                                                                            |
 | importProxy              | nil           | The proxy configuration to be used by the importer pod when accessing a http data source. When the ImportProxy is empty, the Cluster Wide-Proxy (Openshift) configurations are used. ImportProxy has four parameters: `ImportProxy.HTTPProxy` that defines the proxy http url, the `ImportProxy.HTTPSProxy` that determines the roxy https url, and the `ImportProxy.noProxy` which enforce that a list of hostnames and/or CIDRs will be not proxied, and finally, the `ImportProxy.TrustedCAProxy`, the ConfigMap name of an user-provided trusted certificate authority (CA) bundle to be added to the importer pod CA bundle. |
 | insecureRegistries       | nil           | List of TLS disabled registries. |
-| dataVolumeTTLSeconds     | nil           | Time in seconds after DataVolume completion it can be garbage collected. Disabled by default. |
 | tlsSecurityProfile       | nil           | Used by operators to apply cluster-wide TLS security settings to operands. |
 
 filesystemOverhead configuration:
@@ -38,10 +37,6 @@ kubectl patch cdi cdi  --type='json' -p='[{ "op" : "add" , "path" : "/spec/confi
 ```bash
 kubectl patch cdi cdi  --type='json' -p='[{ "op" : "add" , "path" : "/spec/config/filesystemOverhead/global" , "value" : "0.0" }]'
 ```
-To configure dataVolumeTTLSeconds (e.g. enable DataVolume garbage collection)
-```bash
-kubectl patch cdi cdi --patch '{"spec": {"config": {"dataVolumeTTLSeconds": "0"}}}' --type merge
-```
 ## Getting
 
 CDI configuration may be retrieved by any authenticated user in the cluster by checking the `status` of the `CDIConfig` singleton

doc/datavolumes.md

@@ -5,22 +5,6 @@ Data Volumes(DV) are an abstraction on top of Persistent Volume Claims(PVC) and
 
 Why is this an improvement over simply looking at the state annotation created and managed by CDI? Data Volumes provide a versioned API that other projects like [Kubevirt](https://github.com/kubevirt/kubevirt) can integrate with. This way those projects can rely on an API staying the same for a particular version and have guarantees about what that API will look like. Any changes to the API will result in a new version of the API.
 
-### Garbage collection of successfully completed DataVolumes (disabled by default)
-Once the PVC population process is completed, its corresponding DV has no use, so it can be garbage collected.
-
-Some GC motivations:
-* Keeping the DV around after the fact sometimes confuses users, thinking they should modify the DV to have the matching PVC react. For example, resizing PVC seems to confuse users because they see the DV.
-* When user deletes a PVC that is owned by a DV, she is confused when the PVC is re-created.
-* Restore a backed up VM without the need to recreate the DataVolume is much simpler.
-* Replicate a workload to another cluster without the need to mutate the PVC and DV with special annotations in order for them to behave as expected in the new cluster.
-
-However, after several releases we decided to disable GC by default, as unfortunately it violates fundamental principle of Kubernetes. CR should not be auto-deleted when it completes its role (Job with TTLSecondsAfterFinished 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).
-
-GC can be configured in [CDIConfig](cdi-config.md), so users cannot assume the DV exists after completion. When the desired PVC exists, but its DV does not exist, it means that the PVC was successfully populated and the DV was garbage collected. To prevent a DV from being garbage collected (when enabled in CDIConfig), it should be annotated with:
-```yaml
-cdi.kubevirt.io/storage.deleteAfterCompletion: "false"
-```
-
 ### Status phases
 The following statuses are possible.
 * 'Blank': No status available.

manifests/templates/upgrade-testing-artifacts.yaml.in

@@ -11,7 +11,6 @@ metadata:
   namespace: cdi-testing-old-version-artifacts
   annotations:
     cdi.kubevirt.io/storage.bind.immediate.requested: "true"
-    cdi.kubevirt.io/storage.deleteAfterCompletion: "false"
 spec:
   source:
       http:

pkg/apiserver/webhooks/BUILD.bazel

@@ -61,7 +61,6 @@ go_test(
     embed = [":go_default_library"],
     deps = [
         "//pkg/client/clientset/versioned/fake:go_default_library",
-        "//pkg/common:go_default_library",
         "//pkg/controller/common:go_default_library",
         "//staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1:go_default_library",
         "//vendor/github.com/appscode/jsonpatch:go_default_library",

pkg/apiserver/webhooks/datavolume-mutate.go

@@ -34,7 +34,6 @@ import (
 
 	cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"
 	cdiclient "kubevirt.io/containerized-data-importer/pkg/client/clientset/versioned"
-	"kubevirt.io/containerized-data-importer/pkg/common"
 	cc "kubevirt.io/containerized-data-importer/pkg/controller/common"
 	"kubevirt.io/containerized-data-importer/pkg/token"
 )
@@ -82,20 +81,6 @@ func (wh *dataVolumeMutatingWebhook) Admit(ar admissionv1.AdmissionReview) *admi
 
 	modifiedDataVolume := dataVolume.DeepCopy()
 
-	if ar.Request.Operation == admissionv1.Create {
-		config, err := wh.cdiClient.CdiV1beta1().CDIConfigs().Get(context.TODO(), common.ConfigName, metav1.GetOptions{})
-		if err != nil {
-			return toAdmissionResponseError(err)
-		}
-		// Garbage collection is disabled by default
-		// Annotate DV for GC only if GC is enabled in CDIConfig and the DV is not annotated to prevent GC
-		if cc.GetDataVolumeTTLSeconds(config) >= 0 {
-			if modifiedDataVolume.Annotations[cc.AnnDeleteAfterCompletion] != "false" {
-				cc.AddAnnotation(modifiedDataVolume, cc.AnnDeleteAfterCompletion, "true")
-			}
-		}
-	}
-
 	targetNamespace, targetName := dataVolume.Namespace, dataVolume.Name
 	if targetNamespace == "" {
 		targetNamespace = ar.Request.Namespace

pkg/apiserver/webhooks/datavolume-mutate_test.go

@@ -38,11 +38,9 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	fakeclient "k8s.io/client-go/kubernetes/fake"
 	k8stesting "k8s.io/client-go/testing"
-	"k8s.io/utils/ptr"
 
 	cdicorev1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"
 	cdiclientfake "kubevirt.io/containerized-data-importer/pkg/client/clientset/versioned/fake"
-	"kubevirt.io/containerized-data-importer/pkg/common"
 	cc "kubevirt.io/containerized-data-importer/pkg/controller/common"
 )
 
@@ -144,7 +142,7 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 			}
 			resp := mutateDVs(key, ar, true)
 			Expect(resp.Allowed).To(BeTrue())
-			Expect(resp.Patch).ToNot(BeNil())
+			Expect(resp.Patch).To(BeNil())
 		},
 			Entry("with static volume annotation", cc.AnnCheckStaticVolume),
 			Entry("with prePopulated volume annotation", cc.AnnPrePopulated),
@@ -183,10 +181,9 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 				},
 			}
 
-			resp := mutateDVsEx(key, ar, true, 0, []runtime.Object{dataSource})
+			resp := mutateDVs(key, ar, true, dataSource)
 			Expect(resp.Allowed).To(BeTrue())
 			Expect(resp.Patch).ToNot(BeNil())
-
 			var patchObjs []jsonpatch.Operation
 			err := json.Unmarshal(resp.Patch, &patchObjs)
 			Expect(err).ToNot(HaveOccurred())
@@ -272,7 +269,7 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 
 			resp := mutateDVs(key, ar, false)
 			Expect(resp.Allowed).To(BeTrue())
-			Expect(resp.Patch).ToNot(BeNil())
+			Expect(resp.Patch).To(BeNil())
 		},
 			Entry("with static volume annotation", cc.AnnCheckStaticVolume),
 			Entry("with prePopulated volume annotation", cc.AnnPrePopulated),
@@ -323,7 +320,7 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 
 			resp := mutateDVs(key, ar, false)
 			Expect(resp.Allowed).To(BeTrue())
-			Expect(resp.Patch).ToNot(BeNil())
+			Expect(resp.Patch).To(BeNil())
 		},
 			Entry("with static volume annotation", cc.AnnCheckStaticVolume),
 			Entry("with prePopulated volume annotation", cc.AnnPrePopulated),
@@ -350,72 +347,21 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 			resp := mutateDVs(key, ar, true)
 			Expect(resp.Allowed).To(BeTrue())
 			Expect(resp.Patch).ToNot(BeNil())
-
 			var patchObjs []jsonpatch.Operation
 			err := json.Unmarshal(resp.Patch, &patchObjs)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(patchObjs).Should(HaveLen(1))
 			Expect(patchObjs[0].Operation).Should(Equal("add"))
 			Expect(patchObjs[0].Path).Should(Equal("/metadata/annotations"))
-
 		},
 			Entry("succeed with explicit namespace", "testNamespace"),
 			Entry("succeed with same (default) namespace", "default"),
 			Entry("succeed with empty namespace", ""),
 		)
-
-		DescribeTable("should", func(ttl int) {
-			dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
-			dvBytes, _ := json.Marshal(&dataVolume)
-
-			ar := &admissionv1.AdmissionReview{
-				Request: &admissionv1.AdmissionRequest{
-					Operation: admissionv1.Create,
-					Resource: metav1.GroupVersionResource{
-						Group:    cdicorev1.SchemeGroupVersion.Group,
-						Version:  cdicorev1.SchemeGroupVersion.Version,
-						Resource: "datavolumes",
-					},
-					Object: runtime.RawExtension{
-						Raw: dvBytes,
-					},
-				},
-			}
-
-			resp := mutateDVsEx(key, ar, true, int32(ttl), nil)
-			Expect(resp.Allowed).To(BeTrue())
-
-			if ttl < 0 {
-				Expect(resp.Patch).To(BeNil())
-				return
-			}
-
-			Expect(resp.Patch).ToNot(BeNil())
-
-			var patchObjs []jsonpatch.Operation
-			err := json.Unmarshal(resp.Patch, &patchObjs)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(patchObjs).Should(HaveLen(1))
-			Expect(patchObjs[0].Operation).Should(Equal("add"))
-			Expect(patchObjs[0].Path).Should(Equal("/metadata/annotations"))
-
-			ann, ok := patchObjs[0].Value.(map[string]interface{})
-			Expect(ok).Should(BeTrue())
-			val, ok := ann[cc.AnnDeleteAfterCompletion].(string)
-			Expect(ok).Should(BeTrue())
-			Expect(val).Should(Equal("true"))
-		},
-			Entry("set GC annotation if TTL is set", 0),
-			Entry("not set GC annotation if TTL is disabled", -1),
-		)
 	})
 })
 
-func mutateDVs(key *rsa.PrivateKey, ar *admissionv1.AdmissionReview, isAuthorized bool) *admissionv1.AdmissionResponse {
-	return mutateDVsEx(key, ar, isAuthorized, 0, nil)
-}
-
-func mutateDVsEx(key *rsa.PrivateKey, ar *admissionv1.AdmissionReview, isAuthorized bool, ttl int32, cdiObjects []runtime.Object) *admissionv1.AdmissionResponse {
+func mutateDVs(key *rsa.PrivateKey, ar *admissionv1.AdmissionReview, isAuthorized bool, cdiObjects ...runtime.Object) *admissionv1.AdmissionResponse {
 	defaultNs := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}}
 	testNs := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "testNamespace"}}
 	client := fakeclient.NewSimpleClientset(&defaultNs, &testNs)
@@ -433,11 +379,7 @@ func mutateDVsEx(key *rsa.PrivateKey, ar *admissionv1.AdmissionReview, isAuthori
 		return true, sar, nil
 	})
 
-	cdiConfig := cc.MakeEmptyCDIConfigSpec(common.ConfigName)
-	cdiConfig.Spec.DataVolumeTTLSeconds = ptr.To[int32](ttl)
-	objs := []runtime.Object{cdiConfig}
-	objs = append(objs, cdiObjects...)
-	cdiClient := cdiclientfake.NewSimpleClientset(objs...)
+	cdiClient := cdiclientfake.NewSimpleClientset(cdiObjects...)
 	wh := NewDataVolumeMutatingWebhook(client, cdiClient, key)
 	return serve(ar, wh)
 }

pkg/controller/common/util.go

@@ -95,8 +95,6 @@ const (
 	// AnnExternalPopulation annotation marks a PVC as "externally populated", allowing the import-controller to skip it
 	AnnExternalPopulation = AnnAPIGroup + "/externalPopulation"
 
-	// AnnDeleteAfterCompletion is PVC annotation for deleting DV after completion
-	AnnDeleteAfterCompletion = AnnAPIGroup + "/storage.deleteAfterCompletion"
 	// AnnPodRetainAfterCompletion is PVC annotation for retaining transfer pods after completion
 	AnnPodRetainAfterCompletion = AnnAPIGroup + "/storage.pod.retainAfterCompletion"
 
@@ -256,9 +254,6 @@ const (
 	// be dynamically provisioned. Its value is the name of the selected node.
 	AnnSelectedNode = "volume.kubernetes.io/selected-node"
 
-	// AnnGarbageCollected is a PVC annotation indicating that the PVC was garbage collected
-	AnnGarbageCollected = AnnAPIGroup + "/garbageCollected"
-
 	// CloneUniqueID is used as a special label to be used when we search for the pod
 	CloneUniqueID = AnnAPIGroup + "/storage.clone.cloneUniqeId"
 
@@ -1419,16 +1414,6 @@ func IsErrCacheNotStarted(err error) bool {
 	return errors.As(err, &target)
 }
 
-// GetDataVolumeTTLSeconds gets the current DataVolume TTL in seconds if GC is enabled, or < 0 if GC is disabled
-// Garbage collection is disabled by default
-func GetDataVolumeTTLSeconds(config *cdiv1.CDIConfig) int32 {
-	const defaultDataVolumeTTLSeconds = -1
-	if config.Spec.DataVolumeTTLSeconds != nil {
-		return *config.Spec.DataVolumeTTLSeconds
-	}
-	return defaultDataVolumeTTLSeconds
-}
-
 // NewImportDataVolume returns new import DataVolume CR
 func NewImportDataVolume(name string) *cdiv1.DataVolume {
 	return &cdiv1.DataVolume{

pkg/controller/datavolume/BUILD.bazel

@@ -7,7 +7,6 @@ go_library(
         "conditions.go",
         "controller-base.go",
         "external-population-controller.go",
-        "garbagecollect.go",
         "import-controller.go",
         "pvc-clone-controller.go",
         "snapshot-clone-controller.go",
@@ -32,7 +31,6 @@ go_library(
         "//vendor/github.com/go-logr/logr:go_default_library",
         "//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1:go_default_library",
         "//vendor/github.com/pkg/errors:go_default_library",
-        "//vendor/k8s.io/api/authorization/v1:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/api/storage/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",

pkg/controller/datavolume/controller-base.go

@@ -539,12 +539,6 @@ func (r *ReconcilerBase) syncDvPvcState(log logr.Logger, req reconcile.Request,
 	}
 
 	if syncState.pvc != nil {
-		if err := r.garbageCollect(&syncState, log); err != nil {
-			return syncState, err
-		}
-		if syncState.result != nil || syncState.dv == nil {
-			return syncState, nil
-		}
 		if err := r.validatePVC(dv, syncState.pvc); err != nil {
 			return syncState, err
 		}