nacx commented on this pull request.
Some small comments about the predicates and timeout configuration. Overall
looks good. hanks @trevorflanagan!
> + public static final String SERVER_STARTED_PREDICATE =
> "SERVER_STARTED_PREDICATE";
+ public static final String SERVER_STOPPED_PREDICATE =
"SERVER_STOPPED_PREDICATE";
+ public static final String SERVER_DELETED_PREDICATE =
"SERVER_DELETED_PREDICATE";
+ public static final String SERVER_NORMAL_PREDICATE =
"SERVER_NORMAL_PREDICATE";
+ public static final String VM_TOOLS_RUNNING_PREDICATE =
"VM_TOOLS_RUNNING_PREDICATE";
+
+ @Override
+ protected void configure() {
+
+ }
+
+ @Provides
+ @Named(VLAN_DELETED_PREDICATE)
+ protected Predicate<String> provideVlanDeletedPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new VlanState(api.getNetworkApi(), State.DELETED),
timeouts.imageAvailable,
The `imageAvailable` timeout is meant to be used by the `ImageExtension` when
waiting for a new image to be created. Instead of using it, just create a
property such as `operation-timeout` that can be configured as a generic
timeout for DimensionDataOperations, and get it injected here and in other
methods that don't have a matching timeout property in the
`ComputeServiceConstants.Timeouts` class.
> + pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
+ }
+
+ @Provides
+ @Named(NETWORK_DOMAIN_NORMAL_PREDICATE)
+ protected Predicate<String> provideNetworkDomainNormalPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new NetworkDomainState(api.getNetworkApi(), State.NORMAL),
timeouts.imageAvailable,
+ pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
+ }
+
+ @Provides
+ @Named(SERVER_STARTED_PREDICATE)
+ protected Predicate<String> provideServerStartedPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new ServerStatus(api.getServerApi(), true, true),
timeouts.imageAvailable,
Use `timeouts.nodeRunning`.
> + }
+
+ @Provides
+ @Named(SERVER_STARTED_PREDICATE)
+ protected Predicate<String> provideServerStartedPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new ServerStatus(api.getServerApi(), true, true),
timeouts.imageAvailable,
+ pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
+ }
+
+ @Provides
+ @Named(SERVER_STOPPED_PREDICATE)
+ @VisibleForTesting
+ public Predicate<String> provideServerStoppedPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new ServerStatus(api.getServerApi(), false, true),
timeouts.imageAvailable,
`timeouts.nodeSuspended`
> +
+ @Provides
+ @Named(SERVER_STOPPED_PREDICATE)
+ @VisibleForTesting
+ public Predicate<String> provideServerStoppedPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new ServerStatus(api.getServerApi(), false, true),
timeouts.imageAvailable,
+ pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
+ }
+
+ @Provides
+ @Named(SERVER_DELETED_PREDICATE)
+ @VisibleForTesting
+ public Predicate<String> provideServerDeletedPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new ServerState(api.getServerApi(), State.DELETED),
timeouts.imageAvailable,
`timeouts.nodeTerminated`
> +
+ @Provides
+ @Named(SERVER_NORMAL_PREDICATE)
+ protected Predicate<String> provideServerNormalPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new ServerState(api.getServerApi(), State.NORMAL),
timeouts.imageAvailable,
+ pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
+ }
+
+ @Provides
+ @Named(VM_TOOLS_RUNNING_PREDICATE)
+ protected Predicate<String> provideVMToolsRunningPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new VMToolsRunningStatus(api.getServerApi()),
timeouts.imageAvailable, pollPeriod.pollInitialPeriod,
+ pollPeriod.pollMaxPeriod);
+ }
In general, use the existing timeouts if they make sense, or use a
Dimensiondata specific timeout. The timeouts configured in the
`ComputeServiceConstants.Timeouts` class are automatically used by jclouds when
waiting for a node being created. For example, perhaps there is no need to use
the "nodeRunning" timeout for a server creation, as that is the timeout jclouds
uses to wait for the entire process since the user calls "createNode" until it
finishes creating it, which involves several additional steps.
Configure timeout values at your own criteria and provide the corresponding
properties, with reasonable defaults, to let users configure them as needed.
> + protected Predicate<String> provideServerNormalPredicate(final
> DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new ServerState(api.getServerApi(), State.NORMAL),
timeouts.imageAvailable,
+ pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
+ }
+
+ @Provides
+ @Named(VM_TOOLS_RUNNING_PREDICATE)
+ protected Predicate<String> provideVMToolsRunningPredicate(final
DimensionDataCloudControlApi api,
+ final ComputeServiceConstants.Timeouts timeouts, final
ComputeServiceConstants.PollPeriod pollPeriod) {
+ return retry(new VMToolsRunningStatus(api.getServerApi()),
timeouts.imageAvailable, pollPeriod.pollInitialPeriod,
+ pollPeriod.pollMaxPeriod);
+ }
+
+ private class VlanState implements Predicate<String>
+
[style minor] Remove this line.
> + private ServerStatus(final ServerApi api, final boolean started, final
> boolean deployed) {
+ this.api = api;
+ this.started = started;
+ this.deployed = deployed;
+ }
+
+ @Override
+ public boolean apply(final String serverId) {
+ checkNotNull(serverId, "serverId");
+ logger.trace("looking for state on Server %s", serverId);
+ final Server server = api.getServer(serverId);
+
+ // perhaps request isn't available, yet
+ if (server == null)
+ return false;
+ logger.trace("%s: looking for server %s deployed: currently: %s",
server, server.state());
Missing one parameter for the log message.
> + private final NetworkApi networkApi;
+
+ private NetworkDomainState(final NetworkApi networkApi, final State
state) {
+ this.networkApi = networkApi;
+ this.state = state;
+ }
+
+ @Override
+ public boolean apply(final String networkDomainId) {
+ checkNotNull(networkDomainId, "networkDomainId");
+ logger.trace("looking for state on network domain %s",
networkDomainId);
+ final NetworkDomain networkDomain =
networkApi.getNetworkDomain(networkDomainId);
+ final boolean isDeleted = networkDomain == null && state ==
State.DELETED;
+ return isDeleted || (networkDomain != null && networkDomain.state()
== state);
+ }
+ }
If this predicate pattern is meant to be used in more classes, consider
extracting it to a base class.You could create an interface `WithStatus` (or
whatever) that provides the `state()` method, and that would allow you to write
a generic predicate that checks the status with just an abstract method to get
the `WithStatus` object to be checked. Just an idea to avoid duplicating code
if the pattern is going to be the same for all objects with state.
> + this.started = started;
+ this.deployed = deployed;
+ }
+
+ @Override
+ public boolean apply(final String serverId) {
+ checkNotNull(serverId, "serverId");
+ logger.trace("looking for state on Server %s", serverId);
+ final Server server = api.getServer(serverId);
+
+ // perhaps request isn't available, yet
+ if (server == null)
+ return false;
+ logger.trace("%s: looking for server %s deployed: currently: %s",
server, server.state());
+ if (server.state().isFailed()) {
+ throw new IllegalStateException(MessageFormat.format("Server {0}
is in FAILED state", server.id()));
Better `String.format`?
> + }
+ }
+ }
+
+ private class VMToolsRunningStatus implements Predicate<String> {
+
+ private final ServerApi api;
+
+ private VMToolsRunningStatus(final ServerApi api) {
+ this.api = api;
+ }
+
+ @Override
+ public boolean apply(final String serverId) {
+ checkNotNull(serverId, "serverId");
+ logger.trace("looking for state on Server %s", serverId);
"Looking for guest tools state"?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/427#pullrequestreview-83731458
