You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Svet <no...@github.com> on 2017/01/29 17:20:04 UTC
[jclouds/jclouds-labs] Load existing machines info on startup (#355)
Lets jclouds manage vagrant machines from previous runs.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/355
-- Commit Summary --
* Load existing machines info on startup
* Update vagrant provider limitations
-- File Changes --
M vagrant/README.md (1)
M vagrant/src/main/java/org/jclouds/vagrant/api/VagrantApiFacade.java (9)
A vagrant/src/main/java/org/jclouds/vagrant/api/VagrantBoxApiFacade.java (28)
M vagrant/src/main/java/org/jclouds/vagrant/compute/VagrantComputeServiceAdapter.java (68)
M vagrant/src/main/java/org/jclouds/vagrant/config/VagrantComputeServiceContextModule.java (23)
M vagrant/src/main/java/org/jclouds/vagrant/domain/VagrantNode.java (4)
M vagrant/src/main/java/org/jclouds/vagrant/functions/MachineToNodeMetadata.java (41)
A vagrant/src/main/java/org/jclouds/vagrant/internal/ImageSupplier.java (57)
M vagrant/src/main/java/org/jclouds/vagrant/internal/MachineConfig.java (9)
M vagrant/src/main/java/org/jclouds/vagrant/internal/VagrantCliFacade.java (14)
A vagrant/src/main/java/org/jclouds/vagrant/internal/VagrantExistingMachines.java (156)
M vagrant/src/main/java/org/jclouds/vagrant/internal/VagrantNodeRegistry.java (17)
M vagrant/src/test/java/org/jclouds/vagrant/functions/MachineToNodeMetadataTest.java (79)
A vagrant/src/test/java/org/jclouds/vagrant/internal/VagrantNodeLoaderTest.java (116)
M vagrant/src/test/java/org/jclouds/vagrant/internal/VagrantNodeRegistryTest.java (12)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/355.patch
https://github.com/jclouds/jclouds-labs/pull/355.diff
--
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/355
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/46e7e737) and [2.0.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/e4c809db). Thanks @neykov!
--
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/355#issuecomment-276630571
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Svet <no...@github.com>.
neykov commented on this pull request.
> this.timeSupplier = timeSupplier;
+ this.nodes = Suppliers.memoize(new ConcurrentWrapperSupplier(existingMachines));
The `ConcurrentHashMap` is an implementation detail of the `VagrantNodeRegistry`. The wrapping supplier+memoize is just a mechanism to achieve lazy loading, not something that's expected to be provided by external wiring (so that we don't do IO in the constructor).
That's at least my take on it, don't mind making it pluggable if you still think that'd be useful.
--
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/355
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @@ -73,36 +93,41 @@ public long getDelay(TimeUnit unit) {
return unit.convert(expiryTime - timeSupplier.get(), TimeUnit.MILLISECONDS);
}
}
- private Map<String, VagrantNode> nodes = new ConcurrentHashMap<String, VagrantNode>();
- private DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
+
+ private final DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
Hmmm... AFAIK that only happens in AWS because the nodes don't disappear directly. jclouds expects the nodes to be gone or in terminated state (as per the [node terminated predicate](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/predicates/AtomicNodeTerminated.java)).
This looks great and we've learned something! But we'd better keep the code as simple as possible. Could you try removing the terminated node cache and test the provider if you just completely remove the machines? It should still work as expected.
--
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/355
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Svet <no...@github.com>.
@nacx addressed comments.
--
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/355#issuecomment-276450652
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Ignasi Barrera <no...@github.com>.
Closed #355.
--
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/355#event-944569401
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
Looks good in general! I've added a few comments to complete my understanding on how the provider works..
Could you also elaborate a bit the motivation behind having split the Vagrant Facade in two different interfaces?
> this.timeSupplier = timeSupplier;
+ this.nodes = Suppliers.memoize(new ConcurrentWrapperSupplier(existingMachines));
Would it make sense to configure this supplier (qualified with `@Memoized` if appropriate) in a Guice module? It looks more flexible and correct to get injected what you're gonna use instead of decorating the arguments?
> @@ -73,36 +93,41 @@ public long getDelay(TimeUnit unit) {
return unit.convert(expiryTime - timeSupplier.get(), TimeUnit.MILLISECONDS);
}
}
- private Map<String, VagrantNode> nodes = new ConcurrentHashMap<String, VagrantNode>();
- private DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
+
+ private final DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
Didn't know about this type of queue. The way it is used here looks awesome! :)
> @@ -73,36 +93,41 @@ public long getDelay(TimeUnit unit) {
return unit.convert(expiryTime - timeSupplier.get(), TimeUnit.MILLISECONDS);
}
}
- private Map<String, VagrantNode> nodes = new ConcurrentHashMap<String, VagrantNode>();
- private DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
+
+ private final DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
Just to understand the whole picture. Is this used as an in-memory cache of recently terminated nodes (and why)? If not, is there any other purpose for 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/355#pullrequestreview-19429117
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Svet <no...@github.com>.
neykov commented on this pull request.
> @@ -73,36 +93,41 @@ public long getDelay(TimeUnit unit) {
return unit.convert(expiryTime - timeSupplier.get(), TimeUnit.MILLISECONDS);
}
}
- private Map<String, VagrantNode> nodes = new ConcurrentHashMap<String, VagrantNode>();
- private DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
+
+ private final DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
Me neither before writing this code :). Turned out really handy.
If I remember correctly jclouds needs to see `terminated` status after destroying the machine. I remove all traces of the machine on termination so I won't be able to tell the difference between `terminated` and `not created` if I don't keep this around.
--
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/355
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Svet <no...@github.com>.
>Could you also elaborate a bit the motivation behind having split the Vagrant Facade in two different interfaces?
The `<B>` started leaking in classes that don't really deal with boxes at all. Looking at the changes that must've been code that I've removed since as it was additional to `VagrantComputeServiceAdapter`. Still I think it's useful as `<B>` is now confined just to `ImageSupplier` which indeed does handle `B`'s.
--
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/355#issuecomment-276513508
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @@ -73,36 +93,41 @@ public long getDelay(TimeUnit unit) {
return unit.convert(expiryTime - timeSupplier.get(), TimeUnit.MILLISECONDS);
}
}
- private Map<String, VagrantNode> nodes = new ConcurrentHashMap<String, VagrantNode>();
- private DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
+
+ private final DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
Agree. Mind squashing the commits so I can merge it? Thanks for all explanations!
--
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/355
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Svet <no...@github.com>.
neykov commented on this pull request.
> +import com.google.common.base.Supplier;
+import com.google.common.collect.Collections2;
+import com.google.inject.Inject;
+
+public class ImageSupplier<B> implements Supplier<Collection<Image>>, Function<String, Image> {
+ private final Function<Collection<B>, Collection<B>> outdatedBoxesFilter;
+ private final VagrantBoxApiFacade.Factory<B> cliFactory;
+ private final Function<B, Image> boxToImage;
+
+ @Inject
+ ImageSupplier(Function<Collection<B>, Collection<B>> outdatedBoxesFilter,
+ VagrantBoxApiFacade.Factory<B> cliFactory,
+ Function<B, Image> boxToImage) {
+ this.outdatedBoxesFilter = checkNotNull(outdatedBoxesFilter, "outdatedBoxesFilter");
+ this.cliFactory = checkNotNull(cliFactory, "cliFactory");
+ this.boxToImage = checkNotNull(boxToImage, "boxToImage");
Wdym redundant? Is it because non-Nullable and guice guarantees a value?
Remove from all constructors or just those you flagged?
--
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/355
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Ignasi Barrera <no...@github.com>.
nacx requested changes on this pull request.
> +import com.google.common.base.Supplier;
+import com.google.common.collect.Collections2;
+import com.google.inject.Inject;
+
+public class ImageSupplier<B> implements Supplier<Collection<Image>>, Function<String, Image> {
+ private final Function<Collection<B>, Collection<B>> outdatedBoxesFilter;
+ private final VagrantBoxApiFacade.Factory<B> cliFactory;
+ private final Function<B, Image> boxToImage;
+
+ @Inject
+ ImageSupplier(Function<Collection<B>, Collection<B>> outdatedBoxesFilter,
+ VagrantBoxApiFacade.Factory<B> cliFactory,
+ Function<B, Image> boxToImage) {
+ this.outdatedBoxesFilter = checkNotNull(outdatedBoxesFilter, "outdatedBoxesFilter");
+ this.cliFactory = checkNotNull(cliFactory, "cliFactory");
+ this.boxToImage = checkNotNull(boxToImage, "boxToImage");
Remove the redundant null checks.
> + protected Logger logger = Logger.NULL;
+
+ private final File home;
+ private final MachineConfig.Factory machineConfigFactory;
+ private final Supplier<Collection<Image>> imageLister;
+ private final Supplier<? extends Map<String, Hardware>> hardwareSupplier;
+
+ @Inject
+ VagrantExistingMachines(@Named(VagrantConstants.JCLOUDS_VAGRANT_HOME) String home,
+ MachineConfig.Factory machineConfigFactory,
+ Supplier<Collection<Image>> imageLister,
+ Supplier<? extends Map<String, Hardware>> hardwareSupplier) {
+ this.home = new File(checkNotNull(home, "home"));
+ this.machineConfigFactory = checkNotNull(machineConfigFactory, "machineConfigFactory");
+ this.imageLister = checkNotNull(imageLister, "imageLister");
+ this.hardwareSupplier = checkNotNull(hardwareSupplier, "hardwareSupplier");
Remove the redundant null checks.
> this.timeSupplier = timeSupplier;
+ this.nodes.putAll(getExistingNodes(existingMachines));
It would be better to make the `VagrantExistingMachines` a supplier, to lazily load them. Otherwise all the load and parsing happens at injection time, in the constructor, where we should avoid throwing IO exceptions and other exceptions that could be raised while loading the existing machines.
--
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/355#pullrequestreview-19043408
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Svet <no...@github.com>.
neykov commented on this pull request.
> @@ -73,36 +93,41 @@ public long getDelay(TimeUnit unit) {
return unit.convert(expiryTime - timeSupplier.get(), TimeUnit.MILLISECONDS);
}
}
- private Map<String, VagrantNode> nodes = new ConcurrentHashMap<String, VagrantNode>();
- private DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
+
+ private final DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
Interesting. I'll check that - agree we shouldn't keep it if not needed.
Not related to this PR, so shouldn't block merge.
--
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/355
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> +import com.google.common.base.Supplier;
+import com.google.common.collect.Collections2;
+import com.google.inject.Inject;
+
+public class ImageSupplier<B> implements Supplier<Collection<Image>>, Function<String, Image> {
+ private final Function<Collection<B>, Collection<B>> outdatedBoxesFilter;
+ private final VagrantBoxApiFacade.Factory<B> cliFactory;
+ private final Function<B, Image> boxToImage;
+
+ @Inject
+ ImageSupplier(Function<Collection<B>, Collection<B>> outdatedBoxesFilter,
+ VagrantBoxApiFacade.Factory<B> cliFactory,
+ Function<B, Image> boxToImage) {
+ this.outdatedBoxesFilter = checkNotNull(outdatedBoxesFilter, "outdatedBoxesFilter");
+ this.cliFactory = checkNotNull(cliFactory, "cliFactory");
+ this.boxToImage = checkNotNull(boxToImage, "boxToImage");
Yes, Guice already fails if a value is missing so we'd better remove those null checks. I know there are many constructors like this in the codebase but were trying to avoid the redundant checks in all new contributions.
--
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/355
Re: [jclouds/jclouds-labs] Load existing machines info on startup
(#355)
Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.
--
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/355#pullrequestreview-19523690