[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[asfgit]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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)
+
 

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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)
+
+ 

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[rafaelweingartner]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[rafaelweingartner]

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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[rafaelweingartner]

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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 + 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[cristofolini]

Github user cristofolini commented on the pull request:

https://github.com/apache/cloudstack/pull/1454#issuecomment-20457
  
@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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[alexandrelimassantana]

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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[jburwell]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9323: Fix cancel host maintena...

[abhinandanprateek]

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 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---