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 annotations from DV to PVC. #899

Merged
merged 1 commit into from Jul 30, 2019

Conversation

awels
Copy link
Member

@awels awels commented Jul 30, 2019

Signed-off-by: Alexander Wels awels@redhat.com

What this PR does / why we need it:
Pass annotations from Data Volume to 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 #

Special notes for your reviewer:

Release note:

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XS labels Jul 30, 2019
@awels awels force-pushed the pass_provision_annotation branch from ced3910 to bb387ab Compare July 30, 2019 15:14
Signed-off-by: Alexander Wels <awels@redhat.com>
Copy link
Member

@aglitke aglitke left a comment

Choose a reason for hiding this comment

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

Looks good to me but I'd update the PR title and description to reflect that we how have a general annotation passthrough mechanism.

@awels awels changed the title Pass provisionOnNode annotation to PVC if on data volume. Pass annotations from DV to PVC. Jul 30, 2019
@awels
Copy link
Member Author

awels commented Jul 30, 2019

Done

@aglitke
Copy link
Member

aglitke commented Jul 30, 2019

LGTM

@@ -939,6 +939,10 @@ func newPersistentVolumeClaim(dataVolume *cdiv1.DataVolume) (*corev1.PersistentV

annotations := make(map[string]string)

for k, v := range dataVolume.ObjectMeta.Annotations {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, but I would suggest at least considering a PVC-Annotations section on the DV. Passing through is certainly easy and works; but it's not always applicable and at some point you may run the risk of collisions. I'm good either way, but I do think it should be mentioned.

Copy link
Member Author

@awels awels Jul 30, 2019

Choose a reason for hiding this comment

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

Agreed, but that would cause us to have to be vendored into kubevirt, this is a fairly minimal change that doesn't require vendoring. And yes the collisions is a concern, but we have some order guarantee, if we collide with annotations created by the controller, those annotations will win.

We can do a follow up PR for master to add this field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about that vs an annotations key, but regardless; I'm fine with this for now.
Just waiting for CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I guess we won't wait for CI to run

Copy link
Member Author

Choose a reason for hiding this comment

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

We did wait, actually, but we have known failures that happened, I verified that those are the only failures.

@awels awels merged commit 9e4c3de into kubevirt:master Jul 30, 2019
awels added a commit to awels/containerized-data-importer that referenced this pull request Aug 19, 2019
Signed-off-by: Alexander Wels <awels@redhat.com>
awels added a commit that referenced this pull request Aug 19, 2019
Signed-off-by: Alexander Wels <awels@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants