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

Bug 1779116: vm template should create url import dataVolumes on its creation #3665

Merged

Conversation

atiratree
Copy link
Member

  • show URL provision source for templates
  • provision source changed to Disk (Attach Cloned Disk) for vms created
    from such templates
  • add warning to the template disk modal

@mareklibra @matthewcarleton Can you take a look?

As you can see bellow, I changed a provision source of VM to Disk to truthfully reflect the situation.

I added a warning to disk-modal in template creation to show the difference between template url and vm url.

Is this enough? Should I show this also in the general tab so the user knows what is going on? Or in the review?

Templates Creation

3

2

Inspecting the template source

1

Creating VM from the template

4

6

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Dec 4, 2019
@openshift-ci-robot
Copy link
Contributor

@suomiy: This pull request references Bugzilla bug 1779116, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1779116: vm template should create url import dataVolumes on its creation

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.

@openshift-ci-robot openshift-ci-robot added component/kubevirt Related to kubevirt-plugin size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 4, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2019
@atiratree atiratree force-pushed the kubevirt.bug.1779116 branch 2 times, most recently from 8aa566c to d45e207 Compare December 4, 2019 15:54
@matthewcarleton
Copy link
Contributor

@suomiy just so I know I understand this correctly:

  1. User creates a template and the source is via a URL
  2. Datavolume is created with what was provided via the URL
  3. VM is created with that Datavolume and not via the URL from the template

@atiratree
Copy link
Member Author

@suomiy just so I know I understand this correctly:
3. VM is created with that Datavolume and not via the URL from the template

yup, although VM clones the PVC and does not use the original Datavolume or original PVC ever again

I guess we should modify that disk modal message a bit?

@matthewcarleton
Copy link
Contributor

@suomiy ya I'm wondering why we are alerting them of this. Is it something that should concern them? Is it just because this will take storage space so we want to let them know?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2019
@atiratree
Copy link
Member Author

Because the behaviour is different for VM and VM template and for other VM template datavolumes

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 6, 2019
@matthewcarleton
Copy link
Contributor

Do you think we should be talking about DataVolumes here if we don't show them anywhere in the UI?

@atiratree
Copy link
Member Author

Not sure. Maybe PVC should be enough? - but that takes a while before it is ready (datavolume implies that)

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Dec 6, 2019

I wonder if we could do something like this:
Screen Shot 2019-12-06 at 1 27 44 PM
Using PVC is a little odd too since we don't expose that in the VM currently.

@atiratree
Copy link
Member Author

That is just a consequence and doesn't say what is going on currently. They might not even expect that some pvc gets allocated with the creation of template.

I think PVC should be ok, as that is a pretty common term in k8s. Datavolumes not so much - unless you are familiar with CDI,

@matthewcarleton
Copy link
Contributor

ok. It would be nice eventually if we could surface the idea of a PVC in the creation flow.

…creation

- show URL provision source for templates
- provision source changed to Disk (Attach Cloned Disk) for vms created
from such templates
- add warning to the template disk modal
@atiratree
Copy link
Member Author

updated

aa

@mareklibra
Copy link
Contributor

/lgtm

I do not think we need to expose this back-end stuff to the user so explicitly.
IMO, (s)he do not care. A note in the documentation might be good enough.

But as this is already agreed, I am not against it.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mareklibra, suomiy

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@matthewcarleton
Copy link
Contributor

@mareklibra I am relying on the developer perspective here to better understand the use case for this. I do think that exposing PVCs here don't really help a CNV user since they don't necessarily know what a PVC is and won't get a better understanding of that by creating a VM from this template. I don't feel strongly about showing/hiding this in general though.

@openshift-merge-robot openshift-merge-robot merged commit 76914c5 into openshift:master Dec 9, 2019
@openshift-ci-robot
Copy link
Contributor

@suomiy: All pull requests linked via external trackers have merged. Bugzilla bug 1779116 has been moved to the MODIFIED state.

In response to this:

Bug 1779116: vm template should create url import dataVolumes on its creation

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.

@spadgett spadgett added this to the v4.4 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/kubevirt Related to kubevirt-plugin lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants