Vagrant provider #160
Vagrant provider #160
Conversation
|
Thanks @neykov! |
|
As suggested by @andreaturli remote support could be added with the help of https://github.com/fjsanpedro/vagrant-node, but local is where vagrant is most useful. |
0217443 to
4329f93
Compare
|
Added live tests and missing functionality. I've been using the provider the whole time since creating it and it's been working great. Good to review. |
|
bump @nacx, @andreaturli |
|
A couple of unit tests missing, but will be easy to add if everything else is good to go. |
|
Thanks @neykov! Looking at it, the bindings just run the required vagrant commands, so there is no actual API exposed by vagrant, right? In that case I'd say ti is OK for us to add the bindings dependency (given it has no transitive dependencies), but we can't rely on a snapshot version that is not publicly available. We can only depend on fixed versions that are available in Maven Central, so this would be the first thing to address. |
1d7e6ac to
2ebedbc
Compare
|
@nacx There's no vagrant API, so it's using the cli under the hood. It's possible to install a plugin which exposes an API, but since the provider makes most sense when used locally better not introduce another abstraction layer (and add extra setup steps for users). I've updated the dependency to a released version (no transitive dependencies there) and rebased. Live tests are passing. |
36389c3 to
b463c65
Compare
|
Hi @nacx. Bumped version to |
|
My apologies @neykov. This pull request got lost in my review queue. I'll review it later today and make sure we move forward with this. |
| * `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 |
There was a problem hiding this comment.
Update this. The bindings are in Maven Central and the project can be build just as any other jclouds provider.
| <packaging>bundle</packaging> | ||
|
|
||
| <properties> | ||
| <jclouds.version>2.1.0-SNAPSHOT</jclouds.version> |
There was a problem hiding this comment.
Remove this and use project.parent.version consistently for all dependencies.
| import org.jclouds.compute.config.ComputeServiceProperties; | ||
| import org.jclouds.vagrant.config.VagrantComputeServiceContextModule; | ||
|
|
||
| public class VagrantApiMetadata extends BaseApiMetadata { |
There was a problem hiding this comment.
Add the auto-service dependency:
<dependency>
<groupId>com.google.auto.service</groupId>
<artifactId>auto-service</artifactId>
<scope>provided</scope>
</dependency>And annotate this as @AutoService(ApiMetadata.class).
| 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>(); |
There was a problem hiding this comment.
Use a proper Guava LoadingCache then?
| import vagrant.api.domain.SshConfig; | ||
|
|
||
| public class VagrantComputeServiceAdapter implements ComputeServiceAdapter<VagrantNode, Hardware, Box, Location> { | ||
| private File nodeContainer; |
| } | ||
|
|
||
| @Override | ||
| @Test(enabled = false) |
There was a problem hiding this comment.
Like in the other tests, better leave it enabled, but doing nothing.
| assertEquals(defaultTemplate.getImage().getOperatingSystem().is64Bit(), true); | ||
| assertEquals(defaultTemplate.getHardware().getName(), "micro"); | ||
| assertEquals(getCores(defaultTemplate.getHardware()), 1.0d); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Do you mean this particular test or the class altogether?
Seems there are useful tests in there that still apply to this provider. For example all of the testAuto* methods. I think it's worth keeping it.
|
|
||
| public class BoxToImageTest { | ||
|
|
||
| } |
|
|
||
| public class MachineStateToJcloudsStatusTest { | ||
|
|
||
| } |
|
|
||
| public class MachineToNodeMetadataTest { | ||
|
|
||
| } |
|
Thanks for taking the time to go through the code. Will implement the changes and get back to you. |
|
thx @neykov, sorry for being late in this discussion. Just thinking aloud, would it be acceptable for you to donate the https://github.com/neykov/vagrant-java-bindings to jclouds as well? I'm seeing it as a maven submodule of jclouds-vagrant which is completely in sync with the jclouds-vagrant requirements. @neykov @nacx wdyt? |
| * `mvn clean install` | ||
| * Copy `target/vagrant-2.0.0-SNAPSHOT.jar` to jcloud's classpath | ||
|
|
||
| Using in Apache Brooklyn |
There was a problem hiding this comment.
I think this is pretty standard, not sure it needs to be in this README, wdyt?
There was a problem hiding this comment.
The catch is that Vagrant doesn't have credential and identity, while Brooklyn requires them. Setting any values will do, but it needs to be there.
Agree it's too user specific, so here's probably not the best place to put it.
Let's keep it around for some more time until moved to a better place.
| Local caching proxy | ||
| ------------------- | ||
|
|
||
| ### Polipo |
|
|
||
| Use `polipo` for caching proxy. On OS X install with | ||
| ``` | ||
| sudo brew install polipo |
|
|
||
| * `sudo chown root:wheel /Library/LaunchDaemons/fr.jussieu.pps.polipo.plist` | ||
| * `sudo chmod 755 /Library/LaunchDaemons/fr.jussieu.pps.polipo.plist` | ||
| * `sudo launchctl -w load /Library/LaunchDaemons/fr.jussieu.pps.polipo.plist` |
There was a problem hiding this comment.
which os x version have you tested these notes ? I get
$ sudo launchctl -w load /Library/LaunchDaemons/fr.jussieu.pps.polipo.plist
Unrecognized subcommand: -w
Usage: launchctl <subcommand> ... | help [subcommand]
Many subcommands take a target specifier that refers to a domain or service
using macOS 10.12.1
There was a problem hiding this comment.
it's sudo launchctl load -w - updated the README and suggested a change in the linked SO answer.
|
|
||
| Download a Vagrant box: | ||
| ``` | ||
| vagrant box add ubuntu/trustry64 |
There was a problem hiding this comment.
It may be useful to reference this issue Varying-Vagrant-Vagrants/VVV#354 as I'm hitting it :)
There was a problem hiding this comment.
actually sudo rm /opt/vagrant/embedded/bin/curl doesn't help for me. can you double-check?
There was a problem hiding this comment.
That's core vagrant functionality. If box add doesn't work, then there's something broken in your vagrant install.
Some things to try:
- running it with
--debugat the end - any giveaways there? - does it fail with any box or just
ubuntu/trusty64? - what's in your
~/.vagrant.d/boxesfolder?
There was a problem hiding this comment.
I tried with debug, not much more there, it fails only with ubuntu/trusty64 so it is probably my setup. thanks for checking
There was a problem hiding this comment.
This one could be related as well - hashicorp/vagrant#7997
There was a problem hiding this comment.
possibly in my case it was a combination of the above and a previously downloaded ubuntu/trusty64 on my machine, thx
|
Addressed all comments I haven't replied directly to. Also a lot of improvements all around the board. |
|
@andreaturli Don't mind moving the project under the jclouds umbrella. What I'd like though is to keep it self-contained without any additional dependencies like guice, guava or jclouds logging. That's to allow anyone using it without pulling in jclouds dependencies. |
There are no issues for keeping a the non-dependency approach (I love that!). However, if we want the project to be part of jclouds we should stick to some conventions (such as using the jclouds logging). jclouds is about portability, and that not only refers to implement some interfaces, but also to having common ways and patterns to configure stuff and operate. jclouds users should "feel" they use consistent APIs that follow a concrete set of principles, not that jclouds is a "grouping" of random libraries. With this considerations in mind, I'd say it is better to keep the project as an external dependency, if you prefer to keep it as-is. |
|
Fully agree @nacx, that was my point as well. If you prefer to avoid the additional dependency I can bring in just the bits that are needed for the jclouds provider? |
Your call :) I don't have a special issue with this dependency, because it does not add any transitive dependencies that conflict in how jclouds works. We usually recommend not adding dependencies because that often leads to use existing api clients instead of following the jclouds mechanisms, but I'd say it is OK to have dependencies like this one. It is important to note, however, that is is easier to keep things under control if they are in the jclouds source tree (for example if future versions of your library add other dependencies or change behaviour, etc). But your call :) |
|
thanks @nacx and @andreaturli for your opinions - here's what I'll end up doing then:
|
|
Sounds like a plan! Thanks! |
|
I've abstracted away the access to vagrant - it's all in |
| static class VagrantCliFacade implements VagrantApiFacade<Box> { | ||
|
|
||
| private final VagrantApi vagrant; | ||
| private final VagrantOutputRecorder outputRecorder; |
There was a problem hiding this comment.
I think the Function interface is not the best one to create instances of VagrantCliFacade. What about Guice assisted inject? This is how this is done in jclouds, and having a factory instead of a function makes more sense in this case.
public class VagrantCliFacade implements VagrantApiFacade<Box> {
interface Factory {
VagrantCliFacade create(File path);
}
private final VagrantApi vagrant;
private final VagrantOutputRecorder outputRecorder;
@Inject
VagrantCliFacade(VagrantWireLogger wireLogger, @Assisted File path) {
this.outputRecorder = new VagrantOutputRecorder(wireLogger);
this.vagrant = Vagrant.forPath(path, outputRecorder);
}
...
}Then just add the following to the VagrantComputeServiceContextModule:
install(new FactoryModuleBuilder().build(VagrantCliFacade.Factory.class));And now you can inject the VagrantCliFacade.Factory object to the compute service adapter, and call its create method where needed to produce the facade.
There was a problem hiding this comment.
Agree a factory would be better suited. That's a nice trick, I've implemented it.
|
Here's an example output from a test run: |
| protected TemplateBuilder templateBuilder() { | ||
| return super.templateBuilder() | ||
| .imageId(getImageId()); | ||
| } |
There was a problem hiding this comment.
I've just seen this. Instead of hardcoding the fact that you want to use a custom image to run the tests, use the existing "jclouds.template" property. If you use that property, you can safely remove this method and the "getImage" one.
You can configure a default value in the pom.xml (like in digitalocean2 for example), and run the tests with -Dtest.vagrant.template="imageId=foo/bar". This will keep the tests aligned with the rest of the providers.
Can you share the results of running the entire It is not a merge-blocker If many of them fail, but we need them passing if we want to promote the provider out of labs. |
|
|
Thanks! Mind squashing the commits into a single one to make the merge cleaner? I'll have a final look then and merge the PR. Thanks! |
55c1fcd to
6e48429
Compare
|
Happy to hear that @nacx. Squashed. |
|
🎉 thanks @nacx @andreaturli. |
Implements ComputeServiceAdapter for Vagrant.
How to use:
vagrant box add ubuntu/trusty64compute.templateBuilder().imageId("ubuntu/trusty64").build();The machines in a single group will be created as a multi-machine config in vagrant.
Uses https://github.com/neykov/vagrant-java-bindings to drive vagrant.