Support suspending machines #763
Conversation
|
@neykov I think the existing stop effector behaviour is your work so would appreciate your wisdom on these additions. |
sjcorbett
force-pushed
the
feature/suspend-resume-location
branch
from
14b23cb to
1d1910f
Compare
|
incubator-brooklyn-pull-requests #1499 FAILURE |
|
incubator-brooklyn-pull-requests #1498 SUCCESS |
|
Seems we've got unpredictable tests again.. |
| .parameter(StopSoftwareParameters.STOP_MACHINE_MODE) | ||
| .impl(newSuspendEffectorTask()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
What are the semantics of stopMachineMode here? Would IF_NOT_STOPPED really mean IF_NOT_SUSPENDED? How does this interact with stop? Is a suspended machine stoppable via stop(IF_NOT_STOPPED)?
There was a problem hiding this comment.
Suspending a machine has the same semantics as stopping a machine. They both use MachineLifecycleEffectorTasks#doStop. The former calls MachineProvisioningLocation#release (which becomes JcloudsLocation#releaseNode), the latter MachineManagementMixins.SuspendsMachines#suspendMachine (which becomes JcloudsLocation#suspendMachine). A suspended machine cannot subsequently be stopped if IF_NOT_STOPPED is true because MachineLifecycleEffectorTasks#doStop sets the entity's state to STOPPED.
There was a problem hiding this comment.
.. except of course that when suspending a machine JcloudsLocation does not call the pre- and post-release methods of any attached JcloudsLocationCustomizers. I think this is fair given that the machine may be resumed.
|
Finished review, a couple of comments worth addressing. ResumeLocation will tie nicely with your proposal for binding JcloudsLocation to existing machines. |
sjcorbett
force-pushed
the
feature/suspend-resume-location
branch
from
1d1910f to
bfdb092
Compare
|
incubator-brooklyn-pull-requests #1534 SUCCESS |
|
I think I've addressed the comments. I took the opportunity to name all the anonymous classes in |
And uses them in JcloudsLocation.
Leaves it to subclasses to attach the effector to entities.
sjcorbett
force-pushed
the
feature/suspend-resume-location
branch
from
bfdb092 to
fcb1af0
Compare
|
incubator-brooklyn-pull-requests #1542 SUCCESS |
|
We have no way to trigger the |
|
Thanks for the comments. I will merge this. |
| @Override | ||
| public Void call(ConfigBag parameters) { | ||
| Collection<? extends Location> locations = null; | ||
| return new StartEffectorBody(); |
There was a problem hiding this comment.
This will break rebinding as the anonymous classes will be needed when reconstructing the entities. The approach used to keep backwards compatibility in similar cases is to keep the methods creating the anonymous classes but not use them, i.e. rename to newStartEffectorTaskDeprecated and change its visibility to private. Only needed for the newXXXEffectorTask methods. Once #738 is merged one could instead add code to do the rename on the fly.
Apologies for writing this late, was just able to review this.
There was a problem hiding this comment.
I think both re-introducing the effector anonymous classes (in inaccessible methods) or reverting is a valid way to go.
The effect should be quite limited (I'd say non-existent) because users have to explicitly re-configure the key with a LifecycleEffectorTasks instance. Using it as a default value doesn't get it serialized.
There was a problem hiding this comment.
@sjcorbett @neykov This is a bit scary. I just hit it in my testing for a customer release. But that's because I tested a release candidate a day ago and then again today with this change.
Looking at what is persisted (with non-static classes etc), it feels scary for trying to support (or transform) that in future releases.
I'm going to change SoftwareProcess. LIFECYCLE_EFFECTOR_TASKS so that if the config is null it defaults to as before; if it's non-null then it will use the config value. That means we can take our time over whether we want to have a SoftwareProcessDriverLifecycleEffectorTasks instances persisted by pretty much every customer.
There was a problem hiding this comment.
Actually, I see that this is kind-of separate from LIFECYCLE_EFFECTOR_TASKS.
@sjcorbett Why will it not affect previous users, where their code did LIFECYCLE_TASKS.attachLifecycleEffectors(this) so will have populated the effector body?
Locations can indicate that they can suspend machines by implementing a new
SuspendsMachinesinterface.A use of this is given in
MachineLifecycleEffectorTasks, which provides asuspendeffector implementation. Attaching this effector to entities is left to subclasses of MLET.An interface for locations that can resume machines is also given but unused. I began an implementation but realised that we need to discuss what it means to "resume" a machine. Eventually
JcloudsLocationshould implementSuspendResumeLocationrather thanSuspendsMachines.