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, + }, + }, + }, + } +}