Enable DataVolume garbage collection by default (#2421)

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 disabled. This should be fixed there.

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

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

api/openapi-spec/swagger.json

@@ -4961,7 +4961,7 @@
     "type": "object",
     "properties": {
      "dataVolumeTTLSeconds": {
-      "description": "DataVolumeTTLSeconds is the time in seconds after DataVolume completion it can be garbage collected.",
+      "description": "DataVolumeTTLSeconds is the time in seconds after DataVolume completion it can be garbage collected. The default is 0 sec. To disable GC use -1.",
       "type": "integer",
       "format": "int32"
      },

doc/cdi-config.md

@@ -16,7 +16,7 @@ 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 after DataVolume completion it can be garbage collected. |
+| dataVolumeTTLSeconds     | nil           | Time in seconds after DataVolume completion it can be garbage collected. The default is 0 sec. To disable GC use -1. |
 | tlsSecurityProfile       | nil           | Used by operators to apply cluster-wide TLS security settings to operands. |
 
 filesystemOverhead configuration:
@@ -38,6 +38,10 @@ 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. disable DataVolume garbage collection)
+```bash
+kubectl patch cdi cdi --patch '{"spec": {"config": {"dataVolumeTTLSeconds": "-1"}}}' --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

@@ -6,15 +6,15 @@ 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
-Once the PVC population process is completed, its corresponding DV has no use, so it is better garbage collected.
+Once the PVC population process is completed, its corresponding DV has no use, so it is garbage collected by default.
 
 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.
 
-GC can be dynamically configured in [CDIConfig](cdi-config.md), so we recommend users not to 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, it should be annotated with:
+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, it should be annotated with:
 ```yaml
 cdi.kubevirt.io/storage.deleteAfterCompletion: "false"
 ```

pkg/apiserver/webhooks/BUILD.bazel

@@ -69,6 +69,7 @@ go_test(
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/client-go/kubernetes/fake:go_default_library",
         "//vendor/k8s.io/client-go/testing:go_default_library",
+        "//vendor/k8s.io/utils/pointer:go_default_library",
         "//vendor/kubevirt.io/controller-lifecycle-operator-sdk/api:go_default_library",
     ],
 )

pkg/apiserver/webhooks/datavolume-mutate.go

@@ -111,7 +111,7 @@ func (wh *dataVolumeMutatingWebhook) Admit(ar admissionv1.AdmissionReview) *admi
 		if err != nil {
 			return toAdmissionResponseError(err)
 		}
-		if config.Spec.DataVolumeTTLSeconds != nil {
+		if controller.GetDataVolumeTTLSeconds(config) >= 0 {
 			if modifiedDataVolume.Annotations == nil {
 				modifiedDataVolume.Annotations = make(map[string]string)
 			}

pkg/apiserver/webhooks/datavolume-mutate_test.go

@@ -38,6 +38,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	fakeclient "k8s.io/client-go/kubernetes/fake"
 	k8stesting "k8s.io/client-go/testing"
+	"k8s.io/utils/pointer"
 
 	cdiclientfake "kubevirt.io/containerized-data-importer/pkg/client/clientset/versioned/fake"
 	"kubevirt.io/containerized-data-importer/pkg/common"
@@ -156,7 +157,7 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 				},
 			}
 
-			resp := mutateDVsEx(key, ar, true, nil, []runtime.Object{dataSource})
+			resp := mutateDVsEx(key, ar, true, 0, []runtime.Object{dataSource})
 			Expect(resp.Allowed).To(BeTrue())
 			Expect(resp.Patch).ToNot(BeNil())
 
@@ -278,7 +279,7 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 			Entry("succeed with empty namespace", ""),
 		)
 
-		DescribeTable("should", func(ttl *int32) {
+		DescribeTable("should", func(ttl int) {
 			dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
 			dvBytes, _ := json.Marshal(&dataVolume)
 
@@ -296,10 +297,10 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 				},
 			}
 
-			resp := mutateDVsEx(key, ar, true, ttl, nil)
+			resp := mutateDVsEx(key, ar, true, int32(ttl), nil)
 			Expect(resp.Allowed).To(BeTrue())
 
-			if ttl == nil {
+			if ttl < 0 {
 				Expect(resp.Patch).To(BeNil())
 				return
 			}
@@ -319,18 +320,17 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 			Expect(ok).Should(BeTrue())
 			Expect(val).Should(Equal("true"))
 		},
-			Entry("set GC annotation if TTL is set", &[]int32{0}[0]),
-			Entry("not set GC annotation if TTL is not set", nil),
+			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, nil, nil)
+	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 mutateDVsEx(key *rsa.PrivateKey, ar *admissionv1.AdmissionReview, isAuthorized bool, ttl int32, 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)
@@ -349,7 +349,7 @@ func mutateDVsEx(key *rsa.PrivateKey, ar *admissionv1.AdmissionReview, isAuthori
 	})
 
 	cdiConfig := controller.MakeEmptyCDIConfigSpec(common.ConfigName)
-	cdiConfig.Spec.DataVolumeTTLSeconds = ttl
+	cdiConfig.Spec.DataVolumeTTLSeconds = pointer.Int32(ttl)
 	objs := []runtime.Object{cdiConfig}
 	objs = append(objs, cdiObjects...)
 	cdiClient := cdiclientfake.NewSimpleClientset(objs...)

pkg/controller/datavolume-controller.go

@@ -2777,35 +2777,40 @@ func (r *DatavolumeReconciler) garbageCollect(dataVolume *cdiv1.DataVolume, pvc
 	if dataVolume.Status.Phase != cdiv1.Succeeded {
 		return nil, nil
 	}
-	if allowed, err := r.isGarbageCollectionAllowed(dataVolume); !allowed || err != nil {
-		log.Info("Garbage Collection is not allowed, due to owner finalizers update RBAC")
-		return nil, err
-	}
 	cdiConfig := &cdiv1.CDIConfig{}
 	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: common.ConfigName}, cdiConfig); err != nil {
 		return nil, err
 	}
-	dvTTL := cdiConfig.Spec.DataVolumeTTLSeconds
-	if dvTTL == nil || *dvTTL < 0 {
+	dvTTL := GetDataVolumeTTLSeconds(cdiConfig)
+	if dvTTL < 0 {
 		log.Info("Garbage Collection is disabled")
 		return nil, nil
 	}
+	if allowed, err := r.isGarbageCollectionAllowed(dataVolume, log); !allowed || err != nil {
+		return nil, err
+	}
 	// Current DV still has TTL, so reconcile will return with the needed RequeueAfter
-	if delta := getDeltaTTL(dataVolume, *dvTTL); delta > 0 {
+	if delta := getDeltaTTL(dataVolume, dvTTL); delta > 0 {
 		return &reconcile.Result{RequeueAfter: delta}, nil
 	}
-	if err := r.detachPvcDeleteDv(pvc, dataVolume, log); err != nil {
+	if err := r.detachPvcDeleteDv(pvc, dataVolume); err != nil {
 		return nil, err
 	}
 	return &reconcile.Result{}, nil
 }
 
-func (r *DatavolumeReconciler) isGarbageCollectionAllowed(dv *cdiv1.DataVolume) (bool, error) {
+func (r *DatavolumeReconciler) isGarbageCollectionAllowed(dv *cdiv1.DataVolume, log logr.Logger) (bool, error) {
+	dvDelete := dv.Annotations[AnnDeleteAfterCompletion]
+	if dvDelete != "true" {
+		log.Info("DataVolume is not annotated to be garbage collected")
+		return false, nil
+	}
 	for _, ref := range dv.OwnerReferences {
 		if ref.BlockOwnerDeletion == nil || *ref.BlockOwnerDeletion == false {
 			continue
 		}
 		if allowed, err := r.canUpdateFinalizers(ref); !allowed || err != nil {
+			log.Info("DataVolume cannot be garbage collected, due to owner finalizers update RBAC")
 			return false, err
 		}
 	}
@@ -2835,15 +2840,7 @@ func (r *DatavolumeReconciler) canUpdateFinalizers(ownerRef metav1.OwnerReferenc
 	return ssar.Status.Allowed, nil
 }
 
-func (r *DatavolumeReconciler) detachPvcDeleteDv(pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume, log logr.Logger) error {
-	if dv.Status.Phase != cdiv1.Succeeded {
-		return nil
-	}
-	dvDelete := dv.Annotations[AnnDeleteAfterCompletion]
-	if dvDelete != "true" {
-		log.Info("DataVolume is not annotated to be garbage collected")
-		return nil
-	}
+func (r *DatavolumeReconciler) detachPvcDeleteDv(pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) error {
 	updatePvcOwnerRefs(pvc, dv)
 	delete(pvc.Annotations, AnnPopulatedFor)
 	if err := r.updatePVC(pvc); err != nil {
@@ -2885,6 +2882,15 @@ func getDeltaTTL(dv *cdiv1.DataVolume, ttl int32) time.Duration {
 	return delta
 }
 
+// GetDataVolumeTTLSeconds gets the current DataVolume TTL in seconds if GC is enabled, or < 0 if GC is disabled
+func GetDataVolumeTTLSeconds(config *cdiv1.CDIConfig) int32 {
+	const defaultDataVolumeTTLSeconds = 0
+	if config.Spec.DataVolumeTTLSeconds != nil {
+		return *config.Spec.DataVolumeTTLSeconds
+	}
+	return defaultDataVolumeTTLSeconds
+}
+
 // validateCloneAndSourcePVC checks if the source PVC of a clone exists and does proper validation
 func (r *DatavolumeReconciler) validateCloneAndSourcePVC(datavolume *cdiv1.DataVolume) (bool, error) {
 	sourcePvc, err := r.findSourcePvc(datavolume)

pkg/operator/resources/crds_generated.go

@@ -2207,7 +2207,8 @@ spec:
                 properties:
                   dataVolumeTTLSeconds:
                     description: DataVolumeTTLSeconds is the time in seconds after
-                      DataVolume completion it can be garbage collected.
+                      DataVolume completion it can be garbage collected. The default
+                      is 0 sec. To disable GC use -1.
                     format: int32
                     type: integer
                   featureGates:
@@ -4624,7 +4625,8 @@ spec:
             properties:
               dataVolumeTTLSeconds:
                 description: DataVolumeTTLSeconds is the time in seconds after DataVolume
-                  completion it can be garbage collected.
+                  completion it can be garbage collected. The default is 0 sec. To
+                  disable GC use -1.
                 format: int32
                 type: integer
               featureGates:

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/types.go

@@ -261,7 +261,7 @@ type DataVolumeStatus struct {
 	Conditions   []DataVolumeCondition `json:"conditions,omitempty" optional:"true"`
 }
 
-//DataVolumeList provides the needed parameters to do request a list of Data Volumes from the system
+// DataVolumeList provides the needed parameters to do request a list of Data Volumes from the system
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 type DataVolumeList struct {
 	metav1.TypeMeta `json:",inline"`
@@ -359,7 +359,7 @@ const DataVolumeCloneSourceSubresource = "source"
 // see https://github.com/kubernetes/code-generator/issues/59
 // +genclient:nonNamespaced
 
-//StorageProfile provides a CDI specific recommendation for storage parameters
+// StorageProfile provides a CDI specific recommendation for storage parameters
 // +genclient
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 // +kubebuilder:object:root=true
@@ -373,15 +373,15 @@ type StorageProfile struct {
 	Status StorageProfileStatus `json:"status,omitempty"`
 }
 
-//StorageProfileSpec defines specification for StorageProfile
+// StorageProfileSpec defines specification for StorageProfile
 type StorageProfileSpec struct {
 	// CloneStrategy defines the preferred method for performing a CDI clone
 	CloneStrategy *CDICloneStrategy `json:"cloneStrategy,omitempty"`
 	// ClaimPropertySets is a provided set of properties applicable to PVC
 	ClaimPropertySets []ClaimPropertySet `json:"claimPropertySets,omitempty"`
 }
 
-//StorageProfileStatus provides the most recently observed status of the StorageProfile
+// StorageProfileStatus provides the most recently observed status of the StorageProfile
 type StorageProfileStatus struct {
 	// The StorageClass name for which capabilities are defined
 	StorageClass *string `json:"storageClass,omitempty"`
@@ -405,7 +405,7 @@ type ClaimPropertySet struct {
 	VolumeMode *corev1.PersistentVolumeMode `json:"volumeMode,omitempty" protobuf:"bytes,6,opt,name=volumeMode,casttype=PersistentVolumeMode"`
 }
 
-//StorageProfileList provides the needed parameters to request a list of StorageProfile from the system
+// StorageProfileList provides the needed parameters to request a list of StorageProfile from the system
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 type StorageProfileList struct {
 	metav1.TypeMeta `json:",inline"`
@@ -685,7 +685,7 @@ type CDIStatus struct {
 	sdkapi.Status `json:",inline"`
 }
 
-//CDIList provides the needed parameters to do request a list of CDIs from the system
+// CDIList provides the needed parameters to do request a list of CDIs from the system
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 type CDIList struct {
 	metav1.TypeMeta `json:",inline"`
@@ -713,20 +713,20 @@ type CDIConfig struct {
 	Status CDIConfigStatus `json:"status,omitempty"`
 }
 
-//Percent is a string that can only be a value between [0,1)
+// Percent is a string that can only be a value between [0,1)
 // (Note: we actually rely on reconcile to reject invalid values)
 // +kubebuilder:validation:Pattern=`^(0(?:\.\d{1,3})?|1)$`
 type Percent string
 
-//FilesystemOverhead defines the reserved size for PVCs with VolumeMode: Filesystem
+// FilesystemOverhead defines the reserved size for PVCs with VolumeMode: Filesystem
 type FilesystemOverhead struct {
 	// Global is how much space of a Filesystem volume should be reserved for overhead. This value is used unless overridden by a more specific value (per storageClass)
 	Global Percent `json:"global,omitempty"`
 	// StorageClass specifies how much space of a Filesystem volume should be reserved for safety. The keys are the storageClass and the values are the overhead. This value overrides the global value
 	StorageClass map[string]Percent `json:"storageClass,omitempty"`
 }
 
-//CDIConfigSpec defines specification for user configuration
+// CDIConfigSpec defines specification for user configuration
 type CDIConfigSpec struct {
 	// Override the URL used when uploading to a DataVolume
 	UploadProxyURLOverride *string `json:"uploadProxyURLOverride,omitempty"`
@@ -745,14 +745,14 @@ type CDIConfigSpec struct {
 	Preallocation *bool `json:"preallocation,omitempty"`
 	// InsecureRegistries is a list of TLS disabled registries
 	InsecureRegistries []string `json:"insecureRegistries,omitempty"`
-	// DataVolumeTTLSeconds is the time in seconds after DataVolume completion it can be garbage collected.
+	// DataVolumeTTLSeconds is the time in seconds after DataVolume completion it can be garbage collected. The default is 0 sec. To disable GC use -1.
 	// +optional
 	DataVolumeTTLSeconds *int32 `json:"dataVolumeTTLSeconds,omitempty"`
 	// TLSSecurityProfile is used by operators to apply cluster-wide TLS security settings to operands.
 	TLSSecurityProfile *ocpconfigv1.TLSSecurityProfile `json:"tlsSecurityProfile,omitempty"`
 }
 
-//CDIConfigStatus provides the most recently observed status of the CDI Config resource
+// CDIConfigStatus provides the most recently observed status of the CDI Config resource
 type CDIConfigStatus struct {
 	// The calculated upload proxy URL
 	UploadProxyURL *string `json:"uploadProxyURL,omitempty"`
@@ -769,7 +769,7 @@ type CDIConfigStatus struct {
 	Preallocation bool `json:"preallocation,omitempty"`
 }
 
-//CDIConfigList provides the needed parameters to do request a list of CDIConfigs from the system
+// CDIConfigList provides the needed parameters to do request a list of CDIConfigs from the system
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 type CDIConfigList struct {
 	metav1.TypeMeta `json:",inline"`
@@ -779,7 +779,7 @@ type CDIConfigList struct {
 	Items []CDIConfig `json:"items"`
 }
 
-//ImportProxy provides the information on how to configure the importer pod proxy.
+// ImportProxy provides the information on how to configure the importer pod proxy.
 type ImportProxy struct {
 	// HTTPProxy is the URL http://<username>:<pswd>@<ip>:<port> of the import proxy for HTTP requests.  Empty means unset and will not result in the import pod env var.
 	// +optional