Skip to content

Commit

Permalink
feat: Copy labels from source to DataSource (#3377)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

---------

Signed-off-by: Felix Matouschek <[email protected]>
  • Loading branch information
@0xFelix
0xFelix authored Aug 16, 2024
1 parent c15ad1d commit f357368
Show file tree
Hide file tree
Showing 11 changed files with 348 additions and 98 deletions.
12 changes: 12 additions & 0 deletions pkg/controller/common/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,8 @@ var (
AnnPriorityClassName: "",
AnnPodMultusDefaultNetwork: "",
}

validLabelsMatch = regexp.MustCompile(`^([\w.]+\.kubevirt.io|kubevirt.io)/[\w-]+$`)
)

// FakeValidator is a fake token validator
Expand Down Expand Up @@ -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) {
Expand Down
48 changes: 48 additions & 0 deletions pkg/controller/common/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
60 changes: 35 additions & 25 deletions pkg/controller/dataimportcron-controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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{
Expand All @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down
25 changes: 11 additions & 14 deletions pkg/controller/dataimportcron-controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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 (
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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())
Expand Down
28 changes: 19 additions & 9 deletions pkg/controller/datasource-controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
}

Expand All @@ -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")
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
Loading

0 comments on commit f357368

Please sign in to comment.