nacx requested changes on this pull request.
> @@ -66,6 +69,7 @@ public static Properties defaultProperties() {
properties.put(POLL_MAX_PERIOD, 2L * 10L);
properties.put(PROPERTY_SO_TIMEOUT, 60000 * 5);
properties.put(PROPERTY_CONNECTION_TIMEOUT, 60000 * 5);
+ properties.put(PROPERTY_MAX_RATE_LIMIT_WAIT, 20);
Note that the value is in milliseconds, so under a rate limited environment
this is likely to have no effect.
> logger.trace(">> provisioning complete for server. returned
> id='%s'", server.id());
} catch (Exception ex) {
logger.error(ex, ">> failed to provision server. rollbacking..");
throw Throwables.propagate(ex);
}
- return new NodeAndInitialCredentials<Server>(server, server.id(), null);
+ LoginCredentials serverCredentials = LoginCredentials.builder()
+ .user(loginUser)
+ .password(password)
+ .privateKey(privateKey)
+ .build();
Remove this and don't pass the initial credentials (now it is unused). We can
discuss the details of the node access once everything else is in place.
> }
@Override
- public List<HardwareFlavour> listHardwareProfiles() {
- return api.serverApi().listHardwareFlavours();
+ public List<Hardware> listHardwareProfiles() {
Why it is now a hardcoded list instead of calling the API?
> - logger.trace(">> found image [%s].", image.name());
- return image;
+ GenericQueryOptions options = new GenericQueryOptions();
+ options.options(0, 0, null, id, null);
+ try {
+ List<ServerAppliance> list = api.serverApplianceApi().list(options);
+ if (list.size() > 0) {
+ ServerAppliance appliance = list.get(0);
+ List<SingleServerAppliance.AvailableDataCenters>
availableDatacenters = new
ArrayList<SingleServerAppliance.AvailableDataCenters>();
+ for (String dcId : appliance.availableDataCenters()) {
+
availableDatacenters.add(SingleServerAppliance.AvailableDataCenters.create(dcId,
""));
+ }
+ SingleServerAppliance image =
SingleServerAppliance.create(appliance.id(), appliance.name(),
availableDatacenters, appliance.osInstallationBase(),
+ appliance.osFamily(), appliance.os(),
appliance.osVersion(), appliance.osArchitecture(), appliance.osImageType(),
appliance.minHddSize(),
+ appliance.type(), appliance.state(), appliance.version(),
appliance.categories(), appliance.eulaUrl());
+ if (image != null) {
This check is redundant.
> logger.debug("Destroying %s ...", id);
+ try {
+ GenericQueryOptions options = new GenericQueryOptions().options(0, 0,
null, node.name() + " firewall policy", null);
+ List<FirewallPolicy> firewallRules =
api.firewallPolicyApi().list(options);
+ for (FirewallPolicy firewallRule : firewallRules) {
+ api.firewallPolicyApi().delete(firewallRule.id());
+ }
+ } catch (Exception ex) {
+ logger.debug("no firewall policies found for %s ...", id);
+ }
The logic to delete the firewalls should be better moved to a
`OneAndOneComputeService` class that implements the
`cleanUpIncidentalResourcesOfDeadNodes` method. Thake the [azurecompute-arm
implementation](https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/AzureComputeService.java#L103)
as an example.
> + return OsFamily.UBUNTU;
+ }
+
+ if (osFamily.toLowerCase().contains("debian")) {
+ return OsFamily.DEBIAN;
+ }
+
+ if (osFamily.toLowerCase().contains("centos")) {
+ return OsFamily.CENTOS;
+ }
+
+ if (osFamily.toLowerCase().contains("ArchLinux")) {
+ return OsFamily.ARCH;
+ }
+ }
+ return OsFamily.UNRECOGNIZED;
Better do something like https://github.com/jclouds/jclouds-labs/pull/353 to
consider all known OsFamilies.
> + @Override
+ public Map<?, ListenableFuture<Void>> execute(String group, int count,
final Template template,
+ Set<NodeMetadata> goodNodes, Map<NodeMetadata, Exception> badNodes,
+ Multimap<NodeMetadata, CustomizationResponse>
customizationResponses) {
+
+ logger.info(">> looking for a datacenter in %s",
template.getLocation().getId());
+
+ // Try to find an existing datacenter in the selected location
+ DataCenter dataCenter = find(api.dataCenterApi().list(), new
Predicate<DataCenter>() {
+ @Override
+ public boolean apply(DataCenter input) {
+ // The location field is not populated when getting the list of
datacenters
+ DataCenter details = api.dataCenterApi().get(input.id());
+ return details != null &&
template.getLocation().getId().equals((details.countryCode().toLowerCase()));
+ }
+ }, null);
Don't use `null` as a default value and let it fail, since it is mandatory.
> +import com.google.inject.Inject;
+import javax.annotation.Resource;
+import javax.inject.Named;
+import static org.jclouds.Constants.PROPERTY_MAX_RATE_LIMIT_WAIT;
+import static org.jclouds.Constants.PROPERTY_MAX_RETRIES;
+import org.jclouds.http.HttpCommand;
+import org.jclouds.http.HttpResponse;
+import org.jclouds.http.HttpRetryHandler;
+import org.jclouds.logging.Logger;
+
+/**
+ * Retry handler that takes into account the provider rate limit and delays the
+ * requests until they are known to succeed.
+ */
+@Beta
+public abstract class AliRateLimitRetryHandler implements HttpRetryHandler {
Remove this 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/338#pullrequestreview-18837311
