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

Integration of Data volume using CDI populators #2722

Merged
merged 15 commits into from
Jun 14, 2023
Merged

Integration of Data volume using CDI populators #2722

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5689e10
move cleanup out of dv deletion
ShellyKa13 May 16, 2023
1b43c6d
Use populator if csi storage class exists
ShellyKa13 May 18, 2023
c65dcc8
Update population targetPVC with pvc prime annotations
ShellyKa13 May 18, 2023
64851a9
Adjust UT with new behavior
ShellyKa13 May 22, 2023
d537bbe
updates after review
ShellyKa13 May 23, 2023
8de69d9
Fix import populator report progress
ShellyKa13 May 28, 2023
2df98bc
Prevent requeue upload dv when failing to find progress report pod
ShellyKa13 May 29, 2023
4dd5b26
Remove size inflation in populators
ShellyKa13 May 31, 2023
dfb3fd8
Adjust functional tests to handle dvs using populators
ShellyKa13 May 18, 2023
a3aac62
Fix clone test
ShellyKa13 Jun 1, 2023
1ae48ef
add shouldUpdateProgress variable to know if need to update progress
ShellyKa13 Jun 1, 2023
6690f5e
Change update of annotation from denied list to allowed list
ShellyKa13 Jun 1, 2023
5beb9b8
fix removing annotations from pv when rebinding
ShellyKa13 Jun 5, 2023
beeff5a
More fixes and UT
ShellyKa13 Jun 6, 2023
8e0ad95
a bit more updates and UTs
ShellyKa13 Jun 12, 2023
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
Prev Previous commit
Next Next commit
Change update of annotation from denied list to allowed list 
Instead if checking if the annotation on pvcPrime is not desired
go over desired list and if the annotation exists add it.

Signed-off-by: Shelly Kagan <skagan@redhat.com>
  • Loading branch information
@ShellyKa13
ShellyKa13 committed Jun 12, 2023
commit 6690f5e980ad63374ba72f8208aa61dc02d061f5
9 changes: 0 additions & 9 deletions pkg/controller/populators/import-populator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,6 @@ func (r *ImportPopulatorReconciler) updatePVCForPopulation(pvc *corev1.Persisten
annotations[cc.AnnSource] = cc.SourceNone
}

func deleteBoundConditionIfNeeded(pvc, pvcPrime *corev1.PersistentVolumeClaim) {
if _, ok := pvcPrime.Annotations[cc.AnnBoundCondition]; !ok {
delete(pvc.Annotations, cc.AnnBoundCondition)
delete(pvc.Annotations, cc.AnnBoundConditionMessage)
delete(pvc.Annotations, cc.AnnBoundConditionReason)
}
}

func updateVddkAnnotations(pvc, pvcPrime *corev1.PersistentVolumeClaim) {
if cc.GetSource(pvcPrime) != cc.SourceVDDK {
return
Expand All @@ -213,7 +205,6 @@ func (r *ImportPopulatorReconciler) updateImportAnnotations(pvc, pvcPrime *corev
r.log.Error(err, "Failed to update import progress for pvc %s/%s", pvc.Namespace, pvc.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why just log the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont want to prevent the phase update in case there is some issue with the progress pod

}
updateVddkAnnotations(pvc, pvcPrime)
deleteBoundConditionIfNeeded(pvc, pvcPrime)
}

// Progress reporting
Expand Down
17 changes: 9 additions & 8 deletions pkg/controller/populators/populator-base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package populators
import (
"context"
"reflect"
"strings"

"github.com/go-logr/logr"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -216,15 +215,17 @@ func (r *ReconcilerBase) createPVCPrime(pvc *corev1.PersistentVolumeClaim, sourc

type updatePVCAnnotationsFunc func(pvc, pvcPrime *corev1.PersistentVolumeClaim)

var desiredAnnotations = []string{cc.AnnPodPhase, cc.AnnPodReady, cc.AnnPodRestarts, cc.AnnPreallocationRequested, cc.AnnPreallocationApplied, cc.AnnRunningCondition, cc.AnnRunningConditionMessage, cc.AnnRunningConditionReason, cc.AnnBoundCondition, cc.AnnBoundConditionMessage, cc.AnnBoundConditionReason}
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this correctly, the dv will have bound condition true if pvc' is bound. That does not seem right to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the update boundcondition on the dv doesnt update the condition to true unless the pvc is bound:
https://github.com/kubevirt/containerized-data-importer/blob/main/pkg/controller/datavolume/conditions.go#L128-L143
so seems to me its ok.

Copy link
Member

Choose a reason for hiding this comment

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

Why bother passing over the annotations then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember there was some tests checking this. I can remove this update and see how it affects our tests if it bothers you.


func (r *ReconcilerBase) updatePVCWithPVCPrimeAnnotations(pvc, pvcPrime *corev1.PersistentVolumeClaim, updateFunc updatePVCAnnotationsFunc) error {
pvcCopy := pvc.DeepCopy()
for k, v := range pvcPrime.GetAnnotations() {
if strings.Contains(k, common.CDIAnnKey) &&
!strings.Contains(k, cc.AnnImmediateBinding) &&
!strings.Contains(k, cc.AnnPopulatorKind) &&
!strings.Contains(k, "upload") &&
!strings.Contains(k, "import") {
cc.AddAnnotation(pvcCopy, k, v)
for _, ann := range desiredAnnotations {
if value, ok := pvcPrime.GetAnnotations()[ann]; ok {
cc.AddAnnotation(pvcCopy, ann, value)
} else if _, ok := pvcCopy.GetAnnotations()[ann]; ok {
// if the desired Annotation was deleted from pvcPrime
// delete it also in the target pvc
delete(pvcCopy.Annotations, ann)
}
}
if updateFunc != nil {
Expand Down