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] Vagrant provider (#160)
Date Mon, 12 Dec 2016 23:22:44 GMT
nacx requested changes on this pull request.

Thanks @neykov! I've reviewed the PR and added my comments.

> +Vagrant provider for jclouds
+============================
+
+Building
+--------
+
+1. Vagrant bindings
+  * `git clone https://github.com/neykov/vagrant-java-bindings`
+  * `cd vagrant-java-bindings`
+  * `mvn clean install`
+  * Copy `target/vagrant-java-bindings-0.0.1-SNAPSHOT.jar` to jcloud's classpath
+2. Vagrant provider
+  * `git clone https://github.com/neykov/jclouds-vagrant`
+  * `cd jclouds-vagrant/vagrant`
+  * `mvn clean install`
+  * Copy `target/vagrant-2.0.0-SNAPSHOT.jar` to jcloud's classpath

Update this. The bindings are in Maven Central and the project can be build just as any other
jclouds provider.

> +-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+  <parent>
+    <groupId>org.apache.jclouds.labs</groupId>
+    <artifactId>jclouds-labs</artifactId>
+    <version>2.1.0-SNAPSHOT</version>
+  </parent>
+
+  <artifactId>vagrant</artifactId>
+  <name>Vagrant provider</name>
+  <packaging>bundle</packaging>
+
+  <properties>
+    <jclouds.version>2.1.0-SNAPSHOT</jclouds.version>

Remove this and use `project.parent.version` consistently for all dependencies.

> + * 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.vagrant;
+
+import java.net.URI;
+import java.util.Properties;
+
+import org.jclouds.apis.ApiMetadata;
+import org.jclouds.apis.internal.BaseApiMetadata;
+import org.jclouds.compute.ComputeServiceContext;
+import org.jclouds.compute.config.ComputeServiceProperties;
+import org.jclouds.vagrant.config.VagrantComputeServiceContextModule;
+
+public class VagrantApiMetadata extends BaseApiMetadata {

Add the auto-service dependency:
```xml
<dependency>
  <groupId>com.google.auto.service</groupId>
  <artifactId>auto-service</artifactId>
  <scope>provided</scope>
</dependency>
```

And annotate this as `@AutoService(ApiMetadata.class)`.

> +import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
+import com.google.common.io.Files;
+
+import vagrant.Vagrant;
+import vagrant.api.VagrantApi;
+import vagrant.api.domain.Box;
+import vagrant.api.domain.Machine;
+import vagrant.api.domain.MachineState;
+import vagrant.api.domain.SshConfig;
+
+public class VagrantComputeServiceAdapter implements ComputeServiceAdapter<VagrantNode,
Hardware, Box, Location> {
+   private File nodeContainer;
+   // TODO FIXME - use just as cache, expire items
+   private static Map<String, VagrantNode> machines = new HashMap<String, VagrantNode>();

Use a proper Guava LoadingCache then?

> +import com.google.common.base.Throwables;
+import com.google.common.collect.FluentIterable;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
+import com.google.common.io.Files;
+
+import vagrant.Vagrant;
+import vagrant.api.VagrantApi;
+import vagrant.api.domain.Box;
+import vagrant.api.domain.Machine;
+import vagrant.api.domain.MachineState;
+import vagrant.api.domain.SshConfig;
+
+public class VagrantComputeServiceAdapter implements ComputeServiceAdapter<VagrantNode,
Hardware, Box, Location> {
+   private File nodeContainer;

Can this be final?

> +
+import vagrant.Vagrant;
+import vagrant.api.VagrantApi;
+import vagrant.api.domain.Box;
+import vagrant.api.domain.Machine;
+import vagrant.api.domain.MachineState;
+import vagrant.api.domain.SshConfig;
+
+public class VagrantComputeServiceAdapter implements ComputeServiceAdapter<VagrantNode,
Hardware, Box, Location> {
+   private File nodeContainer;
+   // TODO FIXME - use just as cache, expire items
+   private static Map<String, VagrantNode> machines = new HashMap<String, VagrantNode>();
+   private static final Pattern INTERFACE = Pattern.compile("inet ([0-9\\.]+)/(\\d+)");
+   
+   @Inject
+   public VagrantComputeServiceAdapter(@Named("vagrant.container-root") String nodeContainer)
{

Remove the public modifier to leave the injection constructor in default scope.

> +      public String getGroup() {
+         return group;
+      }
+      public String getName() {
+         return name;
+      }
+   }
+
+   @Override
+   public NodeAndInitialCredentials<VagrantNode> createNodeWithGroupEncodedIntoName(String
group, String name, Template template) {
+      MachineName machineName = new MachineName(group, name);
+      VagrantApi vagrant = getMachine(machineName);
+      init(vagrant.getPath(), name, template);
+
+      Machine newMachine = new Machine();
+      newMachine.setId(group + "/" + name);

By default, the name parameter should already have the group encoded. This is called by [this
strategy class](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/impl/CreateNodesWithGroupEncodedIntoNameThenAddToSet.java#L123)
that already takes care of encoding the group in the names, so it does not look OK to encode
the group again in the machine id.

> +//      newMachine.setId(group + "/" + name);
+
+      VagrantNode node = new VagrantNode(newMachine);
+
+      machines.put(newMachine.getId(), node);
+
+      return startMachine(vagrant, node);
+   }
+
+   private NodeAndInitialCredentials<VagrantNode> startMachine(VagrantApi vagrant,
VagrantNode node) {
+      Machine newMachine = node.getMachine();
+
+      vagrant.up(newMachine.getName());
+
+      SshConfig sshConfig = vagrant.sshConfig(newMachine.getName());
+      node.setSshConfig(sshConfig);

Only apply the SSH config if the OsFamily is not Windows?

> +      String privateKey;
+      try {
+         privateKey = Files.toString(new File(sshConfig.getIdentityFile()), Charset.defaultCharset());
+      } catch (IOException e) {
+         throw new IllegalStateException("Invalid private key " + sshConfig.getIdentityFile(),
e);
+      }
+
+      LoginCredentials loginCredentials = LoginCredentials.builder()
+            .user(sshConfig.getUser())
+            .privateKey(privateKey)
+            .build();
+      return new NodeAndInitialCredentials<VagrantNode>(node, newMachine.getId(), loginCredentials);
+   }
+
+   private Collection<String> getNetworks(String name, VagrantApi vagrant) {
+       String networks = vagrant.ssh(name, "ip address show | grep 'scope global'");

What if the image does not come with the iproute2 package installed? Given that we're parsing
command output, better use the ifconfig command?

> +
+      LoginCredentials loginCredentials = LoginCredentials.builder()
+            .user(sshConfig.getUser())
+            .privateKey(privateKey)
+            .build();
+      return new NodeAndInitialCredentials<VagrantNode>(node, newMachine.getId(), loginCredentials);
+   }
+
+   private Collection<String> getNetworks(String name, VagrantApi vagrant) {
+       String networks = vagrant.ssh(name, "ip address show | grep 'scope global'");
+       Matcher m = INTERFACE.matcher(networks);
+       Collection<String> ips = new ArrayList<String>();
+       while (m.find()) {
+          String network = m.group(1);
+          // TODO figure out a more generic approach to ignore unreachable networkds (this
one is the NAT'd address).
+          if (network.startsWith("10.")) continue;

A possible trick to detect unreachable networks would be to try to add a static route to an
IP of that network. If there is more than one hop to it, the command will fail.

> +          if (network.startsWith("10.")) continue;
+          ips.add(network);
+       }
+       return ips;
+    }
+
+   private String getHostname(String name, VagrantApi vagrant) {
+       return vagrant.ssh(name, "hostname").trim();
+    }
+
+   private void init(File path, String name, Template template) {
+      try {
+         writeVagrantfile(path);
+         initMachineConfig(path, name, template);
+      } catch (IOException e) {
+         throw new RuntimeException(e);

Throw a meaningful exception with a meaningful message.

> +               }
+            }, null);
+   }
+
+   @Override
+   public Iterable<Location> listLocations() {
+      return ImmutableList.of(
+              new LocationBuilder().id("localhost").description("localhost").scope(LocationScope.HOST).build());
+   }
+
+   @Override
+   public VagrantNode getNode(String id) {
+      // needed for BaseComputeServiceLiveTest.testAScriptExecutionAfterBootWithBasicTemplate()
+      // waits for the thread updating the credentialStore to execute
+      try {
+         Thread.sleep(200);

Why is this really needed? We shouldn't rely on this kind of things.

> +         Thread.currentThread().interrupt();
+         throw Throwables.propagate(e);
+      }
+
+      return machines.get(id);
+//      MachineName machine = new MachineName(id);
+//      if (getMachinePath(machine).exists()) {
+//         return getMachine(machine).status(machine.getName());
+//      } else {
+//         Machine ret = new Machine();
+//         ret.setId(id);
+//         ret.setName(machine.getName());
+//         ret.setPath(getMachinePath(machine));
+//         ret.setStatus(null);
+//         return ret;
+//      }

Remove all the commented code.

> +   @Override
+   public void destroyNode(String id) {
+       VagrantNode node = machines.get(id);
+       node.setMachineState(null);
+      MachineName machine = new MachineName(id);
+      getMachine(machine).destroy(machine.getName());
+      VagrantUtils.deleteFolder(getMachinePath(machine));
+   }
+
+   @Override
+   public void rebootNode(String id) {
+      MachineName machine = new MachineName(id);
+      try {
+          getMachine(machine).halt(machine.getName());
+      } catch (IllegalStateException e) {
+          getMachine(machine).haltForced(machine.getName());

Add some logs to let users properly trace what is happening.

> +           super(nodeContainer, credentialStore, statement);
+        }
+
+        @Override
+        public NodeMetadata apply(NodeMetadata input) {
+           input = super.apply(input);
+           if (input.getCredentials() != null) {
+              credentialStore.put("node#" + input.getId(), input.getCredentials());
+              updateMachine(nodeContainer, input, input.getCredentials());
+           }
+           return input;
+        }
+
+     }
+
+     //TODO copy&paste code, clean up & extract into function for reuse

Do it? :)

> +        if (credentials.getOptionalPassword().isPresent()) {
+            config.put("password", credentials.getOptionalPassword().get());
+        }
+        if (credentials.getOptionalPrivateKey().isPresent()) {
+            File privateKeyFile = new File(machineConfig.getParentFile(), name + "." + credentials.getUser()
+ ".key");
+            config.put("private_key_path", privateKeyFile.getAbsolutePath());
+            write(privateKeyFile, credentials.getOptionalPrivateKey().get());
+        }
+
+        write(machineConfig, config);
+     }
+
+    private static String readFile(File machineConfig) {
+        String str;
+        try {
+            str = Files.toString(machineConfig, Charsets.UTF_8);

[minor] Remove the variable and just return here.

> +   @Override
+   protected void configure() {
+      super.configure();
+      bind(new TypeLiteral<ComputeServiceAdapter<VagrantNode, Hardware, Box, Location>>()
{
+      }).to(VagrantComputeServiceAdapter.class);
+      bind(new TypeLiteral<Function<VagrantNode, NodeMetadata>>() {
+      }).to(MachineToNodeMetadata.class);
+      bind(new TypeLiteral<Function<MachineState, Status>>() {
+      }).to(MachineStateToJcloudsStatus.class);
+      bind(new TypeLiteral<Function<Box, Image>>() {
+      }).to(BoxToImage.class);
+      bind(new TypeLiteral<Function<Hardware, Hardware>>() {
+      }).to(this.<Hardware>getIdentityFunction());
+      bind(new TypeLiteral<Function<Location, Location>>() {
+      }).to(this.<Location>getIdentityFunction());
+      bind(PopulateDefaultLoginCredentialsForImageStrategy.class).to(VagrantDefaultImageCredentials.class);

Consider adding support for arbitrary cpu and ram configurations: http://jclouds.apache.org/blog/2016/08/22/arbitrary-cpu-ram/

> + * 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.vagrant.domain;
+
+import java.util.Collection;
+
+import com.google.common.collect.ImmutableSet;
+
+import vagrant.api.domain.Machine;
+import vagrant.api.domain.MachineState;
+import vagrant.api.domain.SshConfig;
+
+public class VagrantNode {

Use Google Auto for domain classes.

> + * 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.vagrant.domain;
+
+import java.util.Collection;
+
+import com.google.common.collect.ImmutableSet;
+
+import vagrant.api.domain.Machine;
+import vagrant.api.domain.MachineState;
+import vagrant.api.domain.SshConfig;
+
+public class VagrantNode {

And for any value classes in general.

> +import org.jclouds.domain.LoginCredentials;
+import org.jclouds.javax.annotation.Nullable;
+
+import com.google.common.base.Charsets;
+import com.google.common.io.Files;
+import com.google.inject.Singleton;
+
+@Singleton
+public class VagrantDefaultImageCredentials implements PopulateDefaultLoginCredentialsForImageStrategy
{
+
+    protected final LoginCredentials creds;
+    protected final Map<String, Credentials> credentialStore;
+    protected final Map<OsFamily, LoginCredentials> osFamilyToCredentials;
+
+    @Inject
+    public VagrantDefaultImageCredentials(@Nullable @Named("image") LoginCredentials creds,

Configure the injection constructors in all classes in default scope.

> +          File boxPath = getImagePath(image);
+          String config = readBoxConfig(boxPath);
+          LoginCredentials boxCreds = parseBoxCredentials(config);
+          if (boxCreds != null) {
+              return boxCreds;
+          } else {
+              // TODO pass default insecure_private_key and "vagrant" password
+              return LoginCredentials.builder().user("vagrant").build();
+          }
+       }
+    }
+
+    private LoginCredentials parseBoxCredentials(String config) {
+        // TODO search for xxx.ssh.username = "custom_username"
+        // "vagrant" is the default username
+        return null;

This needs to be implemented?

> +              return boxCreds;
+          } else {
+              // TODO pass default insecure_private_key and "vagrant" password
+              return LoginCredentials.builder().user("vagrant").build();
+          }
+       }
+    }
+
+    private LoginCredentials parseBoxCredentials(String config) {
+        // TODO search for xxx.ssh.username = "custom_username"
+        // "vagrant" is the default username
+        return null;
+     }
+
+     private String readBoxConfig(File boxPath) {
+        if (boxPath.exists()) return "";

Should it be negated?

> +    @Inject
+    public MachineToNodeMetadata(Function<MachineState, Status> toPortableNodeStatus,
+            @Memoized Supplier<Set<? extends Location>> locations,
+            Map<String, Credentials> credentialStore) {
+        this.toPortableNodeStatus = toPortableNodeStatus;
+        this.locations = locations;
+        this.credentialStore = credentialStore;
+    }
+
+    @Override
+    public NodeMetadata apply(VagrantNode node) {
+        Machine input = node.getMachine();
+        NodeMetadataBuilder nodeMetadataBuilder = new NodeMetadataBuilder()
+        .ids(input.getId())
+        .name(input.getName())
+        .group(input.getPath().getName())

This should be parsed from the name of the machine, according to how the group parameter is
configured in the compute service adapter.

> +
+   @Override
+   public Box getImage(final String id) {
+      return Iterables.find(listImages(),
+            new Predicate<Box>() {
+               @Override
+               public boolean apply(Box input) {
+                  return id.equals(input.getName());
+               }
+            }, null);
+   }
+
+   @Override
+   public Iterable<Location> listLocations() {
+      return ImmutableList.of(
+              new LocationBuilder().id("localhost").description("localhost").scope(LocationScope.HOST).build());

This should have as parent the location provided by the `JustProvider` supplier.

> +
+import vagrant.api.domain.Machine;
+import vagrant.api.domain.MachineState;
+import vagrant.api.domain.SshConfig;
+
+public class MachineToNodeMetadata implements Function<VagrantNode, NodeMetadata> {
+    private final Function<MachineState, Status> toPortableNodeStatus;
+    private final Supplier<Set<? extends Location>> locations;
+    private final Map<String, Credentials> credentialStore;
+
+    @Inject
+    public MachineToNodeMetadata(Function<MachineState, Status> toPortableNodeStatus,
+            @Memoized Supplier<Set<? extends Location>> locations,
+            Map<String, Credentials> credentialStore) {
+        this.toPortableNodeStatus = toPortableNodeStatus;
+        this.locations = locations;

If there is always a single location, just parse the list here instead of looking for it on
every call to the function.

> +        this.locations = locations;
+        this.credentialStore = credentialStore;
+    }
+
+    @Override
+    public NodeMetadata apply(VagrantNode node) {
+        Machine input = node.getMachine();
+        NodeMetadataBuilder nodeMetadataBuilder = new NodeMetadataBuilder()
+        .ids(input.getId())
+        .name(input.getName())
+        .group(input.getPath().getName())
+//      .operatingSystem(null)
+        .location(Iterables.getOnlyElement(locations.get()))
+        .hostname(input.getName())
+        .status(toPortableNodeStatus.apply(input.getStatus()))
+        .loginPort(22)

OsFamily dependant too?

> @@ -0,0 +1,15 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# 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.
+org.jclouds.vagrant.VagrantApiMetadata

Remove this file once the auto-service annotation is in place.

> +   }
+
+   @Override
+   protected void checkTagsInNodeEquals(NodeMetadata node, ImmutableSet<String> tags)
{
+      // Vagrant doesn't support tags
+      // TODO Could store it in the json
+   }
+
+   @Override
+   protected void checkUserMetadataContains(NodeMetadata node, ImmutableMap<String, String>
userMetadata) {
+      // Vagrant doesn't support user metadata
+      // TODO Could store it in the json
+   }
+
+   @Override
+   @Test(enabled = false)

Like in the other tests, better leave it enabled, but doing nothing.

> +   }
+
+   @Override
+   protected Set<String> getIso3166Codes() {
+      return ImmutableSet.of();
+   }
+
+   @Test
+   @Override
+   public void testDefaultTemplateBuilder() throws IOException {
+      Template defaultTemplate = view.getComputeService().templateBuilder().build();
+      assertEquals(defaultTemplate.getImage().getOperatingSystem().getFamily(), UBUNTU);
+      assertEquals(defaultTemplate.getImage().getOperatingSystem().is64Bit(), true);
+      assertEquals(defaultTemplate.getHardware().getName(), "micro");
+      assertEquals(getCores(defaultTemplate.getHardware()), 1.0d);
+   }

This test class is used to make sure jclouds is up to date with the Image offering of the
providers. Given that this  works only for local images, it does not really make sense to
have this test class.

> + * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.jclouds.vagrant.functions;
+
+public class BoxToImageTest {
+
+}

Implement the tests.

> + * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.jclouds.vagrant.functions;
+
+public class MachineStateToJcloudsStatusTest {
+
+}

Same here.

> + * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.jclouds.vagrant.functions;
+
+public class MachineToNodeMetadataTest {
+
+}

Same here.

-- 
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/160#pullrequestreview-12560755
Mime
View raw message