You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Trevor Flanagan <no...@github.com> on 2017/12/14 17:19:55 UTC
[jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
@nacx as requested I have done this piece of refactoring that was requested in https://github.com/jclouds/jclouds-labs/pull/426 and https://github.com/jclouds/jclouds-labs/pull/400#discussion_r127640050.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/427
-- Commit Summary --
* Refactor DimensionDataCloudControlResponseUtils waitFor* methods so that they are injectable.
-- File Changes --
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/DimensionDataCloudControlApiMetadata.java (4)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/CleanupServer.java (23)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/config/DimensionDataCloudControlComputeServiceContextModule.java (254)
D dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/predicates/NetworkDomainState.java (50)
D dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/predicates/ServerState.java (58)
D dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/predicates/ServerStatus.java (58)
D dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/predicates/VMToolsRunningStatus.java (51)
D dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/predicates/VlanState.java (52)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/utils/DimensionDataCloudControlResponseUtils.java (49)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/CleanupServerTest.java (27)
D dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/config/DimensionDataCloudControlComputeServiceContextModule.java (45)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/features/NetworkApiLiveTest.java (16)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/features/ServerApiLiveTest.java (26)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/internal/BaseDimensionDataCloudControlApiLiveTest.java (53)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/427.patch
https://github.com/jclouds/jclouds-labs/pull/427.diff
--
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
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
Just one minor left. Thanks!
> }
@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,
+ return retry(new ServerStatus(api.getServerApi(), false, true), timeouts.nodeTerminated,
nodeSuspended?
--
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-84067119
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Ignasi Barrera <no...@github.com>.
Closed #427.
--
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#event-1392092950
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed tp [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/889b34b9) and [2.0.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/bf34887e). Thanks @trevorflanagan!
--
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#issuecomment-352425739
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Ignasi Barrera <no...@github.com>.
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
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Trevor Flanagan <no...@github.com>.
@trevorflanagan pushed 1 commit.
349211f Refactor DimensionDataCloudControlResponseUtils waitFor* methods so that they are injectable. PR Updates.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/427/files/aef83ec9c7da8ab535ebea93aa67b3d57423aab8..349211f91a115149ea2e1c2ecd3339f3a977d278
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> + 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);
+ }
+ }
Makes sense, for now we should not require more of these Predicates. But I might take this on during my next set of refactoring.
--
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#discussion_r157273679
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Trevor Flanagan <no...@github.com>.
@trevorflanagan pushed 1 commit.
aef83ec Refactor DimensionDataCloudControlResponseUtils waitFor* methods so that they are injectable. PR Updates.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/427/files/b88b6c7de2757742ee3dc602ad12981f77d05fc7..aef83ec9c7da8ab535ebea93aa67b3d57423aab8
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> }
@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,
+ return retry(new ServerStatus(api.getServerApi(), false, true), timeouts.nodeTerminated,
@nacx good spot, thanks.
--
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#discussion_r157431336
Re: [jclouds/jclouds-labs] Refactor
DimensionDataCloudControlResponseUtils waitFor* methods so they are injectable
(#427)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> +import org.jclouds.dimensiondata.cloudcontrol.domain.NetworkDomain;
+import org.jclouds.dimensiondata.cloudcontrol.domain.Server;
+import org.jclouds.dimensiondata.cloudcontrol.domain.State;
+import org.jclouds.dimensiondata.cloudcontrol.domain.Vlan;
+import org.jclouds.dimensiondata.cloudcontrol.domain.VmTools;
+import org.jclouds.dimensiondata.cloudcontrol.features.NetworkApi;
+import org.jclouds.dimensiondata.cloudcontrol.features.ServerApi;
+import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+import java.text.MessageFormat;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static org.jclouds.util.Predicates2.retry;
+
+public class DimensionDataCloudControlComputeServiceContextModule extends AbstractModule {
This was in the test package for some reason. I have moved it across to production code. I have also simplified the Module so that it extends `com.google.inject.AbstractModule`. Originally it extended `ComputeServiceAdapterContextModule<ServerWithExternalIp, BaseImage, BaseImage, Datacenter>` but this causes problems in our live tests since not all of our functions are complete yet.
--
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-83581092