CLOUDSTACK-9182: Some running VMs turned off on manual migration when auto migration failed while host preparing for maintenance. #1252
Conversation
|
Did someone test it? This can be very handy. LGTM. |
|
@rodrigo93 I agree but it seems that this behavioral change might be a compatibility problem to some. I would like this to be tweekable with a setting. People may rely on exception being thrown atm. |
|
@DaanHoogland I see... With that option in settings, things could be more manageable. That is a good suggestion. |
| } else { | ||
| HostVO host = _hostDao.findById(hostId); | ||
| if (!cleanUpEvenIfUnableToStop && vm.getState() == State.Running && host.getResourceState() == ResourceState.PrepareForMaintenance) { | ||
| s_logger.debug("Host in PrepareForMaintenance state - Failed to stop VM with id: " + vm.getId()); |
There was a problem hiding this comment.
hello @sureshanaparti.
How about you extract the content in the 'if' at line 1478 to an "isHostPreparingForMaintenance" method or something like that? it will explain better the if content, and you can do a test case to this method too
Ty.
There was a problem hiding this comment.
@pedro-martins The condition "host.getResourceState() == ResourceState.PrepareForMaintenance" only, in the if statement above, states that the Host is preparing for maintenance. Other conditions doesn't relate to it and so cannot go in "isHostPreparingForMaintenance" method.
|
@sureshanaparti There are some comments, please address them. |
|
@sureshanaparti please rebase against latest master, how do we test your change; additional tests would be great |
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests: Skipped tests: Passed test suits: |
3920f77 to
b3faffa
Compare
|
@rhtyd Addressed the changes suggested and rebased against latest master. Tested this manually. Try reboot, migrate VM when host is preparing for maintenance with this patch. |
|
@sureshanaparti thanks, do you think this would be useful for 4.9? Can you change the PR's base branch to 4.9, rebase your branch against 4.9, thanks. |
b3faffa to
4ce379e
Compare
4ce379e to
165de1e
Compare
|
@rhtyd Rebased against 4.9 |
|
Thanks @sureshanaparti |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
@sureshanaparti is there an existing marvin test for this, or if you can add one? |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-315 |
|
Trillian queue is full, I'll kick tests tonight. |
|
@rhtyd no existing marvin test for this. will add the test. |
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests:
Skipped tests: Passed test suits: |
165de1e to
5731ec8
Compare
5731ec8 to
9514056
Compare
6f2585f to
6ae9556
Compare
|
blueorangutan package |
| @@ -4695,6 +4699,22 @@ private boolean checkIfHostIsDedicated(HostVO host) { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Checks whether the host is preparing for maintenance mode or not. If not, throws exception with the VM id and operation performed on it | |||
There was a problem hiding this comment.
comment states the reverse of the intended "If not, throws exception..." should be "If so, throws .."
also as you are making javadoc (not needed for a private method, i'd say) best to add a @throws tag as this is the most important functionality of this method.
There was a problem hiding this comment.
@DaanHoogland Removed javadoc for this private method.
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
6ae9556 to
fb4f579
Compare
a53d597 to
ec9111b
Compare
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-750 |
ec9111b to
1d88597
Compare
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-751 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1138)
|
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/r2si930m8xxzavs/AAAzNrnoF1fC3auFrvsKo_8-a?dl=0 Failed tests:
Skipped tests: Passed test suits: |
There was a problem hiding this comment.
@sureshanaparti can you check the CI fails, other than that looks good
… auto migration failed while host preparing for maintenance. Fix: Block VMOperations if Host in PrepareForMaintenance mode. VM operations (Stop, Reboot, Destroy, Migrate to host) are not allowed when Host in PrepareForMaintenance mode.
1d88597 to
bb630e7
Compare
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1098 |
|
@blueorangutan test centos7 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
|
Trillian test result (tid-1520)
|
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1527)
|
|
Tests LGTM, merging. |
CLOUDSTACK-9182: Some running VMs turned off on manual migration when auto migration failed while host preparing for maintenance.
Fix: Block VMOperations if Host in PrepareForMaintenance mode. VM operations (Stop, Reboot, Destroy, Migrate to host) are not allowed when Host in PrepareForMaintenance mode.