[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218945625
  
@swill I've tried to fix them here: 
https://github.com/apache/cloudstack/pull/1544


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218944171
  
@swill thanks, will have a look at it


---
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-9299: Out-of-band Management f...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218875130
  
Jenkins for #1537 is also being held up by these tests:
```
---
 T E S T S
---
Running 
org.apache.cloudstack.outofbandmanagement.driver.ipmitool.IpmitoolWrapperTest
Tests run: 8, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 300.337 sec 
<<< FAILURE! - in 
org.apache.cloudstack.outofbandmanagement.driver.ipmitool.IpmitoolWrapperTest

testExecuteCommands(org.apache.cloudstack.outofbandmanagement.driver.ipmitool.IpmitoolWrapperTest)
  Time elapsed: 300.086 sec  <<< FAILURE!
java.lang.AssertionError: null
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertTrue(Assert.java:52)
at 
org.apache.cloudstack.outofbandmanagement.driver.ipmitool.IpmitoolWrapperTest.testExecuteCommands(IpmitoolWrapperTest.java:112)

Results :
Failed tests: 
  IpmitoolWrapperTest.testExecuteCommands:112 null

Tests run: 8, Failures: 1, Errors: 0, Skipped: 0
```


---
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-9299: Out-of-band Management f...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218861696
  
I see the following error which is causing the PR #1297 to fail.  
Suggestions?
```

+-+--++
| test_oobm_zchange_password  | exceptions.Exception | 
5.220  |

+-+--++
```


---
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-9299: Out-of-band Management f...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1502


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218784278
  
@swill @rhtyd LGTM based on code review


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218677864
  
tag:mergeready

/cc @swill all green 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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218666241
  
@jburwell fixed the ProcessRunner issues, please do a final review and LGTM 
or suggest changes. Thanks.

@nvazquez @swill I've fixed two CI issues (Travis and Jenkins issues) in 
this PR as well


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd closed the pull request at:

https://github.com/apache/cloudstack/pull/1502


---
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-9299: Out-of-band Management f...

[rhtyd]

GitHub user rhtyd reopened a pull request:

https://github.com/apache/cloudstack/pull/1502

CLOUDSTACK-9299: Out-of-band Management for CloudStack

Support access to a host’s out-of-band management interface (e.g. IPMI, 
iLO,
DRAC, etc.) to manage host power operations (on/off etc.) and querying 
current
power state in CloudStack.

Given the wide range of out-of-band management interfaces such as iLO and 
iDRA,
the service implementation allows for development of separate drivers as 
plugins.
This feature comes with a ipmitool based driver that uses the
ipmitool (http://linux.die.net/man/1/ipmitool) to communicate with any
out-of-band management interface that support IPMI 2.0.

This feature allows following common use-cases:
- Restarting stalled/failed hosts
- Powering off under-utilised hosts
- Powering on hosts for provisioning or to increase capacity
- Allowing system administrators to see the current power state of the host

For testing this feature, please install `ipmitool` (using yum/apt/brew) 
and `ipmisim`:
https://pypi.python.org/pypi/ipmisim

The default ipmitool location is assumed in /usr/bin, if this is different 
in your env please fix the global setting, see FS for details on various global 
settings.

FS:

https://cwiki.apache.org/confluence/display/CLOUDSTACK/Out-of-band+Management+for+CloudStack

/cc @jburwell @swill @abhinandanprateek @murali-reddy @borisstoyanov

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/shapeblue/cloudstack outofband-master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1502.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 #1502


commit 07564469e9a2ddd0334e8bdd30deed3ed53c3f09
Author: Rohit Yadav 
Date:   2015-12-28T11:07:03Z

CLOUDSTACK-9299: Out-of-band Management for CloudStack

Support access to a host’s out-of-band management interface (e.g. IPMI, 
iLO,
DRAC, etc.) to manage host power operations (on/off etc.) and querying 
current
power state in CloudStack.

Given the wide range of out-of-band management interfaces such as iLO and 
iDRA,
the service implementation allows for development of separate drivers as 
plugins.
This feature comes with a ipmitool based driver that uses the
ipmitool (http://linux.die.net/man/1/ipmitool) to communicate with any
out-of-band management interface that support IPMI 2.0.

This feature allows following common use-cases:
- Restarting stalled/failed hosts
- Powering off under-utilised hosts
- Powering on hosts for provisioning or to increase capacity
- Allowing system administrators to see the current power state of the host

For testing this feature `ipmisim` can be used:
https://pypi.python.org/pypi/ipmisim

FS:

https://cwiki.apache.org/confluence/display/CLOUDSTACK/Out-of-band+Management+for+CloudStack

Signed-off-by: Rohit Yadav 

commit 4d5e8df2f952c4e0594c8fc4d11181f6d3da3811
Author: Rohit Yadav 
Date:   2016-05-06T00:22:07Z

travis: Use patched version of ipmitool for tests

- For out-of-band management feature (CLOUDSTACK-9299) use patched version 
of
  ipmitool that would work on trusty travis machines
- The ipmitool used is from xenial/16.04 release with patch from RedHat
  https://bugzilla.redhat.com/show_bug.cgi?id=1286035
- Installs ipmitool from xenial repositories to get all the dependencies
  and then install patched deb version
- Skip test if the known failure occurs

Signed-off-by: Rohit Yadav 

commit e122bbfbfcf13dcec6e4848e8a8b0047d060
Author: Rohit Yadav 
Date:   2016-05-11T08:59:22Z

HypervisorUtilsTest: increate timeout to 8seconds

Increases timeout to a larger value to avoid failures in VM environments 
such as
TravisCI.

Signed-off-by: Rohit Yadav 

commit 6135f6d98fa6b363b0468c17a8e713aeb5054437
Author: Rohit Yadav 
Date:   2016-05-12T05:34:37Z

CLOUDSTACK-9378: Fix for #1497

Reorder cleanup items so cleanup won't fail

Signed-off-by: Rohit Yadav 




---
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-9299: Out-of-band Management f...

[serg38]

Github user serg38 commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218543261
  
@swill PR1539 passed Jenkins and Travis. After you merge it should resolve 
the issue in other PRs 


---
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-9299: Out-of-band Management f...

[nvazquez]

Github user nvazquez commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218520519
  
@rhtyd @kiwiflyer @swill a PR for fixing the problem #1539 


---
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-9299: Out-of-band Management f...

[nvazquez]

Github user nvazquez commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218468571
  
@rhtyd sure, I'll examine this. This tests are introduced in #1497, I'll 
work on this


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218421442
  
@nvazquez can you check why test_03_list_snapshots failed in the Travis 
run, with tearDown exception.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218373527
  
@jburwell can you do a final review, LGTM or share further improvements. 
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62790898
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
--- End diff --

@jburwell I'll modify the executeCommands method to accept an executor; the 
ipmitool driver would create and re-use a fixedThreadPool


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62790877
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
--- End diff --

@jburwell ipmitool driver does not execute the command with a timeout, the 
utility needs to be generic which is why an option is provided to no consider 
timeout (0 = no timeout). The users of the utility need to make the decision 
and not this utility. For certain cases, it may be acceptable to execute a 
process without any time boundary therefore the utility should allow for both 
use-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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62661909
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
--- End diff --

Reuse does not prevent overwhelming the management server.  A flood of new 
requests would spawn an explosion of thread creation.  A ``fixedThreadPool`` or 
a bounded blocking queue would protect against such a scenario.

One approach would be to be convert make this class instanceable and pass 
in a pool size via a constructor.  The OOBM subsystem could then initialize it 
with a global setting or a sane default value.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62661197
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
--- End diff --

@rhtyd I don't think it acceptable to allow a process without any time 
boundary.  The concern is that a series of stalled commands could consume the 
thread pool and starve out other clients.   However, as a convenience to users 
of the class, it also makes senses that we would require the specification of a 
timeout on each call.  

A solution to this issue would be as follows:

1. Define a ``private final static Duration DEFAULT_COMMAND_TIMEOUT``
1. Provide an ``executeCommands`` override implementation:
```
public static ProcessResult executeCommands(final List commands) {
return executeCommands(commands, DEFAULT_COMMAND_TIMEOUT);
}
```
1. Modify the preconditions of this method to require a timeout greater 
than zero and simplify it to remove handling of that case.

This approach not only ensures that all commands are (softly) time-bounded, 
but it also avoid the [flag argument 
anti-pattern](http://martinfowler.com/bliki/FlagArgument.html).


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62656777
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,109 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) throws 
IOException {
+final StringBuilder sb = new StringBuilder();
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+sb.append(line);
+sb.append("\n");
+}
+return sb.toString();
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
+final Future processFuture = 
processExecutor.submit(new Callable() {
+@Override
+public Integer call() throws Exception {
+return process.waitFor();
+}
+});
+try {
+retVal = 
processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS);
+} catch (ExecutionException e) {
+retVal = -1;
+stdError = e.getMessage();
+if (LOG.isTraceEnabled()) {
+LOG.trace("Failed to complete the requested 
command due to execution error: " + e.getMessage());
+}
+} catch (TimeoutException e) {
+retVal = -1;
+stdError = "Operation timed out, aborted";
+if (LOG.isTraceEnabled()) {
+LOG.trace("Failed to complete the requested 
command within timeout: " + e.getMessage());
+}
+} finally {
+if (Strings.isNullOrEmpty(stdError)) {
+stdOutput = readStream(process.getInputStream());
--- End diff --

Of course I've not, I'll play with it.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62655993
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,109 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) throws 
IOException {
+final StringBuilder sb = new StringBuilder();
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+sb.append(line);
+sb.append("\n");
+}
+return sb.toString();
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
+final Future processFuture = 
processExecutor.submit(new Callable() {
+@Override
+public Integer call() throws Exception {
+return process.waitFor();
+}
+});
+try {
+retVal = 
processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS);
+} catch (ExecutionException e) {
+retVal = -1;
+stdError = e.getMessage();
+if (LOG.isTraceEnabled()) {
+LOG.trace("Failed to complete the requested 
command due to execution error: " + e.getMessage());
+}
+} catch (TimeoutException e) {
+retVal = -1;
+stdError = "Operation timed out, aborted";
+if (LOG.isTraceEnabled()) {
+LOG.trace("Failed to complete the requested 
command within timeout: " + e.getMessage());
+}
+} finally {
+if (Strings.isNullOrEmpty(stdError)) {
+stdOutput = readStream(process.getInputStream());
--- End diff --

Have you considered using Guava's ``CharStreams.toString(new 
BufferedWriter(process.getInputStream))`` instead of ``readStream``?


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62655962
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,109 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) throws 
IOException {
+final StringBuilder sb = new StringBuilder();
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+sb.append(line);
+sb.append("\n");
+}
+return sb.toString();
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
+final Future processFuture = 
processExecutor.submit(new Callable() {
+@Override
+public Integer call() throws Exception {
+return process.waitFor();
+}
+});
+try {
+retVal = 
processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS);
+} catch (ExecutionException e) {
+retVal = -1;
+stdError = e.getMessage();
+if (LOG.isTraceEnabled()) {
+LOG.trace("Failed to complete the requested 
command due to execution error: " + e.getMessage());
+}
+} catch (TimeoutException e) {
+retVal = -1;
+stdError = "Operation timed out, aborted";
+if (LOG.isTraceEnabled()) {
+LOG.trace("Failed to complete the requested 
command within timeout: " + e.getMessage());
+}
+} finally {
+if (Strings.isNullOrEmpty(stdError)) {
+stdOutput = readStream(process.getInputStream());
+stdError = readStream(process.getErrorStream());
--- End diff --

Have you considered using Guava's ``CharStreams.toString(new 
BufferedWriter(process.getErrorStream))`` instead of ``readStream``?


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

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62616838
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessResult.java ---
@@ -0,0 +1,46 @@
+// 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.
+
+package org.apache.cloudstack.utils.process;
+
+public class ProcessResult {
--- End diff --

Not making this nested inside ProcessRunner, ProcessResult is used in oobm 
driver. The constructor is the only way to initialize the inner items, which 
are final. The class if final 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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62616710
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
+final Future processFuture = 
processExecutor.submit(new Callable() {
+@Override
+public Integer call() throws Exception {
+return process.waitFor();
+}
+});
+try {
+retVal = 
processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS);
+} catch (ExecutionException | TimeoutException e) {
--- End diff --

Fixed


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62616608
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
--- End diff --

Fixed


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62616616
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
--- End diff --

Thrown


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-218059135
  
@jburwell as mentioned in comments above, due to a bug in ipmitool itself 
(https://bugzilla.redhat.com/show_bug.cgi?id=1286035) some executions may fail. 
I'll add code to skip/workaround that known bug for the specific failing test, 
I had already added some code to skip test when the specific ipmitool bug was 
hit.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62612293
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
+final Future processFuture = 
processExecutor.submit(new Callable() {
+@Override
+public Integer call() throws Exception {
+return process.waitFor();
+}
+});
+try {
+retVal = 
processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS);
+} catch (ExecutionException | TimeoutException e) {
+retVal = -1;
+stdError = "Operation timed out, aborted";
+if (LOG.isTraceEnabled()) {
+LOG.trace("Failed to complete the requested 
command within timeout: " + e.getMessage());
+}
+} finally {
+if (Strings.isNullOrEmpty(stdError)) {
+stdOutput = readStream(process.getInputStream());
+stdError = readStream(process.getErrorStream());
+}
+}
+} else {
+retVal = process.waitFor();
+stdOutput = readStream(process.getInputStream());
+stdError = readStream(process.getErrorStream());
+}
+process.destroy();
--- End diff --

process.destroy() is common to both cases when timeout is 0 or non-zero; 
cleanup is ensured in either 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62612221
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
--- End diff --

This is a general utility, so it won't be possible to configure the 
executor service pool size here without passing a global setting variable on 
initialization. The cached pool ensures to reuse threads and still a better way 
than spawning separate threads on each run. Wrt oobm, for each issue command 
only one thread is spawned that execs an ipmitool process, a maximum of 250 (or 
size specified in db.properties which forces the size of max. no of concurrent 
async jobs) ipmitool processes can be spawned at any given time.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62612254
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessResult.java ---
@@ -0,0 +1,46 @@
+// 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.
+
+package org.apache.cloudstack.utils.process;
+
+public class ProcessResult {
--- End diff --

Will fix that.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62612076
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
+final Future processFuture = 
processExecutor.submit(new Callable() {
+@Override
+public Integer call() throws Exception {
+return process.waitFor();
+}
+});
+try {
+retVal = 
processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS);
+} catch (ExecutionException | TimeoutException e) {
--- End diff --

Will split the catch cases separately.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62612035
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
--- End diff --

Yes, in case of initialization where we are checking the version etc a 
timeout may not be specified (0 = wait until process returns/exits). Also, this 
is a general utility not specifically tied to oobm.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62570929
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
+final Future processFuture = 
processExecutor.submit(new Callable() {
+@Override
+public Integer call() throws Exception {
+return process.waitFor();
+}
+});
+try {
+retVal = 
processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS);
+} catch (ExecutionException | TimeoutException e) {
+retVal = -1;
+stdError = "Operation timed out, aborted";
+if (LOG.isTraceEnabled()) {
+LOG.trace("Failed to complete the requested 
command within timeout: " + e.getMessage());
+}
+} finally {
+if (Strings.isNullOrEmpty(stdError)) {
+stdOutput = readStream(process.getInputStream());
+stdError = readStream(process.getErrorStream());
+}
+}
+} else {
+retVal = process.waitFor();
+stdOutput = readStream(process.getInputStream());
+stdError = readStream(process.getErrorStream());
+}
+process.destroy();
--- End diff --

Should this call be part of a ``finally`` block to ensure cleanup in all 
circumstances?


---
If your 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62570751
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessResult.java ---
@@ -0,0 +1,46 @@
+// 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.
+
+package org.apache.cloudstack.utils.process;
+
+public class ProcessResult {
--- End diff --

Could this class be collapsed as a ``static final`` class in 
``ProcessRunner``?  Also, the class should be ``final`` and the constructor to 
should either be default (standalone class) or ``private`` (static inner class).


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62570445
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
--- End diff --

A cached thread pool is unbounded.  Should we consider the use of a bounded 
thread pool to prevent shell processes from overwhelming the management server?


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62569864
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
+final Future processFuture = 
processExecutor.submit(new Callable() {
+@Override
+public Integer call() throws Exception {
+return process.waitFor();
+}
+});
+try {
+retVal = 
processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS);
+} catch (ExecutionException | TimeoutException e) {
--- End diff --

Why are signaling a failure with a return value instead of an exception?


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62569447
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
--- End diff --

Why don't we re-throw this exception as an unchecked exception?  Failing to 
read the standard out and error streams seems like it would represent a failure 
of the operation.  


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62569193
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
--- End diff --

Convert to a ``StringBuilder`` to avoid unnecessary String re-allocation.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62568855
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,112 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.log4j.Logger;
+import org.joda.time.Duration;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private static final ExecutorService processExecutor = 
Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner"));
+
+private static String readStream(final InputStream inputStream) {
+String text = null;
+try {
+final BufferedReader bufferedReader = new BufferedReader(new 
InputStreamReader(inputStream));
+String line;
+while ((line = bufferedReader.readLine()) != null) {
+if (Strings.isNullOrEmpty(text)) {
+text = line;
+} else {
+text = text + "\n" + line;
+}
+}
+} catch (IOException e) {
+if (LOG.isTraceEnabled()) {
+LOG.trace("ProcessRunner::readStream failed due to: " + 
e.getMessage());
+}
+}
+return text;
+}
+
+public static ProcessResult executeCommands(final List 
commands, final Duration timeOut) {
+Preconditions.checkArgument(commands != null && timeOut != null);
+
+int retVal = -2;
+String stdOutput = null;
+String stdError = null;
+
+try {
+final Process process = new 
ProcessBuilder().command(commands).start();
+if (timeOut.getStandardSeconds() > 0) {
--- End diff --

Should a command without a timeout be accepted? 


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217944726
  
@rhtyd it looks like one of the OOBM smoke tests failed on the Travis 
build.  Could you please investigate?


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217775304
  
@jburwell I've fixed the outstanding issues, please re-review and suggest 
any other improvement or LGTM. 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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r6206
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
+ProcessRunner result = new ProcessRunner();
+try {
+Process process = new 
ProcessBuilder().command(commands).start();
+StreamGobbler stdInputGobbler = new 
StreamGobbler(process.getInputStream());
+StreamGobbler stdErrorGobbler = new 
StreamGobbler(process.getErrorStream());
--- End diff --

inheritIO is not useful here as we want to capture the input/output stream 
in stdErr and stdOut. Using future and a cache thread pool executor, I've 
removed the explicit use of a stream gobbler here.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62444371
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
+ProcessRunner result = new ProcessRunner();
+try {
+Process process = new 
ProcessBuilder().command(commands).start();
+StreamGobbler stdInputGobbler = new 
StreamGobbler(process.getInputStream());
+StreamGobbler stdErrorGobbler = new 
StreamGobbler(process.getErrorStream());
+stdInputGobbler.start();
+stdErrorGobbler.start();
+
+if (timeOutSeconds > 0) {
+ProcessWaitForThread worker = new 
ProcessWaitForThread(process);
--- End diff --

Removed use of this idiom, instead using Future and .get(timeout, ...).


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62442464
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
--- End diff --

Fixed, made members final.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62442087
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
--- End diff --

Fixed


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62441896
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -412,6 +428,36 @@ protected void runInContext() {
 }
 }
 
+class HostOutOfBandManagementStatsCollector extends 
ManagedContextRunnable {
+@Override
+protected void runInContext() {
+try {
+s_logger.debug("HostOutOfBandManagementStatsCollector is 
running...");
+List outOfBandManagementHosts = 
outOfBandManagementDao.findAllByManagementServer(ManagementServerNode.getManagementServerId());
+if (outOfBandManagementHosts == null) {
+return;
+}
+for (OutOfBandManagement outOfBandManagementHost : 
outOfBandManagementHosts) {
+Host host = 
_hostDao.findById(outOfBandManagementHost.getHostId());
+if (host == null) {
+continue;
+}
+if 
(outOfBandManagementService.isOutOfBandManagementEnabled(host)) {
+
outOfBandManagementService.submitBackgroundPowerSyncTask(host);
+} else {
+if (outOfBandManagementHost.getPowerState() != 
OutOfBandManagement.PowerState.Disabled) {
+if 
(outOfBandManagementService.transitionPowerStateToDisabled(Collections.singletonList(host)))
 {
+s_logger.debug("Out-of-band management was 
disabled in zone/cluster/host, disabled power state for host id:" + 
host.getId());
--- End diff --

Fixed


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62441858
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -412,6 +428,36 @@ protected void runInContext() {
 }
 }
 
+class HostOutOfBandManagementStatsCollector extends 
ManagedContextRunnable {
+@Override
+protected void runInContext() {
+try {
+s_logger.debug("HostOutOfBandManagementStatsCollector is 
running...");
+List outOfBandManagementHosts = 
outOfBandManagementDao.findAllByManagementServer(ManagementServerNode.getManagementServerId());
+if (outOfBandManagementHosts == null) {
+return;
+}
+for (OutOfBandManagement outOfBandManagementHost : 
outOfBandManagementHosts) {
+Host host = 
_hostDao.findById(outOfBandManagementHost.getHostId());
+if (host == null) {
+continue;
+}
+if 
(outOfBandManagementService.isOutOfBandManagementEnabled(host)) {
+
outOfBandManagementService.submitBackgroundPowerSyncTask(host);
+} else {
+if (outOfBandManagementHost.getPowerState() != 
OutOfBandManagement.PowerState.Disabled) {
+if 
(outOfBandManagementService.transitionPowerStateToDisabled(Collections.singletonList(host)))
 {
--- End diff --

Fixed


---
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-9299: Out-of-band Management f...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217606318
  


### CI RESULTS

```
Tests Run: 85
  Skipped: 0
   Failed: 1
   Errors: 0
 Duration: 6h 07m 49s
```

**Summary of the problem(s):**
```
FAIL: test_03_vpc_privategw_restart_vpc_cleanup 
(integration.smoke.test_privategw_acl.TestPrivateGwACL)
--
Traceback (most recent call last):
  File 
"/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 
265, in test_03_vpc_privategw_restart_vpc_cleanup
self.performVPCTests(vpc_off, True)
  File 
"/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 
332, in performVPCTests
self.check_pvt_gw_connectivity(vm2, public_ip_2, vm1.nic[0].ipaddress)
  File 
"/data/git/cs1/cloudstack/test/integration/smoke/test_privategw_acl.py", line 
559, in check_pvt_gw_connectivity
"Ping to outside world from VM should be successful"
AssertionError: Ping to outside world from VM should be successful
--
Additional details in: /tmp/MarvinLogs/test_network_FIVJNA/results.txt
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_31_06_LPI7VW:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_31_06_LPI7VW/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_31_06_LPI7VW/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_31_06_LPI7VW/runinfo.txt)

**`/tmp/MarvinLogs/test_network_FIVJNA:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_network_FIVJNA/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_network_FIVJNA/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_network_FIVJNA/runinfo.txt)

**`/tmp/MarvinLogs/test_vpc_routers_WUHEVI:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_vpc_routers_WUHEVI/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_vpc_routers_WUHEVI/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1502/tmp/MarvinLogs/test_vpc_routers_WUHEVI/runinfo.txt)


Uploads will be available until `2016-07-07 02:00:00 +0200 CEST`

*Comment created by [`upr comment`](https://github.com/cloudops/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-9299: Out-of-band Management f...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217515255
  
Both this one and dynamic roles are queued up for CI as soon as I stabilize 
master.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217501592
  
@jburwell thanks, I'll sort them out. Meanwhile, if we can get the 
dynamic-roles PR merged today/tomorrow, I can use the annotations/validation 
usage in APIs and further improve the PR


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217434570
  
@rhtyd @swill we are very close.  I have a couple of outstanding comments 
-- particularly around potential thread explosion in ``ProcessRunner``.  
Ultimately, I think the issues are straightforward to address.

/cc @borisstoyanov


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62325927
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
+ProcessRunner result = new ProcessRunner();
+try {
+Process process = new 
ProcessBuilder().command(commands).start();
+StreamGobbler stdInputGobbler = new 
StreamGobbler(process.getInputStream());
+StreamGobbler stdErrorGobbler = new 
StreamGobbler(process.getErrorStream());
--- End diff --

The ``StreamGobbler`` idiom was replaced with ``ProcessBuilder.inheritIO`` 
in Java 1.7+.  Using it would simplify this code and allow the removal of the 
``StreamGobbler`` class.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62325553
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
+ProcessRunner result = new ProcessRunner();
+try {
+Process process = new 
ProcessBuilder().command(commands).start();
+StreamGobbler stdInputGobbler = new 
StreamGobbler(process.getInputStream());
+StreamGobbler stdErrorGobbler = new 
StreamGobbler(process.getErrorStream());
+stdInputGobbler.start();
+stdErrorGobbler.start();
+
+if (timeOutSeconds > 0) {
+ProcessWaitForThread worker = new 
ProcessWaitForThread(process);
--- End diff --

I am concerned about unbounded thread growth here.  Please consider using a 
bounded ``Executor`` and a 
[``ListenableFuture``](https://github.com/google/guava/wiki/ListenableFutureExplained)
 here.  It would also remove the need for the ``ProcessWaitForThread`` class.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62322683
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
+
+public String getStdOutput() {
+return stdOutput;
+}
+
+public void setStdOutput(String stdOutput) {
+this.stdOutput = stdOutput;
+}
+
+public String getStdError() {
+return stdError;
+}
+
+public void setStdError(String stdError) {
+this.stdError = stdError;
+}
+
+public int getReturnCode() {
+return returnCode;
+}
+
+public void setReturnCode(int returnCode) {
+this.returnCode = returnCode;
+}
+
+public static ProcessRunner executeCommands(List commands, 
long timeOutSeconds) {
--- End diff --

Consider adding ``Preconditions.checkArguments`` to verify that 
``commands`` is not ``null``.  

Also, consider using JodaTime's ``Duration`` to represent the timeout as 
number with a unit of measure.  This step is not required for LGTM, but would 
be a good refactoring if we fit it in.



---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62322543
  
--- Diff: 
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java ---
@@ -0,0 +1,111 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.utils.process;
+
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.List;
+
+public class ProcessRunner {
+public static final Logger LOG = Logger.getLogger(ProcessRunner.class);
+
+private String stdOutput;
+private String stdError;
+private int returnCode = -1;
--- End diff --

Consider making this class immutable and/or creating a static, immutable 
inner class (e.g. ``ProcessResult``).  


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62322259
  
--- Diff: server/test/com/cloud/resource/MockResourceManagerImpl.java ---
@@ -172,6 +173,12 @@ public Cluster getCluster(final Long clusterId) {
 return null;
 }
 
+@Override
+public DataCenter getZone(Long zoneId) {
+// TODO Auto-generated method stub
--- End diff --

Because we have tools to search for TODOs, please remove the comment if 
there is nothing outstanding to be done.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321985
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -412,6 +428,36 @@ protected void runInContext() {
 }
 }
 
+class HostOutOfBandManagementStatsCollector extends 
ManagedContextRunnable {
+@Override
+protected void runInContext() {
+try {
+s_logger.debug("HostOutOfBandManagementStatsCollector is 
running...");
+List outOfBandManagementHosts = 
outOfBandManagementDao.findAllByManagementServer(ManagementServerNode.getManagementServerId());
+if (outOfBandManagementHosts == null) {
+return;
+}
+for (OutOfBandManagement outOfBandManagementHost : 
outOfBandManagementHosts) {
+Host host = 
_hostDao.findById(outOfBandManagementHost.getHostId());
+if (host == null) {
+continue;
+}
+if 
(outOfBandManagementService.isOutOfBandManagementEnabled(host)) {
+
outOfBandManagementService.submitBackgroundPowerSyncTask(host);
+} else {
+if (outOfBandManagementHost.getPowerState() != 
OutOfBandManagement.PowerState.Disabled) {
+if 
(outOfBandManagementService.transitionPowerStateToDisabled(Collections.singletonList(host)))
 {
+s_logger.debug("Out-of-band management was 
disabled in zone/cluster/host, disabled power state for host id:" + 
host.getId());
--- End diff --

Please wrap in a ``if (LOGGER.isDebugEnabled)`` check.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321963
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -412,6 +428,36 @@ protected void runInContext() {
 }
 }
 
+class HostOutOfBandManagementStatsCollector extends 
ManagedContextRunnable {
+@Override
+protected void runInContext() {
+try {
+s_logger.debug("HostOutOfBandManagementStatsCollector is 
running...");
+List outOfBandManagementHosts = 
outOfBandManagementDao.findAllByManagementServer(ManagementServerNode.getManagementServerId());
+if (outOfBandManagementHosts == null) {
+return;
+}
+for (OutOfBandManagement outOfBandManagementHost : 
outOfBandManagementHosts) {
+Host host = 
_hostDao.findById(outOfBandManagementHost.getHostId());
+if (host == null) {
+continue;
+}
+if 
(outOfBandManagementService.isOutOfBandManagementEnabled(host)) {
+
outOfBandManagementService.submitBackgroundPowerSyncTask(host);
+} else {
+if (outOfBandManagementHost.getPowerState() != 
OutOfBandManagement.PowerState.Disabled) {
+if 
(outOfBandManagementService.transitionPowerStateToDisabled(Collections.singletonList(host)))
 {
--- End diff --

Collapse these two ``if`` blocks into a single ``else if`` on line 447 to 
improve readability.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321858
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -251,8 +262,9 @@ public boolean start() {
 }
 
 private void init(Map configs) {
-_executor = Executors.newScheduledThreadPool(4, new 
NamedThreadFactory("StatsCollector"));
+_executor = Executors.newScheduledThreadPool(6, new 
NamedThreadFactory("StatsCollector"));
--- End diff --

I understand that ``6`` is in the ``init`` method.  My question is how did 
we arrive at that number?  Why is 6 instead of 2 or 10?


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321576
  
--- Diff: 
api/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementService.java
 ---
@@ -0,0 +1,51 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.dc.DataCenter;
+import com.cloud.host.Host;
+import com.cloud.org.Cluster;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.framework.config.ConfigKey;
+
+import java.util.List;
+
+public interface OutOfBandManagementService {
+
+ConfigKey OutOfBandManagementActionTimeout = new 
ConfigKey("Advanced", Long.class, "outofbandmanagement.action.timeout", 
"60",
+"The out of band management action timeout in seconds, 
configurable by cluster", true, ConfigKey.Scope.Cluster);
+
+ConfigKey OutOfBandManagementSyncThreadInterval = new 
ConfigKey("Advanced", Long.class, "outofbandmanagement.sync.interval", 
"30",
+"The interval (in milliseconds) when the out-of-band 
management background sync are retrieved", true, ConfigKey.Scope.Global);
+
+ConfigKey OutOfBandManagementSyncThreadPoolSize = new 
ConfigKey("Advanced", Integer.class, 
"outofbandmanagement.sync.poolsize", "50",
+"The out of band management background sync thread pool size", 
true, ConfigKey.Scope.Global);
+
+long getId();
+boolean isOutOfBandManagementEnabled(Host host);
+void submitBackgroundPowerSyncTask(Host host);
+boolean transitionPowerStateToDisabled(List hosts);
--- End diff --

@rhtyd the issue is that it is a wider type specification that is 
necessary.  It can also cause erasure compiler errors.  Therefore, we should 
only be using  when it is truly necessary.  In this case, the 
``OutOfBandManagementService`` only operates on instances of ``Host``.


---
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-9299: Out-of-band Management f...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r62321148
  
--- Diff: 
api/src/org/apache/cloudstack/api/command/admin/outofbandmanagement/IssueOutOfBandManagementPowerActionCmd.java
 ---
@@ -0,0 +1,128 @@
+// 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.
+package org.apache.cloudstack.api.command.admin.outofbandmanagement;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.host.Host;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiCommandJobType;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.HostResponse;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import 
org.apache.cloudstack.outofbandmanagement.OutOfBandManagement.PowerOperation;
+import 
org.apache.cloudstack.outofbandmanagement.OutOfBandManagementService;
+
+import javax.inject.Inject;
+
+@APICommand(name = "issueOutOfBandManagementPowerAction", description = 
"Initiates the specified power action to the host's out-of-band management 
interface",
+responseObject = OutOfBandManagementResponse.class, 
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, authorized = 
{RoleType.Admin})
+public class IssueOutOfBandManagementPowerActionCmd extends BaseAsyncCmd {
+@Inject
+private OutOfBandManagementService outOfBandManagementService;
+
+/
+ API parameters /
+/
+
+@Parameter(name = ApiConstants.HOST_ID, type = CommandType.UUID, 
entityType = HostResponse.class, required = true, description = "the ID of the 
host")
+private Long hostId;
+
+@Parameter(name = ApiConstants.TIMEOUT, type = CommandType.LONG, 
description = "optional operation timeout in seconds that overrides the global 
or cluster-level out-of-band management timeout setting")
+private Long actionTimeout;
+
+@Parameter(name = ApiConstants.ACTION, type = CommandType.STRING, 
required = true, description = "out-of-band management power actions, valid 
actions are: ON, OFF, CYCLE, RESET, SOFT, STATUS")
+private String outOfBandManagementPowerOperation;
+
+/
+/// API Implementation///
+/
+
+@Override
+public String getCommandName() {
+return "issueoutofbandmanagementpoweractionresponse";
+}
+
+private void validateParams() {
+if (getHostId() == null || getHostId() < 1L) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, 
"Invalid host ID: " + getHostId());
+}
+if (Strings.isNullOrEmpty(getOutOfBandManagementPowerOperation())) 
{
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, 
"Invalid out-of-band management power action: " + 
getOutOfBandManagementPowerOperation());
+}
+}
+
+@Override
+public void execute() throws ResourceUnavailableException, 
InsufficientCapacityException, ServerApiException, 
ConcurrentOperationException, ResourceAllocationException, 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[borisstoyanov]

Github user borisstoyanov commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217414515
  
I've just run final sanity tests on this PR, all the OOBM features are 
working and did not observed any side effects, so I'm happy with the latest 
changes getting merged.

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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217413162
  
@swill this PR is ready for CI test run and merge, 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-9299: Out-of-band Management f...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217319127
  
OK. Sounds good. :) Thx... 


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217318995
  
@swill I've added a Travis specific change and Travis specific patched 
ipmitool version (using patch from 
https://bugzilla.redhat.com/show_bug.cgi?id=1286035), I'm hoping with this the 
test should not cause random failure. If it happens to fail randomly in future, 
we can disable it from routine Travis runs.


---
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-9299: Out-of-band Management f...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-217271201
  
So is this going to be another reason Travis fails randomly?  :(  If we 
know it is going to fail randomly, we probably want to remove it from the 
Travis tests to make sure it does not create false negatives for everyone.  If 
it fails the tests in travis, does that mean that the functionality also 
randomly 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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-216908315
  
For the issue reported by Travis, I force it to install ipmitool version 
1.8.16; but looks like even that version causes the same failure (some times).


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-216907666
  
@swill the issue is actually with ipmitool binary itself 
https://bugzilla.redhat.com/show_bug.cgi?id=1286035  where it may sometimes 
fail due to errors in ipmitool's handling of sessions. I'll see if I can work 
around that.


---
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-9299: Out-of-band Management f...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-216884501
  
BTW, travis is showing exceptions in OOBM now, so you probably want to 
review those...


---
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-9299: Out-of-band Management f...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-216884149
  
Please force push to kick off travis again.  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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1502#issuecomment-216472391
  
@jburwell I've fixed based on your review, please re-review and advise 
further improvements, thanks

Integration test after last PR review changes:

=== TestName: test_oobm_background_powerstate_sync | Status : SUCCESS ===

=== TestName: test_oobm_change_password | Status : SUCCESS ===

=== TestName: test_oobm_configure_default_driver | Status : SUCCESS ===

=== TestName: test_oobm_configure_invalid_driver | Status : SUCCESS ===

=== TestName: test_oobm_disable_feature_invalid | Status : SUCCESS ===

=== TestName: test_oobm_disable_feature_valid | Status : SUCCESS ===

=== TestName: test_oobm_enable_feature_invalid | Status : SUCCESS ===

=== TestName: test_oobm_enable_feature_valid | Status : SUCCESS ===

=== TestName: test_oobm_enabledisable_across_clusterzones | Status : 
SUCCESS ===

=== TestName: test_oobm_issue_power_cycle | Status : SUCCESS ===

=== TestName: test_oobm_issue_power_off | Status : SUCCESS ===

=== TestName: test_oobm_issue_power_on | Status : SUCCESS ===

=== TestName: test_oobm_issue_power_reset | Status : SUCCESS ===

=== TestName: test_oobm_issue_power_soft | Status : SUCCESS ===

=== TestName: test_oobm_issue_power_status | Status : SUCCESS ===

=== TestName: test_oobm_multiple_mgmt_server_ownership | Status : SUCCESS 
===



---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61850479
  
--- Diff: 
server/test/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceTest.java
 ---
@@ -0,0 +1,119 @@
+// 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.
+
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.common.collect.ImmutableMap;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.runners.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class OutOfBandManagementServiceTest {
+
+OutOfBandManagementServiceImpl oobmService = new 
OutOfBandManagementServiceImpl();
+
+@Test
+public void testOutOfBandManagementDriverResponseEvent() {
+OutOfBandManagementDriverResponse r = new 
OutOfBandManagementDriverResponse("some result", "some error", false);
+
+r.setSuccess(false);
+r.setAuthFailure(false);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.Unknown);
+
+r.setSuccess(false);
+r.setAuthFailure(true);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.AuthError);
+
+r.setAuthFailure(false);
+r.setSuccess(true);
+r.setPowerState(OutOfBandManagement.PowerState.On);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.On);
+
+r.setPowerState(OutOfBandManagement.PowerState.Off);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.Off);
+
+r.setPowerState(OutOfBandManagement.PowerState.Disabled);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.Disabled);
+}
+
+private ImmutableMap 
buildRandomOptionsMap() {
+ImmutableMap.Builder builder = 
new ImmutableMap.Builder<>();
+builder.put(OutOfBandManagement.Option.ADDRESS, "localhost");
+builder.put(OutOfBandManagement.Option.DRIVER, "ipmitool");
+return builder.build();
+}
+
+@Test
+public void testUpdateOutOfBandManagementConfigValid() {
+OutOfBandManagement config = new OutOfBandManagementVO(123L);
+Assert.assertEquals(config.getPowerState(), 
OutOfBandManagement.PowerState.Disabled);
+config = oobmService.updateConfig(config, buildRandomOptionsMap());
+Assert.assertEquals(config.getAddress(), "localhost");
+Assert.assertEquals(config.getDriver(), "ipmitool");
+Assert.assertEquals(config.getPowerState(), 
OutOfBandManagement.PowerState.Disabled);
+}
+
+@Test
+public void testUpdateOutOfBandManagementConfigInValid() {
+try {
+oobmService.updateConfig(null, buildRandomOptionsMap());
+Assert.fail("CloudRuntimeException was expect for out-of-band 
management not configured for the host");
+} catch (CloudRuntimeException ignored) {
+}
+
+try {
+oobmService.updateConfig(null, null);
+Assert.fail("CloudRuntimeException was expect for out-of-band 
management not configured for the host");
+} catch (CloudRuntimeException ignored) {
+}
+
+OutOfBandManagement config = new OutOfBandManagementVO(123L);
+config.setAddress(null);
+config = oobmService.updateConfig(config, null);
+Assert.assertEquals(config.getAddress(), null);
+Assert.assertEquals(config.getPowerState(), 
OutOfBandManagement.PowerState.Disabled);
--- End diff --

Fixed, split into three unit tests


---
If your project is set up for it, you 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61850255
  
--- Diff: test/integration/smoke/test_outofbandmanagement.py ---
@@ -0,0 +1,561 @@
+# 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.
+
+
+import marvin
+from marvin.cloudstackTestCase import *
+from marvin.cloudstackAPI import *
+from marvin.lib.utils import *
+from marvin.lib.base import *
+from marvin.lib.common import *
+from marvin.lib.utils import (random_gen)
+from nose.plugins.attrib import attr
+
+from ipmisim.ipmisim import IpmiServerContext, IpmiServer, 
ThreadedIpmiServer
+
+import socket
+import sys
+import thread
+import time
+
+
+class TestOutOfBandManagement(cloudstackTestCase):
+""" Test cases for out of band management
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.hypervisor = self.testClient.getHypervisorInfo()
+self.dbclient = self.testClient.getDbConnection()
+self.services = self.testClient.getParsedTestDataConfig()
+self.mgtSvrDetails = self.config.__dict__["mgtSvr"][0].__dict__
+
+self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
+self.host = None
+self.server = None
+
+# use random port for ipmisim
+s = socket.socket()
+s.bind(('', 0))
+self.serverPort = s.getsockname()[1]
+s.close()
+
+self.cleanup = []
+
+
+def tearDown(self):
+try:
+self.dbclient.execute("delete from oobm where port=%d" % 
self.getIpmiServerPort())
+self.dbclient.execute("delete from mshost_peer where 
peer_runid=%s" % self.getFakeMsRunId())
+self.dbclient.execute("delete from mshost where runid=%s" % 
self.getFakeMsRunId())
+self.dbclient.execute("delete from cluster_details where 
name='outOfBandManagementEnabled'")
+self.dbclient.execute("delete from data_center_details where 
name='outOfBandManagementEnabled'")
+cleanup_resources(self.apiclient, self.cleanup)
+if self.server:
+self.server.shutdown()
+self.server.server_close()
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+
+def getFakeMsId(self):
+return 1234567890
--- End diff --

fixed


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61850243
  
--- Diff: test/integration/smoke/test_outofbandmanagement.py ---
@@ -0,0 +1,561 @@
+# 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.
+
+
+import marvin
+from marvin.cloudstackTestCase import *
+from marvin.cloudstackAPI import *
+from marvin.lib.utils import *
+from marvin.lib.base import *
+from marvin.lib.common import *
+from marvin.lib.utils import (random_gen)
+from nose.plugins.attrib import attr
+
+from ipmisim.ipmisim import IpmiServerContext, IpmiServer, 
ThreadedIpmiServer
+
+import socket
+import sys
+import thread
+import time
+
+
+class TestOutOfBandManagement(cloudstackTestCase):
+""" Test cases for out of band management
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.hypervisor = self.testClient.getHypervisorInfo()
+self.dbclient = self.testClient.getDbConnection()
+self.services = self.testClient.getParsedTestDataConfig()
+self.mgtSvrDetails = self.config.__dict__["mgtSvr"][0].__dict__
+
+self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
+self.host = None
+self.server = None
+
+# use random port for ipmisim
+s = socket.socket()
+s.bind(('', 0))
+self.serverPort = s.getsockname()[1]
+s.close()
+
+self.cleanup = []
+
+
+def tearDown(self):
+try:
+self.dbclient.execute("delete from oobm where port=%d" % 
self.getIpmiServerPort())
+self.dbclient.execute("delete from mshost_peer where 
peer_runid=%s" % self.getFakeMsRunId())
+self.dbclient.execute("delete from mshost where runid=%s" % 
self.getFakeMsRunId())
+self.dbclient.execute("delete from cluster_details where 
name='outOfBandManagementEnabled'")
+self.dbclient.execute("delete from data_center_details where 
name='outOfBandManagementEnabled'")
+cleanup_resources(self.apiclient, self.cleanup)
+if self.server:
+self.server.shutdown()
+self.server.server_close()
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+
+def getFakeMsId(self):
+return 1234567890
+
+
+def getFakeMsRunId(self):
+return 123456
--- End diff --

Fixed, uses random.randint 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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61849798
  
--- Diff: test/integration/smoke/test_outofbandmanagement.py ---
@@ -0,0 +1,561 @@
+# 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.
+
+
+import marvin
+from marvin.cloudstackTestCase import *
+from marvin.cloudstackAPI import *
+from marvin.lib.utils import *
+from marvin.lib.base import *
+from marvin.lib.common import *
+from marvin.lib.utils import (random_gen)
+from nose.plugins.attrib import attr
+
+from ipmisim.ipmisim import IpmiServerContext, IpmiServer, 
ThreadedIpmiServer
+
+import socket
+import sys
+import thread
+import time
+
+
+class TestOutOfBandManagement(cloudstackTestCase):
+""" Test cases for out of band management
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.hypervisor = self.testClient.getHypervisorInfo()
+self.dbclient = self.testClient.getDbConnection()
+self.services = self.testClient.getParsedTestDataConfig()
+self.mgtSvrDetails = self.config.__dict__["mgtSvr"][0].__dict__
+
+self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
+self.host = None
+self.server = None
+
+# use random port for ipmisim
+s = socket.socket()
+s.bind(('', 0))
+self.serverPort = s.getsockname()[1]
+s.close()
+
+self.cleanup = []
+
+
+def tearDown(self):
+try:
+self.dbclient.execute("delete from oobm where port=%d" % 
self.getIpmiServerPort())
+self.dbclient.execute("delete from mshost_peer where 
peer_runid=%s" % self.getFakeMsRunId())
+self.dbclient.execute("delete from mshost where runid=%s" % 
self.getFakeMsRunId())
+self.dbclient.execute("delete from cluster_details where 
name='outOfBandManagementEnabled'")
+self.dbclient.execute("delete from data_center_details where 
name='outOfBandManagementEnabled'")
+cleanup_resources(self.apiclient, self.cleanup)
+if self.server:
+self.server.shutdown()
+self.server.server_close()
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+
+def getFakeMsId(self):
+return 1234567890
+
+
+def getFakeMsRunId(self):
+return 123456
+
+
+def getHost(self, hostId=None):
+if self.host and hostId is None:
+return self.host
+
+response = list_hosts(
+self.apiclient,
+zoneid=self.zone.id,
+type='Routing',
+id=hostId
+)
+if len(response) > 0:
+self.host = response[0]
+return self.host
+raise self.skipTest("No hosts found, skipping out-of-band 
management test")
+
+
+def getIpmiServerIp(self):
+s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+s.connect((self.mgtSvrDetails["mgtSvrIp"], 
self.mgtSvrDetails["port"]))
+return s.getsockname()[0]
+
+
+def getIpmiServerPort(self):
+return self.serverPort
+
+
+def getOobmConfigCmd(self):
+cmd = 
configureOutOfBandManagement.configureOutOfBandManagementCmd()
+cmd.driver = 'ipmitool' # The default available driver
+cmd.address = self.getIpmiServerIp()
+cmd.port = self.getIpmiServerPort()
+cmd.username = 'admin'
+cmd.password = 'password'
+cmd.hostid = self.getHost().id
+return cmd
+
+
+def getOobmEnableCmd(self):
+cmd = 
enableOutOfBandManagementForHost.enableOutOfBandManagementForHostCmd()
+ 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61849729
  
--- Diff: test/integration/smoke/test_outofbandmanagement.py ---
@@ -0,0 +1,561 @@
+# 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.
+
+
+import marvin
+from marvin.cloudstackTestCase import *
+from marvin.cloudstackAPI import *
+from marvin.lib.utils import *
+from marvin.lib.base import *
+from marvin.lib.common import *
+from marvin.lib.utils import (random_gen)
+from nose.plugins.attrib import attr
+
+from ipmisim.ipmisim import IpmiServerContext, IpmiServer, 
ThreadedIpmiServer
+
+import socket
+import sys
+import thread
+import time
+
+
+class TestOutOfBandManagement(cloudstackTestCase):
+""" Test cases for out of band management
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.hypervisor = self.testClient.getHypervisorInfo()
+self.dbclient = self.testClient.getDbConnection()
+self.services = self.testClient.getParsedTestDataConfig()
+self.mgtSvrDetails = self.config.__dict__["mgtSvr"][0].__dict__
+
+self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
+self.host = None
+self.server = None
+
+# use random port for ipmisim
+s = socket.socket()
+s.bind(('', 0))
+self.serverPort = s.getsockname()[1]
+s.close()
+
+self.cleanup = []
+
+
+def tearDown(self):
+try:
+self.dbclient.execute("delete from oobm where port=%d" % 
self.getIpmiServerPort())
+self.dbclient.execute("delete from mshost_peer where 
peer_runid=%s" % self.getFakeMsRunId())
+self.dbclient.execute("delete from mshost where runid=%s" % 
self.getFakeMsRunId())
+self.dbclient.execute("delete from cluster_details where 
name='outOfBandManagementEnabled'")
+self.dbclient.execute("delete from data_center_details where 
name='outOfBandManagementEnabled'")
+cleanup_resources(self.apiclient, self.cleanup)
+if self.server:
+self.server.shutdown()
+self.server.server_close()
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+
+def getFakeMsId(self):
+return 1234567890
+
+
+def getFakeMsRunId(self):
+return 123456
+
+
+def getHost(self, hostId=None):
+if self.host and hostId is None:
+return self.host
+
+response = list_hosts(
+self.apiclient,
+zoneid=self.zone.id,
+type='Routing',
+id=hostId
+)
+if len(response) > 0:
+self.host = response[0]
+return self.host
+raise self.skipTest("No hosts found, skipping out-of-band 
management test")
+
+
+def getIpmiServerIp(self):
+s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+s.connect((self.mgtSvrDetails["mgtSvrIp"], 
self.mgtSvrDetails["port"]))
+return s.getsockname()[0]
+
+
+def getIpmiServerPort(self):
+return self.serverPort
+
+
+def getOobmConfigCmd(self):
+cmd = 
configureOutOfBandManagement.configureOutOfBandManagementCmd()
+cmd.driver = 'ipmitool' # The default available driver
+cmd.address = self.getIpmiServerIp()
+cmd.port = self.getIpmiServerPort()
+cmd.username = 'admin'
+cmd.password = 'password'
+cmd.hostid = self.getHost().id
+return cmd
+
+
+def getOobmEnableCmd(self):
+cmd = 
enableOutOfBandManagementForHost.enableOutOfBandManagementForHostCmd()
+ 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61848360
  
--- Diff: test/integration/smoke/test_outofbandmanagement.py ---
@@ -0,0 +1,561 @@
+# 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.
+
+
+import marvin
+from marvin.cloudstackTestCase import *
+from marvin.cloudstackAPI import *
+from marvin.lib.utils import *
+from marvin.lib.base import *
+from marvin.lib.common import *
+from marvin.lib.utils import (random_gen)
+from nose.plugins.attrib import attr
+
+from ipmisim.ipmisim import IpmiServerContext, IpmiServer, 
ThreadedIpmiServer
+
+import socket
+import sys
+import thread
+import time
+
+
+class TestOutOfBandManagement(cloudstackTestCase):
+""" Test cases for out of band management
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.hypervisor = self.testClient.getHypervisorInfo()
+self.dbclient = self.testClient.getDbConnection()
+self.services = self.testClient.getParsedTestDataConfig()
+self.mgtSvrDetails = self.config.__dict__["mgtSvr"][0].__dict__
+
+self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
+self.host = None
+self.server = None
+
+# use random port for ipmisim
+s = socket.socket()
+s.bind(('', 0))
+self.serverPort = s.getsockname()[1]
+s.close()
+
+self.cleanup = []
+
+
+def tearDown(self):
+try:
+self.dbclient.execute("delete from oobm where port=%d" % 
self.getIpmiServerPort())
+self.dbclient.execute("delete from mshost_peer where 
peer_runid=%s" % self.getFakeMsRunId())
+self.dbclient.execute("delete from mshost where runid=%s" % 
self.getFakeMsRunId())
+self.dbclient.execute("delete from cluster_details where 
name='outOfBandManagementEnabled'")
+self.dbclient.execute("delete from data_center_details where 
name='outOfBandManagementEnabled'")
+cleanup_resources(self.apiclient, self.cleanup)
+if self.server:
+self.server.shutdown()
+self.server.server_close()
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+
+def getFakeMsId(self):
+return 1234567890
+
+
+def getFakeMsRunId(self):
+return 123456
+
+
+def getHost(self, hostId=None):
+if self.host and hostId is None:
+return self.host
+
+response = list_hosts(
+self.apiclient,
+zoneid=self.zone.id,
+type='Routing',
+id=hostId
+)
+if len(response) > 0:
+self.host = response[0]
+return self.host
+raise self.skipTest("No hosts found, skipping out-of-band 
management test")
+
+
+def getIpmiServerIp(self):
+s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+s.connect((self.mgtSvrDetails["mgtSvrIp"], 
self.mgtSvrDetails["port"]))
+return s.getsockname()[0]
+
+
+def getIpmiServerPort(self):
+return self.serverPort
+
+
+def getOobmConfigCmd(self):
+cmd = 
configureOutOfBandManagement.configureOutOfBandManagementCmd()
+cmd.driver = 'ipmitool' # The default available driver
+cmd.address = self.getIpmiServerIp()
+cmd.port = self.getIpmiServerPort()
+cmd.username = 'admin'
+cmd.password = 'password'
+cmd.hostid = self.getHost().id
+return cmd
+
+
+def getOobmEnableCmd(self):
+cmd = 
enableOutOfBandManagementForHost.enableOutOfBandManagementForHostCmd()
+ 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61847657
  
--- Diff: test/integration/smoke/test_outofbandmanagement.py ---
@@ -0,0 +1,561 @@
+# 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.
+
+
+import marvin
+from marvin.cloudstackTestCase import *
+from marvin.cloudstackAPI import *
+from marvin.lib.utils import *
+from marvin.lib.base import *
+from marvin.lib.common import *
+from marvin.lib.utils import (random_gen)
+from nose.plugins.attrib import attr
+
+from ipmisim.ipmisim import IpmiServerContext, IpmiServer, 
ThreadedIpmiServer
+
+import socket
+import sys
+import thread
+import time
+
+
+class TestOutOfBandManagement(cloudstackTestCase):
+""" Test cases for out of band management
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.hypervisor = self.testClient.getHypervisorInfo()
+self.dbclient = self.testClient.getDbConnection()
+self.services = self.testClient.getParsedTestDataConfig()
+self.mgtSvrDetails = self.config.__dict__["mgtSvr"][0].__dict__
+
+self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
+self.host = None
+self.server = None
+
+# use random port for ipmisim
+s = socket.socket()
+s.bind(('', 0))
+self.serverPort = s.getsockname()[1]
+s.close()
+
+self.cleanup = []
+
+
+def tearDown(self):
+try:
+self.dbclient.execute("delete from oobm where port=%d" % 
self.getIpmiServerPort())
+self.dbclient.execute("delete from mshost_peer where 
peer_runid=%s" % self.getFakeMsRunId())
+self.dbclient.execute("delete from mshost where runid=%s" % 
self.getFakeMsRunId())
+self.dbclient.execute("delete from cluster_details where 
name='outOfBandManagementEnabled'")
+self.dbclient.execute("delete from data_center_details where 
name='outOfBandManagementEnabled'")
+cleanup_resources(self.apiclient, self.cleanup)
+if self.server:
+self.server.shutdown()
+self.server.server_close()
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+
+def getFakeMsId(self):
+return 1234567890
+
+
+def getFakeMsRunId(self):
+return 123456
+
+
+def getHost(self, hostId=None):
+if self.host and hostId is None:
+return self.host
+
+response = list_hosts(
+self.apiclient,
+zoneid=self.zone.id,
+type='Routing',
+id=hostId
+)
+if len(response) > 0:
+self.host = response[0]
+return self.host
+raise self.skipTest("No hosts found, skipping out-of-band 
management test")
+
+
+def getIpmiServerIp(self):
+s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+s.connect((self.mgtSvrDetails["mgtSvrIp"], 
self.mgtSvrDetails["port"]))
+return s.getsockname()[0]
+
+
+def getIpmiServerPort(self):
+return self.serverPort
+
+
+def getOobmConfigCmd(self):
+cmd = 
configureOutOfBandManagement.configureOutOfBandManagementCmd()
+cmd.driver = 'ipmitool' # The default available driver
+cmd.address = self.getIpmiServerIp()
+cmd.port = self.getIpmiServerPort()
+cmd.username = 'admin'
+cmd.password = 'password'
+cmd.hostid = self.getHost().id
+return cmd
+
+
+def getOobmEnableCmd(self):
+cmd = 
enableOutOfBandManagementForHost.enableOutOfBandManagementForHostCmd()
+ 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61847561
  
--- Diff: 
server/test/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceTest.java
 ---
@@ -0,0 +1,119 @@
+// 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.
+
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.common.collect.ImmutableMap;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.runners.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class OutOfBandManagementServiceTest {
+
+OutOfBandManagementServiceImpl oobmService = new 
OutOfBandManagementServiceImpl();
+
+@Test
+public void testOutOfBandManagementDriverResponseEvent() {
+OutOfBandManagementDriverResponse r = new 
OutOfBandManagementDriverResponse("some result", "some error", false);
+
+r.setSuccess(false);
+r.setAuthFailure(false);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.Unknown);
+
+r.setSuccess(false);
+r.setAuthFailure(true);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.AuthError);
+
+r.setAuthFailure(false);
+r.setSuccess(true);
+r.setPowerState(OutOfBandManagement.PowerState.On);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.On);
+
+r.setPowerState(OutOfBandManagement.PowerState.Off);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.Off);
+
+r.setPowerState(OutOfBandManagement.PowerState.Disabled);
+Assert.assertEquals(r.toEvent(), 
OutOfBandManagement.PowerState.Event.Disabled);
+}
+
+private ImmutableMap 
buildRandomOptionsMap() {
+ImmutableMap.Builder builder = 
new ImmutableMap.Builder<>();
+builder.put(OutOfBandManagement.Option.ADDRESS, "localhost");
+builder.put(OutOfBandManagement.Option.DRIVER, "ipmitool");
+return builder.build();
+}
+
+@Test
+public void testUpdateOutOfBandManagementConfigValid() {
+OutOfBandManagement config = new OutOfBandManagementVO(123L);
+Assert.assertEquals(config.getPowerState(), 
OutOfBandManagement.PowerState.Disabled);
+config = oobmService.updateConfig(config, buildRandomOptionsMap());
+Assert.assertEquals(config.getAddress(), "localhost");
+Assert.assertEquals(config.getDriver(), "ipmitool");
+Assert.assertEquals(config.getPowerState(), 
OutOfBandManagement.PowerState.Disabled);
+}
+
+@Test
+public void testUpdateOutOfBandManagementConfigInValid() {
+try {
+oobmService.updateConfig(null, buildRandomOptionsMap());
+Assert.fail("CloudRuntimeException was expect for out-of-band 
management not configured for the host");
+} catch (CloudRuntimeException ignored) {
+}
+
+try {
+oobmService.updateConfig(null, null);
+Assert.fail("CloudRuntimeException was expect for out-of-band 
management not configured for the host");
+} catch (CloudRuntimeException ignored) {
+}
+
+OutOfBandManagement config = new OutOfBandManagementVO(123L);
+config.setAddress(null);
+config = oobmService.updateConfig(config, null);
+Assert.assertEquals(config.getAddress(), null);
+Assert.assertEquals(config.getPowerState(), 
OutOfBandManagement.PowerState.Disabled);
+}
+
+@Test
+public void testGetOutOfBandManagementOptionsValid() {
+

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61847460
  
--- Diff: server/test/com/cloud/resource/MockResourceManagerImpl.java ---
@@ -172,6 +173,12 @@ public Cluster getCluster(final Long clusterId) {
 return null;
 }
 
+@Override
+public DataCenter getZone(Long zoneId) {
+// TODO Auto-generated method stub
--- End diff --

This is a test mock, from test 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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61847434
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
+public class OutOfBandManagementServiceImpl extends ManagerBase implements 
OutOfBandManagementService, Manager, Configurable {
+public static final Logger LOG = 
Logger.getLogger(OutOfBandManagementServiceImpl.class);
+
+@Inject
+private ClusterDetailsDao clusterDetailsDao;
+@Inject
+private DataCenterDetailsDao dataCenterDetailsDao;
+@Inject
+private OutOfBandManagementDao outOfBandManagementDao;
+@Inject
+private HostDao hostDao;
+@Inject
+private AlertManager alertMgr;
+
+private String name;
+private long serviceId;
+
+private List outOfBandManagementDrivers = 
new ArrayList<>();
+private Map 
outOfBandManagementDriversMap = new HashMap();
+
+private static final String OOBM_ENABLED_DETAIL = 
"outOfBandManagementEnabled";
+private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_HOST = 120;
+
+private Cache hostAlertCache;
+private static ExecutorService backgroundSyncExecutor;
+
+private String getOutOfBandManagementHostLock(long id) {
+return "oobm.host." + id;
+}
+
+private 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61847380
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
+public class OutOfBandManagementServiceImpl extends ManagerBase implements 
OutOfBandManagementService, Manager, Configurable {
+public static final Logger LOG = 
Logger.getLogger(OutOfBandManagementServiceImpl.class);
+
+@Inject
+private ClusterDetailsDao clusterDetailsDao;
+@Inject
+private DataCenterDetailsDao dataCenterDetailsDao;
+@Inject
+private OutOfBandManagementDao outOfBandManagementDao;
+@Inject
+private HostDao hostDao;
+@Inject
+private AlertManager alertMgr;
+
+private String name;
+private long serviceId;
+
+private List outOfBandManagementDrivers = 
new ArrayList<>();
+private Map 
outOfBandManagementDriversMap = new HashMap();
+
+private static final String OOBM_ENABLED_DETAIL = 
"outOfBandManagementEnabled";
+private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_HOST = 120;
+
+private Cache hostAlertCache;
+private static ExecutorService backgroundSyncExecutor;
+
+private String getOutOfBandManagementHostLock(long id) {
+return "oobm.host." + id;
+}
+
+private 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61847246
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
+public class OutOfBandManagementServiceImpl extends ManagerBase implements 
OutOfBandManagementService, Manager, Configurable {
+public static final Logger LOG = 
Logger.getLogger(OutOfBandManagementServiceImpl.class);
+
+@Inject
+private ClusterDetailsDao clusterDetailsDao;
+@Inject
+private DataCenterDetailsDao dataCenterDetailsDao;
+@Inject
+private OutOfBandManagementDao outOfBandManagementDao;
+@Inject
+private HostDao hostDao;
+@Inject
+private AlertManager alertMgr;
+
+private String name;
+private long serviceId;
+
+private List outOfBandManagementDrivers = 
new ArrayList<>();
+private Map 
outOfBandManagementDriversMap = new HashMap();
+
+private static final String OOBM_ENABLED_DETAIL = 
"outOfBandManagementEnabled";
+private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_HOST = 120;
+
+private Cache hostAlertCache;
+private static ExecutorService backgroundSyncExecutor;
+
+private String getOutOfBandManagementHostLock(long id) {
+return "oobm.host." + id;
+}
+
+private 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61847192
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
+public class OutOfBandManagementServiceImpl extends ManagerBase implements 
OutOfBandManagementService, Manager, Configurable {
+public static final Logger LOG = 
Logger.getLogger(OutOfBandManagementServiceImpl.class);
+
+@Inject
+private ClusterDetailsDao clusterDetailsDao;
+@Inject
+private DataCenterDetailsDao dataCenterDetailsDao;
+@Inject
+private OutOfBandManagementDao outOfBandManagementDao;
+@Inject
+private HostDao hostDao;
+@Inject
+private AlertManager alertMgr;
+
+private String name;
+private long serviceId;
+
+private List outOfBandManagementDrivers = 
new ArrayList<>();
+private Map 
outOfBandManagementDriversMap = new HashMap();
+
+private static final String OOBM_ENABLED_DETAIL = 
"outOfBandManagementEnabled";
+private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_HOST = 120;
+
+private Cache hostAlertCache;
+private static ExecutorService backgroundSyncExecutor;
+
+private String getOutOfBandManagementHostLock(long id) {
+return "oobm.host." + id;
+}
+
+private 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61847113
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
+public class OutOfBandManagementServiceImpl extends ManagerBase implements 
OutOfBandManagementService, Manager, Configurable {
+public static final Logger LOG = 
Logger.getLogger(OutOfBandManagementServiceImpl.class);
+
+@Inject
+private ClusterDetailsDao clusterDetailsDao;
+@Inject
+private DataCenterDetailsDao dataCenterDetailsDao;
+@Inject
+private OutOfBandManagementDao outOfBandManagementDao;
+@Inject
+private HostDao hostDao;
+@Inject
+private AlertManager alertMgr;
+
+private String name;
+private long serviceId;
+
+private List outOfBandManagementDrivers = 
new ArrayList<>();
+private Map 
outOfBandManagementDriversMap = new HashMap();
+
+private static final String OOBM_ENABLED_DETAIL = 
"outOfBandManagementEnabled";
+private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_HOST = 120;
+
+private Cache hostAlertCache;
+private static ExecutorService backgroundSyncExecutor;
+
+private String getOutOfBandManagementHostLock(long id) {
+return "oobm.host." + id;
+}
+
+private 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61846972
  
--- Diff: 
api/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementService.java
 ---
@@ -0,0 +1,51 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.dc.DataCenter;
+import com.cloud.host.Host;
+import com.cloud.org.Cluster;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.framework.config.ConfigKey;
+
+import java.util.List;
+
+public interface OutOfBandManagementService {
+
+ConfigKey OutOfBandManagementActionTimeout = new 
ConfigKey("Advanced", Long.class, "outofbandmanagement.action.timeout", 
"60",
+"The out of band management action timeout in seconds, 
configurable by cluster", true, ConfigKey.Scope.Cluster);
+
+ConfigKey OutOfBandManagementSyncThreadInterval = new 
ConfigKey("Advanced", Long.class, "outofbandmanagement.sync.interval", 
"30",
+"The interval (in milliseconds) when the out-of-band 
management background sync are retrieved", true, ConfigKey.Scope.Global);
+
+ConfigKey OutOfBandManagementSyncThreadPoolSize = new 
ConfigKey("Advanced", Integer.class, 
"outofbandmanagement.sync.poolsize", "50",
+"The out of band management background sync thread pool size", 
true, ConfigKey.Scope.Global);
+
+long getId();
+boolean isOutOfBandManagementEnabled(Host host);
+void submitBackgroundPowerSyncTask(Host host);
+boolean transitionPowerStateToDisabled(List hosts);
--- End diff --

There is nothing wrong with the code, it's a valid declaration and usage of 
generics that accepts any list of concrete items that implement the Host 
interface; and a classic usage of `PECS` 
http://stackoverflow.com/questions/2723397/what-is-pecs-producer-extends-consumer-super

Once RBAC PR is merged, I'll use the ListUtils in there that would return a 
List along with fix the validateParams with api annotation validators.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61846775
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
+public class OutOfBandManagementServiceImpl extends ManagerBase implements 
OutOfBandManagementService, Manager, Configurable {
+public static final Logger LOG = 
Logger.getLogger(OutOfBandManagementServiceImpl.class);
+
+@Inject
+private ClusterDetailsDao clusterDetailsDao;
+@Inject
+private DataCenterDetailsDao dataCenterDetailsDao;
+@Inject
+private OutOfBandManagementDao outOfBandManagementDao;
+@Inject
+private HostDao hostDao;
+@Inject
+private AlertManager alertMgr;
+
+private String name;
+private long serviceId;
+
+private List outOfBandManagementDrivers = 
new ArrayList<>();
+private Map 
outOfBandManagementDriversMap = new HashMap();
+
+private static final String OOBM_ENABLED_DETAIL = 
"outOfBandManagementEnabled";
+private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_HOST = 120;
+
+private Cache hostAlertCache;
+private static ExecutorService backgroundSyncExecutor;
--- End diff --

variables are configured in configure(), we should not declare them final 
without initializing them.


---
If your 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61846633
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
+public class OutOfBandManagementServiceImpl extends ManagerBase implements 
OutOfBandManagementService, Manager, Configurable {
+public static final Logger LOG = 
Logger.getLogger(OutOfBandManagementServiceImpl.class);
+
+@Inject
+private ClusterDetailsDao clusterDetailsDao;
+@Inject
+private DataCenterDetailsDao dataCenterDetailsDao;
+@Inject
+private OutOfBandManagementDao outOfBandManagementDao;
+@Inject
+private HostDao hostDao;
+@Inject
+private AlertManager alertMgr;
+
+private String name;
+private long serviceId;
+
+private List outOfBandManagementDrivers = 
new ArrayList<>();
+private Map 
outOfBandManagementDriversMap = new HashMap();
+
+private static final String OOBM_ENABLED_DETAIL = 
"outOfBandManagementEnabled";
+private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_HOST = 120;
+
+private Cache hostAlertCache;
+private static ExecutorService backgroundSyncExecutor;
+
+private String getOutOfBandManagementHostLock(long id) {
+return "oobm.host." + id;
+}
+
+private 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61846419
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
--- End diff --

removed


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61846451
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementServiceImpl.java
 ---
@@ -0,0 +1,532 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.dc.ClusterDetailsDao;
+import com.cloud.dc.ClusterDetailsVO;
+import com.cloud.dc.DataCenter;
+import com.cloud.dc.DataCenterDetailVO;
+import com.cloud.dc.dao.DataCenterDetailsDao;
+import com.cloud.domain.Domain;
+import com.cloud.event.ActionEvent;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.host.Host;
+import com.cloud.host.dao.HostDao;
+import com.cloud.org.Cluster;
+import com.cloud.utils.component.Manager;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.google.common.base.Strings;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import 
org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+@Component
+@Local(value = {OutOfBandManagementService.class})
+public class OutOfBandManagementServiceImpl extends ManagerBase implements 
OutOfBandManagementService, Manager, Configurable {
+public static final Logger LOG = 
Logger.getLogger(OutOfBandManagementServiceImpl.class);
+
+@Inject
+private ClusterDetailsDao clusterDetailsDao;
+@Inject
+private DataCenterDetailsDao dataCenterDetailsDao;
+@Inject
+private OutOfBandManagementDao outOfBandManagementDao;
+@Inject
+private HostDao hostDao;
+@Inject
+private AlertManager alertMgr;
+
+private String name;
+private long serviceId;
+
+private List outOfBandManagementDrivers = 
new ArrayList<>();
+private Map 
outOfBandManagementDriversMap = new HashMap();
+
+private static final String OOBM_ENABLED_DETAIL = 
"outOfBandManagementEnabled";
+private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_HOST = 120;
+
+private Cache hostAlertCache;
+private static ExecutorService backgroundSyncExecutor;
+
+private String getOutOfBandManagementHostLock(long id) {
+return "oobm.host." + id;
+}
+
+private 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61846300
  
--- Diff: 
server/src/org/apache/cloudstack/outofbandmanagement/OutOfBandManagementBackgroundTask.java
 ---
@@ -0,0 +1,45 @@
+// 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.
+
+package org.apache.cloudstack.outofbandmanagement;
+
+import com.cloud.host.Host;
+
+public class OutOfBandManagementBackgroundTask implements Runnable {
+final private OutOfBandManagementService service;
+final private Host host;
+final private OutOfBandManagement.PowerOperation powerOperation;
+
+public OutOfBandManagementBackgroundTask(OutOfBandManagementService 
service, Host host, OutOfBandManagement.PowerOperation powerOperation) {
+this.service = service;
+this.host = host;
+this.powerOperation = powerOperation;
+}
+
+@Override
+public String toString() {
+return String.format("[OOBM Task] Power operation:%s on 
Host:%d(%s)", powerOperation, host.getId(), host.getName());
+}
+
+@Override
+public void run() {
+try {
+service.executeOutOfBandManagementPowerOperation(host, 
powerOperation, null);
+} catch (Exception ignored) {
--- End diff --

fixed


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61846062
  
--- Diff: server/src/com/cloud/server/StatsCollector.java ---
@@ -251,8 +262,9 @@ public boolean start() {
 }
 
 private void init(Map configs) {
-_executor = Executors.newScheduledThreadPool(4, new 
NamedThreadFactory("StatsCollector"));
+_executor = Executors.newScheduledThreadPool(6, new 
NamedThreadFactory("StatsCollector"));
--- End diff --

max. number of statscollector jobs configured in the init(); see 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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61846016
  
--- Diff: 
plugins/outofbandmanagement-drivers/ipmitool/test/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapperTest.java
 ---
@@ -0,0 +1,107 @@
+//
+// 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.
+//
+
+package org.apache.cloudstack.outofbandmanagement.driver.ipmitool;
+
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import java.util.Arrays;
+import java.util.List;
+
+@RunWith(MockitoJUnitRunner.class)
+public class IpmitoolWrapperTest {
+@Test
+public void testParsePowerCommandValid() {
+
Assert.assertEquals(IpmitoolWrapper.parsePowerCommand(OutOfBandManagement.PowerOperation.ON),
 "on");
+
Assert.assertEquals(IpmitoolWrapper.parsePowerCommand(OutOfBandManagement.PowerOperation.OFF),
 "off");
+
Assert.assertEquals(IpmitoolWrapper.parsePowerCommand(OutOfBandManagement.PowerOperation.CYCLE),
 "cycle");
+
Assert.assertEquals(IpmitoolWrapper.parsePowerCommand(OutOfBandManagement.PowerOperation.RESET),
 "reset");
+
Assert.assertEquals(IpmitoolWrapper.parsePowerCommand(OutOfBandManagement.PowerOperation.SOFT),
 "soft");
+
Assert.assertEquals(IpmitoolWrapper.parsePowerCommand(OutOfBandManagement.PowerOperation.STATUS),
 "status");
+}
+
+@Test
+public void testParsePowerCommandInvalid() {
+try {
+IpmitoolWrapper.parsePowerCommand(null);
+Assert.fail("IpmitoolWrapper.parsePowerCommand failed to throw 
exception on invalid power state");
+} catch (IllegalStateException e) {
+}
+}
+
+@Test
+public void testParsePowerState() {
+Assert.assertEquals(IpmitoolWrapper.parsePowerState(null), 
OutOfBandManagement.PowerState.Unknown);
+Assert.assertEquals(IpmitoolWrapper.parsePowerState(""), 
OutOfBandManagement.PowerState.Unknown);
+Assert.assertEquals(IpmitoolWrapper.parsePowerState(" "), 
OutOfBandManagement.PowerState.Unknown);
+Assert.assertEquals(IpmitoolWrapper.parsePowerState("invalid 
data"), OutOfBandManagement.PowerState.Unknown);
+Assert.assertEquals(IpmitoolWrapper.parsePowerState("Chassis Power 
is on"), OutOfBandManagement.PowerState.On);
+Assert.assertEquals(IpmitoolWrapper.parsePowerState("Chassis Power 
is off"), OutOfBandManagement.PowerState.Off);
+}
+
+@Test
+public void testGetIpmiToolCommandArgs() {
+List args = 
IpmitoolWrapper.getIpmiToolCommandArgs("binpath", "intf", "1", null);
+assert args != null;
+Assert.assertEquals(args.size(), 6);
+Assert.assertArrayEquals(args.toArray(), new String[]{"binpath", 
"-I", "intf", "-R", "1", "-v"});
+
+ImmutableMap.Builder argMap = 
new ImmutableMap.Builder<>();
+argMap.put(OutOfBandManagement.Option.DRIVER, "ipmitool");
+argMap.put(OutOfBandManagement.Option.ADDRESS, "127.0.0.1");
+List argsWithOpts = 
IpmitoolWrapper.getIpmiToolCommandArgs("binpath", "intf", "1", argMap.build(), 
"user", "list");
+assert argsWithOpts != null;
+Assert.assertEquals(argsWithOpts.size(), 10);
+Assert.assertArrayEquals(argsWithOpts.toArray(), new 
String[]{"binpath", "-I", "intf", "-R", "1", "-v", "-H", "127.0.0.1", "user", 
"list"});
+}
+
+@Test
+public void testFindIpmiUser() {
+// Invalid data
+try {
+

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61845867
  
--- Diff: 
plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapper.java
 ---
@@ -0,0 +1,166 @@
+// 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.
+
+package org.apache.cloudstack.outofbandmanagement.driver.ipmitool;
+
+import com.cloud.utils.StringUtils;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.cloudstack.utils.process.ProcessRunner;
+import org.apache.log4j.Logger;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class IpmitoolWrapper {
+public static final Logger LOG = 
Logger.getLogger(IpmitoolWrapper.class);
+
+public static String 
parsePowerCommand(OutOfBandManagement.PowerOperation operation) {
+if (operation == null) {
+throw new IllegalStateException("Invalid power operation 
requested");
+}
+switch (operation) {
+case ON:
+case OFF:
+case CYCLE:
+case RESET:
+case SOFT:
+case STATUS:
+break;
+default:
+throw new IllegalStateException("Invalid power operation 
requested");
+}
+return operation.toString().toLowerCase();
+}
+
+public static OutOfBandManagement.PowerState parsePowerState(final 
String standardOutput) {
+if (Strings.isNullOrEmpty(standardOutput)) {
+return OutOfBandManagement.PowerState.Unknown;
+}
+if (standardOutput.equals("Chassis Power is on")) {
+return OutOfBandManagement.PowerState.On;
+} else if (standardOutput.equals("Chassis Power is off")) {
+return OutOfBandManagement.PowerState.Off;
+}
+return OutOfBandManagement.PowerState.Unknown;
+}
+
+public static List getIpmiToolCommandArgs(final String 
ipmiToolPath, final String ipmiInterface, final String retries,
+  final 
ImmutableMap options, String... commands) {
+
+ImmutableList.Builder ipmiToolCommands = 
ImmutableList.builder()
+
.add(ipmiToolPath)
+.add("-I")
+
.add(ipmiInterface)
+.add("-R")
+.add(retries)
+.add("-v");
+
+if (options != null) {
+for (ImmutableMap.Entry 
option : options.entrySet()) {
+switch (option.getKey()) {
+case ADDRESS:
+ipmiToolCommands.add("-H");
+break;
+case PORT:
+ipmiToolCommands.add("-p");
+break;
+case USERNAME:
+ipmiToolCommands.add("-U");
+break;
+case PASSWORD:
+ipmiToolCommands.add("-P");
+break;
+default:
+continue;
+}
+ipmiToolCommands.add(option.getValue());
+}
  

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61845772
  
--- Diff: 
plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolOutOfBandManagementDriver.java
 ---
@@ -0,0 +1,159 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement.driver.ipmitool;
+
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagementDriver;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand;
+import 
org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverResponse;
+import org.apache.log4j.Logger;
+
+import javax.ejb.Local;
+import java.util.Arrays;
+import java.util.List;
+
+@Local(value = {OutOfBandManagementDriver.class})
+public class IpmitoolOutOfBandManagementDriver extends AdapterBase 
implements OutOfBandManagementDriver, Configurable {
+public static final Logger LOG = 
Logger.getLogger(IpmitoolOutOfBandManagementDriver.class);
+
+private static volatile boolean isDriverEnabled = false;
+private static boolean isIpmiToolBinAvailable = false;
--- End diff --

ipmitool path can be changed without restarting mgmt server, the global 
setting is dynamic in nature. The path is read from db for each cmd. In case 
ipmitool path needs tobe updated or tool need to be installed, this saves 
admins from restarting mgmt server. Both are static as Spring creates only one 
instance of this class and injects them to the core oobm service; we use them 
to check whether the driver is enabled and ipmitool binary is available.

On each execute, the initDriver is called to initialize the driver is 
ipmitool driver is not successfully initialized.


---
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-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61845431
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/outofbandmanagement/dao/OutOfBandManagementDaoImpl.java
 ---
@@ -0,0 +1,163 @@
+// 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.
+
+package org.apache.cloudstack.outofbandmanagement.dao;
+
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.db.Attribute;
+import com.cloud.utils.db.DB;
+import com.cloud.utils.db.Filter;
+import com.cloud.utils.db.GenericDaoBase;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+import com.cloud.utils.db.TransactionLegacy;
+import com.cloud.utils.db.UpdateBuilder;
+import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagementVO;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.util.List;
+
+@DB
+@Component
+@Local(value = {OutOfBandManagementDao.class})
+public class OutOfBandManagementDaoImpl extends 
GenericDaoBase implements OutOfBandManagementDao {
+private static final Logger LOG = 
Logger.getLogger(OutOfBandManagementDaoImpl.class);
+
+private SearchBuilder HostSearch;
+private SearchBuilder ManagementServerSearch;
+private SearchBuilder 
OutOfBandManagementOwnerSearch;
+private SearchBuilder StateUpdateSearch;
+
+private Attribute PowerStateAttr;
+private Attribute MsIdAttr;
+private Attribute UpdateTimeAttr;
+
+public OutOfBandManagementDaoImpl() {
+super();
+
+HostSearch = createSearchBuilder();
+HostSearch.and("hostId", HostSearch.entity().getHostId(), 
SearchCriteria.Op.EQ);
+HostSearch.done();
+
+ManagementServerSearch = createSearchBuilder();
+ManagementServerSearch.and("server", 
ManagementServerSearch.entity().getManagementServerId(), SearchCriteria.Op.EQ);
+ManagementServerSearch.done();
+
+OutOfBandManagementOwnerSearch = createSearchBuilder();
+OutOfBandManagementOwnerSearch.and("server", 
OutOfBandManagementOwnerSearch.entity().getManagementServerId(), 
SearchCriteria.Op.EQ);
+OutOfBandManagementOwnerSearch.or("serverNull", 
OutOfBandManagementOwnerSearch.entity().getManagementServerId(), 
SearchCriteria.Op.NULL);
+OutOfBandManagementOwnerSearch.done();
+
+StateUpdateSearch = createSearchBuilder();
+StateUpdateSearch.and("status", 
StateUpdateSearch.entity().getPowerState(), SearchCriteria.Op.EQ);
+StateUpdateSearch.and("id", StateUpdateSearch.entity().getId(), 
SearchCriteria.Op.EQ);
+StateUpdateSearch.and("update", 
StateUpdateSearch.entity().getUpdateCount(), SearchCriteria.Op.EQ);
+StateUpdateSearch.done();
+
+PowerStateAttr = _allAttributes.get("powerState");
+MsIdAttr = _allAttributes.get("managementServerId");
+UpdateTimeAttr = _allAttributes.get("updateTime");
+assert (PowerStateAttr != null && MsIdAttr != null && 
UpdateTimeAttr != null) : "Couldn't find one of these attributes";
+}
+
+@Override
+public OutOfBandManagement findByHost(long hostId) {
+SearchCriteria sc = 
HostSearch.create("hostId", hostId);
+return findOneBy(sc);
+}
+
+@Override
+public List findAllByManagementServer(long 
serverId) {
+SearchCriteria sc = 
OutOfBandManagementOwnerSearch.create();
+sc.setParameters("server", serverId);
+return listBy(sc, new Filter(OutOfBandManagementVO.class, 
"updateTime", true, null, null));
+}
+
+private void 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61845392
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/outofbandmanagement/dao/OutOfBandManagementDaoImpl.java
 ---
@@ -0,0 +1,163 @@
+// 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.
+
+package org.apache.cloudstack.outofbandmanagement.dao;
+
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.db.Attribute;
+import com.cloud.utils.db.DB;
+import com.cloud.utils.db.Filter;
+import com.cloud.utils.db.GenericDaoBase;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+import com.cloud.utils.db.TransactionLegacy;
+import com.cloud.utils.db.UpdateBuilder;
+import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagementVO;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.util.List;
+
+@DB
+@Component
+@Local(value = {OutOfBandManagementDao.class})
+public class OutOfBandManagementDaoImpl extends 
GenericDaoBase implements OutOfBandManagementDao {
+private static final Logger LOG = 
Logger.getLogger(OutOfBandManagementDaoImpl.class);
+
+private SearchBuilder HostSearch;
+private SearchBuilder ManagementServerSearch;
+private SearchBuilder 
OutOfBandManagementOwnerSearch;
+private SearchBuilder StateUpdateSearch;
+
+private Attribute PowerStateAttr;
+private Attribute MsIdAttr;
+private Attribute UpdateTimeAttr;
+
+public OutOfBandManagementDaoImpl() {
+super();
+
+HostSearch = createSearchBuilder();
+HostSearch.and("hostId", HostSearch.entity().getHostId(), 
SearchCriteria.Op.EQ);
+HostSearch.done();
+
+ManagementServerSearch = createSearchBuilder();
+ManagementServerSearch.and("server", 
ManagementServerSearch.entity().getManagementServerId(), SearchCriteria.Op.EQ);
+ManagementServerSearch.done();
+
+OutOfBandManagementOwnerSearch = createSearchBuilder();
+OutOfBandManagementOwnerSearch.and("server", 
OutOfBandManagementOwnerSearch.entity().getManagementServerId(), 
SearchCriteria.Op.EQ);
+OutOfBandManagementOwnerSearch.or("serverNull", 
OutOfBandManagementOwnerSearch.entity().getManagementServerId(), 
SearchCriteria.Op.NULL);
+OutOfBandManagementOwnerSearch.done();
+
+StateUpdateSearch = createSearchBuilder();
+StateUpdateSearch.and("status", 
StateUpdateSearch.entity().getPowerState(), SearchCriteria.Op.EQ);
+StateUpdateSearch.and("id", StateUpdateSearch.entity().getId(), 
SearchCriteria.Op.EQ);
+StateUpdateSearch.and("update", 
StateUpdateSearch.entity().getUpdateCount(), SearchCriteria.Op.EQ);
+StateUpdateSearch.done();
+
+PowerStateAttr = _allAttributes.get("powerState");
+MsIdAttr = _allAttributes.get("managementServerId");
+UpdateTimeAttr = _allAttributes.get("updateTime");
+assert (PowerStateAttr != null && MsIdAttr != null && 
UpdateTimeAttr != null) : "Couldn't find one of these attributes";
+}
+
+@Override
+public OutOfBandManagement findByHost(long hostId) {
+SearchCriteria sc = 
HostSearch.create("hostId", hostId);
+return findOneBy(sc);
+}
+
+@Override
+public List findAllByManagementServer(long 
serverId) {
+SearchCriteria sc = 
OutOfBandManagementOwnerSearch.create();
+sc.setParameters("server", serverId);
+return listBy(sc, new Filter(OutOfBandManagementVO.class, 
"updateTime", true, null, null));
+}
+
+private void 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61845251
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/outofbandmanagement/dao/OutOfBandManagementDaoImpl.java
 ---
@@ -0,0 +1,163 @@
+// 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.
+
+package org.apache.cloudstack.outofbandmanagement.dao;
+
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.db.Attribute;
+import com.cloud.utils.db.DB;
+import com.cloud.utils.db.Filter;
+import com.cloud.utils.db.GenericDaoBase;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+import com.cloud.utils.db.TransactionLegacy;
+import com.cloud.utils.db.UpdateBuilder;
+import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagementVO;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.util.List;
+
+@DB
+@Component
+@Local(value = {OutOfBandManagementDao.class})
+public class OutOfBandManagementDaoImpl extends 
GenericDaoBase implements OutOfBandManagementDao {
+private static final Logger LOG = 
Logger.getLogger(OutOfBandManagementDaoImpl.class);
+
+private SearchBuilder HostSearch;
+private SearchBuilder ManagementServerSearch;
+private SearchBuilder 
OutOfBandManagementOwnerSearch;
+private SearchBuilder StateUpdateSearch;
+
+private Attribute PowerStateAttr;
+private Attribute MsIdAttr;
+private Attribute UpdateTimeAttr;
+
+public OutOfBandManagementDaoImpl() {
+super();
+
+HostSearch = createSearchBuilder();
+HostSearch.and("hostId", HostSearch.entity().getHostId(), 
SearchCriteria.Op.EQ);
+HostSearch.done();
+
+ManagementServerSearch = createSearchBuilder();
+ManagementServerSearch.and("server", 
ManagementServerSearch.entity().getManagementServerId(), SearchCriteria.Op.EQ);
+ManagementServerSearch.done();
+
+OutOfBandManagementOwnerSearch = createSearchBuilder();
+OutOfBandManagementOwnerSearch.and("server", 
OutOfBandManagementOwnerSearch.entity().getManagementServerId(), 
SearchCriteria.Op.EQ);
+OutOfBandManagementOwnerSearch.or("serverNull", 
OutOfBandManagementOwnerSearch.entity().getManagementServerId(), 
SearchCriteria.Op.NULL);
+OutOfBandManagementOwnerSearch.done();
+
+StateUpdateSearch = createSearchBuilder();
+StateUpdateSearch.and("status", 
StateUpdateSearch.entity().getPowerState(), SearchCriteria.Op.EQ);
+StateUpdateSearch.and("id", StateUpdateSearch.entity().getId(), 
SearchCriteria.Op.EQ);
+StateUpdateSearch.and("update", 
StateUpdateSearch.entity().getUpdateCount(), SearchCriteria.Op.EQ);
+StateUpdateSearch.done();
+
+PowerStateAttr = _allAttributes.get("powerState");
+MsIdAttr = _allAttributes.get("managementServerId");
+UpdateTimeAttr = _allAttributes.get("updateTime");
+assert (PowerStateAttr != null && MsIdAttr != null && 
UpdateTimeAttr != null) : "Couldn't find one of these attributes";
+}
+
+@Override
+public OutOfBandManagement findByHost(long hostId) {
+SearchCriteria sc = 
HostSearch.create("hostId", hostId);
+return findOneBy(sc);
+}
+
+@Override
+public List findAllByManagementServer(long 
serverId) {
+SearchCriteria sc = 
OutOfBandManagementOwnerSearch.create();
+sc.setParameters("server", serverId);
+return listBy(sc, new Filter(OutOfBandManagementVO.class, 
"updateTime", true, null, null));
+}
+
+private void 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61845126
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/outofbandmanagement/dao/OutOfBandManagementDaoImpl.java
 ---
@@ -0,0 +1,163 @@
+// 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.
+
+package org.apache.cloudstack.outofbandmanagement.dao;
+
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.db.Attribute;
+import com.cloud.utils.db.DB;
+import com.cloud.utils.db.Filter;
+import com.cloud.utils.db.GenericDaoBase;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+import com.cloud.utils.db.TransactionLegacy;
+import com.cloud.utils.db.UpdateBuilder;
+import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagementVO;
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import javax.ejb.Local;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.util.List;
+
+@DB
+@Component
+@Local(value = {OutOfBandManagementDao.class})
+public class OutOfBandManagementDaoImpl extends 
GenericDaoBase implements OutOfBandManagementDao {
+private static final Logger LOG = 
Logger.getLogger(OutOfBandManagementDaoImpl.class);
+
+private SearchBuilder HostSearch;
+private SearchBuilder ManagementServerSearch;
+private SearchBuilder 
OutOfBandManagementOwnerSearch;
+private SearchBuilder StateUpdateSearch;
+
+private Attribute PowerStateAttr;
+private Attribute MsIdAttr;
+private Attribute UpdateTimeAttr;
+
+public OutOfBandManagementDaoImpl() {
+super();
+
+HostSearch = createSearchBuilder();
+HostSearch.and("hostId", HostSearch.entity().getHostId(), 
SearchCriteria.Op.EQ);
+HostSearch.done();
+
+ManagementServerSearch = createSearchBuilder();
+ManagementServerSearch.and("server", 
ManagementServerSearch.entity().getManagementServerId(), SearchCriteria.Op.EQ);
+ManagementServerSearch.done();
+
+OutOfBandManagementOwnerSearch = createSearchBuilder();
+OutOfBandManagementOwnerSearch.and("server", 
OutOfBandManagementOwnerSearch.entity().getManagementServerId(), 
SearchCriteria.Op.EQ);
+OutOfBandManagementOwnerSearch.or("serverNull", 
OutOfBandManagementOwnerSearch.entity().getManagementServerId(), 
SearchCriteria.Op.NULL);
+OutOfBandManagementOwnerSearch.done();
+
+StateUpdateSearch = createSearchBuilder();
+StateUpdateSearch.and("status", 
StateUpdateSearch.entity().getPowerState(), SearchCriteria.Op.EQ);
+StateUpdateSearch.and("id", StateUpdateSearch.entity().getId(), 
SearchCriteria.Op.EQ);
+StateUpdateSearch.and("update", 
StateUpdateSearch.entity().getUpdateCount(), SearchCriteria.Op.EQ);
+StateUpdateSearch.done();
+
+PowerStateAttr = _allAttributes.get("powerState");
+MsIdAttr = _allAttributes.get("managementServerId");
+UpdateTimeAttr = _allAttributes.get("updateTime");
+assert (PowerStateAttr != null && MsIdAttr != null && 
UpdateTimeAttr != null) : "Couldn't find one of these attributes";
+}
+
+@Override
+public OutOfBandManagement findByHost(long hostId) {
+SearchCriteria sc = 
HostSearch.create("hostId", hostId);
+return findOneBy(sc);
+}
+
+@Override
+public List findAllByManagementServer(long 
serverId) {
+SearchCriteria sc = 
OutOfBandManagementOwnerSearch.create();
+sc.setParameters("server", serverId);
+return listBy(sc, new Filter(OutOfBandManagementVO.class, 
"updateTime", true, null, null));
+}
+
+private void 

[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...

[rhtyd]

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1502#discussion_r61844518
  
--- Diff: 
api/src/org/apache/cloudstack/outofbandmanagement/driver/OutOfBandManagementDriverPowerCommand.java
 ---
@@ -0,0 +1,33 @@
+// 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.
+package org.apache.cloudstack.outofbandmanagement.driver;
+
+import com.google.common.collect.ImmutableMap;
+import org.apache.cloudstack.outofbandmanagement.OutOfBandManagement;
+
+public class OutOfBandManagementDriverPowerCommand extends 
OutOfBandManagementDriverCommand {
--- End diff --

Fixed


---
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.
---