jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] Oneandone compute api (#338)
Date Tue, 10 Jan 2017 11:22:30 GMT
nacx requested changes on this pull request.

Thanks @alibazlamit!
I've added some comments. Let's address them first, and then we'll fix the live tests once
we are happy with the implementation.

> @@ -32,6 +32,7 @@
     <packaging>bundle</packaging>
     
     <properties>
+        <checkstyle.skip>true</checkstyle.skip>

Remove this

> @@ -49,10 +50,21 @@ public OneAndOneProviderMetadata(Builder builder) {
 
    public static Properties defaultProperties() {
       Properties properties = OneAndOneApiMetadata.defaultProperties();
+
+      properties.setProperty(PROPERTY_REGIONS, "de,us,es,gb");
+//      properties.setProperty(PROPERTY_ISO3166_CODES, "DE,US,ES,GB");

This one is not needed if you set the codes in the provider metadata, so it could be removed.

> @@ -49,10 +50,21 @@ public OneAndOneProviderMetadata(Builder builder) {
 
    public static Properties defaultProperties() {
       Properties properties = OneAndOneApiMetadata.defaultProperties();
+
+      properties.setProperty(PROPERTY_REGIONS, "de,us,es,gb");
+//      properties.setProperty(PROPERTY_ISO3166_CODES, "DE,US,ES,GB");
+//      properties.setProperty(PROPERTY_REGION + ".de." + ISO3166_CODES, "DE-DEU");
+//      properties.setProperty(PROPERTY_REGION + ".us." + ISO3166_CODES, "US-USA");
+//      properties.setProperty(PROPERTY_REGION + ".es." + ISO3166_CODES, "ES-ESP");
+//      properties.setProperty(PROPERTY_REGION + ".gb." + ISO3166_CODES, "GB-GBR");

These ones qualify the iso code for each region, so they should be added back.

> +import org.jclouds.domain.LoginCredentials;
+import org.jclouds.logging.Logger;
+import org.jclouds.rest.ResourceNotFoundException;
+
+@Singleton
+public class OneandoneComputeServiceAdapter implements ComputeServiceAdapter<Server, HardwareFlavour,
Image, Location> {
+
+   @Resource
+   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+   protected Logger logger = Logger.NULL;
+
+   private final OneAndOneApi api;
+   private final Predicate<Server> waitServerUntilAvailable;
+   private final List<DataCenter> datacetners;
+
+//   private static final Integer DEFAULT_LAN_ID = 1;

Remove this

> +import org.jclouds.domain.Location;
+import org.jclouds.domain.LocationScope;
+import org.jclouds.domain.LoginCredentials;
+import org.jclouds.logging.Logger;
+import org.jclouds.rest.ResourceNotFoundException;
+
+@Singleton
+public class OneandoneComputeServiceAdapter implements ComputeServiceAdapter<Server, HardwareFlavour,
Image, Location> {
+
+   @Resource
+   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+   protected Logger logger = Logger.NULL;
+
+   private final OneAndOneApi api;
+   private final Predicate<Server> waitServerUntilAvailable;
+   private final List<DataCenter> datacetners;

Delete this unused variable

> +   private final OneAndOneApi api;
+   private final Predicate<Server> waitServerUntilAvailable;
+   private final List<DataCenter> datacetners;
+
+//   private static final Integer DEFAULT_LAN_ID = 1;
+   @Inject
+   OneandoneComputeServiceAdapter(OneAndOneApi api,
+           @Named(POLL_PREDICATE_SERVER) Predicate<Server> waitServerUntilAvailable)
{
+      this.api = api;
+      this.datacetners = ImmutableList.of();
+      this.waitServerUntilAvailable = waitServerUntilAvailable;
+   }
+
+   @Override
+   public NodeAndInitialCredentials<Server> createNodeWithGroupEncodedIntoName(String
group, String name, Template template) {
+      checkArgument(template.getLocation().getScope() == LocationScope.REGION, "Template
must use a REGION-scoped location");

Only available locations will get here, so this check is redundant. Let's better remove it.

> +                 .hardware(hardwareRequest)
+                 .applianceId(image.getId())
+                 .dataCenterId(dataCenterId)
+                 .password(password)
+                 .powerOn(Boolean.TRUE).build();
+
+         logger.trace("<< provisioning server '%s'", serverRequest);
+
+         server = api.serverApi().create(serverRequest);
+         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);
+      }
+      waitServerUntilAvailable.apply(server);

There is no need to wait here. Jclouds will already wait for the server to be running, so
let's better remove this.

> +      }
+
+      // provision server
+      final Server server;
+      Double cores = ComputeServiceUtils.getCores(hardware);
+
+      try {
+         List<? extends Processor> processors = hardware.getProcessors();
+         org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware hardwareRequest
+                 = org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware.create(processors.size(),
cores, hardware.getRam(), hdds);
+         final Server.CreateServer serverRequest = Server.CreateServer.builder()
+                 .name(name)
+                 .hardware(hardwareRequest)
+                 .applianceId(image.getId())
+                 .dataCenterId(dataCenterId)
+                 .password(password)

Is it mandatory to force a password login? What about ssh key access?

> +   }
+
+   @Override
+   public Iterable<Location> listLocations() {
+      // Will never be called
+      throw new UnsupportedOperationException("Locations are configured in jclouds properties");
+   }
+
+   @Override
+   public Server getNode(String id) {
+      return api.serverApi().get(id);
+   }
+
+   @Override
+   public void destroyNode(String id) {
+      waitServerUntilAvailable.apply(getNode(id));

Is this really needed here and in the next state-change methods?

> +import org.jclouds.compute.domain.internal.TemplateBuilderImpl;
+import org.jclouds.domain.Location;
+import static org.jclouds.util.Predicates2.retry;
+
+public class OneAndOneComputeServiceContextModule extends
+        ComputeServiceAdapterContextModule<Server, HardwareFlavour, org.apache.jclouds.oneandone.rest.domain.Image,
Location> {
+
+   @SuppressWarnings("unchecked")
+   @Override
+   protected void configure() {
+      super.configure();
+
+      bind(new TypeLiteral<ComputeServiceAdapter<Server, HardwareFlavour, org.apache.jclouds.oneandone.rest.domain.Image,
Location>>() {
+      }).to(OneandoneComputeServiceAdapter.class);
+
+      bind(TemplateBuilderImpl.class).to(ArbitraryCpuRamTemplateBuilderImpl.class);

Can users set custom CPU/RAM values even if there are no such predefined hardware flavours
in OneAndOne? If not, remove this binding.

> +   public void destroyNode(String id) {
+      waitServerUntilAvailable.apply(getNode(id));
+      logger.debug("Destroying %s ...", id);
+      api.serverApi().delete(id);
+      logger.debug("Destroyed %s!", id);
+   }
+
+   @Override
+   public void rebootNode(String id) {
+      waitServerUntilAvailable.apply(getNode(id));
+      api.serverApi().updateStatus(id, Server.UpdateStatus.create(Types.ServerAction.REBOOT,
Types.ServerActionMethod.HARDWARE));
+   }
+
+   @Override
+   public void resumeNode(String id) {
+      waitServerUntilAvailable.apply(getNode(id));

This won't work? You wait until the node is powered on when you want to resume it. This will
take forever.

> +               return OperatingSystem.builder()
+                       .description(OsFamily.LINUX.value())
+                       .family(OsFamily.LINUX)
+                       .build();
+            default:
+               break;
+         }
+      }
+      return OperatingSystem.builder()
+              .description(OsFamily.UNRECOGNIZED.value())
+              .family(OsFamily.UNRECOGNIZED)
+              .build();
+   }
+
+   private boolean is64Bit(int architecture) {
+      switch (architecture) {

Just `return architecture != 32;`

> +import com.google.common.base.Function;
+import com.google.common.collect.ImmutableList;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.jclouds.oneandone.rest.domain.HardwareFlavour;
+import org.jclouds.compute.domain.Hardware;
+import org.jclouds.compute.domain.HardwareBuilder;
+import org.jclouds.compute.domain.Processor;
+import org.jclouds.compute.domain.Volume;
+import org.jclouds.compute.domain.VolumeBuilder;
+
+public class HardwareFlavourToHardware implements Function<HardwareFlavour, Hardware>
{
+
+   @Override
+   public Hardware apply(HardwareFlavour from) {
+      int MinRamSize = (int) from.hardware().ram();

Start variable name in lowercase

> +   @Override
+   public NodeMetadata apply(final Server server) {
+      checkNotNull(server, "Null server");
+
+      DataCenter dataCenter = api.dataCenterApi().get(server.datacenter().id());
+      Location location = find(locations.get(), idEquals(dataCenter.location()));
+
+      double size = 0d;
+      List<Volume> volumes = Lists.newArrayList();
+      List<Hdd> storages = server.hardware().hdds();
+      if (storages != null) {
+         for (Hdd storage : storages) {
+            size += storage.size();
+            volumes.add(fnVolume.apply(storage));
+         }
+      }

Always building a custom hardware object? This seems not right. You should take into account
prefedined flavours.

> +      }
+
+      // Build hardware
+      String id = String.format("cpu=%f,ram=%d,disk=%f", server.hardware().coresPerProcessor(),
(int) server.hardware().ram(), size);
+      Hardware hardware = new HardwareBuilder()
+              .ids(id)
+              .name(id)
+              .ram((int) server.hardware().ram())
+              .processor(new Processor(server.hardware().coresPerProcessor(), 1d))
+              .hypervisor("kvm")
+              .volumes(volumes)
+              .location(location)
+              .build();
+
+      // Collect ips
+      List<String> addresses = fnCollectIps.apply(server.ips());

No need for that dummy function in the constructor. Remove it and change this to something
like:

```java
List<String> addresses = Lists.transform(new Function<ServerIp, String>() {
   @Override public String apply(ServerIp in) {
      return in.ip();
   }
});
```

> +         case DEPLOYING:
+         case POWERING_OFF:
+         case POWERING_ON:
+         case REBOOTING:
+         case REMOVING:
+            return NodeMetadata.Status.PENDING;
+         case POWERED_OFF:
+            return NodeMetadata.Status.SUSPENDED;
+         case POWERED_ON:
+            return NodeMetadata.Status.RUNNING;
+         default:
+            return NodeMetadata.Status.UNRECOGNIZED;
+      }
+   }
+
+   static OperatingSystem mapOsType(OSFamliyType osType) {

This is a duplicate of the one in the Image transformation function. Reuse it.

>        return retry(new PrivateNetworkReadyPredicate(
               api),
               constants.pollTimeout(), constants.pollPeriod(), constants.pollMaxPeriod(),
TimeUnit.SECONDS);
    }
 
    @Provides
-   @Named(POLL_PREDICATE_SERVER)
-   Predicate<Server> provideServerReadyPredicate(final OneAndOneApi api, OneAndOneConstants
constants) {
+   Predicate<Server> provideServerReadyPredicate(final OneAndOneApi api, ComputeConstants
constants) {

Do we have now two different predicates to check if a server is ready?

> + * Unless required by applicable law or agreed to in writing, software
+ * 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.apache.jclouds.oneandone.rest.domain;
+
+public enum Location {
+
+   DE("de", "Germany"),
+   US("us", "USA"),
+   ES("es", "Spain"),
+   GB("gb", "UNITED KINGDOM"),
+   UNRECOGNIZED("unrecognized", "Unrecognized location"),
+   MOCK("mock", "Mock");

What are the unrecognized and mock locations for? Can we remove them?

> + *
+ * Unless required by applicable law or agreed to in writing, software
+ * 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.apache.jclouds.oneandone.rest.domain;
+
+import com.google.common.base.Enums;
+
+/**
+ * Marker interface for {@link org.jclouds.profitbricks.domain.Image} and
+ * {@link org.jclouds.profitbricks.domain.Snapshot}
+ */
+public interface Provisionable {

Remove this class.

> +   @Override
+   protected Module getSshModule() {
+      return new SshjSshClientModule();
+   }
+
+   @Override
+   protected LoggingModule getLoggingModule() {
+      return new SLF4JLoggingModule();
+   }
+
+   @Override
+   protected Iterable<Module> setupModules() {
+      ImmutableSet.Builder<Module> modules = ImmutableSet.builder();
+      modules.addAll(super.setupModules());
+      return modules.build();
+   }

Remove this method.

> +   @Override
+   protected LoggingModule getLoggingModule() {
+      return new SLF4JLoggingModule();
+   }
+
+   @Override
+   protected Iterable<Module> setupModules() {
+      ImmutableSet.Builder<Module> modules = ImmutableSet.builder();
+      modules.addAll(super.setupModules());
+      return modules.build();
+   }
+
+   @Override
+   public void testOptionToNotBlock() throws Exception {
+      // OneAndOne implementation intentionally blocks until the node is 'AVAILABLE'
+   }

There should be no need for that, so this method could be removed.

> +   }
+
+   @Override
+   protected void checkUserMetadataContains(NodeMetadata node, ImmutableMap<String, String>
userMetadata) {
+      // OneAndOne doesn't support user metadata
+   }
+
+   @Override
+   protected void checkResponseEqualsHostname(ExecResponse execResponse, NodeMetadata node1)
{
+      // OneAndOne doesn't support hostname
+   }
+
+   @Override
+   protected void checkOsMatchesTemplate(NodeMetadata node) {
+      // Not enough description from API to match template
+   }

Can you provide an example response for the getImage call? Isn't anything there we can use
to determine the OS?

> +
+   public OneAndOneTemplateBuilderLiveTest() {
+      this.provider = "oneandone";
+   }
+
+   @Override
+   protected Set<String> getIso3166Codes() {
+      return ImmutableSet.of("DE-DEU", "US-USA", "ES-ESP", "GB-GBR");
+   }
+
+   @Override
+   protected Iterable<Module> setupModules() {
+      ImmutableSet.Builder<Module> modules = ImmutableSet.builder();
+      modules.addAll(super.setupModules());
+      return modules.build();
+   }

Remove this method.

> +            // Destroy all nodes in the group but also make sure to destroy other created
nodes that might not be in it.
+            // The "testCreateTwoNodesWithOneSpecifiedName" creates nodes with an explicit
name that puts them outside the group,
+            // so the list of nodes should also be taken into account when destroying the
nodes.
+            client.destroyNodesMatching(Predicates.<NodeMetadata>or(inGroup(group),
(Predicate<NodeMetadata>) in(nodes)));
+         }
+      } catch (Exception e) {
+
+      }
+      super.tearDownContext();
+   }
+
+   @Override
+   protected Module getSshModule() {
+      throw new IllegalStateException("ssh is required for this test!");
+   }
+}

Remove this file.

> +import static com.google.common.collect.Iterables.getOnlyElement;
+import com.google.inject.Module;
+import static org.assertj.core.api.Assertions.assertThat;
+import org.jclouds.compute.domain.ExecResponse;
+import org.jclouds.compute.domain.NodeMetadata;
+import org.jclouds.compute.domain.Template;
+import org.jclouds.compute.internal.BaseComputeServiceLiveTest;
+import static org.jclouds.compute.predicates.NodePredicates.inGroup;
+import org.jclouds.logging.config.LoggingModule;
+import org.jclouds.logging.slf4j.config.SLF4JLoggingModule;
+import org.jclouds.sshj.config.SshjSshClientModule;
+import org.testng.annotations.Test;
+
+@Test(groups = "live", singleThreaded = true, testName = "OneAndOneComputeServiceLiveTest")
+public class OneAndOneComputeServiceLiveTest extends BaseComputeServiceLiveTest {
+//    public class OneAndOneComputeServiceLiveTest extends baseclass {

Remove this comment

> +
+   private DataCenterApi dataCenterApi() {
+
+      return api.dataCenterApi();
+   }
+
+   @Test
+   public void testList() {
+      dataCenters = dataCenterApi().list();
+      currentDataCenter = dataCenters.get(0);
+      assertNotNull(dataCenters);
+      assertFalse(dataCenters.isEmpty());
+      Assert.assertTrue(dataCenters.size() > 0);
+   }
+
+   @Test(dependsOnMethods = "testList")

No need for this dependency

-- 
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-15879083
Mime
View raw message