From 9796b2550be97c5ed1c8b55ee6f9142352f392a3 Mon Sep 17 00:00:00 2001 From: Ido Aharon Date: Mon, 12 Jun 2023 18:57:47 +0300 Subject: [PATCH] Delete old version DV's with DIC GC Signed-off-by: Ido Aharon --- pkg/controller/BUILD.bazel | 1 + pkg/controller/dataimportcron-controller.go | 23 +++- .../dataimportcron-controller_test.go | 45 +++++++ tests/dataimportcron_test.go | 117 ++++++++++++++++++ 4 files changed, 184 insertions(+), 2 deletions(-) diff --git a/pkg/controller/BUILD.bazel b/pkg/controller/BUILD.bazel index 12bbb1b92a..2102297c92 100644 --- a/pkg/controller/BUILD.bazel +++ b/pkg/controller/BUILD.bazel @@ -113,6 +113,7 @@ go_test( "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/intstr:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library", diff --git a/pkg/controller/dataimportcron-controller.go b/pkg/controller/dataimportcron-controller.go index 650dd7c802..348dec9b8d 100644 --- a/pkg/controller/dataimportcron-controller.go +++ b/pkg/controller/dataimportcron-controller.go @@ -804,7 +804,7 @@ func (r *DataImportCronReconciler) garbageCollectOldImports(ctx context.Context, maxImports = int(*cron.Spec.ImportsToKeep) } - if err := r.garbageCollectPVCs(ctx, cron.Namespace, selector, maxImports); err != nil { + if err := r.garbageCollectPVCs(ctx, cron.Namespace, cron.Name, selector, maxImports); err != nil { return err } if err := r.garbageCollectSnapshots(ctx, cron.Namespace, selector, maxImports); err != nil { @@ -814,7 +814,7 @@ func (r *DataImportCronReconciler) garbageCollectOldImports(ctx context.Context, return nil } -func (r *DataImportCronReconciler) garbageCollectPVCs(ctx context.Context, namespace string, selector labels.Selector, maxImports int) error { +func (r *DataImportCronReconciler) garbageCollectPVCs(ctx context.Context, namespace, cronName string, selector labels.Selector, maxImports int) error { pvcList := &corev1.PersistentVolumeClaimList{} if err := r.client.List(ctx, pvcList, &client.ListOptions{Namespace: namespace, LabelSelector: selector}); err != nil { @@ -832,6 +832,25 @@ func (r *DataImportCronReconciler) garbageCollectPVCs(ctx context.Context, names } } + dvList := &cdiv1.DataVolumeList{} + if err := r.client.List(ctx, dvList, &client.ListOptions{Namespace: namespace, LabelSelector: selector}); err != nil { + return err + } + + for _, dv := range dvList.Items { + pvc := &corev1.PersistentVolumeClaim{} + if err := r.client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: dv.Name}, pvc); err != nil { + return err + } + + if pvc.Labels[common.DataImportCronLabel] != cronName { + r.log.Info("Deleting dv/pvc", "name", pvc.Name, "pvc.uid", pvc.UID) + if err := r.deleteDvPvc(ctx, dv.Name, dv.Namespace); err != nil { + return err + } + } + } + return nil } diff --git a/pkg/controller/dataimportcron-controller_test.go b/pkg/controller/dataimportcron-controller_test.go index 89c8396831..0b008c0fa7 100644 --- a/pkg/controller/dataimportcron-controller_test.go +++ b/pkg/controller/dataimportcron-controller_test.go @@ -37,6 +37,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" @@ -706,6 +707,50 @@ var _ = Describe("All DataImportCron Tests", func() { Entry("has no tag", imageStreamName, 1), ) + It("XXXXXX Should succeed garbage collecting old version DVs", func() { + cron = newDataImportCron(cronName) + cron.Annotations[AnnSourceDesiredDigest] = testDigest + reconciler = createDataImportCronReconciler(cron) + + // labeled dv + dv := cc.NewImportDataVolume("test-dv") + dv.Labels = map[string]string{common.DataImportCronLabel: cronName} + err := reconciler.client.Create(context.TODO(), dv) + Expect(err).ToNot(HaveOccurred()) + + pvc := cc.CreatePvc(dv.Name, dv.Namespace, nil, nil) + pvc.OwnerReferences = []metav1.OwnerReference{ + *metav1.NewControllerRef(dv, schema.GroupVersionKind{ + Group: cdiv1.SchemeGroupVersion.Group, + Version: cdiv1.SchemeGroupVersion.Version, + Kind: "DataVolume", + }), + } + err = reconciler.client.Create(context.TODO(), pvc) + Expect(err).ToNot(HaveOccurred()) + + // unlabeled dv + unlabeledDv := cc.NewImportDataVolume("test-dv2") + err = reconciler.client.Create(context.TODO(), unlabeledDv) + Expect(err).ToNot(HaveOccurred()) + + unlabeledPvc := cc.CreatePvc(unlabeledDv.Name, unlabeledDv.Namespace, nil, nil) + err = reconciler.client.Create(context.TODO(), unlabeledPvc) + Expect(err).ToNot(HaveOccurred()) + + reconciler.garbageCollectOldImports(context.TODO(), cron) + + // Ensure the labeled DV is deleted. The labeled PVC will not be deleted here, as there is no relevant controller. + err = reconciler.client.Get(context.TODO(), dvKey(dv.Name), dv) + Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + + // Ensure unlabeled DV and PVC are not deleted + err = reconciler.client.Get(context.TODO(), dvKey(unlabeledDv.Name), unlabeledDv) + Expect(err).ToNot(HaveOccurred()) + err = reconciler.client.Get(context.TODO(), dvKey(unlabeledPvc.Name), unlabeledPvc) + Expect(err).ToNot(HaveOccurred()) + }) + It("should pass through metadata to DataVolume and DataSource", func() { cron = newDataImportCron(cronName) cron.Annotations[AnnSourceDesiredDigest] = testDigest diff --git a/tests/dataimportcron_test.go b/tests/dataimportcron_test.go index 9eb967cb9e..2811cdaadd 100644 --- a/tests/dataimportcron_test.go +++ b/tests/dataimportcron_test.go @@ -48,6 +48,7 @@ var _ = Describe("DataImportCron", func() { ns string scName string originalProfileSpec *cdiv1.StorageProfileSpec + dvName = "dv-garbage" ) BeforeEach(func() { @@ -367,6 +368,122 @@ var _ = Describe("DataImportCron", func() { Entry("[test_id:8266] succeed deleting error DVs when importing new ones", false, true, 2, cdiv1.DataImportCronSourceFormatPvc), ) + It("[test_id:XXXX] succeed garbage collecting old version dv's", func() { + reg, err := getDataVolumeSourceRegistry(f) + //Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) + //defer utils.RemoveInsecureRegistry(f.CrClient, *reg.URL) + + By(fmt.Sprintf("Create labeled DataVolume %s for garbage collection test", dvName)) + dv := utils.NewDataVolumeWithRegistryImport(dvName, "5Gi", "") + dv.Spec.Source.Registry = reg + dv.Labels = map[string]string{common.DataImportCronLabel: cronName} + cc.AddAnnotation(dv, cc.AnnDeleteAfterCompletion, "false") + dv, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, ns, dv) + Expect(err).ToNot(HaveOccurred()) + + By("Wait for import completion") + err = utils.WaitForDataVolumePhase(f, ns, cdiv1.Succeeded, dv.Name) + Expect(err).ToNot(HaveOccurred(), "Datavolume not in phase succeeded in time") + + By(fmt.Sprintf("Verify pvc was created %s", dv.Name)) + pvc, err := utils.WaitForPVC(f.K8sClient, ns, dv.Name) + Expect(err).ToNot(HaveOccurred()) + By(fmt.Sprintf("Verify DataImportCronLabel is passed to the pvc: %s", pvc.Labels[common.DataImportCronLabel])) + Expect(pvc.Labels[common.DataImportCronLabel]).To(Equal(cronName)) + + pvc.Labels[common.DataImportCronLabel] = "" + By("Update DataImportCron label to be empty in the pvc") + _, err = f.K8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Update(context.TODO(), pvc, metav1.UpdateOptions{}) + Expect(err).ToNot(HaveOccurred()) + + By(fmt.Sprintf("Create new DataImportCron %s, url %s", cronName, *reg.URL)) + cron = utils.NewDataImportCron(cronName, "5Gi", scheduleEveryMinute, dataSourceName, importsToKeep, *reg) + retentionPolicy := cdiv1.DataImportCronRetainNone + cron.Spec.RetentionPolicy = &retentionPolicy + cron, err = f.CdiClient.CdiV1beta1().DataImportCrons(ns).Create(context.TODO(), cron, metav1.CreateOptions{}) + Expect(err).ToNot(HaveOccurred()) + + By("Wait for conditions") + waitForConditions(corev1.ConditionFalse, corev1.ConditionTrue) + + By("Verify CurrentImports update") + currentImportDvName := cron.Status.CurrentImports[0].DataVolumeName + Expect(currentImportDvName).ToNot(BeEmpty()) + + dataSource, err := f.CdiClient.CdiV1beta1().DataSources(ns).Get(context.TODO(), dataSourceName, metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + + if pvc := dataSource.Spec.Source.PVC; pvc != nil { + By(fmt.Sprintf("Verify pvc was created %s", pvc.Name)) + _, err = utils.WaitForPVC(f.K8sClient, ns, pvc.Name) + Expect(err).ToNot(HaveOccurred()) + + By("Wait for import completion") + err = utils.WaitForDataVolumePhase(f, ns, cdiv1.Succeeded, pvc.Name) + Expect(err).ToNot(HaveOccurred(), "Datavolume not in phase succeeded in time") + } else if snap := dataSource.Spec.Source.Snapshot; snap != nil { + By("Wait for snapshot to be ready") + snapshot := &snapshotv1.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: snap.Name, + Namespace: snap.Namespace, + }, + } + snapshot = utils.WaitSnapshotReady(f.CrClient, snapshot) + By("Verify snapshot is ready to be used") + Expect(snapshot.Status.ReadyToUse).ToNot(BeNil()) + Expect(*snapshot.Status.ReadyToUse).To(BeTrue()) + } + + By("Check garbage collection") + Eventually(func() int { + dvList, err := f.CdiClient.CdiV1beta1().DataVolumes(ns).List(context.TODO(), metav1.ListOptions{}) + Expect(err).ToNot(HaveOccurred()) + return len(dvList.Items) + }, dataImportCronTimeout, pollingInterval).Should(Equal(0), "Garbage collection failed cleaning old imports") + + if pvc := dataSource.Spec.Source.PVC; pvc != nil { + By("Check that the PVC is timestamped and not garbage collected") + pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(ns).Get(context.TODO(), pvc.Name, metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + // timestamp + lastUse := pvc.Annotations[controller.AnnLastUseTime] + Expect(lastUse).ToNot(BeEmpty()) + ts, err := time.Parse(time.RFC3339Nano, lastUse) + Expect(err).ToNot(HaveOccurred()) + Expect(ts).To(BeTemporally("<", time.Now())) + } else if snap := dataSource.Spec.Source.Snapshot; snap != nil { + By("Check that the snapshot is timestamped and not garbage collected") + snapshot := &snapshotv1.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: snap.Name, + Namespace: snap.Namespace, + }, + } + snapshot = utils.WaitSnapshotReady(f.CrClient, snapshot) + lastUse := snapshot.Annotations[controller.AnnLastUseTime] + Expect(lastUse).ToNot(BeEmpty()) + ts, err := time.Parse(time.RFC3339Nano, lastUse) + Expect(err).ToNot(HaveOccurred()) + Expect(ts).To(BeTemporally("<", time.Now())) + } + + By("Delete cron") + err = f.CdiClient.CdiV1beta1().DataImportCrons(ns).Delete(context.TODO(), cronName, metav1.DeleteOptions{}) + Expect(err).ToNot(HaveOccurred()) + + By("Verify DataSource deletion") + Eventually(func() bool { + _, err := f.CdiClient.CdiV1beta1().DataSources(ns).Get(context.TODO(), dataSourceName, metav1.GetOptions{}) + return errors.IsNotFound(err) + }, dataImportCronTimeout, pollingInterval).Should(BeTrue(), "DataSource was not deleted") + + By("Verify original PVC deletion") + _, err = f.K8sClient.CoreV1().PersistentVolumeClaims(ns).Get(context.TODO(), dv.Name, metav1.GetOptions{}) + Expect(err).To(HaveOccurred()) + }) + It("[test_id:10040] Should get digest updated by external poller", func() { By("Create DataImportCron with only initial poller job") cron = utils.NewDataImportCron(cronName, "5Gi", scheduleOnceAYear, dataSourceName, importsToKeep, *reg)