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.

Sign up for GitHub

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

Jump to bottom

Ensure priority class is assigned to pod populating pvc prime #2864

Merged
merged 4 commits into from
Aug 25, 2023
Merged

Ensure priority class is assigned to pod populating pvc prime #2864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ba3a40
Ensure priority class is assigned to pod populating pvc prime
awels Aug 23, 2023
00eb6dd
Restore the logger as it was logging the pvc name as well
awels Aug 23, 2023
c0295df
Have upload also use the populator, can't do clone
awels Aug 23, 2023
d935b40
Test for priority class in upload as well as fix typo in cloning test.
awels Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/controller/common/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1941,6 +1941,7 @@ func SetPvcAllowedAnnotations(obj metav1.Object, pvc *corev1.PersistentVolumeCla
allowedAnnotations := map[string]string{
AnnPodNetwork: "",
AnnPodSidecarInjection: AnnPodSidecarInjectionDefault,
AnnPriorityClassName: "",
AnnPodMultusDefaultNetwork: ""}
for ann, def := range allowedAnnotations {
val, ok := pvc.Annotations[ann]
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/datavolume/controller-base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,7 +1027,6 @@ func (r *ReconcilerBase) newPersistentVolumeClaim(dataVolume *cdiv1.DataVolume,
annotations[cc.AnnPriorityClassName] = dataVolume.Spec.PriorityClassName
}
annotations[cc.AnnPreallocationRequested] = strconv.FormatBool(cc.GetPreallocation(context.TODO(), r.client, dataVolume.Spec.Preallocation))

pvc := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Expand Down
14 changes: 7 additions & 7 deletions pkg/controller/populators/clone-populator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ var _ = Describe("Clone populator tests", func() {
return target, source
}

initinializedTargetAndDataSource := func() (*corev1.PersistentVolumeClaim, *cdiv1.VolumeCloneSource) {
initializedTargetAndDataSource := func() (*corev1.PersistentVolumeClaim, *cdiv1.VolumeCloneSource) {
target, source := targetAndDataSource()
target.Annotations = map[string]string{
AnnClonePhase: clone.PendingPhaseName,
Expand All @@ -126,7 +126,7 @@ var _ = Describe("Clone populator tests", func() {
}

succeededTarget := func() *corev1.PersistentVolumeClaim {
target, _ := initinializedTargetAndDataSource()
target, _ := initializedTargetAndDataSource()
target.Annotations[AnnClonePhase] = string(clone.SucceededPhaseName)
target.Spec.VolumeName = "volume"
return target
Expand Down Expand Up @@ -257,7 +257,7 @@ var _ = Describe("Clone populator tests", func() {
})

It("should be in error phase if plan returns an error", func() {
target, source := initinializedTargetAndDataSource()
target, source := initializedTargetAndDataSource()
reconciler := createClonePopulatorReconciler(target, storageClass(), source)
reconciler.planner = &fakePlanner{
planError: fmt.Errorf("plan error"),
Expand All @@ -270,7 +270,7 @@ var _ = Describe("Clone populator tests", func() {
})

It("should be in error phase if phase returns an error", func() {
target, source := initinializedTargetAndDataSource()
target, source := initializedTargetAndDataSource()
reconciler := createClonePopulatorReconciler(target, storageClass(), source)
reconciler.planner = &fakePlanner{
planResult: []clone.Phase{
Expand All @@ -288,7 +288,7 @@ var _ = Describe("Clone populator tests", func() {
})

DescribeTable("should report phase name and progress", func(ownedByDataVolume bool) {
target, source := initinializedTargetAndDataSource()
target, source := initializedTargetAndDataSource()
if ownedByDataVolume {
target.OwnerReferences = []metav1.OwnerReference{
{
Expand Down Expand Up @@ -341,7 +341,7 @@ var _ = Describe("Clone populator tests", func() {
)

It("should be in error phase if progress returns an error", func() {
target, source := initinializedTargetAndDataSource()
target, source := initializedTargetAndDataSource()
reconciler := createClonePopulatorReconciler(target, storageClass(), source)
reconciler.planner = &fakePlanner{
planResult: []clone.Phase{
Expand All @@ -362,7 +362,7 @@ var _ = Describe("Clone populator tests", func() {
})

It("should go to succeeded phase if all phases are done", func() {
target, source := initinializedTargetAndDataSource()
target, source := initializedTargetAndDataSource()
reconciler := createClonePopulatorReconciler(target, storageClass(), source)
reconciler.planner = &fakePlanner{
planResult: []clone.Phase{
Expand Down
15 changes: 12 additions & 3 deletions pkg/controller/populators/import-populator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,10 @@ var _ = Describe("Import populator tests", func() {
Expect(result.RequeueAfter).To(BeNumerically(">", 0))
})

It("Should create PVC Prime with proper import annotations", func() {
targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, nil, nil, corev1.ClaimBound)
DescribeTable("Should create PVC Prime with proper import annotations", func(key, value, expectedValue string) {
targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, map[string]string{}, nil, corev1.ClaimBound)
targetPvc.Spec.DataSourceRef = dataSourceRef
targetPvc.Annotations[key] = value
volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)

By("Reconcile")
Expand Down Expand Up @@ -268,7 +269,15 @@ var _ = Describe("Import populator tests", func() {
Expect(pvcPrime.GetAnnotations()[AnnPreallocationRequested]).To(Equal("true"))
Expect(pvcPrime.GetAnnotations()[AnnEndpoint]).To(Equal("http://example.com/data"))
Expect(pvcPrime.GetAnnotations()[AnnSource]).To(Equal(SourceHTTP))
})
Expect(pvcPrime.Annotations[key]).To(Equal(expectedValue))
},
Entry("No extra annotations", "", "", ""),
Entry("Invalid extra annotation is not passed", "invalid", "test", ""),
Entry("Priority class is passed", AnnPriorityClassName, "test", "test"),
Entry("pod network is passed", AnnPodNetwork, "test", "test"),
Entry("side car injection is passed", AnnPodSidecarInjection, AnnPodSidecarInjectionDefault, AnnPodSidecarInjectionDefault),
Entry("multus default network is passed", AnnPodMultusDefaultNetwork, "test", "test"),
)

It("shouldn't error when reconciling PVC with non-import DataSourceRef", func() {
targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, nil, nil, corev1.ClaimBound)
Expand Down
12 changes: 6 additions & 6 deletions pkg/controller/populators/populator-base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func (r *ReconcilerBase) updatePVCWithPVCPrimeAnnotations(pvc, pvcPrime *corev1.

// reconcile functions

func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorController, log logr.Logger) (reconcile.Result, error) {
func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorController, pvcNameLogger logr.Logger) (reconcile.Result, error) {
// Get the target PVC
pvc := &corev1.PersistentVolumeClaim{}
if err := r.client.Get(context.TODO(), req.NamespacedName, pvc); err != nil {
Expand All @@ -259,7 +259,7 @@ func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorCon

// We first perform the common reconcile steps.
// We should only continue if we get a valid PVC'
pvcPrime, err := r.reconcileCommon(pvc, populator, log)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove the log passed to reconcile too? I don't remember if I added this for some reason or just by mistake.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I just checked, and the log passed in has a WithValues so it might actually make sense to keep everything due to that. The log will contain the PVC name and namespace in the log line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that's it, I think we should keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lets see if we can make it clearer this is the case otherwise I will do this again. I was like why are we passing this log, the struct has a logger.

pvcPrime, err := r.reconcileCommon(pvc, populator, pvcNameLogger)
if err != nil || pvcPrime == nil {
return reconcile.Result{}, err
}
Expand All @@ -273,9 +273,9 @@ func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorCon
return r.reconcileCleanup(pvcPrime)
}

func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, populator populatorController, log logr.Logger) (*corev1.PersistentVolumeClaim, error) {
func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, populator populatorController, pvcNameLogger logr.Logger) (*corev1.PersistentVolumeClaim, error) {
if pvc.DeletionTimestamp != nil {
log.V(1).Info("PVC being terminated, ignoring")
pvcNameLogger.V(1).Info("PVC being terminated, ignoring")
return nil, nil
}

Expand All @@ -292,12 +292,12 @@ func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, popu
// We should ignore PVCs that aren't for this populator to handle
dataSourceRef := pvc.Spec.DataSourceRef
if !IsPVCDataSourceRefKind(pvc, r.sourceKind) {
log.V(1).Info("reconciled unexpected PVC, ignoring")
pvcNameLogger.V(1).Info("reconciled unexpected PVC, ignoring")
return nil, nil
}
// TODO: Remove this check once we support cross-namespace dataSourceRef
if dataSourceRef.Namespace != nil {
log.V(1).Info("cross-namespace dataSourceRef not supported yet, ignoring")
pvcNameLogger.V(1).Info("cross-namespace dataSourceRef not supported yet, ignoring")
return nil, nil
}

Expand Down
47 changes: 47 additions & 0 deletions pkg/controller/populators/upload-populator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,53 @@ var _ = Describe("Datavolume controller reconcile loop", func() {
Entry("with pod succeded phase", string(corev1.PodFailed)),
Entry("with pod succeded phase", string(corev1.PodSucceeded)),
)

DescribeTable("Should create PVC Prime with proper upload annotations", func(key, value, expectedValue string) {
pvc := newUploadPopulatorPVC("test-pvc")
cc.AddAnnotation(pvc, AnnPVCPrimeName, PVCPrimeName(pvc))
uploadPV := uploadPV(pvc)

volumeUploadSourceCR := newUploadPopulatorCR("", false)
scName := "test-sc"
sc := cc.CreateStorageClassWithProvisioner(scName, map[string]string{cc.AnnDefaultStorageClass: "true"}, map[string]string{}, "csi-plugin")
pvc.Annotations[key] = value

By("Reconcile")
r := createUploadPopulatorReconciler(pvc, volumeUploadSourceCR, sc, uploadPV)
result, err := r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-pvc", Namespace: metav1.NamespaceDefault}})
Expect(err).To(Not(HaveOccurred()))
Expect(result).To(Not(BeNil()))

By("Checking events recorded")
close(r.recorder.(*record.FakeRecorder).Events)
found := false
for event := range r.recorder.(*record.FakeRecorder).Events {
if strings.Contains(event, createdPVCPrimeSuccessfully) {
found = true
}
}
r.recorder = nil
Expect(found).To(BeTrue())

By("Checking PVC' annotations")
pvcPrime, err := r.getPVCPrime(pvc)
Expect(err).ToNot(HaveOccurred())
Expect(pvcPrime).ToNot(BeNil())
// make sure we didnt inflate size
Expect(pvcPrime.Spec.Resources.Requests[corev1.ResourceStorage]).To(Equal(resource.MustParse("1Gi")))
Expect(pvcPrime.GetAnnotations()).ToNot(BeNil())
Expect(pvcPrime.GetAnnotations()[cc.AnnImmediateBinding]).To(Equal(""))
Expect(pvcPrime.GetAnnotations()[cc.AnnUploadRequest]).To(Equal(""))
Expect(pvcPrime.GetAnnotations()[cc.AnnPopulatorKind]).To(Equal(cdiv1.VolumeUploadSourceRef))
Expect(pvcPrime.Annotations[key]).To(Equal(expectedValue))
},
Entry("No extra annotations", "", "", ""),
Entry("Invalid extra annotation is not passed", "invalid", "test", ""),
Entry("Priority class is passed", cc.AnnPriorityClassName, "test", "test"),
Entry("pod network is passed", cc.AnnPodNetwork, "test", "test"),
Entry("side car injection is passed", cc.AnnPodSidecarInjection, cc.AnnPodSidecarInjectionDefault, cc.AnnPodSidecarInjectionDefault),
Entry("multus default network is passed", cc.AnnPodMultusDefaultNetwork, "test", "test"),
)
})

func newUploadPopulatorPVC(name string) *corev1.PersistentVolumeClaim {
Expand Down
2 changes: 0 additions & 2 deletions tests/datavolume_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3006,7 +3006,6 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
dataVolume := utils.NewDataVolumeWithHTTPImport(dataVolumeName, "1Gi", fmt.Sprintf(utils.TinyCoreQcow2URLRateLimit, f.CdiInstallNs))
By(fmt.Sprintf("creating new datavolume %s with priority class", dataVolume.Name))
dataVolume.Spec.PriorityClassName = "system-cluster-critical"
dataVolume.Annotations[controller.AnnPodRetainAfterCompletion] = "true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove the annotation from the next two (upload and clone) tests? Other than that looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

So the cloner is problematic, as that pod disappears too quickly and the test fails..

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

Expand All @@ -3033,7 +3032,6 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
dataVolume := utils.NewDataVolumeForUpload(dataVolumeName, "1Gi")
By(fmt.Sprintf("creating new datavolume %s with priority class\"", dataVolume.Name))
dataVolume.Spec.PriorityClassName = "system-cluster-critical"
dataVolume.Annotations[controller.AnnPodRetainAfterCompletion] = "true"
dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume)
Expect(err).ToNot(HaveOccurred())

Expand Down