From 0b6f314f6e17eb6bfffe76ab64c73e8aabb4a14d Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Wed, 11 May 2016 12:02:28 +0530 Subject: [PATCH] CLOUDSTACK-9299: Sync changes from upstream oobm PR Signed-off-by: Rohit Yadav --- .../IpmitoolOutOfBandManagementDriver.java | 39 ++++--- .../driver/ipmitool/IpmitoolWrapper.java | 33 ++++-- .../driver/ipmitool/IpmitoolWrapperTest.java | 81 +++++++------- .../smoke/test_outofbandmanagement.py | 1 - tools/marvin/setup.py | 2 +- .../utils/process/ProcessRunner.java | 102 +++++++++--------- .../cloudstack/utils/process/ProcessTest.java | 18 +++- 7 files changed, 160 insertions(+), 116 deletions(-) diff --git a/plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolOutOfBandManagementDriver.java b/plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolOutOfBandManagementDriver.java index 92c4d1d21a8..bad3cb61ac6 100644 --- a/plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolOutOfBandManagementDriver.java +++ b/plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolOutOfBandManagementDriver.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.outofbandmanagement.driver.ipmitool; import com.cloud.utils.component.AdapterBase; +import com.cloud.utils.concurrency.NamedThreadFactory; import com.cloud.utils.exception.CloudRuntimeException; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; @@ -24,6 +25,7 @@ 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.OutOfBandManagementService; import org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverChangePasswordCommand; import org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverCommand; import org.apache.cloudstack.outofbandmanagement.driver.OutOfBandManagementDriverPowerCommand; @@ -33,20 +35,25 @@ import org.joda.time.Duration; import java.util.Arrays; import java.util.List; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; -public class IpmitoolOutOfBandManagementDriver extends AdapterBase implements OutOfBandManagementDriver, Configurable { +public final 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; - ConfigKey IpmiToolPath = new ConfigKey("Advanced", String.class, "outofbandmanagement.ipmitool.path", "/usr/bin/ipmitool", + private final ExecutorService ipmitoolExecutor = Executors.newFixedThreadPool(OutOfBandManagementService.SyncThreadPoolSize.value(), new NamedThreadFactory("IpmiToolDriver")); + private final IpmitoolWrapper IPMITOOL = new IpmitoolWrapper(ipmitoolExecutor); + + public final ConfigKey IpmiToolPath = new ConfigKey("Advanced", String.class, "outofbandmanagement.ipmitool.path", "/usr/bin/ipmitool", "The out of band management ipmitool path used by the IpmiTool driver. Default: /usr/bin/ipmitool.", true, ConfigKey.Scope.Global); - ConfigKey IpmiToolInterface = new ConfigKey("Advanced", String.class, "outofbandmanagement.ipmitool.interface", "lanplus", + public final ConfigKey IpmiToolInterface = new ConfigKey("Advanced", String.class, "outofbandmanagement.ipmitool.interface", "lanplus", "The out of band management IpmiTool driver interface to use. Default: lanplus. Valid values are: lan, lanplus, open etc.", true, ConfigKey.Scope.Global); - ConfigKey IpmiToolRetries = new ConfigKey("Advanced", String.class, "outofbandmanagement.ipmitool.retries", "1", + public final ConfigKey IpmiToolRetries = new ConfigKey("Advanced", String.class, "outofbandmanagement.ipmitool.retries", "1", "The out of band management IpmiTool driver retries option -R. Default 1.", true, ConfigKey.Scope.Global); private String getIpmiUserId(ImmutableMap options, final Duration timeOut) { @@ -55,16 +62,16 @@ public class IpmitoolOutOfBandManagementDriver extends AdapterBase implements Ou throw new CloudRuntimeException("Empty IPMI user configured, cannot proceed to find user's ID"); } - final List ipmiToolCommands = IpmitoolWrapper.getIpmiToolCommandArgs(IpmiToolPath.value(), + final List ipmiToolCommands = IPMITOOL.getIpmiToolCommandArgs(IpmiToolPath.value(), IpmiToolInterface.value(), IpmiToolRetries.value(), options, "user", "list"); - OutOfBandManagementDriverResponse output = IpmitoolWrapper.executeCommands(ipmiToolCommands, timeOut); + final OutOfBandManagementDriverResponse output = IPMITOOL.executeCommands(ipmiToolCommands, timeOut); if (!output.isSuccess()) { throw new CloudRuntimeException("Failed to find IPMI user to change password, error: " + output.getError()); } - final String userId = IpmitoolWrapper.findIpmiUser(output.getResult(), username); + final String userId = IPMITOOL.findIpmiUser(output.getResult(), username); if (Strings.isNullOrEmpty(userId)) { throw new CloudRuntimeException("No IPMI user ID found for the username: " + username); } @@ -97,30 +104,31 @@ public class IpmitoolOutOfBandManagementDriver extends AdapterBase implements Ou } private OutOfBandManagementDriverResponse execute(final OutOfBandManagementDriverPowerCommand cmd) { - List ipmiToolCommands = IpmitoolWrapper.getIpmiToolCommandArgs(IpmiToolPath.value(), + List ipmiToolCommands = IPMITOOL.getIpmiToolCommandArgs(IpmiToolPath.value(), IpmiToolInterface.value(), IpmiToolRetries.value(), - cmd.getOptions(), "chassis", "power", IpmitoolWrapper.parsePowerCommand(cmd.getPowerOperation())); - OutOfBandManagementDriverResponse response = IpmitoolWrapper.executeCommands(ipmiToolCommands, cmd.getTimeout()); + cmd.getOptions(), "chassis", "power", IPMITOOL.parsePowerCommand(cmd.getPowerOperation())); + + final OutOfBandManagementDriverResponse response = IPMITOOL.executeCommands(ipmiToolCommands, cmd.getTimeout()); if (response.isSuccess() && cmd.getPowerOperation().equals(OutOfBandManagement.PowerOperation.STATUS)) { - response.setPowerState(IpmitoolWrapper.parsePowerState(response.getResult().trim())); + response.setPowerState(IPMITOOL.parsePowerState(response.getResult().trim())); } return response; } private OutOfBandManagementDriverResponse execute(final OutOfBandManagementDriverChangePasswordCommand cmd) { - String outOfBandManagementUserId = getIpmiUserId(cmd.getOptions(), cmd.getTimeout()); + final String outOfBandManagementUserId = getIpmiUserId(cmd.getOptions(), cmd.getTimeout()); - List ipmiToolCommands = IpmitoolWrapper.getIpmiToolCommandArgs(IpmiToolPath.value(), + final List ipmiToolCommands = IPMITOOL.getIpmiToolCommandArgs(IpmiToolPath.value(), IpmiToolInterface.value(), IpmiToolRetries.value(), cmd.getOptions(), "user", "set", "password", outOfBandManagementUserId, cmd.getNewPassword()); - return IpmitoolWrapper.executeCommands(ipmiToolCommands, cmd.getTimeout()); + return IPMITOOL.executeCommands(ipmiToolCommands, cmd.getTimeout()); } private void initDriver() { isDriverEnabled = true; - OutOfBandManagementDriverResponse output = IpmitoolWrapper.executeCommands(Arrays.asList(IpmiToolPath.value(), "-V"), Duration.ZERO); + final OutOfBandManagementDriverResponse output = IPMITOOL.executeCommands(Arrays.asList(IpmiToolPath.value(), "-V")); if (output.isSuccess() && output.getResult().startsWith("ipmitool version")) { isIpmiToolBinAvailable = true; LOG.debug("OutOfBandManagementDriver ipmitool initialized: " + output.getResult()); @@ -142,6 +150,7 @@ public class IpmitoolOutOfBandManagementDriver extends AdapterBase implements Ou @Override public boolean stop() { + ipmitoolExecutor.shutdown(); stopDriver(); return true; } diff --git a/plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapper.java b/plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapper.java index 34c2dae4e3e..f5896b20733 100644 --- a/plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapper.java +++ b/plugins/outofbandmanagement-drivers/ipmitool/src/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapper.java @@ -31,11 +31,18 @@ import org.joda.time.Duration; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.ExecutorService; -public class IpmitoolWrapper { +public final class IpmitoolWrapper { public static final Logger LOG = Logger.getLogger(IpmitoolWrapper.class); - public static String parsePowerCommand(OutOfBandManagement.PowerOperation operation) { + private final ProcessRunner RUNNER; + + public IpmitoolWrapper(ExecutorService executor) { + this.RUNNER = new ProcessRunner(executor); + } + + public String parsePowerCommand(OutOfBandManagement.PowerOperation operation) { if (operation == null) { throw new IllegalStateException("Invalid power operation requested"); } @@ -53,7 +60,7 @@ public class IpmitoolWrapper { return operation.toString().toLowerCase(); } - public static OutOfBandManagement.PowerState parsePowerState(final String standardOutput) { + public OutOfBandManagement.PowerState parsePowerState(final String standardOutput) { if (Strings.isNullOrEmpty(standardOutput)) { return OutOfBandManagement.PowerState.Unknown; } @@ -65,10 +72,10 @@ public class IpmitoolWrapper { return OutOfBandManagement.PowerState.Unknown; } - public static List getIpmiToolCommandArgs(final String ipmiToolPath, final String ipmiInterface, final String retries, + public List getIpmiToolCommandArgs(final String ipmiToolPath, final String ipmiInterface, final String retries, final ImmutableMap options, String... commands) { - ImmutableList.Builder ipmiToolCommands = ImmutableList.builder() + final ImmutableList.Builder ipmiToolCommands = ImmutableList.builder() .add(ipmiToolPath) .add("-I") .add(ipmiInterface) @@ -103,7 +110,7 @@ public class IpmitoolWrapper { return ipmiToolCommands.build(); } - public static String findIpmiUser(final String usersList, final String username) { + public String findIpmiUser(final String usersList, final String username) { /** * Expected usersList string contains legends on first line and users on rest * ID Name Callin Link Auth IPMI Msg Channel Priv Limit @@ -116,12 +123,12 @@ public class IpmitoolWrapper { // Assuming user 'Name' index on 2nd position int usernameIndex = 1; - String[] lines = usersList.split("\\r?\\n"); + final String[] lines = usersList.split("\\r?\\n"); if (lines.length < 2) { throw new CloudRuntimeException("Error parsing user ID from ipmi user listing"); } // Find user and name indexes from the 1st line if not on default position - String[] legends = lines[0].split(" +"); + final String[] legends = lines[0].split(" +"); for (int idx = 0; idx < legends.length; idx++) { if (legends[idx].equals("ID")) { idIndex = idx; @@ -133,7 +140,7 @@ public class IpmitoolWrapper { // Find user 'ID' based on provided username and ID/Name positions String userId = null; for (int idx = 1; idx < lines.length; idx++) { - String[] words = lines[idx].split(" +"); + final String[] words = lines[idx].split(" +"); if (usernameIndex < words.length && idIndex < words.length) { if (words[usernameIndex].equals(username)) { userId = words[idIndex]; @@ -143,8 +150,12 @@ public class IpmitoolWrapper { return userId; } - public static OutOfBandManagementDriverResponse executeCommands(final List commands, final Duration timeOut) { - final ProcessResult result = ProcessRunner.executeCommands(commands, timeOut); + public OutOfBandManagementDriverResponse executeCommands(final List commands) { + return executeCommands(commands, ProcessRunner.DEFAULT_MAX_TIMEOUT); + } + + public OutOfBandManagementDriverResponse executeCommands(final List commands, final Duration timeOut) { + final ProcessResult result = RUNNER.executeCommands(commands, timeOut); if (LOG.isTraceEnabled()) { List cleanedCommands = new ArrayList(); int maskNextCommand = 0; diff --git a/plugins/outofbandmanagement-drivers/ipmitool/test/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapperTest.java b/plugins/outofbandmanagement-drivers/ipmitool/test/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapperTest.java index b19fe636490..86eff86b023 100644 --- a/plugins/outofbandmanagement-drivers/ipmitool/test/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapperTest.java +++ b/plugins/outofbandmanagement-drivers/ipmitool/test/org/apache/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolWrapperTest.java @@ -19,11 +19,11 @@ package org.apache.cloudstack.outofbandmanagement.driver.ipmitool; +import com.cloud.utils.concurrency.NamedThreadFactory; 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.joda.time.Duration; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -31,41 +31,44 @@ import org.mockito.runners.MockitoJUnitRunner; import java.util.Arrays; import java.util.List; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; @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"); - } + + private static final ExecutorService ipmitoolExecutor = Executors.newFixedThreadPool(20, new NamedThreadFactory("IpmiToolDriverTest")); + private static final IpmitoolWrapper IPMITOOL = new IpmitoolWrapper(ipmitoolExecutor); @Test + public void testParsePowerCommandValid() { + Assert.assertEquals(IPMITOOL.parsePowerCommand(OutOfBandManagement.PowerOperation.ON), "on"); + Assert.assertEquals(IPMITOOL.parsePowerCommand(OutOfBandManagement.PowerOperation.OFF), "off"); + Assert.assertEquals(IPMITOOL.parsePowerCommand(OutOfBandManagement.PowerOperation.CYCLE), "cycle"); + Assert.assertEquals(IPMITOOL.parsePowerCommand(OutOfBandManagement.PowerOperation.RESET), "reset"); + Assert.assertEquals(IPMITOOL.parsePowerCommand(OutOfBandManagement.PowerOperation.SOFT), "soft"); + Assert.assertEquals(IPMITOOL.parsePowerCommand(OutOfBandManagement.PowerOperation.STATUS), "status"); + } + + @Test(expected = IllegalStateException.class) public void testParsePowerCommandInvalid() { - try { - IpmitoolWrapper.parsePowerCommand(null); - Assert.fail("IpmitoolWrapper.parsePowerCommand failed to throw exception on invalid power state"); - } catch (IllegalStateException e) { - } + IPMITOOL.parsePowerCommand(null); + Assert.fail("IpmitoolWrapper.parsePowerCommand failed to throw exception on invalid power state"); } @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); + Assert.assertEquals(IPMITOOL.parsePowerState(null), OutOfBandManagement.PowerState.Unknown); + Assert.assertEquals(IPMITOOL.parsePowerState(""), OutOfBandManagement.PowerState.Unknown); + Assert.assertEquals(IPMITOOL.parsePowerState(" "), OutOfBandManagement.PowerState.Unknown); + Assert.assertEquals(IPMITOOL.parsePowerState("invalid data"), OutOfBandManagement.PowerState.Unknown); + Assert.assertEquals(IPMITOOL.parsePowerState("Chassis Power is on"), OutOfBandManagement.PowerState.On); + Assert.assertEquals(IPMITOOL.parsePowerState("Chassis Power is off"), OutOfBandManagement.PowerState.Off); } @Test public void testGetIpmiToolCommandArgs() { - List args = IpmitoolWrapper.getIpmiToolCommandArgs("binpath", "intf", "1", null); + List args = IPMITOOL.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"}); @@ -73,35 +76,39 @@ public class IpmitoolWrapperTest { 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"); + List argsWithOpts = IPMITOOL.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 { - IpmitoolWrapper.findIpmiUser("some invalid string\n", "admin"); - Assert.fail("IpmitoolWrapper.findIpmiUser failed to throw exception on invalid data"); - } catch (CloudRuntimeException ignore) { - } - Assert.assertEquals(IpmitoolWrapper.findIpmiUser("some\ninvalid\ndata\n", "admin"), null); + @Test(expected = CloudRuntimeException.class) + public void testFindIpmiUserInvalid() { + IPMITOOL.findIpmiUser("some invalid string\n", "admin"); + Assert.fail("IpmitoolWrapper.findIpmiUser failed to throw exception on invalid data"); - // Valid data + Assert.assertEquals(IPMITOOL.findIpmiUser("some\ninvalid\ndata\n", "admin"), null); + } + + @Test + public void testFindIpmiUserNull() { + Assert.assertEquals(IPMITOOL.findIpmiUser("some\ninvalid\ndata\n", "admin"), null); + } + + @Test + public void testFindIpmiUserValid() { String usersList = "ID Name\t Callin Link Auth\tIPMI Msg Channel Priv Limit\n" + "1 admin true true true ADMINISTRATOR\n" + "2 operator true false false OPERATOR\n" + "3 user true true true USER\n"; - Assert.assertEquals(IpmitoolWrapper.findIpmiUser(usersList, "admin"), "1"); - Assert.assertEquals(IpmitoolWrapper.findIpmiUser(usersList, "operator"), "2"); - Assert.assertEquals(IpmitoolWrapper.findIpmiUser(usersList, "user"), "3"); + Assert.assertEquals(IPMITOOL.findIpmiUser(usersList, "admin"), "1"); + Assert.assertEquals(IPMITOOL.findIpmiUser(usersList, "operator"), "2"); + Assert.assertEquals(IPMITOOL.findIpmiUser(usersList, "user"), "3"); } @Test public void testExecuteCommands() { - OutOfBandManagementDriverResponse r = IpmitoolWrapper.executeCommands(Arrays.asList("ls", "/tmp"), Duration.ZERO); + OutOfBandManagementDriverResponse r = IPMITOOL.executeCommands(Arrays.asList("ls", "/tmp")); Assert.assertTrue(r.isSuccess()); Assert.assertTrue(r.getResult().length() > 0); } diff --git a/test/integration/smoke/test_outofbandmanagement.py b/test/integration/smoke/test_outofbandmanagement.py index 7c2aa6b5da9..05a6d005163 100644 --- a/test/integration/smoke/test_outofbandmanagement.py +++ b/test/integration/smoke/test_outofbandmanagement.py @@ -70,7 +70,6 @@ class TestOutOfBandManagement(cloudstackTestCase): if self.server: self.server.shutdown() self.server.server_close() - IpmiServerContext('reset') except Exception as e: raise Exception("Warning: Exception during cleanup : %s" % e) diff --git a/tools/marvin/setup.py b/tools/marvin/setup.py index 550537727da..c16c341578f 100644 --- a/tools/marvin/setup.py +++ b/tools/marvin/setup.py @@ -51,7 +51,7 @@ setup(name="Marvin", "paramiko >= 1.13.0", "nose >= 1.3.3", "ddt >= 0.4.0", - "ipmisim >= 0.6" + "ipmisim >= 0.7" ], py_modules=['marvin.marvinPlugin'], zip_safe=False, diff --git a/utils/src/org/apache/cloudstack/utils/process/ProcessRunner.java b/utils/src/org/apache/cloudstack/utils/process/ProcessRunner.java index a404da18136..c8ea6bf59a4 100644 --- a/utils/src/org/apache/cloudstack/utils/process/ProcessRunner.java +++ b/utils/src/org/apache/cloudstack/utils/process/ProcessRunner.java @@ -19,43 +19,55 @@ 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 com.google.common.io.CharStreams; 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 final class ProcessRunner { public static final Logger LOG = Logger.getLogger(ProcessRunner.class); - private static final ExecutorService processExecutor = Executors.newCachedThreadPool(new NamedThreadFactory("ProcessRunner")); + // Default maximum timeout of 5 minutes for any command + public static final Duration DEFAULT_MAX_TIMEOUT = new Duration(5 * 60 * 1000); + private final ExecutorService executor; - 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 ProcessRunner(ExecutorService executor) { + this.executor = executor; } - public static ProcessResult executeCommands(final List commands, final Duration timeOut) { - Preconditions.checkArgument(commands != null && timeOut != null); + /** + * Executes a process with provided list of commands with a max default timeout + * of 5 minutes + * @param commands list of string commands + * @return returns process result + */ + public ProcessResult executeCommands(final List commands) { + return executeCommands(commands, DEFAULT_MAX_TIMEOUT); + } + + /** + * Executes a process with provided list of commands with a given timeout that is less + * than or equal to DEFAULT_MAX_TIMEOUT + * @param commands list of string commands + * @param timeOut timeout duration + * @return returns process result + */ + public ProcessResult executeCommands(final List commands, final Duration timeOut) { + Preconditions.checkArgument(commands != null && timeOut != null + && timeOut.getStandardSeconds() > 0L + && (timeOut.compareTo(DEFAULT_MAX_TIMEOUT) <= 0) + && executor != null); int retVal = -2; String stdOutput = null; @@ -63,38 +75,32 @@ public class ProcessRunner { 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()); - } - process.destroy(); + final Future processFuture = executor.submit(new Callable() { + @Override + public Integer call() throws Exception { + return process.waitFor(); } - } else { - retVal = process.waitFor(); - stdOutput = readStream(process.getInputStream()); - stdError = readStream(process.getErrorStream()); + }); + try { + retVal = processFuture.get(timeOut.getStandardSeconds(), TimeUnit.SECONDS); + } catch (ExecutionException e) { + retVal = -2; + 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 = CharStreams.toString(new InputStreamReader(process.getInputStream())); + stdError = CharStreams.toString(new InputStreamReader(process.getErrorStream())); + } + process.destroy(); } if (LOG.isTraceEnabled()) { LOG.trace("Process standard output: " + stdOutput); diff --git a/utils/test/org/apache/cloudstack/utils/process/ProcessTest.java b/utils/test/org/apache/cloudstack/utils/process/ProcessTest.java index 0bcf1656512..b5e6e04c31a 100644 --- a/utils/test/org/apache/cloudstack/utils/process/ProcessTest.java +++ b/utils/test/org/apache/cloudstack/utils/process/ProcessTest.java @@ -19,6 +19,7 @@ package org.apache.cloudstack.utils.process; +import com.cloud.utils.concurrency.NamedThreadFactory; import com.google.common.base.Strings; import org.joda.time.Duration; import org.junit.Assert; @@ -27,13 +28,18 @@ import org.junit.runner.RunWith; import org.mockito.runners.MockitoJUnitRunner; import java.util.Arrays; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; @RunWith(MockitoJUnitRunner.class) public class ProcessTest { + private static final ExecutorService executor = Executors.newFixedThreadPool(10, new NamedThreadFactory("IpmiToolDriverTest")); + private static final ProcessRunner RUNNER = new ProcessRunner(executor); + @Test public void testProcessRunner() { - ProcessResult result = ProcessRunner.executeCommands(Arrays.asList("ls", "/tmp"), Duration.ZERO); + ProcessResult result = RUNNER.executeCommands(Arrays.asList("ls", "/tmp")); Assert.assertEquals(result.getReturnCode(), 0); Assert.assertTrue(Strings.isNullOrEmpty(result.getStdError())); Assert.assertTrue(result.getStdOutput().length() > 0); @@ -41,7 +47,7 @@ public class ProcessTest { @Test public void testProcessRunnerWithTimeout() { - ProcessResult result = ProcessRunner.executeCommands(Arrays.asList("sleep", "5"), Duration.standardSeconds(1)); + ProcessResult result = RUNNER.executeCommands(Arrays.asList("sleep", "5"), Duration.standardSeconds(1)); Assert.assertNotEquals(result.getReturnCode(), 0); Assert.assertTrue(result.getStdError().length() > 0); Assert.assertEquals(result.getStdError(), "Operation timed out, aborted"); @@ -49,9 +55,15 @@ public class ProcessTest { @Test public void testProcessRunnerWithTimeoutAndException() { - ProcessResult result = ProcessRunner.executeCommands(Arrays.asList("ls", "/some/dir/that/should/not/exist"), Duration.standardSeconds(2)); + ProcessResult result = RUNNER.executeCommands(Arrays.asList("ls", "/some/dir/that/should/not/exist"), Duration.standardSeconds(2)); Assert.assertNotEquals(result.getReturnCode(), 0); Assert.assertTrue(result.getStdError().length() > 0); Assert.assertNotEquals(result.getStdError(), "Operation timed out, aborted"); } + + @Test(expected = IllegalArgumentException.class) + public void testProcessRunnerWithMoreThanMaxAllowedTimeout() { + RUNNER.executeCommands(Arrays.asList("ls", "/some/dir/that/should/not/exist"), ProcessRunner.DEFAULT_MAX_TIMEOUT.plus(1000)); + Assert.fail("Illegal argument exception was expected"); + } }