Repository: brooklyn-server Updated Branches: refs/heads/master b6a65650e -> ee3ca1e71
Ensure detection of failed pre.install.command and related commands. Currently 'pre.install.command' and related steps do not fail if the command(s) returns a non-zero exit. This can mean your install will fail but Brooklyn won't detect it (perhaps until some subsequent stage, or not at all). This change adds detection of the return code and failure if it is non-zero. The 'contract' for the methods is added as a comment. The AbstractSoftwareProcessSshDriver is changed to detect the failure and throw an exception. The AbstractSoftwareProcessWinRmDriver already does the right thing and doesn't need changed. While this could break some existing blueprints, I think in such cases it's more likely that it is highlighting a problem that has been missed, rather than causing a problem because someone is explicitly relying on that behaviour. Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/d76315ff Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/d76315ff Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/d76315ff Branch: refs/heads/master Commit: d76315ff46c79bd9353961451d133790e4c65cf3 Parents: 36be4a3 Author: Geoff Macartney <[email protected]> Authored: Thu Jun 16 09:57:28 2016 +0100 Committer: Geoff Macartney <[email protected]> Committed: Fri Jun 17 12:03:08 2016 +0100 ---------------------------------------------------------------------- .../base/AbstractSoftwareProcessDriver.java | 41 +++++++++++++++- .../base/AbstractSoftwareProcessSshDriver.java | 50 ++++++++++++-------- .../base/SoftwareProcessEntityTest.java | 44 +++++++++++++++++ 3 files changed, 115 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/d76315ff/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessDriver.java ---------------------------------------------------------------------- diff --git a/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessDriver.java b/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessDriver.java index 72a80ec..f6449b0 100644 --- a/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessDriver.java +++ b/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessDriver.java @@ -219,16 +219,55 @@ public abstract class AbstractSoftwareProcessDriver implements SoftwareProcessDr */ public void preInstall() {} + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void runPreInstallCommand(); + + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void setup(); + + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void install(); + + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void runPostInstallCommand(); + + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void runPreCustomizeCommand(); + + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void customize(); + + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void runPostCustomizeCommand(); + + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void runPreLaunchCommand(); + + /** + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void launch(); - /** Only run if launch is run (if start is not skipped). */ + + /** + * Only run if launch is run (if start is not skipped). + * Implementations should fail if the return code is non-zero, by throwing some appropriate exception. + */ public abstract void runPostLaunchCommand(); @Override http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/d76315ff/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessSshDriver.java ---------------------------------------------------------------------- diff --git a/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessSshDriver.java b/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessSshDriver.java index 553c2b9..1702c34 100644 --- a/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessSshDriver.java +++ b/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessSshDriver.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import org.apache.brooklyn.config.ConfigKey; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -226,19 +227,31 @@ public abstract class AbstractSoftwareProcessSshDriver extends AbstractSoftwareP return SshEffectorTasks.getSshFlags(getEntity(), getMachine()); } + /** + * @deprecated since 0.10.0 This method will become private in a future release. + */ + @Deprecated public int execute(String command, String summaryForLogging) { return execute(ImmutableList.of(command), summaryForLogging); } + /** + * @deprecated since 0.10.0 This method will become private in a future release. + */ + @Deprecated public int execute(List<String> script, String summaryForLogging) { return execute(Maps.newLinkedHashMap(), script, summaryForLogging); } + /** + * @deprecated since 0.10.0 This method will become private in a future release. + */ @SuppressWarnings({ "rawtypes", "unchecked" }) @Override + @Deprecated public int execute(Map flags2, List<String> script, String summaryForLogging) { // TODO replace with SshEffectorTasks.ssh ?; remove the use of flags - + // TODO log the stdin/stdout/stderr upon error Map flags = Maps.newLinkedHashMap(); if (!flags2.containsKey(IGNORE_ENTITY_SSH_FLAGS)) { flags.putAll(getSshFlags()); @@ -285,46 +298,45 @@ public abstract class AbstractSoftwareProcessSshDriver extends AbstractSoftwareP } } + private void executeSuccessfully(ConfigKey<String> configKey, String label) { + if(Strings.isNonBlank(getEntity().getConfig(configKey))) { + log.debug("Executing {} on entity {}", label, entity.getDisplayName()); + int result = execute(ImmutableList.of(getEntity().getConfig(configKey)), label); + if (0 != result) { + log.debug("Executing {} failed with return code {}", label, result); + throw new IllegalStateException("commands for " + configKey.getName() + " failed with return code " + result); + } + } + } + @Override public void runPreInstallCommand() { - if(Strings.isNonBlank(getEntity().getConfig(BrooklynConfigKeys.PRE_INSTALL_COMMAND))) { - execute(ImmutableList.of(getEntity().getConfig(BrooklynConfigKeys.PRE_INSTALL_COMMAND)), "running pre-install commands"); - } + executeSuccessfully(BrooklynConfigKeys.PRE_INSTALL_COMMAND, "running pre-install commands"); } @Override public void runPostInstallCommand() { - if (Strings.isNonBlank(entity.getConfig(BrooklynConfigKeys.POST_INSTALL_COMMAND))) { - execute(ImmutableList.of(entity.getConfig(BrooklynConfigKeys.POST_INSTALL_COMMAND)), "running post-install commands"); - } + executeSuccessfully(BrooklynConfigKeys.POST_INSTALL_COMMAND, "running post-install commands"); } @Override public void runPreCustomizeCommand() { - if(Strings.isNonBlank(getEntity().getConfig(BrooklynConfigKeys.PRE_CUSTOMIZE_COMMAND))) { - execute(ImmutableList.of(getEntity().getConfig(BrooklynConfigKeys.PRE_CUSTOMIZE_COMMAND)), "running pre-customize commands"); - } + executeSuccessfully(BrooklynConfigKeys.PRE_CUSTOMIZE_COMMAND, "running pre-customize commands"); } @Override public void runPostCustomizeCommand() { - if (Strings.isNonBlank(entity.getConfig(BrooklynConfigKeys.POST_CUSTOMIZE_COMMAND))) { - execute(ImmutableList.of(entity.getConfig(BrooklynConfigKeys.POST_CUSTOMIZE_COMMAND)), "running post-customize commands"); - } + executeSuccessfully(BrooklynConfigKeys.POST_CUSTOMIZE_COMMAND, "running post-customize commands"); } @Override public void runPreLaunchCommand() { - if (Strings.isNonBlank(entity.getConfig(BrooklynConfigKeys.PRE_LAUNCH_COMMAND))) { - execute(ImmutableList.of(entity.getConfig(BrooklynConfigKeys.PRE_LAUNCH_COMMAND)), "running pre-launch commands"); - } + executeSuccessfully(BrooklynConfigKeys.PRE_LAUNCH_COMMAND, "running pre-launch commands"); } @Override public void runPostLaunchCommand() { - if (Strings.isNonBlank(entity.getConfig(BrooklynConfigKeys.POST_LAUNCH_COMMAND))) { - execute(ImmutableList.of(entity.getConfig(BrooklynConfigKeys.POST_LAUNCH_COMMAND)), "running post-launch commands"); - } + executeSuccessfully(BrooklynConfigKeys.POST_LAUNCH_COMMAND, "running post-launch commands"); } /** http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/d76315ff/software/base/src/test/java/org/apache/brooklyn/entity/software/base/SoftwareProcessEntityTest.java ---------------------------------------------------------------------- diff --git a/software/base/src/test/java/org/apache/brooklyn/entity/software/base/SoftwareProcessEntityTest.java b/software/base/src/test/java/org/apache/brooklyn/entity/software/base/SoftwareProcessEntityTest.java index 6fd330a..8e56b18 100644 --- a/software/base/src/test/java/org/apache/brooklyn/entity/software/base/SoftwareProcessEntityTest.java +++ b/software/base/src/test/java/org/apache/brooklyn/entity/software/base/SoftwareProcessEntityTest.java @@ -79,6 +79,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.testng.Assert; import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import com.google.common.collect.ImmutableList; @@ -583,6 +584,39 @@ public class SoftwareProcessEntityTest extends BrooklynAppUnitTestSupport { Assert.assertTrue(entity.getRequiredOpenPorts().contains(9999)); } + @DataProvider + public Object[][] runCommandsProvider() { + return new Object[][] { + {BrooklynConfigKeys.PRE_INSTALL_COMMAND}, + {BrooklynConfigKeys.POST_INSTALL_COMMAND}, + {BrooklynConfigKeys.PRE_CUSTOMIZE_COMMAND}, + {BrooklynConfigKeys.POST_CUSTOMIZE_COMMAND}, + {BrooklynConfigKeys.PRE_LAUNCH_COMMAND}, + {BrooklynConfigKeys.POST_LAUNCH_COMMAND} + }; + } + + @Test(dataProvider = "runCommandsProvider") + public void testCommandFailureCausesEntityFailure(ConfigKey<String> configKey) { + String failingCommand = "unsuccessfulCommand"; + final ConfigurableFailingCommandMachineLocation location = mgmt.getLocationManager().createLocation( + LocationSpec.create(ConfigurableFailingCommandMachineLocation.class) + .configure("address", "localhost") + .configure(ConfigurableFailingCommandMachineLocation.FAILURE_COMMAND, failingCommand) + ); + VanillaSoftwareProcess entity = app.createAndManageChild(EntitySpec.create(VanillaSoftwareProcess.class) + .configure(configKey, failingCommand) + ); + + try { + app.start(ImmutableList.<Location>of(location)); + Asserts.shouldHaveFailedPreviously(); + } catch (Exception e) { + Asserts.expectedFailureContains(e, configKey.getName()); + } + EntityAsserts.assertAttributeEqualsEventually(entity, Attributes.SERVICE_STATE_ACTUAL, Lifecycle.ON_FIRE); + } + @ImplementedBy(MyServiceImpl.class) public interface MyService extends SoftwareProcess { PortAttributeSensorAndConfigKey HTTP_PORT = Attributes.HTTP_PORT; @@ -805,4 +839,14 @@ public class SoftwareProcessEntityTest extends BrooklynAppUnitTestSupport { } + public static class ConfigurableFailingCommandMachineLocation extends SshMachineLocation { + + public static ConfigKey<String> FAILURE_COMMAND = + ConfigKeys.newConfigKey("failingCommand", "A command that will return non zero result", "fail"); + + @Override + public int execScript(Map<String,?> props, String summaryForLogging, List<String> commands, Map<String,?> env) { + return commands.contains(getConfig(FAILURE_COMMAND)) ? 1 : 0; + } + } }
