From f357368bbed51ef0a4817785ecf0ac95a867774b Mon Sep 17 00:00:00 2001 From: Felix Matouschek Date: Fri, 16 Aug 2024 05:54:21 +0200 Subject: [PATCH] feat: Copy labels from source to DataSource (#3377) * cleanup: Extract label copying logic into common pkg Extract the label copying logic from populator-base.go into the common pkg as CopyAllowedLabels func. Signed-off-by: Felix Matouschek * fix(import-populator): Make copying of labels more robust Make copying of labels from a prime PVC to the target PVC more robust, by moving it before rebinding the PV from prime to target. This way we can ensure the labels are already present once the PVC becomes ready. Signed-off-by: Felix Matouschek * cleanup: Do not pass labels from DIC to DS anymore Do not pass labels from a DataImportCron to a DataSource in the dataimportcron-controller anymore. In the future this will be handled by the datasource-controller. Signed-off-by: Felix Matouschek * feat: Copy labels from source to DataSource Copy labels from the source of a DataSource to the labels of the DataSource in the datasource-controller. Signed-off-by: Felix Matouschek * tests: Add e2e tests for copying labels to DataSources Add e2e tests that cover all scenarios where labels should be copied from the source of a DataSource to the DataSource itself. Signed-off-by: Felix Matouschek * feat(dataimportcron-controller): Copy labels to VolumeSnapshots When using VolumeSnapshots copy the labels found on the source PVC to the created or an existing VolumeSnapshot. Signed-off-by: Felix Matouschek --------- Signed-off-by: Felix Matouschek --- pkg/controller/common/util.go | 12 ++ pkg/controller/common/util_test.go | 48 ++++++ pkg/controller/dataimportcron-controller.go | 60 ++++---- .../dataimportcron-controller_test.go | 25 ++-- pkg/controller/datasource-controller.go | 28 ++-- pkg/controller/datasource-controller_test.go | 72 +++++++++ pkg/controller/populators/import-populator.go | 12 +- pkg/controller/populators/populator-base.go | 17 +-- tests/dataimportcron_test.go | 18 ++- tests/datasource_test.go | 138 +++++++++++++++--- tests/utils/populators.go | 16 ++ 11 files changed, 348 insertions(+), 98 deletions(-) diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go index 66bd1b4de8..86f15eb9f3 100644 --- a/pkg/controller/common/util.go +++ b/pkg/controller/common/util.go @@ -382,6 +382,8 @@ var ( AnnPriorityClassName: "", AnnPodMultusDefaultNetwork: "", } + + validLabelsMatch = regexp.MustCompile(`^([\w.]+\.kubevirt.io|kubevirt.io)/[\w-]+$`) ) // FakeValidator is a fake token validator @@ -2030,6 +2032,16 @@ func CopyAllowedAnnotations(srcObj, dstObj metav1.Object) { } } +// CopyAllowedLabels copies allowed labels matching the validLabelsMatch regexp from the +// source map to the destination object allowing overwrites +func CopyAllowedLabels(srcLabels map[string]string, dstObj metav1.Object, overwrite bool) { + for label, value := range srcLabels { + if _, found := dstObj.GetLabels()[label]; (!found || overwrite) && validLabelsMatch.MatchString(label) { + AddLabel(dstObj, label, value) + } + } +} + // ClaimMayExistBeforeDataVolume returns true if the PVC may exist before the DataVolume func ClaimMayExistBeforeDataVolume(c client.Client, pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) (bool, error) { if ClaimIsPopulatedForDataVolume(pvc, dv) { diff --git a/pkg/controller/common/util_test.go b/pkg/controller/common/util_test.go index f3af6ce5e0..a30ed06299 100644 --- a/pkg/controller/common/util_test.go +++ b/pkg/controller/common/util_test.go @@ -318,6 +318,54 @@ var _ = Describe("GetMetricsURL", func() { }) }) +var _ = Describe("CopyAllowedLabels", func() { + const ( + testKubevirtIoKey = "test.kubevirt.io/test" + testKubevirtIoValue = "testvalue" + testInstancetypeKubevirtIoKey = "instancetype.kubevirt.io/default-preference" + testInstancetypeKubevirtIoValue = "testpreference" + testKubevirtIoKeyExisting = "test.kubevirt.io/existing" + testKubevirtIoValueExisting = "existing" + testKubevirtIoNewValueExisting = "newvalue" + testUndesiredKey = "undesired.key" + ) + + It("Should copy desired labels", func() { + srcLabels := map[string]string{ + testKubevirtIoKey: testKubevirtIoValue, + testInstancetypeKubevirtIoKey: testInstancetypeKubevirtIoValue, + testUndesiredKey: "undesired.key", + } + ds := &cdiv1.DataSource{} + CopyAllowedLabels(srcLabels, ds, false) + Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKey, testKubevirtIoValue)) + Expect(ds.Labels).To(HaveKeyWithValue(testInstancetypeKubevirtIoKey, testInstancetypeKubevirtIoValue)) + Expect(ds.Labels).ToNot(HaveKey(testUndesiredKey)) + }) + + DescribeTable("Should overwrite existing labels", func(overwrite bool) { + srcLabels := map[string]string{ + testKubevirtIoKeyExisting: testKubevirtIoNewValueExisting, + } + ds := &cdiv1.DataSource{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + testKubevirtIoKeyExisting: testKubevirtIoValueExisting, + }, + }, + } + CopyAllowedLabels(srcLabels, ds, overwrite) + if overwrite { + Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKeyExisting, testKubevirtIoNewValueExisting)) + } else { + Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKeyExisting, testKubevirtIoValueExisting)) + } + }, + Entry("when override enabled", true), + Entry("not when override disabled", false), + ) +}) + func createPvcNoSize(name, ns string, annotations, labels map[string]string) *v1.PersistentVolumeClaim { return &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/controller/dataimportcron-controller.go b/pkg/controller/dataimportcron-controller.go index 358117974f..0866d26fe7 100644 --- a/pkg/controller/dataimportcron-controller.go +++ b/pkg/controller/dataimportcron-controller.go @@ -105,6 +105,8 @@ const ( defaultImportsToKeepPerCron = 3 ) +var ErrNotManagedByCron = errors.New("DataSource is not managed by this DataImportCron") + // Reconcile loop for DataImportCronReconciler func (r *DataImportCronReconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { dataImportCron := &cdiv1.DataImportCron{} @@ -377,6 +379,15 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c cc.AddAnnotation(snapshot, cc.AnnSourceVolumeMode, string(*volMode)) } } + // Copy labels found on dataSource to the existing snapshot in case of upgrades. + dataSource, err := r.getDataSource(ctx, dataImportCron) + if err != nil { + if !k8serrors.IsNotFound(err) && !errors.Is(err, ErrNotManagedByCron) { + return res, err + } + } else { + cc.CopyAllowedLabels(dataSource.Labels, snapshot, true) + } if err := r.updateSource(ctx, dataImportCron, snapshot); err != nil { return res, err } @@ -494,6 +505,20 @@ func (r *DataImportCronReconciler) getSnapshot(ctx context.Context, cron *cdiv1. return snapshot, nil } +func (r *DataImportCronReconciler) getDataSource(ctx context.Context, dataImportCron *cdiv1.DataImportCron) (*cdiv1.DataSource, error) { + dataSourceName := dataImportCron.Spec.ManagedDataSource + dataSource := &cdiv1.DataSource{} + if err := r.client.Get(ctx, types.NamespacedName{Namespace: dataImportCron.Namespace, Name: dataSourceName}, dataSource); err != nil { + return nil, err + } + if dataSource.Labels[common.DataImportCronLabel] != dataImportCron.Name { + log := r.log.WithName("getCronManagedDataSource") + log.Info("DataSource has no DataImportCron label or is not managed by cron, so it is not updated", "name", dataSourceName, "uid", dataSource.UID, "cron", dataImportCron.Name) + return nil, ErrNotManagedByCron + } + return dataSource, nil +} + func (r *DataImportCronReconciler) updateSource(ctx context.Context, cron *cdiv1.DataImportCron, obj client.Object) error { objCopy := obj.DeepCopyObject() cc.AddAnnotation(obj, AnnLastUseTime, time.Now().UTC().Format(time.RFC3339Nano)) @@ -553,32 +578,23 @@ func (r *DataImportCronReconciler) updateImageStreamDesiredDigest(ctx context.Co func (r *DataImportCronReconciler) updateDataSource(ctx context.Context, dataImportCron *cdiv1.DataImportCron, format cdiv1.DataImportCronSourceFormat) error { log := r.log.WithName("updateDataSource") - dataSourceName := dataImportCron.Spec.ManagedDataSource - dataSource := &cdiv1.DataSource{} - if err := r.client.Get(ctx, types.NamespacedName{Namespace: dataImportCron.Namespace, Name: dataSourceName}, dataSource); err != nil { + dataSource, err := r.getDataSource(ctx, dataImportCron) + if err != nil { if k8serrors.IsNotFound(err) { dataSource = r.newDataSource(dataImportCron) if err := r.client.Create(ctx, dataSource); err != nil { return err } - log.Info("DataSource created", "name", dataSourceName, "uid", dataSource.UID) + log.Info("DataSource created", "name", dataSource.Name, "uid", dataSource.UID) + } else if errors.Is(err, ErrNotManagedByCron) { + return nil } else { return err } } - if dataSource.Labels[common.DataImportCronLabel] == "" { - log.Info("DataSource has no DataImportCron label, so it is not updated", "name", dataSourceName, "uid", dataSource.UID) - return nil - } dataSourceCopy := dataSource.DeepCopy() r.setDataImportCronResourceLabels(dataImportCron, dataSource) - for _, defaultInstanceTypeLabel := range cc.DefaultInstanceTypeLabels { - passCronLabelToDataSource(dataImportCron, dataSource, defaultInstanceTypeLabel) - } - - passCronLabelToDataSource(dataImportCron, dataSource, cc.LabelDynamicCredentialSupport) - sourcePVC := dataImportCron.Status.LastImportedPVC populateDataSource(format, dataSource, sourcePVC) @@ -701,15 +717,14 @@ func (r *DataImportCronReconciler) handleSnapshot(ctx context.Context, dataImpor if err != nil { return err } - labels := map[string]string{ - common.CDILabelKey: common.CDILabelValue, - common.CDIComponentLabel: "", - } desiredSnapshot := &snapshotv1.VolumeSnapshot{ ObjectMeta: metav1.ObjectMeta{ Name: pvc.Name, Namespace: dataImportCron.Namespace, - Labels: labels, + Labels: map[string]string{ + common.CDILabelKey: common.CDILabelValue, + common.CDIComponentLabel: "", + }, }, Spec: snapshotv1.VolumeSnapshotSpec{ Source: snapshotv1.VolumeSnapshotSource{ @@ -719,6 +734,7 @@ func (r *DataImportCronReconciler) handleSnapshot(ctx context.Context, dataImpor }, } r.setDataImportCronResourceLabels(dataImportCron, desiredSnapshot) + cc.CopyAllowedLabels(pvc.GetLabels(), desiredSnapshot, false) currentSnapshot := &snapshotv1.VolumeSnapshot{} if err := r.client.Get(ctx, client.ObjectKeyFromObject(desiredSnapshot), currentSnapshot); err != nil { @@ -1451,12 +1467,6 @@ func passCronAnnotationToDv(cron *cdiv1.DataImportCron, dv *cdiv1.DataVolume, an } } -func passCronLabelToDataSource(cron *cdiv1.DataImportCron, ds *cdiv1.DataSource, ann string) { - if val := cron.Labels[ann]; val != "" { - cc.AddLabel(ds, ann, val) - } -} - func (r *DataImportCronReconciler) newDataSource(cron *cdiv1.DataImportCron) *cdiv1.DataSource { dataSource := &cdiv1.DataSource{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/controller/dataimportcron-controller_test.go b/pkg/controller/dataimportcron-controller_test.go index 2152fbfaf1..49dd7aba56 100644 --- a/pkg/controller/dataimportcron-controller_test.go +++ b/pkg/controller/dataimportcron-controller_test.go @@ -894,7 +894,7 @@ var _ = Describe("All DataImportCron Tests", func() { Expect(err).ToNot(HaveOccurred()) }) - It("should pass through metadata to DataVolume and DataSource", func() { + It("should pass through metadata to DataVolume", func() { cron = newDataImportCron(cronName) cron.Annotations[AnnSourceDesiredDigest] = testDigest @@ -917,20 +917,13 @@ var _ = Describe("All DataImportCron Tests", func() { dvName := imports[0].DataVolumeName Expect(dvName).ToNot(BeEmpty()) - expectLabels := func(labels map[string]string) { - for _, defaultInstanceTypeLabel := range cc.DefaultInstanceTypeLabels { - ExpectWithOffset(1, labels).To(HaveKeyWithValue(defaultInstanceTypeLabel, defaultInstanceTypeLabel)) - } - ExpectWithOffset(1, labels).To(HaveKeyWithValue(cc.LabelDynamicCredentialSupport, "true")) - } - dv := &cdiv1.DataVolume{} Expect(reconciler.client.Get(context.TODO(), dvKey(dvName), dv)).To(Succeed()) - expectLabels(dv.Labels) - dataSource = &cdiv1.DataSource{} - Expect(reconciler.client.Get(context.TODO(), dataSourceKey(cron), dataSource)).To(Succeed()) - expectLabels(dataSource.Labels) + for _, defaultInstanceTypeLabel := range cc.DefaultInstanceTypeLabels { + Expect(dv.Labels).To(HaveKeyWithValue(defaultInstanceTypeLabel, defaultInstanceTypeLabel)) + } + Expect(dv.Labels).To(HaveKeyWithValue(cc.LabelDynamicCredentialSupport, "true")) }) var ( @@ -1049,7 +1042,7 @@ var _ = Describe("All DataImportCron Tests", func() { Expect(dv.Annotations[cc.AnnImmediateBinding]).To(Equal("true")) }) - It("Should create snapshot, and update DataImportCron and DataSource once its ready to use", func() { + It("Should create snapshot, and update DataImportCron once its ready to use", func() { cron = newDataImportCron(cronName) dataSource = nil retentionPolicy := cdiv1.DataImportCronRetainNone @@ -1078,7 +1071,9 @@ var _ = Describe("All DataImportCron Tests", func() { Expect(*dv.Spec.Source.Registry.URL).To(Equal(testRegistryURL + "@" + testDigest)) Expect(dv.Annotations[cc.AnnImmediateBinding]).To(Equal("true")) - pvc := cc.CreatePvc(dv.Name, dv.Namespace, nil, nil) + pvc := cc.CreatePvc(dv.Name, dv.Namespace, nil, map[string]string{ + testKubevirtIoKey: testKubevirtIoValue, + }) pvc.Spec.VolumeMode = ptr.To[corev1.PersistentVolumeMode]("dummy") err = reconciler.client.Create(context.TODO(), pvc) Expect(err).ToNot(HaveOccurred()) @@ -1126,6 +1121,8 @@ var _ = Describe("All DataImportCron Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(*snap.Status.ReadyToUse).To(BeTrue()) Expect(*snap.Spec.Source.PersistentVolumeClaimName).To(Equal(dvName)) + // Expect labels to be copied from the source PVC + Expect(snap.Labels).To(HaveKeyWithValue(testKubevirtIoKey, testKubevirtIoValue)) err = reconciler.client.Delete(context.TODO(), cron) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/controller/datasource-controller.go b/pkg/controller/datasource-controller.go index 3c19fc9a8b..cb8acd8dba 100644 --- a/pkg/controller/datasource-controller.go +++ b/pkg/controller/datasource-controller.go @@ -101,18 +101,21 @@ func (r *DataSourceReconciler) update(ctx context.Context, dataSource *cdiv1.Dat } func (r *DataSourceReconciler) handlePvcSource(ctx context.Context, sourcePVC *cdiv1.DataVolumeSourcePVC, dataSource *cdiv1.DataSource) error { - dv := &cdiv1.DataVolume{} ns := cc.GetNamespace(sourcePVC.Namespace, dataSource.Namespace) isReady := false + + pvc := &corev1.PersistentVolumeClaim{} + pvcErr := r.client.Get(ctx, types.NamespacedName{Namespace: ns, Name: sourcePVC.Name}, pvc) + if pvcErr != nil && !k8serrors.IsNotFound(pvcErr) { + return pvcErr + } + + dv := &cdiv1.DataVolume{} if err := r.client.Get(ctx, types.NamespacedName{Namespace: ns, Name: sourcePVC.Name}, dv); err != nil { if !k8serrors.IsNotFound(err) { return err } - pvc := &corev1.PersistentVolumeClaim{} - if err := r.client.Get(ctx, types.NamespacedName{Namespace: ns, Name: sourcePVC.Name}, pvc); err != nil { - if !k8serrors.IsNotFound(err) { - return err - } + if pvcErr != nil { r.log.Info("PVC not found", "name", sourcePVC.Name) updateDataSourceCondition(dataSource, cdiv1.DataSourceReady, corev1.ConditionFalse, "PVC not found", cc.NotFound) } else { @@ -123,7 +126,10 @@ func (r *DataSourceReconciler) handlePvcSource(ctx context.Context, sourcePVC *c } else { updateDataSourceCondition(dataSource, cdiv1.DataSourceReady, corev1.ConditionFalse, fmt.Sprintf("Import DataVolume phase %s", dv.Status.Phase), string(dv.Status.Phase)) } + if isReady { + cc.CopyAllowedLabels(dv.GetLabels(), dataSource, true) + cc.CopyAllowedLabels(pvc.GetLabels(), dataSource, true) updateDataSourceCondition(dataSource, cdiv1.DataSourceReady, corev1.ConditionTrue, "DataSource is ready to be consumed", ready) } @@ -140,6 +146,7 @@ func (r *DataSourceReconciler) handleSnapshotSource(ctx context.Context, sourceS r.log.Info("Snapshot not found", "name", sourceSnapshot.Name) updateDataSourceCondition(dataSource, cdiv1.DataSourceReady, corev1.ConditionFalse, "Snapshot not found", cc.NotFound) } else if cc.IsSnapshotReady(snapshot) { + cc.CopyAllowedLabels(snapshot.GetLabels(), dataSource, true) updateDataSourceCondition(dataSource, cdiv1.DataSourceReady, corev1.ConditionTrue, "DataSource is ready to be consumed", ready) } else { updateDataSourceCondition(dataSource, cdiv1.DataSourceReady, corev1.ConditionFalse, "Snapshot phase is not ready", "SnapshotNotReady") @@ -257,7 +264,8 @@ func addDataSourceControllerWatches(mgr manager.Manager, c controller.Controller DeleteFunc: func(e event.TypedDeleteEvent[*cdiv1.DataVolume]) bool { return true }, // Only DV status phase update is interesting to reconcile UpdateFunc: func(e event.TypedUpdateEvent[*cdiv1.DataVolume]) bool { - return e.ObjectOld.Status.Phase != e.ObjectNew.Status.Phase + return e.ObjectOld.Status.Phase != e.ObjectNew.Status.Phase || + !reflect.DeepEqual(e.ObjectOld.Labels, e.ObjectNew.Labels) }, }, )); err != nil { @@ -272,7 +280,8 @@ func addDataSourceControllerWatches(mgr manager.Manager, c controller.Controller CreateFunc: func(e event.TypedCreateEvent[*corev1.PersistentVolumeClaim]) bool { return true }, DeleteFunc: func(e event.TypedDeleteEvent[*corev1.PersistentVolumeClaim]) bool { return true }, UpdateFunc: func(e event.TypedUpdateEvent[*corev1.PersistentVolumeClaim]) bool { - return e.ObjectOld.Status.Phase != e.ObjectNew.Status.Phase + return e.ObjectOld.Status.Phase != e.ObjectNew.Status.Phase || + !reflect.DeepEqual(e.ObjectOld.Labels, e.ObjectNew.Labels) }, }, )); err != nil { @@ -296,7 +305,8 @@ func addDataSourceControllerWatches(mgr manager.Manager, c controller.Controller CreateFunc: func(e event.TypedCreateEvent[*snapshotv1.VolumeSnapshot]) bool { return true }, DeleteFunc: func(e event.TypedDeleteEvent[*snapshotv1.VolumeSnapshot]) bool { return true }, UpdateFunc: func(e event.TypedUpdateEvent[*snapshotv1.VolumeSnapshot]) bool { - return !reflect.DeepEqual(e.ObjectOld.Status, e.ObjectNew.Status) + return !reflect.DeepEqual(e.ObjectOld.Status, e.ObjectNew.Status) || + !reflect.DeepEqual(e.ObjectOld.Labels, e.ObjectNew.Labels) }, }, )); err != nil { diff --git a/pkg/controller/datasource-controller_test.go b/pkg/controller/datasource-controller_test.go index 4298345893..c3d0d6215f 100644 --- a/pkg/controller/datasource-controller_test.go +++ b/pkg/controller/datasource-controller_test.go @@ -41,6 +41,13 @@ const ( dsName = "test-datasource" pvcName = "test-pvc" snapshotName = "test-snapshot" + + testKubevirtIoKey = "test.kubevirt.io/test" + testKubevirtIoValue = "testvalue" + testInstancetypeKubevirtIoKey = "instancetype.kubevirt.io/default-preference" + testInstancetypeKubevirtIoValue = "testpreference" + testKubevirtIoKeyExisting = "test.kubevirt.io/existing" + testKubevirtIoNewValueExisting = "newvalue" ) var _ = Describe("All DataSource Tests", func() { @@ -139,6 +146,68 @@ var _ = Describe("All DataSource Tests", func() { Expect(err).ToNot(HaveOccurred()) verifyConditions("Source snapshot Deleted", false, NotFound) }) + + DescribeTable("Should copy labels to DataSource", func(source cdiv1.DataSourceSource, createSource func() error) { + ds = createDataSource() + ds.Spec.Source = source + reconciler = createDataSourceReconciler(ds) + verifyConditions("Source does not exist", false, NotFound) + + Expect(createSource()).To(Succeed()) + verifyConditions("DataSource is ready to be consumed", true, ready) + + err := reconciler.client.Get(context.TODO(), dsKey, ds) + Expect(err).ToNot(HaveOccurred()) + Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKey, testKubevirtIoValue)) + Expect(ds.Labels).To(HaveKeyWithValue(testInstancetypeKubevirtIoKey, testInstancetypeKubevirtIoValue)) + Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKeyExisting, testKubevirtIoNewValueExisting)) + }, + Entry("from DataVolume", + cdiv1.DataSourceSource{PVC: &cdiv1.DataVolumeSourcePVC{Namespace: metav1.NamespaceDefault, Name: pvcName}}, + func() error { + dv := NewImportDataVolume(pvcName) + dv.Status.Phase = cdiv1.Succeeded + dv.Labels = map[string]string{ + testKubevirtIoKey: testKubevirtIoValue, + testInstancetypeKubevirtIoKey: testInstancetypeKubevirtIoValue, + testKubevirtIoKeyExisting: testKubevirtIoNewValueExisting, + } + return reconciler.client.Create(context.TODO(), dv) + }, + ), + Entry("from PersistentVolumeClaim", + cdiv1.DataSourceSource{PVC: &cdiv1.DataVolumeSourcePVC{Namespace: metav1.NamespaceDefault, Name: pvcName}}, + func() error { + pvc := CreatePvc(pvcName, metav1.NamespaceDefault, nil, map[string]string{ + testKubevirtIoKey: testKubevirtIoValue, + testInstancetypeKubevirtIoKey: testInstancetypeKubevirtIoValue, + testKubevirtIoKeyExisting: testKubevirtIoNewValueExisting, + }) + return reconciler.client.Create(context.TODO(), pvc) + }, + ), + Entry("from VolumeSnapshot", + cdiv1.DataSourceSource{Snapshot: &cdiv1.DataVolumeSourceSnapshot{Namespace: metav1.NamespaceDefault, Name: snapshotName}}, + func() error { + snap := &snapshotv1.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: snapshotName, + Namespace: metav1.NamespaceDefault, + Labels: map[string]string{ + testKubevirtIoKey: testKubevirtIoValue, + testInstancetypeKubevirtIoKey: testInstancetypeKubevirtIoValue, + testKubevirtIoKeyExisting: testKubevirtIoNewValueExisting, + }, + }, + Spec: snapshotv1.VolumeSnapshotSpec{}, + Status: &snapshotv1.VolumeSnapshotStatus{ + ReadyToUse: ptr.To[bool](true), + }, + } + return reconciler.client.Create(context.TODO(), snap) + }, + ), + ) }) }) @@ -161,6 +230,9 @@ func createDataSource() *cdiv1.DataSource { ObjectMeta: metav1.ObjectMeta{ Name: dsName, Namespace: metav1.NamespaceDefault, + Labels: map[string]string{ + testKubevirtIoKeyExisting: "existing", + }, }, } } diff --git a/pkg/controller/populators/import-populator.go b/pkg/controller/populators/import-populator.go index 4c98484ecc..09f0403905 100644 --- a/pkg/controller/populators/import-populator.go +++ b/pkg/controller/populators/import-populator.go @@ -160,20 +160,20 @@ func (r *ImportPopulatorReconciler) reconcileTargetPVC(pvc, pvcPrime *corev1.Per } if cc.IsPVCComplete(pvcPrime) && cc.IsUnbound(pvc) { - // Once the import is succeeded, we rebind the PV from PVC to target PVC - if err := cc.Rebind(context.TODO(), r.client, pvcPrime, pvc); err != nil { + // Once the import is succeeded, we copy labels and rebind the PV from PVC to target PVC + if pvcCopy, err = r.updatePVCWithPVCPrimeLabels(pvcCopy, pvcPrime.GetLabels()); err != nil { + return reconcile.Result{}, err + } + if err := cc.Rebind(context.TODO(), r.client, pvcPrime, pvcCopy); err != nil { return reconcile.Result{}, err } } } - if pvcCopy, err = r.updatePVCWithPVCPrimeAnnotations(pvcCopy, pvcPrime, r.updateImportAnnotations); err != nil { + if _, err = r.updatePVCWithPVCPrimeAnnotations(pvcCopy, pvcPrime, r.updateImportAnnotations); err != nil { return reconcile.Result{}, err } if cc.IsPVCComplete(pvcPrime) && !cc.IsMultiStageImportInProgress(pvc) { - if err = r.updatePVCWithPVCPrimeLabels(pvcCopy, pvcPrime.GetLabels()); err != nil { - return reconcile.Result{}, err - } r.recorder.Eventf(pvc, corev1.EventTypeNormal, importSucceeded, messageImportSucceeded, pvc.Name) } diff --git a/pkg/controller/populators/populator-base.go b/pkg/controller/populators/populator-base.go index db6a497fad..f5ba10d0b6 100644 --- a/pkg/controller/populators/populator-base.go +++ b/pkg/controller/populators/populator-base.go @@ -19,7 +19,6 @@ package populators import ( "context" "reflect" - "regexp" "github.com/go-logr/logr" @@ -63,10 +62,6 @@ const ( uidField = "metadata.uid" ) -var ( - validLabelsMatch = regexp.MustCompile(`^([\w.]+\.kubevirt.io|kubevirt.io)/[\w-]+$`) -) - // Interface to store populator-specific methods type populatorController interface { // Returns the specific populator CR @@ -254,19 +249,15 @@ func (r *ReconcilerBase) updatePVCWithPVCPrimeAnnotations(pvc, pvcPrime *corev1. return pvcCopy, nil } -func (r *ReconcilerBase) updatePVCWithPVCPrimeLabels(pvc *corev1.PersistentVolumeClaim, pvcPrimeLabels map[string]string) error { +func (r *ReconcilerBase) updatePVCWithPVCPrimeLabels(pvc *corev1.PersistentVolumeClaim, pvcPrimeLabels map[string]string) (*corev1.PersistentVolumeClaim, error) { pvcCopy := pvc.DeepCopy() - for label, value := range pvcPrimeLabels { - if _, found := pvcCopy.GetLabels()[label]; !found && validLabelsMatch.MatchString(label) { - cc.AddLabel(pvcCopy, label, value) - } - } + cc.CopyAllowedLabels(pvcPrimeLabels, pvcCopy, false) if !reflect.DeepEqual(pvc.ObjectMeta, pvcCopy.ObjectMeta) { if err := r.client.Update(context.TODO(), pvcCopy); err != nil { - return err + return nil, err } } - return nil + return pvcCopy, nil } // reconcile functions diff --git a/tests/dataimportcron_test.go b/tests/dataimportcron_test.go index 982b8f7b47..ebc3ecf622 100644 --- a/tests/dataimportcron_test.go +++ b/tests/dataimportcron_test.go @@ -36,6 +36,8 @@ const ( importsToKeep = 1 emptySchedule = "" errorDigest = "sha256:12345678900987654321" + testKubevirtIoKey = "test.kubevirt.io/test" + testKubevirtIoValue = "testvalue" ) var _ = Describe("DataImportCron", Serial, func() { @@ -154,6 +156,7 @@ var _ = Describe("DataImportCron", Serial, func() { }, } snapshot = utils.WaitSnapshotReady(f.CrClient, snapshot) + Expect(snapshot.Labels).To(HaveKeyWithValue(testKubevirtIoKey, testKubevirtIoValue)) deleted, err := utils.WaitPVCDeleted(f.K8sClient, name, ns, 30*time.Second) if err != nil { // work around https://github.com/kubernetes-csi/external-snapshotter/issues/957 @@ -325,16 +328,15 @@ var _ = Describe("DataImportCron", Serial, func() { By("Verify DataSource was updated") var dataSource *cdiv1.DataSource - Eventually(func() bool { + Eventually(func(g Gomega) { dataSource, err = f.CdiClient.CdiV1beta1().DataSources(ns).Get(context.TODO(), cron.Spec.ManagedDataSource, metav1.GetOptions{}) - if errors.IsNotFound(err) { - return false - } - Expect(err).ToNot(HaveOccurred()) + g.Expect(err).ToNot(HaveOccurred()) readyCond := controller.FindDataSourceConditionByType(dataSource, cdiv1.DataSourceReady) - return readyCond != nil && readyCond.Status == corev1.ConditionTrue && - getDataSourceName(format, dataSource) == currentImportDv - }, dataImportCronTimeout, pollingInterval).Should(BeTrue(), "DataSource was not updated") + g.Expect(readyCond).ToNot(BeNil()) + g.Expect(readyCond.Status).To(Equal(corev1.ConditionTrue)) + g.Expect(getDataSourceName(format, dataSource)).To(Equal(currentImportDv)) + g.Expect(dataSource.Labels).To(HaveKeyWithValue(testKubevirtIoKey, testKubevirtIoValue)) + }, dataImportCronTimeout, pollingInterval).Should(Succeed(), "DataSource was not updated") By("Verify cron was updated") Expect(cron.Status.LastImportedPVC).ToNot(BeNil()) diff --git a/tests/datasource_test.go b/tests/datasource_test.go index ef47fce89c..7babe6ac26 100644 --- a/tests/datasource_test.go +++ b/tests/datasource_test.go @@ -12,6 +12,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1" "kubevirt.io/containerized-data-importer/pkg/controller" @@ -29,6 +30,13 @@ var _ = Describe("DataSource", func() { pvc2Name = "pvc2" snap1Name = "snap1" snap2Name = "snap2" + + testKubevirtIoKey = "test.kubevirt.io/test" + testKubevirtIoValue = "testvalue" + testInstancetypeKubevirtIoKey = "instancetype.kubevirt.io/default-preference" + testInstancetypeKubevirtIoValue = "testpreference" + testKubevirtIoKeyExisting = "test.kubevirt.io/existing" + testKubevirtIoNewValueExisting = "newvalue" ) f := framework.NewFramework("datasource-func-test") @@ -59,9 +67,10 @@ var _ = Describe("DataSource", func() { } testURL := func() string { return fmt.Sprintf(utils.TinyCoreQcow2URL, f.CdiInstallNs) } - createDv := func(pvcName, url string) { + createDv := func(pvcName, url string, labels map[string]string) { By(fmt.Sprintf("creating DataVolume %s %s", pvcName, url)) dv := utils.NewDataVolumeWithHTTPImport(pvcName, "1Gi", url) + dv.Labels = labels dv, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) Expect(err).ToNot(HaveOccurred()) By("verifying pvc was created") @@ -70,6 +79,22 @@ var _ = Describe("DataSource", func() { f.ForceBindIfWaitForFirstConsumer(pvc) } + createSnap := func(name string, labels map[string]string) *snapshotv1.VolumeSnapshot { + pvcDef := utils.NewPVCDefinition("snap-source-pvc", "1Gi", nil, nil) + pvcDef.Namespace = f.Namespace.Name + pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Create(context.TODO(), pvcDef, metav1.CreateOptions{}) + Expect(err).ToNot(HaveOccurred()) + f.ForceBindIfWaitForFirstConsumer(pvc) + + snapClass := f.GetSnapshotClass() + snapshot := utils.NewVolumeSnapshot(name, pvc.Namespace, pvc.Name, &snapClass.Name) + snapshot.Labels = labels + err = f.CrClient.Create(context.TODO(), snapshot) + Expect(err).ToNot(HaveOccurred()) + + return snapshot + } + It("[test_id:8041]status conditions should be updated on pvc create/update/delete", func() { By("Create DataSource with no source PVC") ds := newDataSource(ds1Name) @@ -98,7 +123,7 @@ var _ = Describe("DataSource", func() { f.ExpectEvent(dv.Namespace).Should(ContainSubstring(dvc.CloneWithoutSource)) By("Create import DV so the missing DataSource source PVC will be ready") - createDv(pvc1Name, testURL()) + createDv(pvc1Name, testURL(), nil) ds = waitForReadyCondition(ds, corev1.ConditionTrue, "Ready") By("Wait for the clone DV success") @@ -114,6 +139,88 @@ var _ = Describe("DataSource", func() { _ = waitForReadyCondition(ds, corev1.ConditionFalse, "NoSource") }) + DescribeTable("[test_id:TODO] Labels should be copied to DataSource", func(sourceFn func() cdiv1.DataSourceSource, createSource func()) { + By("Create source for DataSource") + createSource() + + By("Create DataSource") + ds := newDataSource(ds1Name) + ds.Spec.Source = sourceFn() + ds, err := f.CdiClient.CdiV1beta1().DataSources(ds.Namespace).Create(context.TODO(), ds, metav1.CreateOptions{}) + Expect(err).ToNot(HaveOccurred()) + ds = waitForReadyCondition(ds, corev1.ConditionTrue, "Ready") + + By("Check for labels on DataSource") + Eventually(func(g Gomega) { + ds, err := f.CdiClient.CdiV1beta1().DataSources(ds.Namespace).Get(context.TODO(), ds.Name, metav1.GetOptions{}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKey, testKubevirtIoValue)) + g.Expect(ds.Labels).To(HaveKeyWithValue(testInstancetypeKubevirtIoKey, testInstancetypeKubevirtIoValue)) + g.Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKeyExisting, testKubevirtIoNewValueExisting)) + }, 60*time.Second, pollingInterval).Should(Succeed()) + }, + Entry("from DataVolume", + func() cdiv1.DataSourceSource { + return cdiv1.DataSourceSource{ + PVC: &cdiv1.DataVolumeSourcePVC{Namespace: f.Namespace.Name, Name: pvc1Name}, + } + }, + func() { + createDv(pvc1Name, testURL(), map[string]string{ + testKubevirtIoKey: testKubevirtIoValue, + testInstancetypeKubevirtIoKey: testInstancetypeKubevirtIoValue, + testKubevirtIoKeyExisting: testKubevirtIoNewValueExisting, + }) + }, + ), + Entry("from PersistentVolumeClaim", + func() cdiv1.DataSourceSource { + return cdiv1.DataSourceSource{ + PVC: &cdiv1.DataVolumeSourcePVC{Namespace: f.Namespace.Name, Name: pvc1Name}, + } + }, + func() { + source := utils.NewVolumeImportSourceWithURLImport(pvc1Name, testURL()) + source, err := f.CdiClient.CdiV1beta1().VolumeImportSources(f.Namespace.Name).Create(context.TODO(), source, metav1.CreateOptions{}) + Expect(err).ToNot(HaveOccurred()) + + pvc := utils.NewPVCDefinition(pvc1Name, "1Gi", nil, map[string]string{ + testKubevirtIoKey: testKubevirtIoValue, + testInstancetypeKubevirtIoKey: testInstancetypeKubevirtIoValue, + testKubevirtIoKeyExisting: testKubevirtIoNewValueExisting, + }) + pvc.Spec.DataSourceRef = &corev1.TypedObjectReference{ + APIGroup: ptr.To(cc.AnnAPIGroup), + Kind: cdiv1.VolumeImportSourceRef, + Name: source.Name, + } + pvc, err = utils.CreatePVCFromDefinition(f.K8sClient, f.Namespace.Name, pvc) + Expect(err).ToNot(HaveOccurred()) + + f.ForceBindIfWaitForFirstConsumer(pvc) + err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, corev1.ClaimBound, pvc.Name) + Expect(err).ToNot(HaveOccurred()) + }, + ), + Entry("from VolumeSnapshot", + func() cdiv1.DataSourceSource { + return cdiv1.DataSourceSource{ + Snapshot: &cdiv1.DataVolumeSourceSnapshot{Namespace: f.Namespace.Name, Name: snap1Name}, + } + }, + func() { + if !f.IsSnapshotStorageClassAvailable() { + Skip("Clone from volumesnapshot does not work without snapshot capable storage") + } + createSnap(snap1Name, map[string]string{ + testKubevirtIoKey: testKubevirtIoValue, + testInstancetypeKubevirtIoKey: testInstancetypeKubevirtIoValue, + testKubevirtIoKeyExisting: testKubevirtIoNewValueExisting, + }) + }, + ), + ) + createDs := func(dsName, pvcName string) *cdiv1.DataSource { By(fmt.Sprintf("creating DataSource %s -> %s", dsName, pvcName)) ds := newDataSource(dsName) @@ -131,7 +238,7 @@ var _ = Describe("DataSource", func() { } It("[test_id:8067]status conditions should be updated when several DataSources refer the same pvc", func() { - createDv(pvc1Name, testURL()) + createDv(pvc1Name, testURL(), nil) ds1 := createDs(ds1Name, pvc1Name) ds2 := createDs(ds2Name, pvc1Name) @@ -142,7 +249,7 @@ var _ = Describe("DataSource", func() { ds1 = waitForReadyCondition(ds1, corev1.ConditionFalse, "NotFound") ds2 = waitForReadyCondition(ds2, corev1.ConditionFalse, "NotFound") - createDv(pvc2Name, testURL()+"bad") + createDv(pvc2Name, testURL()+"bad", nil) updateDsPvc(ds1, pvc2Name) updateDsPvc(ds2, pvc2Name) ds1 = waitForReadyCondition(ds1, corev1.ConditionFalse, "ImportInProgress") @@ -154,14 +261,14 @@ var _ = Describe("DataSource", func() { }) It("status conditions timestamp should be updated when DataSource referred pvc is updated, although condition status does not change", func() { - createDv(pvc1Name, testURL()) + createDv(pvc1Name, testURL(), nil) ds := createDs(ds1Name, pvc1Name) ds = waitForReadyCondition(ds, corev1.ConditionTrue, "Ready") cond := controller.FindDataSourceConditionByType(ds, cdiv1.DataSourceReady) Expect(cond).ToNot(BeNil()) ts := cond.LastTransitionTime - createDv(pvc2Name, testURL()) + createDv(pvc2Name, testURL(), nil) err := utils.WaitForDataVolumePhase(f, f.Namespace.Name, cdiv1.Succeeded, pvc2Name) Expect(err).ToNot(HaveOccurred()) updateDsPvc(ds, pvc2Name) @@ -177,21 +284,6 @@ var _ = Describe("DataSource", func() { }) Context("snapshot source", func() { - createSnap := func(name string) *snapshotv1.VolumeSnapshot { - pvcDef := utils.NewPVCDefinition("snap-source-pvc", "1Gi", nil, nil) - pvcDef.Namespace = f.Namespace.Name - pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Create(context.TODO(), pvcDef, metav1.CreateOptions{}) - Expect(err).ToNot(HaveOccurred()) - f.ForceBindIfWaitForFirstConsumer(pvc) - - snapClass := f.GetSnapshotClass() - snapshot := utils.NewVolumeSnapshot(name, pvc.Namespace, pvc.Name, &snapClass.Name) - err = f.CrClient.Create(context.TODO(), snapshot) - Expect(err).ToNot(HaveOccurred()) - - return snapshot - } - createSnapDs := func(dsName, snapName string) *cdiv1.DataSource { By(fmt.Sprintf("creating DataSource %s -> %s", dsName, snapName)) ds := newDataSource(dsName) @@ -235,7 +327,7 @@ var _ = Describe("DataSource", func() { f.ExpectEvent(dv.Namespace).Should(ContainSubstring(dvc.CloneWithoutSource)) By("Create snapshot so the DataSource will be ready") - snapshot := createSnap(snap1Name) + snapshot := createSnap(snap1Name, nil) ds = waitForReadyCondition(ds, corev1.ConditionTrue, "Ready") By("Wait for the clone DV success") @@ -253,7 +345,7 @@ var _ = Describe("DataSource", func() { }) It("[test_id:9763] status conditions should be updated when several DataSources refer the same snapshot", func() { - snapshot := createSnap(snap1Name) + snapshot := createSnap(snap1Name, nil) ds1 := createSnapDs(ds1Name, snap1Name) ds2 := createSnapDs(ds2Name, snap1Name) diff --git a/tests/utils/populators.go b/tests/utils/populators.go index e18fc06acb..b64daebacb 100644 --- a/tests/utils/populators.go +++ b/tests/utils/populators.go @@ -31,3 +31,19 @@ func NewVolumeImportSourceWithVddkWarmImport(name, pvcName, backingFile, secretR }, } } + +// NewVolumeImportSourceWithUrlImport initializes a VolumeImportSource for a HTTP import from a given URL +func NewVolumeImportSourceWithURLImport(name, url string) *cdiv1.VolumeImportSource { + return &cdiv1.VolumeImportSource{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: cdiv1.VolumeImportSourceSpec{ + Source: &cdiv1.ImportSourceType{ + HTTP: &cdiv1.DataVolumeSourceHTTP{ + URL: url, + }, + }, + }, + } +}