nacx commented on this pull request.
Comments added. Thanks @trevorflanagan!
> +import java.util.Set;
+
+import static com.google.common.base.MoreObjects.firstNonNull;
+import static java.lang.String.format;
+import static org.jclouds.util.Predicates2.retry;
+
+@Singleton
+public class GetOrCreateNetworkDomainThenCreateNodes extends
CreateNodesWithGroupEncodedIntoNameThenAddToSet {
+
+ private final DimensionDataCloudControlApi api;
+ private final ComputeServiceConstants.Timeouts timeouts;
+
+ public static final String DEFAULT_NETWORK_DOMAIN_NAME =
"JCLOUDS_NETWORK_DOMAIN";
+ public static final String DEFAULT_VLAN_NAME = "JCLOUDS_VLAN";
+ public static final String DEFAULT_PRIVATE_IPV4_BASE_ADDRESS = "10.0.0.0";
+ public static final Integer DEFAULT_PRIVATE_IPV4_PREFIX_SIZE = 24;
Better define these constants in the options class.
> + * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.compute.options;
+
+import org.jclouds.compute.options.TemplateOptions;
+import org.jclouds.javax.annotation.Nullable;
+
+public class DimensionDataCloudControlTemplateOptions extends TemplateOptions
implements Cloneable {
+
+ private String networkDomainName;
+ private String vlanName;
+ private String networkDomainId;
+ private String vlanId;
It looks like these IDs are not meant to be configured by users and only used
to carry information to the adapter implementation, so they should not be
exposed in the public API.
Instead of doing this, you could use an intermediate (and internal, not exposed
to users) options class to just carry those computed values. We do something
like this [in the
ProfitBricks](https://github.com/jclouds/jclouds/blob/master/providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/strategy/AssignDataCenterToTemplate.java#L106)
provider.
> + super(addNodeWithGroupStrategy, listNodesStrategy, namingConvention,
> userExecutor,
+ customizeNodeAndAddToGoodMapOrPutExceptionIntoBadMapFactory);
+ this.api = api;
+ this.timeouts = timeouts;
+ }
+
+ @Override
+ public Map<?, ListenableFuture<Void>> execute(final String group, final int
count, final Template template,
+ final Set<NodeMetadata> goodNodes, final Map<NodeMetadata, Exception>
badNodes,
+ final Multimap<NodeMetadata, CustomizationResponse>
customizationResponses) {
+
+ final DimensionDataCloudControlTemplateOptions templateOptions =
template.getOptions()
+ .as(DimensionDataCloudControlTemplateOptions.class);
+
+ String networkDomainName =
firstNonNull(templateOptions.getNetworkDomainName(),
DEFAULT_NETWORK_DOMAIN_NAME);
+ String vlanName = firstNonNull(templateOptions.getVlanName(),
DEFAULT_VLAN_NAME);
The jclouds portable template options have the `networks` attribute (names,
ids, or whatever makes sense for the provider) so users can configure to which
networks the nodes will be connected to. Instead of providing two different
options to configure that, we should try to accommodate the current behavior to
using that `networks` option in a way that is understandable and makes sense to
users.
> + }
+ return vlanId;
+ }
+
+ private String deployVlan(final String networkDomainId, final String
vlanName,
+ final DimensionDataCloudControlTemplateOptions templateOptions) {
+ logger.debug("Creating a vlan %s in network domain '%s' ...", vlanName,
networkDomainId);
+ String defaultPrivateIPv4BaseAddress =
firstNonNull(templateOptions.getDefaultPrivateIPv4BaseAddress(),
+ DEFAULT_PRIVATE_IPV4_BASE_ADDRESS);
+ Integer defaultPrivateIPv4PrefixSize =
firstNonNull(templateOptions.getDefaultPrivateIPv4PrefixSize(),
+ DEFAULT_PRIVATE_IPV4_PREFIX_SIZE);
+
+ String vlanId = api.getNetworkApi()
+ .deployVlan(networkDomainId, vlanName, "vlan created by jclouds",
defaultPrivateIPv4BaseAddress,
+ defaultPrivateIPv4PrefixSize);
+ boolean isVlanReady = retry(new VlanState(api.getNetworkApi(),
State.NORMAL), timeouts.nodeRunning).apply(vlanId);
This is a small PR and I think we should introduce the Guice-configured
predicates here and stop initializing them manually. Let's better properly
configure now the predicates now that we have the compute service module and
get them injected where needed. This can be done in a different PR that unifies
them and removes the static utils class.
--
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/426#pullrequestreview-82408950
