Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass DV labels to PVC #2547

Merged
merged 6 commits into from
Mar 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 3 additions & 16 deletions pkg/controller/datavolume/controller-base.go
Original file line number Diff line number Diff line change
Expand Up @@ -960,21 +960,6 @@ func buildHTTPClient() *http.Client {
return httpClient
}

func passDataVolumeInstancetypeLabelstoPVC(dataVolumeLabels, pvcLabels map[string]string) map[string]string {
instancetypeLabels := []string{
cc.LabelDefaultInstancetype,
cc.LabelDefaultInstancetypeKind,
cc.LabelDefaultPreference,
cc.LabelDefaultPreferenceKind,
}
for _, label := range instancetypeLabels {
if dvLabel, hasLabel := dataVolumeLabels[label]; hasLabel {
pvcLabels[label] = dvLabel
}
}
return pvcLabels
}

// newPersistentVolumeClaim creates a new PVC for the DataVolume resource.
// It also sets the appropriate OwnerReferences on the resource
// which allows handleObject to discover the DataVolume resource
Expand All @@ -986,7 +971,9 @@ func (r *ReconcilerBase) newPersistentVolumeClaim(dataVolume *cdiv1.DataVolume,
if util.ResolveVolumeMode(targetPvcSpec.VolumeMode) == corev1.PersistentVolumeFilesystem {
labels[common.KubePersistentVolumeFillingUpSuppressLabelKey] = common.KubePersistentVolumeFillingUpSuppressLabelValue
}
labels = passDataVolumeInstancetypeLabelstoPVC(dataVolume.GetLabels(), labels)
for k, v := range dataVolume.Labels {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that not all flows we have are using newPersistentVolumeClaim
One example I could find is smart clone which creates the target PVC in

target := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: snapshot.Name,
Namespace: snapshot.Namespace,
Labels: labels,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Alex, there is no DV in that function and the PVC is created from a VolumeSnapshot, so I don't think labels should be passed in this case. wdyt ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a way to grab/pass the associated DV to this func

dataVolume, err := r.getDataVolume(snapshot)

labels[k] = v
}

annotations := make(map[string]string)
for k, v := range dataVolume.ObjectMeta.Annotations {
Expand Down
5 changes: 4 additions & 1 deletion pkg/controller/datavolume/import-controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -464,14 +464,16 @@ var _ = Describe("All DataVolume Tests", func() {
Expect(pvc.Spec.Resources.Requests.Storage().Value()).To(Equal(expectedSize.Value()))
})

It("Should pass annotation from DV to created a PVC on a DV", func() {
It("Should pass annotations and labels from DV to created PVC", func() {
dv := NewImportDataVolume("test-dv")
dv.SetAnnotations(make(map[string]string))
dv.GetAnnotations()["test-ann-1"] = "test-value-1"
dv.GetAnnotations()["test-ann-2"] = "test-value-2"
dv.GetAnnotations()[AnnSource] = "invalid phase should not copy"
dv.GetAnnotations()[AnnPodNetwork] = "data-network"
dv.GetAnnotations()[AnnPodSidecarInjection] = "false"
dv.SetLabels(make(map[string]string))
dv.GetLabels()["test"] = "test-label"
reconciler = createImportReconciler(dv)
_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
Expect(err).ToNot(HaveOccurred())
Expand All @@ -486,6 +488,7 @@ var _ = Describe("All DataVolume Tests", func() {
Expect(pvc.GetAnnotations()[AnnPodNetwork]).To(Equal("data-network"))
Expect(pvc.GetAnnotations()[AnnPodSidecarInjection]).To(Equal("false"))
Expect(pvc.GetAnnotations()[AnnPriorityClassName]).To(Equal("p0"))
Expect(pvc.Labels["test"]).To(Equal("test-label"))
})

It("Should pass annotation from DV with S3 source to created a PVC on a DV", func() {
Expand Down
34 changes: 22 additions & 12 deletions pkg/controller/datavolume/smart-clone-controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ func (r *SmartCloneReconciler) reconcileSnapshot(log logr.Logger, snapshot *snap
if err != nil {
return reconcile.Result{}, err
}
newPvc, err := newPvcFromSnapshot(snapshot.Name, snapshot, targetPvcSpec)

newPvc, err := newPvcFromSnapshot(dataVolume, snapshot.Name, snapshot, targetPvcSpec)
if err != nil {
return reconcile.Result{}, err
}
Expand Down Expand Up @@ -352,7 +353,7 @@ func (r *SmartCloneReconciler) getTargetPVC(dataVolume *cdiv1.DataVolume) (*core
return pvc, nil
}

func newPvcFromSnapshot(name string, snapshot *snapshotv1.VolumeSnapshot, targetPvcSpec *corev1.PersistentVolumeClaimSpec) (*corev1.PersistentVolumeClaim, error) {
func newPvcFromSnapshot(dv *cdiv1.DataVolume, name string, snapshot *snapshotv1.VolumeSnapshot, targetPvcSpec *corev1.PersistentVolumeClaimSpec) (*corev1.PersistentVolumeClaim, error) {
targetPvcSpecCopy := targetPvcSpec.DeepCopy()
restoreSize := snapshot.Status.RestoreSize
if restoreSize == nil {
Expand All @@ -369,22 +370,31 @@ func newPvcFromSnapshot(name string, snapshot *snapshotv1.VolumeSnapshot, target
common.CDILabelKey: common.CDILabelValue,
common.CDIComponentLabel: common.SmartClonerCDILabel,
}
for k, v := range dv.Labels {
labels[k] = v
}

annotations := map[string]string{
AnnSmartCloneRequest: "true",
cc.AnnRunningCondition: string(corev1.ConditionFalse),
cc.AnnRunningConditionMessage: cc.CloneComplete,
cc.AnnRunningConditionReason: "Completed",
annSmartCloneSnapshot: key,
}
for k, v := range dv.ObjectMeta.Annotations {
annotations[k] = v
}

if util.ResolveVolumeMode(targetPvcSpecCopy.VolumeMode) == corev1.PersistentVolumeFilesystem {
labels[common.KubePersistentVolumeFillingUpSuppressLabelKey] = common.KubePersistentVolumeFillingUpSuppressLabelValue
}

target := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: snapshot.Namespace,
Labels: labels,
Annotations: map[string]string{
AnnSmartCloneRequest: "true",
cc.AnnRunningCondition: string(corev1.ConditionFalse),
cc.AnnRunningConditionMessage: cc.CloneComplete,
cc.AnnRunningConditionReason: "Completed",
annSmartCloneSnapshot: key,
},
Name: name,
Namespace: snapshot.Namespace,
Labels: labels,
Annotations: annotations,
},
Spec: corev1.PersistentVolumeClaimSpec{
DataSource: &corev1.TypedLocalObjectReference{
Expand Down
9 changes: 6 additions & 3 deletions pkg/controller/datavolume/smart-clone-controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,10 @@ var _ = Describe("All smart clone tests", func() {
It("Should create PVC if snapshot ready", func() {
dv := newCloneDataVolume("test-dv")
q, _ := resource.ParseQuantity("500Mi")
// Set annotation on DV which we can verify on PVC later
dv.GetAnnotations()["test"] = "test-value"
// Set annotation and label on DV which we can verify on PVC later
dv.Annotations["test"] = "test-value"
dv.Labels = map[string]string{"test": "test-label"}

snapshot := createSnapshotVolume(dv.Name, dv.Namespace, nil)
snapshot.Spec.Source = snapshotv1.VolumeSnapshotSource{
PersistentVolumeClaimName: &[]string{"source"}[0],
Expand All @@ -234,8 +236,9 @@ var _ = Describe("All smart clone tests", func() {
Expect(err).ToNot(HaveOccurred())
Expect(pvc.Labels[common.AppKubernetesVersionLabel]).To(Equal("v0.0.0-tests"))
Expect(pvc.Labels[common.KubePersistentVolumeFillingUpSuppressLabelKey]).To(Equal(common.KubePersistentVolumeFillingUpSuppressLabelValue))
Expect(pvc.Labels["test"]).To(Equal("test-label"))
// Verify PVC's annotation
Expect(pvc.GetAnnotations()["test"]).To(Equal("test-value"))
Expect(pvc.Annotations["test"]).To(Equal("test-value"))
event := <-reconciler.recorder.(*record.FakeRecorder).Events
Expect(event).To(ContainSubstring("Creating PVC for smart-clone is in progress"))
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/datavolume/snapshot-clone-controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ func (r *SnapshotCloneReconciler) getStorageClassCorrespondingToSnapClass(driver
}

func (r *SnapshotCloneReconciler) makePvcFromSnapshot(pvcName string, dv *cdiv1.DataVolume, snapshot *snapshotv1.VolumeSnapshot, targetPvcSpec *corev1.PersistentVolumeClaimSpec) (*corev1.PersistentVolumeClaim, error) {
newPvc, err := newPvcFromSnapshot(pvcName, snapshot, targetPvcSpec)
newPvc, err := newPvcFromSnapshot(dv, pvcName, snapshot, targetPvcSpec)
if err != nil {
return nil, err
}
Expand Down
20 changes: 20 additions & 0 deletions tests/cloner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,13 @@ var _ = Describe("all clone tests", func() {
DescribeTable("[test_id:1355]Should clone data across different namespaces", func(targetSize string) {
pvcDef := utils.NewPVCDefinition(sourcePVCName, "1Gi", nil, nil)
pvcDef.Namespace = f.Namespace.Name

sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile+"; chmod 660 "+testBaseDir+testFile)
targetNs, err := f.CreateNamespace(f.NsPrefix, map[string]string{
framework.NsPrefixLabel: f.NsPrefix,
})
Expect(err).NotTo(HaveOccurred())

f.AddNamespaceToDelete(targetNs)
doFileBasedCloneTest(f, pvcDef, targetNs, "target-dv", targetSize)
},
Expand Down Expand Up @@ -2654,6 +2656,8 @@ var _ = Describe("all clone tests", func() {

for i = 0; i < repeat; i++ {
dataVolume := utils.NewDataVolumeForSnapshotCloningAndStorageSpec(fmt.Sprintf("clone-from-snap-%d", i), size, snapshot.Namespace, snapshot.Name, nil, &volumeMode)
dataVolume.Labels = map[string]string{"test-label-1": "test-label-value-1"}
dataVolume.Annotations = map[string]string{"test-annotation-1": "test-annotation-value-1"}
By(fmt.Sprintf("Create new datavolume %s which will clone from volumesnapshot", dataVolume.Name))
dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, targetNs.Name, dataVolume)
Expect(err).ToNot(HaveOccurred())
Expand All @@ -2671,6 +2675,9 @@ var _ = Describe("all clone tests", func() {
Expect(ok).To(BeFalse())
Expect(pvc.Spec.DataSource.Kind).To(Equal("VolumeSnapshot"))
Expect(pvc.Spec.DataSourceRef.Kind).To(Equal("VolumeSnapshot"))
// All labels and annotations passed
Expect(pvc.Labels["test-label-1"]).To(Equal("test-label-value-1"))
Expect(pvc.Annotations["test-annotation-1"]).To(Equal("test-annotation-value-1"))
}

By("Verify MD5 on one of the DVs")
Expand Down Expand Up @@ -2868,6 +2875,15 @@ func doFileBasedCloneTest(f *framework.Framework, srcPVCDef *v1.PersistentVolume
}
// Create targetPvc in new NS.
targetDV := utils.NewCloningDataVolume(targetDv, targetSize[0], srcPVCDef)
if targetDV.GetLabels() == nil {
targetDV.SetLabels(make(map[string]string))
}
if targetDV.GetAnnotations() == nil {
targetDV.SetAnnotations(make(map[string]string))
}
targetDV.Labels["test-label-1"] = "test-label-key-1"
targetDV.Annotations["test-annotation-1"] = "test-annotation-key-1"

dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, targetNs.Name, targetDV)
Expect(err).ToNot(HaveOccurred())

Expand All @@ -2888,6 +2904,10 @@ func doFileBasedCloneTest(f *framework.Framework, srcPVCDef *v1.PersistentVolume

es := resource.MustParse(targetSize[0])
Expect(es.Cmp(*targetPvc.Status.Capacity.Storage()) <= 0).To(BeTrue())

// All labels and annotations passed
Expect(targetPvc.Labels["test-label-1"]).To(Equal("test-label-key-1"))
Expect(targetPvc.Annotations["test-annotation-1"]).To(Equal("test-annotation-key-1"))
}

func doInUseCloneTest(f *framework.Framework, srcPVCDef *v1.PersistentVolumeClaim, targetNs *v1.Namespace, targetDv string) {
Expand Down
25 changes: 25 additions & 0 deletions tests/datavolume_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,31 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
table.Entry("[test_id:8045]for clone DataVolume", createCloneDataVolume, fillCommand),
)

table.DescribeTable("Should pass all DV labels and annotations to the PVC", func(dvFunc func(string, string, string) *cdiv1.DataVolume, url string) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! one note, we also have:

  • CSI clone
  • Cloning from snapshot source

that will not be covered by the clone test here
maybe we can add a check to some existing tests?

DescribeTable("[test_id:1355]Should clone data across different namespaces", func(targetSize string) {

DescribeTable("Should sucessfully clone without falling back to host assisted", func(volumeMode v1.PersistentVolumeMode, repeat int, crossNamespace bool) {

Sorry for being a pain about this, it's just that when this PR merges we
commit to passing the labels

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Alex. I tried to fix it, hope it's ok

dataVolume := dvFunc("pass-all-labels-dv", "1Gi", url)
dataVolume.Labels = map[string]string{"test-label-1": "test-label-1", "test-label-2": "test-label-2"}
dataVolume.Annotations = map[string]string{"test-annotation-1": "test-annotation-1", "test-annotation-2": "test-annotation-2"}

By(fmt.Sprintf("creating new datavolume %s", dataVolume.Name))
dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume)
Expect(err).ToNot(HaveOccurred())

// verify PVC was created
By("verifying pvc was created and is Bound")
pvc, err := utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name)
Expect(err).ToNot(HaveOccurred())

// All labels and annotations passed
Expect(pvc.Labels["test-label-1"]).To(Equal("test-label-1"))
Expect(pvc.Labels["test-label-2"]).To(Equal("test-label-2"))
Expect(pvc.Annotations["test-annotation-1"]).To(Equal("test-annotation-1"))
Expect(pvc.Annotations["test-annotation-2"]).To(Equal("test-annotation-2"))
},
table.Entry("for import DataVolume", utils.NewDataVolumeWithHTTPImport, tinyCoreIsoURL()),
table.Entry("for upload DataVolume", createUploadDataVolume, tinyCoreIsoURL()),
table.Entry("for clone DataVolume", createCloneDataVolume, fillCommand),
)

It("Should handle a pre populated DV", func() {
By(fmt.Sprintf("initializing dataVolume marked as prePopulated %s", dataVolumeName))
dataVolume := utils.NewDataVolumeWithHTTPImport(dataVolumeName, "1Gi", cirrosURL())
Expand Down