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] Feature/packet 5 (#354)
Date Mon, 30 Jan 2017 09:41:04 GMT
nacx requested changes on this pull request.

Looks pretty good! Mostly comments about server authentication.

> + * defines the connection between the {@link org.jclouds.packet.PacketApi} implementation
and
+ * the jclouds {@link org.jclouds.compute.ComputeService}
+ */
+@Singleton
+public class PacketComputeServiceAdapter implements ComputeServiceAdapter<Device, Plan,
OperatingSystem, Facility> {
+
+   @Resource
+   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+   protected Logger logger = Logger.NULL;
+
+   private final PacketApi api;
+   private final Predicate<String> nodeRunningPredicate;
+   private final String projectId;
+
+   @Inject
+   public PacketComputeServiceAdapter(PacketApi api, @Provider final Supplier<Credentials>
creds, @Named(TIMEOUT_NODE_RUNNING) Predicate<String> nodeRunningPredicate) {

Remove the public modifier and the redundant null checks.

> +   private final PacketApi api;
+   private final Predicate<String> nodeRunningPredicate;
+   private final String projectId;
+
+   @Inject
+   public PacketComputeServiceAdapter(PacketApi api, @Provider final Supplier<Credentials>
creds, @Named(TIMEOUT_NODE_RUNNING) Predicate<String> nodeRunningPredicate) {
+      this.api = checkNotNull(api, "api");
+      this.projectId = creds.get().identity;
+      this.nodeRunningPredicate = nodeRunningPredicate;
+   }
+
+   @Override
+   public NodeAndInitialCredentials<Device> createNodeWithGroupEncodedIntoName(String
group, String name, Template template) {
+
+      PacketTemplateOptions templateOptions = template.getOptions().as(PacketTemplateOptions.class);
+      checkNotNull(templateOptions.getLoginPrivateKey(), "login privateKey must not be null");

Can't packet use a password based login?

> +      String operatingSystem = template.getImage().getId();
+
+      Device device = api.deviceApi(projectId).create(
+              Device.CreateDevice.builder()
+                      .hostname(name)
+                      .plan(plan)
+                      .billingCycle(billingCycle.value())
+                      .facility(facility)
+                      .features(features)
+                      .operatingSystem(operatingSystem)
+                      .locked(locked)
+                      .userdata(userdata)
+                      .tags(tags)
+                      .build()
+              );
+      nodeRunningPredicate.apply(device.id());

Do we need to actually wait to have a complete Device object? If not, remove this since the
base class already performs the wait; we should not actively wait here (otherwise we couldn't
honor the options to not block, etc).

> +                      .features(features)
+                      .operatingSystem(operatingSystem)
+                      .locked(locked)
+                      .userdata(userdata)
+                      .tags(tags)
+                      .build()
+              );
+      nodeRunningPredicate.apply(device.id());
+      device = api.deviceApi(projectId).get(device.id());
+
+      LoginCredentials defaultCredentials = LoginCredentials.builder()
+              .user("root")
+              .privateKey(templateOptions.getLoginPrivateKey())
+              .build();
+
+      return new NodeAndInitialCredentials<Device>(device, device.id(), defaultCredentials);

You'd better pass `null` here to let jclouds determine the credentials based on the options
provided by the user and the default credentials configured per context or per image.

> +            PacketApi api, SshKeyPairGenerator keyGenerator) {
+        super(addNodeWithGroupStrategy, listNodesStrategy, namingConvention, userExecutor,
+                customizeNodeAndAddToGoodMapOrPutExceptionIntoBadMapFactory);
+        this.api = api;
+        this.keyGenerator = keyGenerator;
+    }
+
+    @Override
+    public Map<?, ListenableFuture<Void>> execute(String group, int count, Template
template,
+                                                  Set<NodeMetadata> goodNodes, Map<NodeMetadata,
Exception> badNodes,
+                                                  Multimap<NodeMetadata, CustomizationResponse>
customizationResponses) {
+
+        PacketTemplateOptions options = template.getOptions().as(PacketTemplateOptions.class);
+        Set<String> generatedSshKeyIds = Sets.newHashSet();
+
+        // If no key has been configured and the auto-create option is set, then generate
a key pair

There seems not to be such an auto-create option, so update the comment.

> +   private final PacketApi api;
+   private final Predicate<String> nodeRunningPredicate;
+   private final String projectId;
+
+   @Inject
+   public PacketComputeServiceAdapter(PacketApi api, @Provider final Supplier<Credentials>
creds, @Named(TIMEOUT_NODE_RUNNING) Predicate<String> nodeRunningPredicate) {
+      this.api = checkNotNull(api, "api");
+      this.projectId = creds.get().identity;
+      this.nodeRunningPredicate = nodeRunningPredicate;
+   }
+
+   @Override
+   public NodeAndInitialCredentials<Device> createNodeWithGroupEncodedIntoName(String
group, String name, Template template) {
+
+      PacketTemplateOptions templateOptions = template.getOptions().as(PacketTemplateOptions.class);
+      checkNotNull(templateOptions.getLoginPrivateKey(), "login privateKey must not be null");

Also, we should be reading the "public" key and configuring the device with it. The private
key should remain in the client.

> +@Test(groups = "live", singleThreaded = true, testName = "PacketComputeServiceLiveTest")
+public class PacketComputeServiceLiveTest extends BaseComputeServiceLiveTest {
+
+   public PacketComputeServiceLiveTest() {
+      provider = "packet";
+   }
+
+   @Override
+   protected Module getSshModule() {
+      return new SshjSshClientModule();
+   }
+
+   @Override
+   protected Template buildTemplate(TemplateBuilder templateBuilder) {
+      return super.buildTemplate(templateBuilder);
+   }

No need to override this.

> +      }
+   }
+
+   @Override
+   public void testOptionToNotBlock() throws Exception {
+      // Packet ComputeService implementation has to block until the node
+      // is provisioned, to be able to return it.
+   }
+
+   @Override
+   protected void checkUserMetadataContains(NodeMetadata node, ImmutableMap<String, String>
userMetadata) {
+      // The Packet API does not return the user data
+   }
+
+   @Override
+   public void testAScriptExecutionAfterBootWithBasicTemplate() throws Exception {

Why do we need to override this?

-- 
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/354#pullrequestreview-19039151
Mime
View raw message