Backport main commits to 1.57 release branch (#2764)

* 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 · Jun 21, 2023 · 4b2c171 · 4b2c171
1 parent fae6535
commit 4b2c171
Show file tree

Hide file tree

Showing 74 changed files with 4,372 additions and 1,261 deletions.
diff --git a/WORKSPACE b/WORKSPACE
@@ -2063,37 +2063,25 @@ rpm(
 rpm(
     name = "python3-0__3.9.10-2.el9.aarch64",
     sha256 = "59b8a8f1473acf95f448370d587e5672b83bcfd5693f20421d060229e5565235",
-    urls = [
-        "http://mirror.stream.centos.org/9-stream/BaseOS/aarch64/os/Packages/python3-3.9.10-2.el9.aarch64.rpm",
-        "https://storage.googleapis.com/builddeps/59b8a8f1473acf95f448370d587e5672b83bcfd5693f20421d060229e5565235",
-    ],
+    urls = ["https://storage.googleapis.com/builddeps/59b8a8f1473acf95f448370d587e5672b83bcfd5693f20421d060229e5565235"],
 )
 
 rpm(
     name = "python3-0__3.9.10-2.el9.x86_64",
     sha256 = "e4f0741a08e047f06370791e8b33f952ccac106cdaae43d84db950a551fa7536",
-    urls = [
-        "http://mirror.stream.centos.org/9-stream/BaseOS/x86_64/os/Packages/python3-3.9.10-2.el9.x86_64.rpm",
-        "https://storage.googleapis.com/builddeps/e4f0741a08e047f06370791e8b33f952ccac106cdaae43d84db950a551fa7536",
-    ],
+    urls = ["https://storage.googleapis.com/builddeps/e4f0741a08e047f06370791e8b33f952ccac106cdaae43d84db950a551fa7536"],
 )
 
 rpm(
     name = "python3-libs-0__3.9.10-2.el9.aarch64",
     sha256 = "e2425e2081e428200ab730890402f076f94b7d1befed2f72295dd074b3c7be4b",
-    urls = [
-        "http://mirror.stream.centos.org/9-stream/BaseOS/aarch64/os/Packages/python3-libs-3.9.10-2.el9.aarch64.rpm",
-        "https://storage.googleapis.com/builddeps/e2425e2081e428200ab730890402f076f94b7d1befed2f72295dd074b3c7be4b",
-    ],
+    urls = ["https://storage.googleapis.com/builddeps/e2425e2081e428200ab730890402f076f94b7d1befed2f72295dd074b3c7be4b"],
 )
 
 rpm(
     name = "python3-libs-0__3.9.10-2.el9.x86_64",
     sha256 = "1c2203b76476a223996695dd4e2fca6743101d049b276d8a523a200dc60244f4",
-    urls = [
-        "http://mirror.stream.centos.org/9-stream/BaseOS/x86_64/os/Packages/python3-libs-3.9.10-2.el9.x86_64.rpm",
-        "https://storage.googleapis.com/builddeps/1c2203b76476a223996695dd4e2fca6743101d049b276d8a523a200dc60244f4",
-    ],
+    urls = ["https://storage.googleapis.com/builddeps/1c2203b76476a223996695dd4e2fca6743101d049b276d8a523a200dc60244f4"],
 )
 
 rpm(

diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
@@ -4937,7 +4937,7 @@
     "type": "object",
     "properties": {
      "dataVolumeTTLSeconds": {
-      "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.",
+      "description": "DataVolumeTTLSeconds is the time in seconds after DataVolume completion it can be garbage collected. Disabled by default.",
       "type": "integer",
       "format": "int32"
      },

diff --git a/automation/non-csi-hpp.sh b/automation/non-csi-hpp.sh
@@ -0,0 +1,25 @@
+#!/bin/bash
+#
+# This file is part of the KubeVirt project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Copyright 2023 Red Hat, Inc.
+#
+
+set -ex
+export TARGET=k8s-1.26-centos9
+export KUBEVIRT_STORAGE=hpp
+export HPP_CLASSIC=true
+export CDI_E2E_SKIP=Destructive
+automation/test.sh
diff --git a/cluster-sync/ephemeral_provider.sh b/cluster-sync/ephemeral_provider.sh
@@ -57,7 +57,18 @@ function configure_hpp() {
   _kubectl apply -f https://github.com/kubevirt/hostpath-provisioner-operator/releases/download/$HPP_RELEASE/storageclass-wffc-legacy-csi.yaml
   echo "Waiting for hostpath provisioner to be available"
   _kubectl wait hostpathprovisioners.hostpathprovisioner.kubevirt.io/hostpath-provisioner --for=condition=Available --timeout=480s
-  _kubectl patch storageclass hostpath-csi -p '{"metadata": {"annotations":{"storageclass.kubernetes.io/is-default-class":"true"}}}'
+}
+
+function configure_hpp_classic() {
+  # Configure hpp and default to classic non-csi hostpath-provisioner
+  configure_hpp
+  _kubectl patch storageclass hostpath-provisioner -p '{"metadata": {"annotations":{"storageclass.kubernetes.io/is-default-class":"true"}}}'
+}
+
+function configure_hpp_csi() {
+  # Configure hpp and default to hostpath-csi
+  configure_hpp
+   _kubectl patch storageclass hostpath-csi -p '{"metadata": {"annotations":{"storageclass.kubernetes.io/is-default-class":"true"}}}'
 }
 
 function configure_nfs() {

diff --git a/cluster-sync/kubevirtci/provider.sh b/cluster-sync/kubevirtci/provider.sh
@@ -22,7 +22,11 @@ function configure_storage() {
     fi
   elif [[ $KUBEVIRT_STORAGE == "hpp" ]] ; then
     echo "Installing hostpath provisioner storage"
-    configure_hpp
+    if [[ $HPP_CLASSIC == "true" ]] ; then
+      configure_hpp_classic
+    else
+      configure_hpp_csi
+    fi
   elif [[ $KUBEVIRT_STORAGE == "nfs" ]] ; then
     echo "Installing NFS CSI dynamic storage"
     configure_nfs_csi

diff --git a/doc/cdi-config.md b/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 in seconds after DataVolume completion it can be garbage collected. The default is 0 sec. To disable GC use -1. |
+| 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,9 +38,9 @@ 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)
+To configure dataVolumeTTLSeconds (e.g. enable DataVolume garbage collection)
 ```bash
-kubectl patch cdi cdi --patch '{"spec": {"config": {"dataVolumeTTLSeconds": "-1"}}}' --type merge
+kubectl patch cdi cdi --patch '{"spec": {"config": {"dataVolumeTTLSeconds": "0"}}}' --type merge
 ```
 ## Getting
 

diff --git a/doc/datavolumes.md b/doc/datavolumes.md
@@ -5,16 +5,18 @@ 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 garbage collected by default.
+### 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.
 
-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:
+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"
 ```

diff --git a/hack/build/build-go.sh b/hack/build/build-go.sh
@@ -54,10 +54,14 @@ elif [ "${go_opt}" == "build" ]; then
         outLink=${BIN_DIR}/${BIN_NAME}
         rm -f ${outFile}
         rm -f ${outLink}
+        static_flag=""
+        if [ "$tgt" == "tools/cdi-containerimage-server" ]; then
+          static_flag="static"
+        fi
         (
             cd $tgt
             # Only build executables for linux
-            GOOS=linux go build -o ${outFile} -ldflags '-extldflags "static"' -ldflags "$(cdi::version::ldflags)"
+            GOOS=linux go build -o ${outFile} -tags strictfipsruntime -ldflags '-extldflags $static_flag' -ldflags "$(cdi::version::ldflags)"
 
             ln -sf ${outFile} ${outLink}
         )

diff --git a/pkg/apis/core/v1beta1/openapi_generated.go b/pkg/apis/core/v1beta1/openapi_generated.go
diff --git a/pkg/apiserver/webhooks/dataimportcron-validate.go b/pkg/apiserver/webhooks/dataimportcron-validate.go
@@ -116,13 +116,15 @@ func (wh *dataImportCronValidatingWebhook) validateDataImportCronSpec(request *a
 		return causes
 	}
 
-	if _, err := cronexpr.Parse(spec.Schedule); err != nil {
-		causes = append(causes, metav1.StatusCause{
-			Type:    metav1.CauseTypeFieldValueInvalid,
-			Message: "Illegal cron schedule",
-			Field:   field.Child("Schedule").String(),
-		})
-		return causes
+	if spec.Schedule != "" {
+		if _, err := cronexpr.Parse(spec.Schedule); err != nil {
+			causes = append(causes, metav1.StatusCause{
+				Type:    metav1.CauseTypeFieldValueInvalid,
+				Message: "Illegal cron schedule",
+				Field:   field.Child("Schedule").String(),
+			})
+			return causes
+		}
 	}
 
 	if spec.ImportsToKeep != nil && *spec.ImportsToKeep < 0 {

diff --git a/pkg/apiserver/webhooks/dataimportcron-validate_test.go b/pkg/apiserver/webhooks/dataimportcron-validate_test.go
@@ -121,6 +121,12 @@ var _ = Describe("Validating Webhook", func() {
 			resp := validateDataImportCronCreate(cron)
 			Expect(resp.Allowed).To(BeFalse())
 		})
+		It("should allow DataImportCron with empty cron schedule", func() {
+			cron := newDataImportCron(cdiv1.DataVolumeSourceRegistry{URL: &testRegistryURL})
+			cron.Spec.Schedule = ""
+			resp := validateDataImportCronCreate(cron)
+			Expect(resp.Allowed).To(BeTrue())
+		})
 		It("should reject DataImportCron with illegal ManagedDataSource on create", func() {
 			cron := newDataImportCron(cdiv1.DataVolumeSourceRegistry{URL: &testRegistryURL})
 			cron.Spec.ManagedDataSource = ""

diff --git a/pkg/apiserver/webhooks/datavolume-mutate.go b/pkg/apiserver/webhooks/datavolume-mutate.go
@@ -86,12 +86,11 @@ func (wh *dataVolumeMutatingWebhook) Admit(ar admissionv1.AdmissionReview) *admi
 		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 == nil {
-				modifiedDataVolume.Annotations = make(map[string]string)
-			}
 			if modifiedDataVolume.Annotations[cc.AnnDeleteAfterCompletion] != "false" {
-				modifiedDataVolume.Annotations[cc.AnnDeleteAfterCompletion] = "true"
+				cc.AddAnnotation(modifiedDataVolume, cc.AnnDeleteAfterCompletion, "true")
 			}
 		}
 	}

diff --git a/pkg/controller/clone/common.go b/pkg/controller/clone/common.go
@@ -26,6 +26,11 @@ func IsDataSourcePVC(kind string) bool {
 	return kind == "PersistentVolumeClaim"
 }
 
+// IsDataSourceSnapshot checks for Snapshot source kind
+func IsDataSourceSnapshot(kind string) bool {
+	return kind == "VolumeSnapshot"
+}
+
 // AddCommonLabels adds common labels to a resource
 func AddCommonLabels(obj metav1.Object) {
 	if obj.GetLabels() == nil {