-
Notifications
You must be signed in to change notification settings - Fork 258
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
Pass DV labels to PVC #2547
Conversation
Hi @ido106. Thanks for your PR. I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
So you essentially want to pass all labels from the DV to the PVC? Any particular reason you want that? We are already passing labels from the DV to the import/upload/clone pods for things like istio and I don't think we want to pass those to the PVC. |
So we actually discussed this in the sig-storage meeting and I tried to summarize it in the issue itself; I would understand the argument that you could just grab the PVC and label it after the DV creation, but that is a bit clunky.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! great spot!
Would it be possible to also add a one-liner to some func tests that ensures we pass the labels?
in e.g. import/upload/clone/...
@@ -761,7 +746,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 { |
There was a problem hiding this comment.
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
containerized-data-importer/pkg/controller/datavolume/smart-clone-controller.go
Lines 370 to 374 in b875775
target := &corev1.PersistentVolumeClaim{ | |
ObjectMeta: metav1.ObjectMeta{ | |
Name: snapshot.Name, | |
Namespace: snapshot.Namespace, | |
Labels: labels, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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) |
@ido106 please fill in the What this PR does / why we need it and Release note sections in the card description. |
/ok-to-test |
/retest-required |
@ido106 any update on this? I believe we are waiting on a functional test? |
Ido is in his exam period, so I guess he'll continue his first PR when he's back. |
b875775
to
ea577b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, some things to consider, and maybe it's worth to add one liner checks in some of the functional tests?
@@ -352,7 +352,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(c client.Client, name string, snapshot *snapshotv1.VolumeSnapshot, targetPvcSpec *corev1.PersistentVolumeClaimSpec) (*corev1.PersistentVolumeClaim, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may even be able to just pass
annotations, labels map[string]string
/dv *cdiv1.DataVolume
to this func
(the DV should be already available to you when calling newPvcFromSnapshot
)
return nil, err | ||
} | ||
if dv != nil { | ||
for k, v := range dv.Labels { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we also don't pass the annotations in this case, we probably should
@@ -365,9 +365,9 @@ func (r *ReconcilerBase) getPVC(dv *cdiv1.DataVolume) (*corev1.PersistentVolumeC | |||
return pvc, nil | |||
} | |||
|
|||
func (r *ReconcilerBase) getDataVolume(key types.NamespacedName) (*cdiv1.DataVolume, error) { | |||
func getDataVolume(c client.Client, key types.NamespacedName) (*cdiv1.DataVolume, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not sure why this one needed to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use this function in 2 different places in the code and one of them doesn't have access to the function this way, so I changed the signature to be a regular function and requested the client as a parameter instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK you only use func (r *SmartCloneReconciler) getDataVolume
and not this one
There was a problem hiding this comment.
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 what you mentioned.
Plz let me know if there is any problem and I'll fix it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I just think we should test this in functional tests as well,
since we are now committing that CDI will always pass labels
/test pull-containerized-data-importer-e2e-upg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job Ido! I think after your latest commit this looks great. I'm only commenting some minor nitpicks in case you end up committing again, no need to change them. Everything else looks good to me so I'll give a preventive lgtm, but let's see what other folks have to say.
/lgtm
@@ -369,22 +370,32 @@ func newPvcFromSnapshot(name string, snapshot *snapshotv1.VolumeSnapshot, target | |||
common.CDILabelKey: common.CDILabelValue, | |||
common.CDIComponentLabel: common.SmartClonerCDILabel, | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a massive nitpick but I'd drop this empty line, as done with annotations below
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.Labels = map[string]string{"test": "test-label"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but I'd use the same convention for Annotations and Labels (Get/Set or not).
91c4767
to
c02fa12
Compare
c02fa12
to
3f19b15
Compare
Signed-off-by: Ido Aharon <iaharon@redhat.com>
a6f59bf
to
b058926
Compare
Signed-off-by: Ido Aharon <iaharon@redhat.com>
Signed-off-by: Ido Aharon <iaharon@redhat.com>
b058926
to
dbc9997
Compare
/lgtm awesome! |
/hold I think I see the problem containerized-data-importer/tests/cloner_test.go Lines 2878 to 2879 in dbc9997
The annotations are being overwritten so we lose AnnDeleteAfterCompletion We should probably just append the test annotations/labels to existing ones |
Signed-off-by: Ido Aharon <iaharon@redhat.com>
/hold cancel But we have an unrelated CI issue following 1.56.0 release related to v1alpha1 manifests |
/lgtm |
/retest |
1 similar comment
/retest |
/test pull-containerized-data-importer-e2e-ceph |
/cc @mhenriks |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherrypick release-v1.56 |
@mhenriks: once the present PR merges, I will cherry-pick it on top of release-v1.56 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/override pull-containerized-data-importer-e2e-destructive |
@awels: Overrode contexts on behalf of awels: pull-containerized-data-importer-e2e-destructive In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@mhenriks: new pull request created: #2646 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Ido Aharon iaharon@redhat.com
What this PR does / why we need it:
Pass all the DataVolume labels to its created PVC. It's fixing the current behavior which passes only specific labels, aligning it with DV annotations passing to the PVC.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #2533
Special notes for your reviewer:
Release note: