Disable DV GC by default #2754

arnongilboa · 2023-06-15T13:28:26Z

What this PR does / why we need it:
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 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).

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. Disabling GC on all kubevirt CI lanes beforehand can eliminate the failures, see PR.

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:

Disable DataVolume garbage collection by default

Prevent DV-related CI failures in the next kubevirt-bot kubevirtci bump PR (with bump-cdi), due to: kubevirt/containerized-data-importer#2754 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

mhenriks · 2023-06-16T18:48:01Z

looks okay, what's the deal with the test failures?

awels · 2023-06-16T19:04:53Z

Looks like most failures are happening here https://github.com/kubevirt/containerized-data-importer/blob/main/tests/utils/datavolume.go#L148

arnongilboa · 2023-06-18T13:05:09Z

looks okay, what's the deal with the test failures?

The test was incorrect for the no-GC flow. It should check that PVC with specific UID was deleted, as upon deletion of the last imported PVC DIC creates a new one.

alromeros · 2023-06-20T08:14:58Z

Looks good!
/lgtm

alromeros · 2023-06-20T08:47:37Z

/lgtm

akalenyu

PR looks good, my main concern is versioning this.

Is this planned to merge into 1.57?
It may be ready to do so, but we should carefully decide if that's where we want it.

/cc @awels

akalenyu · 2023-06-20T09:00:25Z

pkg/controller/common/util.go

@@ -1212,7 +1212,7 @@ func IsErrCacheNotStarted(err error) bool {

 // 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
+	const defaultDataVolumeTTLSeconds = -1


Maybe a comment here that GC is disabled by default?

akalenyu · 2023-06-20T09:04:42Z

pkg/apiserver/webhooks/datavolume-mutate.go

 			if modifiedDataVolume.Annotations[cc.AnnDeleteAfterCompletion] != "false" {
-				modifiedDataVolume.Annotations[cc.AnnDeleteAfterCompletion] = "true"
+				cc.AddAnnotation(modifiedDataVolume, cc.AnnDeleteAfterCompletion, "true")


Maybe worth a comment as well, similar to const defaultDataVolumeTTLSeconds = -1
as these are the GC defaulting points

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>

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

akalenyu · 2023-06-20T10:27:08Z

/lgtm

arnongilboa · 2023-06-20T10:46:25Z

PR looks good, my main concern is versioning this.

Is this planned to merge into 1.57? It may be ready to do so, but we should carefully decide if that's where we want it.

/cc @awels

/hold

@aglitke

arnongilboa · 2023-06-20T13:50:20Z

/unhold

arnongilboa · 2023-06-20T14:09:05Z

/retest

mhenriks · 2023-06-20T19:08:06Z

/approve

kubevirt-bot · 2023-06-20T19:08:10Z

[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

* 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>

* 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>

Prevent DV-related CI failures in the next kubevirt-bot kubevirtci bump PR (with bump-cdi), due to: kubevirt/containerized-data-importer#2754 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Jun 15, 2023

kubevirt-bot requested review from awels and maya-r June 15, 2023 13:38

arnongilboa mentioned this pull request Jun 15, 2023

Disable DataVolume GC by default in cluster-deploy kubevirt/kubevirt#9921

Merged

kubevirt-bot assigned alromeros Jun 20, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023

alromeros mentioned this pull request Jun 20, 2023

Add some debug logs on DataImportCron deletes #2761

Closed

arnongilboa force-pushed the disable_dv_gc_by_default branch from 1129490 to f3f03c8 Compare June 20, 2023 08:43

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023

akalenyu reviewed Jun 20, 2023

View reviewed changes

arnongilboa added 4 commits June 20, 2023 13:05

Fix test waiting for PVC deletion with UID

c870ac8

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

Fix clone test assuming DV was GCed

2aecf35

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

Fix DIC controller DV/PVC deletion when snapshot is ready

ab51cfb

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

arnongilboa force-pushed the disable_dv_gc_by_default branch from f3f03c8 to ab51cfb Compare June 20, 2023 10:06

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023

kubevirt-bot assigned akalenyu Jun 20, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 20, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 20, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2023

kubevirt-bot merged commit 0bc6a8a into kubevirt:main Jun 20, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable DV GC by default #2754

Disable DV GC by default #2754

arnongilboa commented Jun 15, 2023 •

edited

mhenriks commented Jun 16, 2023

awels commented Jun 16, 2023

arnongilboa commented Jun 18, 2023

alromeros commented Jun 20, 2023

alromeros commented Jun 20, 2023

akalenyu left a comment •

edited

akalenyu Jun 20, 2023

arnongilboa Jun 20, 2023

akalenyu Jun 20, 2023

arnongilboa Jun 20, 2023

akalenyu commented Jun 20, 2023

arnongilboa commented Jun 20, 2023

arnongilboa commented Jun 20, 2023

arnongilboa commented Jun 20, 2023

mhenriks commented Jun 20, 2023

kubevirt-bot commented Jun 20, 2023

Disable DV GC by default #2754

Disable DV GC by default #2754

Conversation

arnongilboa commented Jun 15, 2023 • edited

mhenriks commented Jun 16, 2023

awels commented Jun 16, 2023

arnongilboa commented Jun 18, 2023

alromeros commented Jun 20, 2023

alromeros commented Jun 20, 2023

akalenyu left a comment • edited

Choose a reason for hiding this comment

akalenyu Jun 20, 2023

Choose a reason for hiding this comment

arnongilboa Jun 20, 2023

Choose a reason for hiding this comment

akalenyu Jun 20, 2023

Choose a reason for hiding this comment

arnongilboa Jun 20, 2023

Choose a reason for hiding this comment

akalenyu commented Jun 20, 2023

arnongilboa commented Jun 20, 2023

arnongilboa commented Jun 20, 2023

arnongilboa commented Jun 20, 2023

mhenriks commented Jun 20, 2023

kubevirt-bot commented Jun 20, 2023

arnongilboa commented Jun 15, 2023 •

edited

akalenyu left a comment •

edited