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.

Sign up for GitHub

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

Jump to bottom

Disconnect storage when disconnecting the VM #18200

Merged
merged 2 commits into from
Nov 28, 2018
Merged

Disconnect storage when disconnecting the VM #18200

merged 2 commits into from
Nov 28, 2018

Conversation

agrare
Copy link
Member

@agrare agrare commented Nov 14, 2018 •
edited
Loading

@agrare
Disconnect storage when disconnecting the VM
f24984b 
Instead of having the EventHandler call disconnect_storage from an event
handler, call it when disconnecting the VM.

https://bugzilla.redhat.com/show_bug.cgi?id=1644770
This was referenced Nov 14, 2018
Merged
Merged
Merged
@agrare
Copy link
Member Author

agrare commented Nov 14, 2018

Hey @Fryguy this is the approach I came up with to disconnect the VM's storage from EmsRefresh by looking at the type of event (overridden by vmware here).

The alternate approach is to have the event_handler queue the disconnect_storage method for the refresh worker to prevent race conditions but this seems simpler (having the disconnect logic all in one place).

@Fryguy
Copy link
Member

Fryguy commented Nov 15, 2018

This looks like a good approach.

@Fryguy
Copy link
Member

Fryguy commented Nov 15, 2018

Everything looks good to me. I found some more potential deletions in manageiq-automation-engine and commented as such in ManageIQ/manageiq-content#472 (comment)

Do you want to unWIP all of this? After that...what is the best merge order?

@agrare
Copy link
Member Author

agrare commented Nov 15, 2018

Yeah I left the dependent ones WIP so they didn't get merged too soon.

Merge order should be this, then ManageIQ/manageiq-providers-vmware#336, then ManageIQ/manageiq-content#472.

@agrare
Copy link
Member Author

agrare commented Nov 15, 2018

Okay un-wip'd everything @Fryguy

@agrare
Remove src_vm_disconnect_storage from ems_event
0a529d7 
Automate no longer needs to call disconnect_storage
@miq-bot
Copy link
Member

miq-bot commented Nov 16, 2018

Checked commits agrare/manageiq@f24984b~...0a529d7 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. 👍

Ladas
Ladas approved these changes Nov 16, 2018
View reviewed changes
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.

👍 makes sense

@JPrause
Copy link
Member

JPrause commented Nov 26, 2018

@agrare since this is for a blocker, where are we with getting this merged.

@agrare
Copy link
Member Author

agrare commented Nov 26, 2018

since this is for a blocker, where are we with getting this merged.

@Fryguy can you take another look? Everything is ready.

@JPrause
Copy link
Member

JPrause commented Nov 27, 2018

@Fryguy can you review/merge. We need this for a blocker and this PR needs to be merged first before other dependent PRs are merged.

@Fryguy Fryguy merged commit 3e466eb into ManageIQ:master Nov 28, 2018
@Fryguy Fryguy added this to the Sprint 100 Ending Dec 3, 2018 milestone Nov 28, 2018
@agrare agrare deleted the bz_1644770_disconnect_storage_when_disconnecting_vms branch November 28, 2018 15:49
simaishi pushed a commit that referenced this pull request Nov 28, 2018
@Fryguy @simaishi
Merge pull request #18200 from agrare/bz_1644770_disconnect_storage_w…
a79c4db 
…hen_disconnecting_vms

Disconnect storage when disconnecting the VM

(cherry picked from commit 3e466eb)

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

Gaprindashvili backport details:

$ git log -1
commit a79c4db3bcaa83e5665782c04f71ff13126a3afd
Author: Jason Frey <[email protected]>
Date:   Wed Nov 28 10:29:37 2018 -0500

    Merge pull request #18200 from agrare/bz_1644770_disconnect_storage_when_disconnecting_vms
    
    Disconnect storage when disconnecting the VM
    
    (cherry picked from commit 3e466eb1f562fba5743a8c4cbb66bf900b8e6a21)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1644770

@agrare agrare mentioned this pull request Nov 29, 2018
Merged
@agrare
Copy link
Member Author

agrare commented Nov 29, 2018

@simaishi ManageIQ/manageiq-providers-vmware#343 should also be backported to hammer when you get there.

simaishi pushed a commit that referenced this pull request Nov 30, 2018
@Fryguy @simaishi
Merge pull request #18200 from agrare/bz_1644770_disconnect_storage_w…
952c1fc 
…hen_disconnecting_vms

Disconnect storage when disconnecting the VM

(cherry picked from commit 3e466eb)

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

Hammer backport details:

$ git log -1
commit 952c1fcc58cf3f4604825bcd67001d2efb3d32ce
Author: Jason Frey <[email protected]>
Date:   Wed Nov 28 10:29:37 2018 -0500

    Merge pull request #18200 from agrare/bz_1644770_disconnect_storage_when_disconnecting_vms
    
    Disconnect storage when disconnecting the VM
    
    (cherry picked from commit 3e466eb1f562fba5743a8c4cbb66bf900b8e6a21)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1644770

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas approved these changes

Assignees

@Fryguy Fryguy

Labels
blocker bug gaprindashvili/backported hammer/backported providers/infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@agrare @Fryguy @miq-bot @JPrause @simaishi @Ladas