You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by nakomis <gi...@git.apache.org> on 2014/12/08 14:08:19 UTC
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
GitHub user nakomis opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/375
Parameterize stop effector
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nakomis/incubator-brooklyn parameterize-stop-effector
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/375.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 #375
----
commit 750a6e19f6b930134786b9b164fa659b9d4b4976
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2014-12-04T15:11:45Z
Adds "None" location to arguments of start effector
commit 1f753f04656efb2ddcf01d5187b8fabe465fb9d2
Author: Martin Harris <gi...@nakomis.com>
Date: 2014-12-08T13:07:47Z
Adds optional parameter to stop effector for stopping underlying machine
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by Martin Harris <ma...@cloudsoftcorp.com>.
Ok, once that's done, I'll rebase your commit out of this PR
On 8 December 2014 at 14:51, sjcorbett <gi...@git.apache.org> wrote:
> Github user sjcorbett commented on the pull request:
>
>
> https://github.com/apache/incubator-brooklyn/pull/375#issuecomment-66125824
>
> My commit breaks one of the tests in the jsgui. I opened and rescinded
> #374 earlier today. I'll fix the issue by the end of the day and open a
> separate 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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
--
Martin Harris
Lead Software Engineer
Cloudsoft Corporation Ltd
www.cloudsoftcorp.com
Mobile: +44 (0)7989 047-855
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#issuecomment-66125824
My commit breaks one of the tests in the jsgui. I opened and rescinded #374 earlier today. I'll fix the issue by the end of the day and open a separate 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#issuecomment-66277459
Rebased
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#discussion_r21532276
--- Diff: software/base/src/main/java/brooklyn/entity/software/MachineLifecycleEffectorTasks.java ---
@@ -521,9 +532,14 @@ protected void restartChildren(ConfigBag parameters) {
* invokes {@link #preStopCustom()}, {@link #stopProcessesAtMachine()}, then finally
* {@link #stopAnyProvisionedMachines()} and sets state {@link Lifecycle#STOPPED}
*/
- public void stop() {
+ public void stop(ConfigBag parameters) {
log.info("Stopping {} in {}", entity(), entity().getLocations());
+ StopSoftwareParameters.StopMachineMode isStopMachine = parameters.get(StopSoftwareParameters.STOP_MACHINE_TYPED);
+
+ if (isStopMachine==null)
+ isStopMachine=StopSoftwareParameters.StopMachineMode.TRUE;
--- End diff --
Ah, the perils of copy-and-paste...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#discussion_r21530013
--- Diff: software/base/src/test/java/brooklyn/entity/basic/SoftwareProcessEntityTest.java ---
@@ -55,9 +44,25 @@
import brooklyn.util.task.Tasks;
import brooklyn.util.text.Strings;
import brooklyn.util.time.Duration;
-
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import org.jclouds.util.Throwables2;
+import org.slf4j.Logger;
--- End diff --
Try not to reorganise imports (I believe the previous was Eclipse's defaults, which is the default across almost all of the brooklyn code base).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#discussion_r21529931
--- Diff: software/base/src/main/java/brooklyn/entity/software/MachineLifecycleEffectorTasks.java ---
@@ -547,40 +563,53 @@ public void stop() {
return "Stop at machine completed with no errors.";
}});
- // Release this machine (even if error trying to stop process - we rethrow that after)
- Task<StopMachineDetails<Integer>> stoppingMachine = DynamicTasks.queue("stopping (machine)", new Callable<StopMachineDetails<Integer>>() { public StopMachineDetails<Integer> call() {
- if (entity().getAttribute(SoftwareProcess.SERVICE_STATE_ACTUAL)==Lifecycle.STOPPED) {
- log.debug("Skipping stop of entity "+entity()+" when already stopped");
- return new StopMachineDetails<Integer>("Already stopped", 0);
- }
- return stopAnyProvisionedMachines();
- }});
-
- DynamicTasks.drain(entity().getConfig(STOP_PROCESS_TIMEOUT), false);
- // shutdown the machine if stopping process fails or takes too long
- synchronized (stoppingMachine) {
- // task also used as mutex by DST when it submits it; ensure it only submits once!
- if (!stoppingMachine.isSubmitted()) {
- // force the stoppingMachine task to run by submitting it here
- log.warn("Submitting machine stop early in background for "+entity()+" because process stop has "+
- (stoppingProcess.isDone() ? "finished abnormally" : "not finished"));
- Entities.submit(entity(), stoppingMachine);
+ Task<StopMachineDetails<Integer>> stoppingMachine = null;
+ if (isStopMachine == StopSoftwareParameters.StopMachineMode.TRUE) {
+ // Release this machine (even if error trying to stop process - we rethrow that after)
+ stoppingMachine = DynamicTasks.queue("stopping (machine)", new Callable<StopMachineDetails<Integer>>() {
+ public StopMachineDetails<Integer> call() {
+ if (entity().getAttribute(SoftwareProcess.SERVICE_STATE_ACTUAL) == Lifecycle.STOPPED) {
+ log.debug("Skipping stop of entity " + entity() + " when already stopped");
+ return new StopMachineDetails<Integer>("Already stopped", 0);
+ }
+ return stopAnyProvisionedMachines();
+ }
+ });
+
+ DynamicTasks.drain(entity().getConfig(STOP_PROCESS_TIMEOUT), false);
+
+ // shutdown the machine if stopping process fails or takes too long
+ synchronized (stoppingMachine) {
+ // task also used as mutex by DST when it submits it; ensure it only submits once!
+ if (!stoppingMachine.isSubmitted()) {
+ // force the stoppingMachine task to run by submitting it here
+ log.warn("Submitting machine stop early in background for "+entity()+" because process stop has "+
+ (stoppingProcess.isDone() ? "finished abnormally" : "not finished"));
+ Entities.submit(entity(), stoppingMachine);
+ }
}
}
try {
- if (stoppingMachine.get().value == 0) {
+ boolean checkStopProcesses = true;
+ // This maintains previous behaviour of silently squashing any errors on the stoppingProcess task if the
+ // stoppingMachine exits with a nonzero value
+ if (stoppingMachine != null) {
+ checkStopProcesses = stoppingMachine.get().value == 0;
+ }
+
+ if (checkStopProcesses) {
// TODO we should test for destruction above, not merely successful "stop", as things like localhost and ssh won't be destroyed
DynamicTasks.waitForLast();
if (sshMachine.isPresent()) {
// throw early errors *only if* there is a machine and we have not destroyed it
stoppingProcess.get();
}
- }
- entity().setAttribute(SoftwareProcess.SERVICE_UP, false);
- ServiceStateLogic.setExpectedState(entity(), Lifecycle.STOPPED);
+ entity().setAttribute(SoftwareProcess.SERVICE_UP, false);
--- End diff --
This looks wrong - should we not always set SERVICE_UP to false and set the expected state, rather than only `if checkStopProcesses`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#issuecomment-66288598
Looks good. One bug spotted (I think), and the rest is very minor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#issuecomment-66256062
@nakomis see #378.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#discussion_r21527868
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcess.java ---
@@ -253,6 +253,21 @@ private ChildStartableMode(boolean isDisabled, boolean isBackground, boolean isL
public enum RestartMachineMode { TRUE, FALSE, AUTO }
}
+
+ public static class StopSoftwareParameters {
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<Object> STOP_MACHINE = ConfigKeys.newConfigKey(Object.class, "stopMachine",
+ "Whether to stop the machine provisioned for this entity: 'true', or 'false' are supported, "
+ + "with the default being 'true'",
+ StopMachineMode.TRUE.toString().toLowerCase());
+
+ // we supply a typed variant for retrieval; we want the untyped (above) to use lower case as the default in the GUI
+ // (very hard if using enum, since enum takes the name, and RendererHints do not apply to parameters)
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<StopMachineMode> STOP_MACHINE_TYPED = ConfigKeys.newConfigKey(StopMachineMode.class, "stopMachine");
+
+ public enum StopMachineMode { TRUE, FALSE }
--- End diff --
I suspect the `enum` is overkill for just a true/false (rather than us using `Boolean`).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/375
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#discussion_r21529738
--- Diff: software/base/src/main/java/brooklyn/entity/software/MachineLifecycleEffectorTasks.java ---
@@ -547,40 +563,53 @@ public void stop() {
return "Stop at machine completed with no errors.";
}});
- // Release this machine (even if error trying to stop process - we rethrow that after)
- Task<StopMachineDetails<Integer>> stoppingMachine = DynamicTasks.queue("stopping (machine)", new Callable<StopMachineDetails<Integer>>() { public StopMachineDetails<Integer> call() {
- if (entity().getAttribute(SoftwareProcess.SERVICE_STATE_ACTUAL)==Lifecycle.STOPPED) {
- log.debug("Skipping stop of entity "+entity()+" when already stopped");
- return new StopMachineDetails<Integer>("Already stopped", 0);
- }
- return stopAnyProvisionedMachines();
- }});
-
- DynamicTasks.drain(entity().getConfig(STOP_PROCESS_TIMEOUT), false);
- // shutdown the machine if stopping process fails or takes too long
- synchronized (stoppingMachine) {
- // task also used as mutex by DST when it submits it; ensure it only submits once!
- if (!stoppingMachine.isSubmitted()) {
- // force the stoppingMachine task to run by submitting it here
- log.warn("Submitting machine stop early in background for "+entity()+" because process stop has "+
- (stoppingProcess.isDone() ? "finished abnormally" : "not finished"));
- Entities.submit(entity(), stoppingMachine);
+ Task<StopMachineDetails<Integer>> stoppingMachine = null;
+ if (isStopMachine == StopSoftwareParameters.StopMachineMode.TRUE) {
+ // Release this machine (even if error trying to stop process - we rethrow that after)
+ stoppingMachine = DynamicTasks.queue("stopping (machine)", new Callable<StopMachineDetails<Integer>>() {
+ public StopMachineDetails<Integer> call() {
+ if (entity().getAttribute(SoftwareProcess.SERVICE_STATE_ACTUAL) == Lifecycle.STOPPED) {
+ log.debug("Skipping stop of entity " + entity() + " when already stopped");
+ return new StopMachineDetails<Integer>("Already stopped", 0);
+ }
+ return stopAnyProvisionedMachines();
+ }
+ });
+
+ DynamicTasks.drain(entity().getConfig(STOP_PROCESS_TIMEOUT), false);
+
+ // shutdown the machine if stopping process fails or takes too long
+ synchronized (stoppingMachine) {
+ // task also used as mutex by DST when it submits it; ensure it only submits once!
+ if (!stoppingMachine.isSubmitted()) {
+ // force the stoppingMachine task to run by submitting it here
+ log.warn("Submitting machine stop early in background for "+entity()+" because process stop has "+
+ (stoppingProcess.isDone() ? "finished abnormally" : "not finished"));
+ Entities.submit(entity(), stoppingMachine);
+ }
}
}
try {
- if (stoppingMachine.get().value == 0) {
+ boolean checkStopProcesses = true;
+ // This maintains previous behaviour of silently squashing any errors on the stoppingProcess task if the
+ // stoppingMachine exits with a nonzero value
+ if (stoppingMachine != null) {
--- End diff --
I'd go for `boolean checkStopProcesses = (stoppingMachine == null || stoppingMachine.get().value == 0);`
But no strong feelings.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#discussion_r21528108
--- Diff: software/base/src/main/java/brooklyn/entity/software/MachineLifecycleEffectorTasks.java ---
@@ -521,9 +532,14 @@ protected void restartChildren(ConfigBag parameters) {
* invokes {@link #preStopCustom()}, {@link #stopProcessesAtMachine()}, then finally
* {@link #stopAnyProvisionedMachines()} and sets state {@link Lifecycle#STOPPED}
*/
- public void stop() {
+ public void stop(ConfigBag parameters) {
log.info("Stopping {} in {}", entity(), entity().getLocations());
+ StopSoftwareParameters.StopMachineMode isStopMachine = parameters.get(StopSoftwareParameters.STOP_MACHINE_TYPED);
+
+ if (isStopMachine==null)
+ isStopMachine=StopSoftwareParameters.StopMachineMode.TRUE;
--- End diff --
General note: I'm a fan of white space around operators (as advocated in the various coding standards such as from Oracle and Google). But consistency with the other code trumps that. Therefore I'd for for ` = ` for he assignment, but ok to not have spaces around the `==` on previous line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#discussion_r21527891
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcess.java ---
@@ -253,6 +253,21 @@ private ChildStartableMode(boolean isDisabled, boolean isBackground, boolean isL
public enum RestartMachineMode { TRUE, FALSE, AUTO }
}
+
+ public static class StopSoftwareParameters {
--- End diff --
Perhaps mark this as `@Beta`.
Could do the same for `RestartSoftwareParameters`, assuming that was added after 0.6.0 was released.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#issuecomment-66266175
@nakomis I've merged https://github.com/apache/incubator-brooklyn/pull/378 - do you want to rebase this, to remove @sjcorbett 's commit from this 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#issuecomment-66293691
PR comments addressed, and rebased into existing commit
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#issuecomment-66307488
Looks good; merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Parameterize stop effector
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/375#discussion_r21530134
--- Diff: software/base/src/test/java/brooklyn/entity/basic/SoftwareProcessEntityTest.java ---
@@ -207,7 +214,55 @@ public void testBasicSoftwareProcessRestarts() throws Exception {
ConfigBag.newInstance().configure(RestartSoftwareParameters.RESTART_MACHINE_TYPED, RestartMachineMode.TRUE)));
t2.asTask().get();
- Assert.assertFalse(d.isRunning());
+ assertFalse(d.isRunning());
+ }
+
+ @Test
+ public void testBasicSoftwareProcessStopsEverything() throws Exception {
+ MyService entity = app.createAndManageChild(EntitySpec.create(MyService.class));
+ entity.start(ImmutableList.of(loc));
+ SimulatedDriver d = (SimulatedDriver) entity.getDriver();
+ Location machine = Iterables.getOnlyElement(entity.getLocations());
+
+ d.events.clear();
+ entity.stop();
+ assertEquals(d.events, ImmutableList.of("stop"));
+ assertEquals(entity.getLocations().size(), 0);
+ assertTrue(loc.getAvailable().contains(machine));
+ }
+
+ @Test
+ public void testBasicSoftwareProcessStopEverythingExplicitly() throws Exception {
+ MyService entity = app.createAndManageChild(EntitySpec.create(MyService.class));
+ entity.start(ImmutableList.of(loc));
+ SimulatedDriver d = (SimulatedDriver) entity.getDriver();
+ Location machine = Iterables.getOnlyElement(entity.getLocations());
+ d.events.clear();
+
+ TaskAdaptable<Void> t1 = Entities.submit(entity, Effectors.invocation(entity, Startable.STOP,
--- End diff --
This is fine, but for future reference there is also a `Entities.invokeEffector(...)` that executes the task for you, and returns the submitted task.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---