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

Exclude already attached VMs from the volume attachment form #409

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Dec 5, 2018

This PR updates the Volume -> Attach to Instance form to exclude instances that are already attached to the selected volume.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1653062

@mansam mansam closed this Dec 5, 2018
@mansam mansam reopened this Dec 5, 2018
Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

👍

the NOT IN query should be performant enough here, expecting this will be in thousands of records. It can have problems on higher scales

@mansam
Copy link
Contributor Author

mansam commented Dec 5, 2018

@Ladas What would be the most performant way to approach the query?

@Ladas
Copy link
Contributor

Ladas commented Dec 5, 2018

@mansam on higher scale, I was playing with outer join of two tables (1 created from a query). But that'd be probably overkill here. :-)

@mansam
Copy link
Contributor Author

mansam commented Dec 5, 2018

(I'm also not quite 100% sure that this is the right way to solve the bug. I need to do a little more investigating of cinder multi-attach to know for sure if this makes sense vs just greying out the option.)

@Ladas
Copy link
Contributor

Ladas commented Dec 5, 2018

@mansam I think it should be ok? You can't attach the same vm to volume twice. For multiattach (given the volume type supports it), you can attach 1 volume to more Vms

@mansam
Copy link
Contributor Author

mansam commented Dec 5, 2018

@Ladas Cool. That's what I thought, just wanted to double check.

@miq-bot
Copy link
Member

miq-bot commented Dec 6, 2018

Checked commit mansam@bf034ab with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@aufi
Copy link
Member

aufi commented Dec 7, 2018

Looks good to me, merging!

@aufi aufi added the bug label Dec 7, 2018
@aufi aufi added this to the Sprint 101 Ending Dec 17, 2018 milestone Dec 7, 2018
@aufi aufi merged commit 66f9fd1 into ManageIQ:master Dec 14, 2018
@simaishi
Copy link
Contributor

@mansam Can this be hammer/yes?

@simaishi
Copy link
Contributor

simaishi commented Feb 6, 2019

@mansam ^ ping

@JPrause
Copy link
Member

JPrause commented Feb 11, 2019

@mansam @aufi can one of you help to answer Satoe's question. We have a hammer build and need this if it can be backported.

@aufi
Copy link
Member

aufi commented Feb 12, 2019

Yes, this should go to Hammer - https://bugzilla.redhat.com/show_bug.cgi?id=1672695

simaishi pushed a commit that referenced this pull request Feb 12, 2019
…olume-attachment-form

Exclude already attached VMs from the volume attachment form

(cherry picked from commit 66f9fd1)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1672695
@simaishi
Copy link
Contributor

Thanks @aufi

Hammer backport details:

$ git log -1
commit 26ab2ac98c5e10ea0c0b498183a82c1fd7f9b8c5
Author: Marek Aufart <aufi.cz@gmail.com>
Date:   Fri Dec 14 15:19:33 2018 +0100

    Merge pull request #409 from mansam/exclude-attached-instances-from-volume-attachment-form
    
    Exclude already attached VMs from the volume attachment form
    
    (cherry picked from commit 66f9fd169cc73519fdfc5bd735cc1f05025706a3)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1672695

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.

None yet

6 participants