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

Fix Host getting disconnected from Cluster when migrating a VM in RHEV #13815

Conversation

agrare
Copy link
Member

@agrare agrare commented Feb 8, 2017

When building a minimal host hash for migrated vm targeted refresh include the ems_cluster with the host so that it doesn't clear the ems_cluster_id when saving.

#13511 fixed saving a VM when it gets migrated to a new host but as a result the cluster link to that host got cleared when it was saved. This adds the cluster link to the minimal host hash so that it is saved properly.

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

@agrare
Copy link
Member Author

agrare commented Feb 8, 2017

cc @borod108 @durandom

@miq-bot
Copy link
Member

miq-bot commented Feb 8, 2017

Checked commit agrare@b448a14 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 👍

@agrare
Copy link
Member Author

agrare commented Feb 8, 2017

Hey @durandom so I think the reason it is clearing the host's ems_cluster is this https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory_infra.rb#L137

It will set h[:ems_cluster_id] to nil even if h[:ems_cluster] wasn't specified. What do you think of only setting the :ems_cluster_id if :ems_cluster was specified? That seems cleaner than having to add the cluster to the host when it shouldn't be needed.

@oourfali
Copy link

oourfali commented Feb 9, 2017

Seems like automation was successful so I think we're good to go.

@borod108
Copy link

borod108 commented Feb 9, 2017

Looks great to me. Since the automation already passed I would suggest leaving this as is and merging, and implement the other, cleaner, way of solving this in a separate PR.

@agrare
Copy link
Member Author

agrare commented Feb 9, 2017

@blomquisg can you review?

@agrare agrare added the blocker label Feb 9, 2017
@blomquisg blomquisg merged commit e0604bc into ManageIQ:master Feb 9, 2017
@blomquisg blomquisg added this to the Sprint 54 Ending Feb 13, 2017 milestone Feb 9, 2017
@agrare agrare deleted the bz_1420003_rhev_fix_missing_host_link_targeted_refresh branch February 9, 2017 19:41
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