You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by abhinandanprateek <gi...@git.apache.org> on 2016/03/28 04:51:11 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
GitHub user abhinandanprateek opened a pull request:
https://github.com/apache/cloudstack/pull/1454
CLOUDSTACK-9323: Fix cancel host maintenance can…
so that if maintenance is cancelled the host come back to normal state gracefully.
Added marvin tests for host maintennace.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shapeblue/cloudstack host-maint
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1454.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1454
----
commit 15f02bcd1f2b103e85e245201c52937ae0ea1a2e
Author: Abhinandan Prateek <ap...@apache.org>
Date: 2016-03-28T02:47:43Z
CLOUDSTACK-9323: Fix Cancel maintenance so that if maintenance is cancelled the host come back to normal state gracefully.
Added marvin tests for host maintennace.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r58964733
--- Diff: tools/marvin/marvin/lib/utils.py ---
@@ -520,4 +520,22 @@ def verifyRouterState(apiclient, routerid, allowedstates):
if routers[0].state.lower() not in allowedstates:
return [FAIL, "state of the router should be in %s but is %s" %
(allowedstates, routers[0].state)]
- return [PASS, None]
\ No newline at end of file
+ return [PASS, None]
+
+
+
+def wait_until(retry_interval=2, no_of_times=2, callback=None, *callback_args):
+ """ Utility method to try out the callback method at most no_of_times with a interval of retry_interval,
+ Will return immediately if callback returns True. The callback method should be written to return a list of values first being a boolean """
+
+ if callback is None:
+ return INVALID_INPUT
+ success = False
+ for i in range(0,no_of_times):
+ time.sleep(retry_interval)
+ success = callback(*callback_args)
+ if success[0]:
+ break
+
+ return success
--- End diff --
Assuming I am reading this correctly, this function will return an array not a Boolean value based on the array operation on line 536.
Would be possible to add a unit test for this function to verify proper return of ``True`` when the callback succeeds and ``False`` when it doesn't pass after the number of retries fails?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-207107811
## CI RESULTS
### 84/85 TESTS PASSED
The test that failed is a test that commonly fails in my environment and has been verified to be an environment issue.
**Associated Uploads**
**`test_host_maintenance_X027L5`:**
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1454/test_host_maintenance_X027L5/results.txt)
**`test_vpc_routers_093RH7`:**
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1454/test_vpc_routers_093RH7/results.txt)
Uploads will be available until `2016-06-07 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/swill/upr).*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-202256571
Marvin test output:
root@ccp:~/cloudstack(host-maint)# ./host_maint.sh
++ date
+ echo Mon Mar 28 11:47:45 IST 2016
Mon Mar 28 11:47:45 IST 2016
+ TMP=/tmp
+ CLOUDDIR=/root/cloudstack
+ mkdir -p /tmp/simulator/smoke/misc
+ nosetests --with-xunit --xunit-file=/tmp/quagga/test_quagga.xml --with-marvin --marvin-config=/root/cloudstack/advanced.cfg /root/cloudstack/test/integration/component/test_host_maintenance.py -s -a tags=advanced,required_hardware=false --zone=Bootcamp --hypervisor=XenServer
==== Marvin Init Started ====
=== Marvin Parse Config Successful ===
=== Marvin Setting TestData Successful===
==== Log Folder Path: /tmp//MarvinLogs//Mar_28_2016_11_47_46_LB5B4I. All logs will be available here ====
=== Marvin Init Logging Successful===
==== Marvin Init Successful ====
test_01_cancel_host_maintenace (integration.component.test_host_maintenance.TestHostMaintenance)
Hypervisor = 34777316-62ae-4590-868a-71daa23dad3d
Hypervisor = 2e872579-efb9-4bb2-ae00-dfb21a994f08
Create VMs as there are not enough vms to check host maintenance
Creating vms = 5
Using template 7e374f74-b471-46bd-bbbe-f457d5376acc
Using service offering 94589b7c-ef32-484b-8ef7-59ad1ba6b69b
Using template 7e374f74-b471-46bd-bbbe-f457d5376acc
Using service offering 9c95b1bf-0ca3-4bba-a00b-1fe61eb078be
VM create = a0901dc0-a568-4d78-8d46-ddf976f6af95
VM create = ebba3c2b-d950-4969-ae34-2cc785023ef9
VM create = 0a9635be-5599-4259-a89b-ad9017e2d4e7
VM create = 9a885588-db3f-4aca-9885-c931fbd6ea71
Host with id 34777316-62ae-4590-868a-71daa23dad3d is in prepareHostForMaintenance
Host with id 34777316-62ae-4590-868a-71daa23dad3d is in cancelHostMaintenance
Host with id 2e872579-efb9-4bb2-ae00-dfb21a994f08 is in prepareHostForMaintenance
Host with id 2e872579-efb9-4bb2-ae00-dfb21a994f08 is in cancelHostMaintenance
=== TestName: test_01_cancel_host_maintenace | Status : SUCCESS ===
===final results are now copied to: /tmp//MarvinLogs/test_host_maintenance_CXM1EL===
++ date
+ echo Mon Mar 28 11:58:36 IST 2016
Mon Mar 28 11:58:36 IST 2016
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r59934099
--- Diff: tools/marvin/marvin/lib/utils.py ---
@@ -520,4 +520,22 @@ def verifyRouterState(apiclient, routerid, allowedstates):
if routers[0].state.lower() not in allowedstates:
return [FAIL, "state of the router should be in %s but is %s" %
(allowedstates, routers[0].state)]
- return [PASS, None]
\ No newline at end of file
+ return [PASS, None]
+
+
+
+def wait_until(retry_interval=2, no_of_times=2, callback=None, *callback_args):
+ """ Utility method to try out the callback method at most no_of_times with a interval of retry_interval,
+ Will return immediately if callback returns True. The callback method should be written to return a list of values first being a boolean """
+
+ if callback is None:
+ return INVALID_INPUT
--- End diff --
@abhinandanprateek while it is permissible Python, it is considered an anti-pattern for a Python function to return different types. In this case, it can return a scalar value or a multi-return. Raising a ``ValueError`` is the idiomatic Python approach to handling an invalid parameter value. In addition to ensuring that the function always returns the same type, raising an error in this manner will cause the test to fail fast without callers having the check the return to properly fail.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r58965346
--- Diff: tools/marvin/marvin/lib/utils.py ---
@@ -520,4 +520,22 @@ def verifyRouterState(apiclient, routerid, allowedstates):
if routers[0].state.lower() not in allowedstates:
return [FAIL, "state of the router should be in %s but is %s" %
(allowedstates, routers[0].state)]
- return [PASS, None]
\ No newline at end of file
+ return [PASS, None]
+
+
+
+def wait_until(retry_interval=2, no_of_times=2, callback=None, *callback_args):
+ """ Utility method to try out the callback method at most no_of_times with a interval of retry_interval,
+ Will return immediately if callback returns True. The callback method should be written to return a list of values first being a boolean """
+
+ if callback is None:
+ return INVALID_INPUT
+ success = False
+ for i in range(0,no_of_times):
+ time.sleep(retry_interval)
+ success = callback(*callback_args)
--- End diff --
Consider using a multi-return to separate ``success`` from the ``result`` such as the following:
```
success, result = callback(*callback_args)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r59152198
--- Diff: tools/marvin/marvin/lib/utils.py ---
@@ -520,4 +520,22 @@ def verifyRouterState(apiclient, routerid, allowedstates):
if routers[0].state.lower() not in allowedstates:
return [FAIL, "state of the router should be in %s but is %s" %
(allowedstates, routers[0].state)]
- return [PASS, None]
\ No newline at end of file
+ return [PASS, None]
+
+
+
+def wait_until(retry_interval=2, no_of_times=2, callback=None, *callback_args):
+ """ Utility method to try out the callback method at most no_of_times with a interval of retry_interval,
+ Will return immediately if callback returns True. The callback method should be written to return a list of values first being a boolean """
+
+ if callback is None:
+ return INVALID_INPUT
--- End diff --
Due to following two reason I preferred returning an error code instead of an exception.
1. Returning a pre-defined error code makes it usage more flexible as raising an exception or continue will be defined by the user of the method.
2. "INVALID_INPUT" is a Marvin error code used by other utility methods to signal bad inputs and this maintains that pattern.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-215303942
Sorry for the delay on this one guys. I think this one is ready now...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r59152218
--- Diff: tools/marvin/marvin/lib/utils.py ---
@@ -520,4 +520,22 @@ def verifyRouterState(apiclient, routerid, allowedstates):
if routers[0].state.lower() not in allowedstates:
return [FAIL, "state of the router should be in %s but is %s" %
(allowedstates, routers[0].state)]
- return [PASS, None]
\ No newline at end of file
+ return [PASS, None]
+
+
+
+def wait_until(retry_interval=2, no_of_times=2, callback=None, *callback_args):
+ """ Utility method to try out the callback method at most no_of_times with a interval of retry_interval,
+ Will return immediately if callback returns True. The callback method should be written to return a list of values first being a boolean """
+
+ if callback is None:
+ return INVALID_INPUT
+ success = False
+ for i in range(0,no_of_times):
+ time.sleep(retry_interval)
+ success = callback(*callback_args)
--- End diff --
Although a list of return values is expected first being boolean value for success. But yes this will make it even more readable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-202217738
@abhinandanprateek I don't see the Marvin test case in the PR. Have you pushed the latest commit?
Also, most of the changes seem to be formatting changes in non-related parts of the class which hides the actual fix. Would it be possible to reverse these formatting changes to reduce the size of the patch to only the change in ``doCancelMaintenance``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-205885703
@abhinandanprateek: Thank you. I will run the whole set of tests against it again tonight. I have to let the current tests finish and run this on its own because I have to change the tests run specifically for this PR. I should have results in the morning.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-211563367
@abhinandanprateek, I believe you are away this week, but can your address @jburwell's comments when you have a chance. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-205780575
Updated marvin test to use the builtin template. @swill
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r58964900
--- Diff: tools/marvin/marvin/lib/utils.py ---
@@ -520,4 +520,22 @@ def verifyRouterState(apiclient, routerid, allowedstates):
if routers[0].state.lower() not in allowedstates:
return [FAIL, "state of the router should be in %s but is %s" %
(allowedstates, routers[0].state)]
- return [PASS, None]
\ No newline at end of file
+ return [PASS, None]
+
+
+
+def wait_until(retry_interval=2, no_of_times=2, callback=None, *callback_args):
+ """ Utility method to try out the callback method at most no_of_times with a interval of retry_interval,
+ Will return immediately if callback returns True. The callback method should be written to return a list of values first being a boolean """
+
+ if callback is None:
+ return INVALID_INPUT
--- End diff --
This function should always return a Boolean value. If the the ``callback`` is ``None``, why not raise a ``ValueError`` rather than return a value? Not only is it more idiomatic Python, but it preserves the type semantics of the function.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1454
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-205047143
@abhinandanprateek would you mind rebasing to current master so I can test this in my CI? Thanks...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-207146770
@swill just reviewed.
@abhinandanprateek I have a few more comments to be addressed on the test cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by alexandrelimassantana <gi...@git.apache.org>.
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r57714466
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -2112,11 +2112,13 @@ private boolean doCancelMaintenance(final long hostId) {
/* TODO: move to listener */
_haMgr.cancelScheduledMigrations(host);
+
+ boolean vms_migrating = false;
final List<VMInstanceVO> vms = _haMgr.findTakenMigrationWork();
for (final VMInstanceVO vm : vms) {
if (vm != null && vm.getHostId() != null && vm.getHostId() == hostId) {
s_logger.info("Unable to cancel migration because the vm is being migrated: " + vm);
--- End diff --
Can you turn the if on line 2119 to a method call like _isVmMigrating(vm, hostId)_? Or even _vm.isMigrating(hostId)_
I think that it will improve the readability of this segment you are working. Also... is there a need to check all VMs ? Once you find one that is migrating do you still need to keep checking if they are migrating? If there is not a need, try changing the loop for a while, or issuing a break.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r57835178
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -2112,11 +2112,13 @@ private boolean doCancelMaintenance(final long hostId) {
/* TODO: move to listener */
_haMgr.cancelScheduledMigrations(host);
+
+ boolean vms_migrating = false;
final List<VMInstanceVO> vms = _haMgr.findTakenMigrationWork();
for (final VMInstanceVO vm : vms) {
if (vm != null && vm.getHostId() != null && vm.getHostId() == hostId) {
s_logger.info("Unable to cancel migration because the vm is being migrated: " + vm);
--- End diff --
@alexandrelimassantana
The reason for not breaking from the for loop is to log info about vms that are under migration. Probably the log level should be increased to warn. These log messages would be valuable for trouble shooting.
On readability front yes the code will be improved further.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-207111301
I have gone through the code, it LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-205892559
@abhinandanprateek slight delay. I am going to blow away my CI setup and reinstall it now to get it running on SSD drives so I can better parallelize my tests. This will delay this test till at least tomorrow evening.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-214242715
@swill @jburwell the concerns noted above are taken care off.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-207109385
@abhinandanprateek tests have passed, thanks for the updates. Can you do a `push -f` to kick off a Jenkins run so we can try to get this PR all green.
@jburwell Does this pass your code review now?
I am trying to get two independent LGTM code reviews before merging whenever possible, so if you have reviewed the code, please let me know.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-202221938
@jsb added the marvin file and reverted to pre-commit formatted code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-205117865
@cristofolini @swill rebased the PR, up for CI now, Thank you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-214542430
LGTM for code
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r58953686
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -2112,11 +2112,13 @@ private boolean doCancelMaintenance(final long hostId) {
/* TODO: move to listener */
_haMgr.cancelScheduledMigrations(host);
+
+ boolean vms_migrating = false;
final List<VMInstanceVO> vms = _haMgr.findTakenMigrationWork();
for (final VMInstanceVO vm : vms) {
- if (vm != null && vm.getHostId() != null && vm.getHostId() == hostId) {
- s_logger.info("Unable to cancel migration because the vm is being migrated: " + vm);
- return false;
+ if (vm.getHostId() != null && vm.getHostId() == hostId) {
--- End diff --
My bad, I was not thinking straight.
The conditional is ok
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-206227732
@swill if it is possible to run integration/component/test_host_maintenance.py first than please do that as it is a new test added for this fix, followed by full marvin suite.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-204574444
@abhinandanprateek Looks like jenkins found some problems with your index. I'd suggest rebasing against the current master making all the needed merges and pushing again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-205473828
### CI RESULTS
**HAS FAILURES, NEEDS WORK!**
Please accress the following issue.
```
======================================================================
ERROR: test_02_cancel_host_maintenace_with_migration_jobs (integration.component.test_host_maintenance.TestHostMaintenance)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs1/cloudstack/test/integration/component/test_host_maintenance.py", line 277, in test_02_cancel_host_maintenace_with_migration_jobs
self.vmlist = self.createVMs(listHost[0].id, no_vm_req)
File "/data/git/cs1/cloudstack/test/integration/component/test_host_maintenance.py", line 108, in createVMs
self.logger.debug("Using template %s " % self.template.id)
AttributeError: 'str' object has no attribute 'id'
```
**Associated Uploads**
**`test_host_maintenance_P9AKTO`:**
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1454/test_host_maintenance_P9AKTO/results.txt)
Uploads will be available until `2016-06-04 00:00:00 +0000 GMT`
*Comment created by [`upr comment`](https://github.com/swill/upr).*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r59155288
--- Diff: test/integration/component/test_host_maintenance.py ---
@@ -0,0 +1,325 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+""" BVT tests for Hosts Maintenance
+"""
+
+# Import Local Modules
+from marvin.codes import FAILED
+from marvin.cloudstackTestCase import *
+from marvin.cloudstackAPI import *
+from marvin.lib.utils import *
+from marvin.lib.base import *
+from marvin.lib.common import *
+from nose.plugins.attrib import attr
+
+from time import sleep
+
+_multiprocess_shared_ = False
+
+
+class TestHostMaintenance(cloudstackTestCase):
+
+ def setUp(self):
+ self.logger = logging.getLogger('TestHM')
+ self.stream_handler = logging.StreamHandler()
+ self.logger.setLevel(logging.DEBUG)
+ self.logger.addHandler(self.stream_handler)
+ self.apiclient = self.testClient.getApiClient()
+ self.hypervisor = self.testClient.getHypervisorInfo()
+ self.dbclient = self.testClient.getDbConnection()
+ self.services = self.testClient.getParsedTestDataConfig()
+ self.zone = get_zone(self.apiclient, self.testClient.getZoneForTests())
+ self.pod = get_pod(self.apiclient, self.zone.id)
+ self.cleanup = []
+ self.services = {
+ "service_offering": {
+ "name": "Ultra Tiny Instance",
+ "displaytext": "Ultra Tiny Instance",
+ "cpunumber": 1,
+ "cpuspeed": 100,
+ "memory": 128,
+ },
+ "vm": {
+ "username": "root",
+ "password": "password",
+ "ssh_port": 22,
+ # Hypervisor type should be same as
+ # hypervisor type of cluster
+ "privateport": 22,
+ "publicport": 22,
+ "protocol": 'TCP',
+ },
+ "natrule": {
+ "privateport": 22,
+ "publicport": 22,
+ "startport": 22,
+ "endport": 22,
+ "protocol": "TCP",
+ "cidrlist": '0.0.0.0/0',
+ },
+ "ostype": 'CentOS 5.3 (64-bit)',
+ "sleep": 60,
+ "timeout": 10,
+ }
+
+
+ def tearDown(self):
+ try:
+ # Clean up, terminate the created templates
+ cleanup_resources(self.apiclient, self.cleanup)
+
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+
+ return
+
+ def createVMs(self, hostId, number):
+
+ self.template = get_template(
+ self.apiclient,
+ self.zone.id,
+ self.services["ostype"]
+ )
+
+ if self.template == FAILED:
+ assert False, "get_template() failed to return template with description %s" % self.services["ostype"]
+
+ self.logger.debug("Using template %s " % self.template.id)
+
+ self.service_offering = ServiceOffering.create(
+ self.apiclient,
+ self.services["service_offering"]
+ )
+ self.logger.debug("Using service offering %s " % self.service_offering.id)
+
+ vms=[]
+ for i in range(0, number):
+ self.services["vm"]["zoneid"] = self.zone.id
+ self.services["vm"]["template"] = self.template.id
+ self.services["vm"]["displayname"] = 'vm' + str(i)
+ self.services["vm"]["hypervisor"] = self.hypervisor
+ vm = VirtualMachine.create(
+ self.apiclient,
+ self.services["vm"],
+ serviceofferingid=self.service_offering.id,
+ hostid=hostId
+ )
+ vms.append(vm)
+ self.cleanup.append(vm)
+ self.logger.debug("VM create = {}".format(vm.id))
+ return vms
+
+ def checkVmMigratingOnHost(self, hostId):
+ vm_migrating=False
+ listVms1 = VirtualMachine.list(
+ self.apiclient,
+ hostid=hostId
+ )
+
+ if (listVms1 is not None):
+ self.logger.debug('Vms found = {} '.format(len(listVms1)))
+ for vm in listVms1:
+ if (vm.state == "Migrating"):
+ self.logger.debug('VirtualMachine on Hyp id = {} is in {}'.format(vm.id, vm.state))
+ vm_migrating=True
+ break
+
+ return (vm_migrating,)
+
+ def NoOfVMsOnHost(self, hostId):
+ listVms = VirtualMachine.list(
+ self.apiclient,
+ hostid=hostId
+ )
+ no_of_vms=0
+ if (listVms is not None):
+ for vm in listVms:
+ self.logger.debug('VirtualMachine on Hyp 1 = {}'.format(vm.id))
+ no_of_vms=no_of_vms+1
+
+ return no_of_vms
+
+ @attr(
+ tags=[
+ "advanced",
+ "advancedns",
+ "smoke",
+ "basic",
+ "eip",
+ "sg"],
+ required_hardware="true")
+ def test_01_cancel_host_maintenace_with_no_migration_jobs(self):
+ listHost = Host.list(
+ self.apiclient,
+ type='Routing',
+ zoneid=self.zone.id,
+ podid=self.pod.id,
+ )
+ for host in listHost:
+ self.logger.debug('1 Hypervisor = {}'.format(host.id))
+
+
+ if (len(listHost) < 2):
+ raise unittest.SkipTest("Cancel host maintenance when VMs are migrating should be tested for 2 or more hosts");
+ return
+
+ vm_migrating=False
+
+ try:
+
+ target_host_id = listHost[0].id
+ other_host_id = listHost[1].id
+
+ cmd = prepareHostForMaintenance.prepareHostForMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.prepareHostForMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in prepareHostForMaintenance'.format(target_host_id))
+
+ #as soon as VM is picked for migration its last hostid is updated to the new host
+ # that is why VM shows up as migrating on the other host
+ vm_migrating = wait_until(1, 10, self.checkVmMigratingOnHost, other_host_id)
+
+ cmd = cancelHostMaintenance.cancelHostMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.cancelHostMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in cancelHostMaintenance'.format(target_host_id) )
+
+
+ target_host_id = listHost[1].id
+ other_host_id = listHost[0].id
+
+ cmd = prepareHostForMaintenance.prepareHostForMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.prepareHostForMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in prepareHostForMaintenance'.format(target_host_id))
+
+ vm_migrating = wait_until(1, 10, self.checkVmMigratingOnHost, other_host_id)
+
+ cmd = cancelHostMaintenance.cancelHostMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.cancelHostMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in cancelHostMaintenance'.format(target_host_id) )
+
+
+ except Exception as e:
+ self.logger.debug("Exception {}".format(e))
+ self.fail("Cancel host maintenance failed {}".format(e[0]))
+
+
+ if (vm_migrating == True):
+ raise unittest.SkipTest("VMs are migrating and the test will not be able to check the conditions the test is intended for");
+
+
+ return
+
+
+
+
+ @attr(
+ tags=[
+ "advanced",
+ "advancedns",
+ "smoke",
+ "basic",
+ "eip",
+ "sg"],
+ required_hardware="true")
+ def test_02_cancel_host_maintenace_with_migration_jobs(self):
+
+ listHost = Host.list(
+ self.apiclient,
+ type='Routing',
+ zoneid=self.zone.id,
+ podid=self.pod.id,
+ )
+ for host in listHost:
+ self.logger.debug('2 Hypervisor = {}'.format(host.id))
+
+ if (len(listHost) != 2):
+ raise unittest.SkipTest("Cancel host maintenance when VMs are migrating can only be tested with 2 hosts");
+ return
+
+
+ no_of_vms = self.NoOfVMsOnHost(listHost[0].id)
+
+ no_of_vms = no_of_vms + self.NoOfVMsOnHost(listHost[0].id)
+
+ if no_of_vms < 5:
+ self.logger.debug("Create VMs as there are not enough vms to check host maintenance")
+ no_vm_req = 5 - no_of_vms
+ if (no_vm_req > 0):
+ self.logger.debug("Creating vms = {}".format(no_vm_req))
+ self.vmlist = self.createVMs(listHost[0].id, no_vm_req)
+
+ vm_migrating=False
+
+ try:
+
+ target_host_id = listHost[0].id
+ other_host_id = listHost[1].id
+
+ cmd = prepareHostForMaintenance.prepareHostForMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.prepareHostForMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in prepareHostForMaintenance'.format(target_host_id))
+
+ #as soon as VM is picked for migration its last hostid is updated to the new host
+ # that is why VM shows up as migrating on the other host
+ vm_migrating = wait_until(1, 10, self.checkVmMigratingOnHost, other_host_id)
+
+ cmd = cancelHostMaintenance.cancelHostMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.cancelHostMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in cancelHostMaintenance'.format(target_host_id) )
+
+
+ target_host_id = listHost[1].id
+ other_host_id = listHost[0].id
+
+ cmd = prepareHostForMaintenance.prepareHostForMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.prepareHostForMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in prepareHostForMaintenance'.format(target_host_id))
+
+ vm_migrating = wait_until(1, 10, self.checkVmMigratingOnHost, other_host_id)
+
+ cmd = cancelHostMaintenance.cancelHostMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.cancelHostMaintenance(cmd)
--- End diff --
The functionality in these lines directly relate to the test case method they are in. Moving them to a method may compact the code, but will make the test case bit obfuscated. With the code expanded it is easy to see what the method is doing, since only the relevant code is expanded there, rest is already compacted by use of methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-214417865
@jburwell can I get your LGTM? I will run CI on this again today because some code has changed since the last run.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r58952716
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -2112,11 +2112,13 @@ private boolean doCancelMaintenance(final long hostId) {
/* TODO: move to listener */
_haMgr.cancelScheduledMigrations(host);
+
+ boolean vms_migrating = false;
final List<VMInstanceVO> vms = _haMgr.findTakenMigrationWork();
for (final VMInstanceVO vm : vms) {
- if (vm != null && vm.getHostId() != null && vm.getHostId() == hostId) {
- s_logger.info("Unable to cancel migration because the vm is being migrated: " + vm);
- return false;
+ if (vm.getHostId() != null && vm.getHostId() == hostId) {
--- End diff --
Is this conditional right?
`vm.getHostId() != null && vm.getHostId() == hostId`
It is looking a little weird to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-205624341
@swill In my environment I am using macchinina templates for testing. These are not there by default but are pretty handy in testing due to their small size. Adding these to CI environment will speed up many such tests. If it is lot of work I will revert to using standard CentOS templates, comments ?
"template_name_xen" : "macchinina-xen",
"template_name_kvm" : "macchinina-kvm",
cc @jburwell
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-203249781
Output from marvin test:
TMP=/tmp
CLOUDDIR=/root/cloudstack-apple
mkdir -p /tmp/simulator/smoke/misc
nosetests --with-xunit --xunit-file=/tmp/test_quagga.xml --with-marvin --marvin-config=/root/cloudstack-apple/advanced.cfg /root/cloudstack-apple/test/integration/component/test_host_maintenance.py -s -a tags=advanced,required_hardware=true --zone=Bootcamp --hypervisor=KVM
==== Marvin Init Started ====
=== Marvin Parse Config Successful ===
=== Marvin Setting TestData Successful===
==== Log Folder Path: /tmp//MarvinLogs//Mar_30_2016_10_02_53_F4MC43. All logs will be available here ====
=== Marvin Init Logging Successful===
==== Marvin Init Successful ====
1 Hypervisor = 3e270f2d-f054-4cbe-85e1-0fbc30e8a414
1 Hypervisor = 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4
Host with id 3e270f2d-f054-4cbe-85e1-0fbc30e8a414 is in prepareHostForMaintenance
Host with id 3e270f2d-f054-4cbe-85e1-0fbc30e8a414 is in cancelHostMaintenance
Host with id 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4 is in prepareHostForMaintenance
Host with id 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4 is in cancelHostMaintenance
=== TestName: test_01_cancel_host_maintenace_with_no_migration_jobs | Status : SUCCESS ===
2 Hypervisor = 3e270f2d-f054-4cbe-85e1-0fbc30e8a414
2 Hypervisor = 3e270f2d-f054-4cbe-85e1-0fbc30e8a414
2 Hypervisor = 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4
2 Hypervisor = 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4
Create VMs as there are not enough vms to check host maintenance
Create VMs as there are not enough vms to check host maintenance
Creating vms = 5
Creating vms = 5
Using template a18cc46d-d4e8-476f-bdf6-9f3e5818a200
Using template a18cc46d-d4e8-476f-bdf6-9f3e5818a200
Using service offering 5bee807e-602a-4203-b9c3-ed72797b1d93
Using service offering 5bee807e-602a-4203-b9c3-ed72797b1d93
VM create = b67468ff-8120-42d7-87fe-2c7361c4afd3
VM create = b67468ff-8120-42d7-87fe-2c7361c4afd3
VM create = 8309716e-03ac-442a-ab18-a80748d5988c
VM create = 8309716e-03ac-442a-ab18-a80748d5988c
VM create = af8d8991-0ee2-44ca-bee4-d706f883dc0a
VM create = af8d8991-0ee2-44ca-bee4-d706f883dc0a
VM create = 1bacebb1-9be7-40a1-99f3-7b7e9deb45b7
VM create = 1bacebb1-9be7-40a1-99f3-7b7e9deb45b7
VM create = eac8b28d-0bea-4912-b6e4-3402ac07e802
VM create = eac8b28d-0bea-4912-b6e4-3402ac07e802
Host with id 3e270f2d-f054-4cbe-85e1-0fbc30e8a414 is in prepareHostForMaintenance
Host with id 3e270f2d-f054-4cbe-85e1-0fbc30e8a414 is in prepareHostForMaintenance
Vms found = 2
Vms found = 2
VirtualMachine on Hyp id = 8309716e-03ac-442a-ab18-a80748d5988c is in Migrating
VirtualMachine on Hyp id = 8309716e-03ac-442a-ab18-a80748d5988c is in Migrating
Host with id 3e270f2d-f054-4cbe-85e1-0fbc30e8a414 is in cancelHostMaintenance
Host with id 3e270f2d-f054-4cbe-85e1-0fbc30e8a414 is in cancelHostMaintenance
Host with id 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4 is in prepareHostForMaintenance
Host with id 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4 is in prepareHostForMaintenance
Vms found = 4
Vms found = 4
VirtualMachine on Hyp id = 8309716e-03ac-442a-ab18-a80748d5988c is in Migrating
VirtualMachine on Hyp id = 8309716e-03ac-442a-ab18-a80748d5988c is in Migrating
Host with id 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4 is in cancelHostMaintenance
Host with id 6a34ca0a-ac2f-44db-a8f4-96b7de01dcd4 is in cancelHostMaintenance
=== TestName: test_02_cancel_host_maintenace_with_migration_jobs | Status : SUCCESS ===
===final results are now copied to: /tmp//MarvinLogs/test_host_maintenance_RF5DFR===
++ date
echo Wed Mar 30 10:06:16 IST 2016 Wed Mar 30 10:06:16 IST 2016
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r58965582
--- Diff: test/integration/component/test_host_maintenance.py ---
@@ -0,0 +1,325 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+""" BVT tests for Hosts Maintenance
+"""
+
+# Import Local Modules
+from marvin.codes import FAILED
+from marvin.cloudstackTestCase import *
+from marvin.cloudstackAPI import *
+from marvin.lib.utils import *
+from marvin.lib.base import *
+from marvin.lib.common import *
+from nose.plugins.attrib import attr
+
+from time import sleep
+
+_multiprocess_shared_ = False
+
+
+class TestHostMaintenance(cloudstackTestCase):
+
+ def setUp(self):
+ self.logger = logging.getLogger('TestHM')
+ self.stream_handler = logging.StreamHandler()
+ self.logger.setLevel(logging.DEBUG)
+ self.logger.addHandler(self.stream_handler)
+ self.apiclient = self.testClient.getApiClient()
+ self.hypervisor = self.testClient.getHypervisorInfo()
+ self.dbclient = self.testClient.getDbConnection()
+ self.services = self.testClient.getParsedTestDataConfig()
+ self.zone = get_zone(self.apiclient, self.testClient.getZoneForTests())
+ self.pod = get_pod(self.apiclient, self.zone.id)
+ self.cleanup = []
+ self.services = {
+ "service_offering": {
+ "name": "Ultra Tiny Instance",
+ "displaytext": "Ultra Tiny Instance",
+ "cpunumber": 1,
+ "cpuspeed": 100,
+ "memory": 128,
+ },
+ "vm": {
+ "username": "root",
+ "password": "password",
+ "ssh_port": 22,
+ # Hypervisor type should be same as
+ # hypervisor type of cluster
+ "privateport": 22,
+ "publicport": 22,
+ "protocol": 'TCP',
+ },
+ "natrule": {
+ "privateport": 22,
+ "publicport": 22,
+ "startport": 22,
+ "endport": 22,
+ "protocol": "TCP",
+ "cidrlist": '0.0.0.0/0',
+ },
+ "ostype": 'CentOS 5.3 (64-bit)',
+ "sleep": 60,
+ "timeout": 10,
+ }
+
+
+ def tearDown(self):
+ try:
+ # Clean up, terminate the created templates
+ cleanup_resources(self.apiclient, self.cleanup)
+
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+
+ return
+
+ def createVMs(self, hostId, number):
+
+ self.template = get_template(
+ self.apiclient,
+ self.zone.id,
+ self.services["ostype"]
+ )
+
+ if self.template == FAILED:
+ assert False, "get_template() failed to return template with description %s" % self.services["ostype"]
+
+ self.logger.debug("Using template %s " % self.template.id)
+
+ self.service_offering = ServiceOffering.create(
+ self.apiclient,
+ self.services["service_offering"]
+ )
+ self.logger.debug("Using service offering %s " % self.service_offering.id)
+
+ vms=[]
+ for i in range(0, number):
+ self.services["vm"]["zoneid"] = self.zone.id
+ self.services["vm"]["template"] = self.template.id
+ self.services["vm"]["displayname"] = 'vm' + str(i)
+ self.services["vm"]["hypervisor"] = self.hypervisor
+ vm = VirtualMachine.create(
+ self.apiclient,
+ self.services["vm"],
+ serviceofferingid=self.service_offering.id,
+ hostid=hostId
+ )
+ vms.append(vm)
+ self.cleanup.append(vm)
+ self.logger.debug("VM create = {}".format(vm.id))
+ return vms
+
+ def checkVmMigratingOnHost(self, hostId):
+ vm_migrating=False
+ listVms1 = VirtualMachine.list(
+ self.apiclient,
+ hostid=hostId
+ )
+
+ if (listVms1 is not None):
+ self.logger.debug('Vms found = {} '.format(len(listVms1)))
+ for vm in listVms1:
+ if (vm.state == "Migrating"):
+ self.logger.debug('VirtualMachine on Hyp id = {} is in {}'.format(vm.id, vm.state))
+ vm_migrating=True
+ break
+
+ return (vm_migrating,)
+
+ def NoOfVMsOnHost(self, hostId):
+ listVms = VirtualMachine.list(
+ self.apiclient,
+ hostid=hostId
+ )
+ no_of_vms=0
+ if (listVms is not None):
+ for vm in listVms:
+ self.logger.debug('VirtualMachine on Hyp 1 = {}'.format(vm.id))
+ no_of_vms=no_of_vms+1
+
+ return no_of_vms
+
+ @attr(
+ tags=[
+ "advanced",
+ "advancedns",
+ "smoke",
+ "basic",
+ "eip",
+ "sg"],
+ required_hardware="true")
+ def test_01_cancel_host_maintenace_with_no_migration_jobs(self):
+ listHost = Host.list(
+ self.apiclient,
+ type='Routing',
+ zoneid=self.zone.id,
+ podid=self.pod.id,
+ )
+ for host in listHost:
+ self.logger.debug('1 Hypervisor = {}'.format(host.id))
+
+
+ if (len(listHost) < 2):
+ raise unittest.SkipTest("Cancel host maintenance when VMs are migrating should be tested for 2 or more hosts");
+ return
+
+ vm_migrating=False
+
+ try:
+
+ target_host_id = listHost[0].id
+ other_host_id = listHost[1].id
+
+ cmd = prepareHostForMaintenance.prepareHostForMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.prepareHostForMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in prepareHostForMaintenance'.format(target_host_id))
+
+ #as soon as VM is picked for migration its last hostid is updated to the new host
+ # that is why VM shows up as migrating on the other host
+ vm_migrating = wait_until(1, 10, self.checkVmMigratingOnHost, other_host_id)
+
+ cmd = cancelHostMaintenance.cancelHostMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.cancelHostMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in cancelHostMaintenance'.format(target_host_id) )
+
+
+ target_host_id = listHost[1].id
+ other_host_id = listHost[0].id
+
+ cmd = prepareHostForMaintenance.prepareHostForMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.prepareHostForMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in prepareHostForMaintenance'.format(target_host_id))
+
+ vm_migrating = wait_until(1, 10, self.checkVmMigratingOnHost, other_host_id)
+
+ cmd = cancelHostMaintenance.cancelHostMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.cancelHostMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in cancelHostMaintenance'.format(target_host_id) )
+
+
+ except Exception as e:
+ self.logger.debug("Exception {}".format(e))
+ self.fail("Cancel host maintenance failed {}".format(e[0]))
+
+
+ if (vm_migrating == True):
+ raise unittest.SkipTest("VMs are migrating and the test will not be able to check the conditions the test is intended for");
+
+
+ return
+
+
+
+
+ @attr(
+ tags=[
+ "advanced",
+ "advancedns",
+ "smoke",
+ "basic",
+ "eip",
+ "sg"],
+ required_hardware="true")
+ def test_02_cancel_host_maintenace_with_migration_jobs(self):
+
+ listHost = Host.list(
+ self.apiclient,
+ type='Routing',
+ zoneid=self.zone.id,
+ podid=self.pod.id,
+ )
+ for host in listHost:
+ self.logger.debug('2 Hypervisor = {}'.format(host.id))
+
+ if (len(listHost) != 2):
+ raise unittest.SkipTest("Cancel host maintenance when VMs are migrating can only be tested with 2 hosts");
+ return
+
+
+ no_of_vms = self.NoOfVMsOnHost(listHost[0].id)
+
+ no_of_vms = no_of_vms + self.NoOfVMsOnHost(listHost[0].id)
+
+ if no_of_vms < 5:
+ self.logger.debug("Create VMs as there are not enough vms to check host maintenance")
+ no_vm_req = 5 - no_of_vms
+ if (no_vm_req > 0):
+ self.logger.debug("Creating vms = {}".format(no_vm_req))
+ self.vmlist = self.createVMs(listHost[0].id, no_vm_req)
+
+ vm_migrating=False
+
+ try:
+
+ target_host_id = listHost[0].id
+ other_host_id = listHost[1].id
+
+ cmd = prepareHostForMaintenance.prepareHostForMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.prepareHostForMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in prepareHostForMaintenance'.format(target_host_id))
+
+ #as soon as VM is picked for migration its last hostid is updated to the new host
+ # that is why VM shows up as migrating on the other host
+ vm_migrating = wait_until(1, 10, self.checkVmMigratingOnHost, other_host_id)
+
+ cmd = cancelHostMaintenance.cancelHostMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.cancelHostMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in cancelHostMaintenance'.format(target_host_id) )
+
+
+ target_host_id = listHost[1].id
+ other_host_id = listHost[0].id
+
+ cmd = prepareHostForMaintenance.prepareHostForMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.prepareHostForMaintenance(cmd)
+
+ self.logger.debug('Host with id {} is in prepareHostForMaintenance'.format(target_host_id))
+
+ vm_migrating = wait_until(1, 10, self.checkVmMigratingOnHost, other_host_id)
+
+ cmd = cancelHostMaintenance.cancelHostMaintenanceCmd()
+ cmd.id = target_host_id
+ response = self.apiclient.cancelHostMaintenance(cmd)
--- End diff --
Lines 296-309 appears to repeat with the only the role of the target_host and other_host varying. Consider extracting into a function that parameterizes these variances to reduce the length of and clarifies the test functions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r58965445
--- Diff: tools/marvin/marvin/lib/utils.py ---
@@ -520,4 +520,22 @@ def verifyRouterState(apiclient, routerid, allowedstates):
if routers[0].state.lower() not in allowedstates:
return [FAIL, "state of the router should be in %s but is %s" %
(allowedstates, routers[0].state)]
- return [PASS, None]
\ No newline at end of file
+ return [PASS, None]
+
+
+
+def wait_until(retry_interval=2, no_of_times=2, callback=None, *callback_args):
+ """ Utility method to try out the callback method at most no_of_times with a interval of retry_interval,
+ Will return immediately if callback returns True. The callback method should be written to return a list of values first being a boolean """
+
+ if callback is None:
+ return INVALID_INPUT
+ success = False
+ for i in range(0,no_of_times):
+ time.sleep(retry_interval)
+ success = callback(*callback_args)
+ if success[0]:
+ break
+
+ return success
--- End diff --
Consider a multi-return -- allowing callers to unambiguously determine success/failure vs the results of the method
```
return (success, result)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-205672352
On comparing test execution times with macchinina Vs Centos builtin template I did not find much of a difference. So modifying the test to go with the standard builtin templates. @swill
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---