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

Allow cloud volume to provide a list of volumes for attach #14058

Merged

Conversation

gberginc
Copy link
Contributor

While opening the view to attach a selected volume to a new instance, the UI will currently ask for the VMs that are available in the cloud tenant of the chosen volume. While it works for OpenStack, Amazon has no notion of tenants currently.

Instead of relying on the tenant's VMs, this patch proposes a new method in the cloud volume allowing it to specify the list of VMs this volume can be attached to. In case of OpenStack, this will be delagated to cloud_tenant and on Amazon it will be delagated to availability_zone (because the volume has to reside in the same AZ).

A simple spec test is provided to validate the available_vms only return the list of VMs in the same cloud tenant. I've got the counterpart ready for Amazon, but will wait to see if this change is acceptable.

Is there any alternative to this approach?

@miq-bot add_label providers/openstack/cloud, providers/amazon, providers/storage, ui

While opening the view to attach a selected volume to a new instance,
the UI will currently ask for the VMs that are available in the cloud
tenant of the chosen volume. While it works for OpenStack, Amazon has no
notion of tenants currently.

Instead of relying on the tenant's VMs, this patch proposes a new method
in the cloud volume allowing it to specify the list of VMs this volume
can be attached to. In case of OpenStack, this will be delagated to
`cloud_tenant` and on Amazon it will be delagated to `availability_zone`
(because the volume has to reside in the same AZ).

A simple spec test is provided to validate the `available_vms` only
return the list of VMs in the same cloud tenant.

Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
@gberginc gberginc force-pushed the add_available_vms_to_cloud_volume branch from 5b2fe35 to aa343b2 Compare February 23, 2017 21:54
@miq-bot
Copy link
Member

miq-bot commented Feb 23, 2017

Checked commit gberginc@aa343b2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. ⭐

@gberginc
Copy link
Contributor Author

gberginc commented Mar 2, 2017

@blomquisg Do you think this is safe to merge? I'd like to add AWS support and then update the UI to get the list of allowed VMs from the volume itself.

@blomquisg blomquisg requested a review from tzumainn March 6, 2017 17:21
@blomquisg
Copy link
Member

@tzumainn can you look over this and make sure that it works for OpenStack?

@gberginc
Copy link
Contributor Author

gberginc commented Mar 6, 2017

When considering this, keep in mind that a change in this line will be made in the UI to ask the volume for the VMs that can be used to attach the volume to, i.e.

@volume.available_vms.each { |vm| @vm_choices[vm.name] = vm.id }

If you prefer, I can push PRs for the UI and Amazon EBS provider.

@tzumainn
Copy link
Contributor

tzumainn commented Mar 6, 2017

@mansam what do you think? you're probably far more experienced with openstack volume shenanigans than I am.

@blomquisg
Copy link
Member

If you prefer, I can push PRs for the UI and Amazon EBS provider.

@gberginc yeah, if you could push the simultaneous PRs and reference back to this one as a dependency that would be good.

In general, this looks good to me. Once I get confirmation from @mansam, then I'll merge this. I won't hold this up on seeing the simultaneous PRs.

@blomquisg blomquisg self-requested a review March 6, 2017 21:50
Copy link
Member

@blomquisg blomquisg left a comment

Choose a reason for hiding this comment

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

Dependent on confirmation from @mansam

@gberginc
Copy link
Contributor Author

gberginc commented Mar 6, 2017

@blomquisg Two additional PRs created in the UI and Amazon repositories to better show the goal of this PR.

Copy link
Contributor

@mansam mansam left a comment

Choose a reason for hiding this comment

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

This looks like a good solution. 👍

@blomquisg blomquisg merged commit 5550428 into ManageIQ:master Mar 7, 2017
@blomquisg blomquisg added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants