You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2015/05/05 01:13:00 UTC
[jclouds-karaf] Removed the uses of the old ChefContext (#65)
nacx wants to merge 1 commit into jclouds:master from nacx:chef-context:
Refactored the Chef integration to get rid of the old `ChefContext` and use the `ApiContext<ChefApi>`. Instead of injecting the `ChefService` everywhere, it is more correct to inject directly the context that provides access to both the `ChefApi` and the `ChefService`.
Verified by running the CLI and creating different Chef services. Both can be individually referenced and commands can be run on each, so looks like all the caching stuff and everything is working as expected.
/cc @iocanel @ccustine
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-karaf/pull/65
-- Commit Summary --
* Removed the uses of the old ChefContext
-- File Changes --
M cache/src/main/java/org/jclouds/karaf/cache/utils/CacheUtils.java (24)
M chef/cache/src/main/java/org/jclouds/karaf/chef/cache/Activator.java (10)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/ChefCommandBase.java (31)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/ChefCommandWithOptions.java (17)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/ChefCookbookListCommand.java (7)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/ChefGroupBootStrap.java (2)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/ChefNodeBootstrap.java (2)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/ChefRunscriptBase.java (15)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/ChefServiceCreateCommand.java (13)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/ChefServiceListCommand.java (6)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/completer/ChefApiCompleter.java (24)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/completer/ChefCompleterSupport.java (9)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/completer/ChefContextNameCompleter.java (17)
M chef/commands/src/main/java/org/jclouds/karaf/chef/commands/completer/CookbookCompleter.java (8)
M chef/commands/src/main/resources/OSGI-INF/blueprint/jclouds-chef-commands.xml (2)
M chef/core/src/main/java/org/jclouds/karaf/chef/core/ChefHelper.java (82)
M chef/services/pom.xml (2)
M chef/services/src/main/java/org/jclouds/karaf/chef/services/Activator.java (20)
M chef/services/src/main/java/org/jclouds/karaf/chef/services/ChefRecipeProvider.java (9)
M chef/services/src/main/java/org/jclouds/karaf/chef/services/ChefServiceFactory.java (25)
M chef/services/src/main/java/org/jclouds/karaf/chef/services/EnvBasedChefRecipeProvider.java (6)
-- Patch Links --
https://github.com/jclouds/jclouds-karaf/pull/65.patch
https://github.com/jclouds/jclouds-karaf/pull/65.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
I think it does not make sense to make them named. The fact that most users will be using the context builder to create directly the ChefApi does not mean the context does not exist. jclouds internally creates the context, which provides the injector and the ChefApi itself, and it has a name (that can be configured using the context builder, and that is what karaf uses). So, the context will always exist; there's no need to duplicate the naming thing to add a name to the ChefApi too.
I wouldn't say we're using the "first named object". We are extracting the name exactly from the same object as before. For example, the compute service does [this](https://github.com/jclouds/jclouds-karaf/blob/master/commands/src/main/java/org/jclouds/karaf/commands/compute/ComputeCommandBase.java#L83) to get the name: service -> view (getContext) -> underlying ApiContext (unwrap) -> name. We have just changed the path, but there's always ben an ApiContext there providing the name.
Thanks for sharing your thoughts! This is a sane discussion that has helped me (and hope others!) understand better how jclouds-karaf works.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-99342852
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -61,7 +65,9 @@ public void start(BundleContext context) throws Exception {
> cacheProviderRegistration = context.registerService(CacheProvider.class.getName(), cacheProvider, new Hashtable<String, Object>());
>
>
> - chefServiceTracker = CacheUtils.createServiceCacheTracker(context, ChefService.class, chefCacheManager);
> + chefServiceTracker = CacheUtils.createServiceCacheTracker(context, new TypeToken<ApiContext<ChefApi>>() {
> + private static final long serialVersionUID = 1L;
The chef-cache project does not have the chef-core as a dependency, and I wanted to avoid adding it just to declare the `TypeToken`. Are you OK leaving this as-is?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29656651
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> @@ -61,7 +65,9 @@ public void start(BundleContext context) throws Exception {
> cacheProviderRegistration = context.registerService(CacheProvider.class.getName(), cacheProvider, new Hashtable<String, Object>());
>
>
> - chefServiceTracker = CacheUtils.createServiceCacheTracker(context, ChefService.class, chefCacheManager);
> + chefServiceTracker = CacheUtils.createServiceCacheTracker(context, new TypeToken<ApiContext<ChefApi>>() {
> + private static final long serialVersionUID = 1L;
> Are you OK leaving this as-is?
Sure!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29692164
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -50,6 +52,10 @@
> public static final String JCLOUDS_CHEF_VALIDATOR_KEY_FILE = "JCLOUDS_CHEF_VALIDATOR_KEY_FILE";
> public static final String JCLOUDS_CHEF_VALIDATOR_CREDENTIAL = "JCLOUDS_CHEF_VALIDATOR_CREDENTIAL";
> public static final String JCLOUDS_CHEF_ENDPOINT = "JCLOUDS_CHEF_ENDPOINT";
> +
> + public static final TypeToken<ApiContext<ChefApi>> CHEF_TOKEN = new TypeToken<ApiContext<ChefApi>>() {
> + private static final long serialVersionUID = 1L;
In this case, I think there's no need to propagate the warnings everywhere. It looks cleaner to me this way. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29656860
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> + Object service = super.addingService(reference);
> + if (serviceToken.isAssignableFrom(service.getClass())) {
> + cacheManager.bindService((T) service);
> + }
> + return service;
> + }
> +
> + @Override
> + public void removedService(ServiceReference reference, Object service) {
> + if (serviceToken.isAssignableFrom(service.getClass())) {
> + cacheManager.unbindService((T) service);
> + }
> + super.removedService(reference, service);
> + }
> + };
> + }
[minor] Indents look off here, but also in the rest of the file, I guess...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29634585
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> if (chefService != null) {
> - Iterable<? extends CookbookVersion> cookbookVersions = chefService.listCookbookVersions();
> + Iterable<? extends CookbookVersion> cookbookVersions = chefService.getApi().chefService()
[minor] Indent off?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29634897
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
> + Object service = super.addingService(reference);
> + if (serviceToken.isAssignableFrom(service.getClass())) {
> + cacheManager.bindService((T) service);
> + }
> + return service;
> + }
> +
> + @Override
> + public void removedService(ServiceReference reference, Object service) {
> + if (serviceToken.isAssignableFrom(service.getClass())) {
> + cacheManager.unbindService((T) service);
> + }
> + super.removedService(reference, service);
> + }
> + };
> + }
Yes, all the files were formatted that way and I didn't want to reformat everything in this PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29656595
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed to master as [7675dabb](http://git-wip-us.apache.org/repos/asf/jclouds-karaf/commit/7675dabb).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-99629022
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> it is more correct to inject directly the context that provides access to both the ChefApi and the ChefService.
A bit of a non-expert question here, but do we _need_ access to both? Would e.g. ChefApi not be enough?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-98886325
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
>A separate note: do we need any additional tests for this?
I don't think so. The existing karaf live tests already exercise the code, so they should be enough.
Good to go then, thanks for the review @demobox!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-99623558
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> We are extracting the name exactly from the same object as before.
Perhaps we were previous also using the "first named object." But given that, I think it makes a lot of sense to keep things as they are.
A separate note: do we need any additional tests for this?
With the exception of the outstanding test question, +1 - looks good to me. Thanks, @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-99527182
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> if (!Strings.isNullOrEmpty(id)) {
> - ChefService service = null;
> - for (ChefService svc : services) {
> - if (id.equals(svc.getContext().unwrap().getName())) {
> - service = svc;
> + ApiContext<ChefApi> service = null;
[minor] Indent off?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29634927
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> @@ -50,6 +52,10 @@
> public static final String JCLOUDS_CHEF_VALIDATOR_KEY_FILE = "JCLOUDS_CHEF_VALIDATOR_KEY_FILE";
> public static final String JCLOUDS_CHEF_VALIDATOR_CREDENTIAL = "JCLOUDS_CHEF_VALIDATOR_CREDENTIAL";
> public static final String JCLOUDS_CHEF_ENDPOINT = "JCLOUDS_CHEF_ENDPOINT";
> +
> + public static final TypeToken<ApiContext<ChefApi>> CHEF_TOKEN = new TypeToken<ApiContext<ChefApi>>() {
> + private static final long serialVersionUID = 1L;
[minor] Do we do this or use `@SuppressWarnings` elsewhere?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29634913
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
>A non-expert question here, but do we need access to both? Would e.g. ChefApi not be enough?
The point here is that jclouds-karaf uses the created jclouds Context name and id to index and build the cache of services. You can create several ComputeService instances, with different credentials/providers, and work with all them at the same time, and karaf indexes them using the Context information.
In Compute and BlobStore, the created Context is the ComputeServiceContext and the BlobStoreContext (which also implement View but that's not relevant here), which produce the corresponding ComputeService and BlobStore objects that are used by jclouds-karaf. Those objects have a "getContext()" method, providing access to the context that created them, and that method is used to get the name of the context when it comes to cache and index the Service objects. It basically caches something like:
ctx-name1 -> ComputeService1
ctx-name2 -> ComputeService2
ctx-name3 -> BlobStore1
ctx-name4 -> ChefService1
The commit that broke karaf removed the ChefContext, because it made no sense: Chef does not have an abstraction view (although it is tightly integrated with the Compute one), and it didn't make sense to have a specific Context implementation for chef that provided nothing, so I removed it. In the absence of a view or specific context implementation, jclouds creates an ApiContext<ApiClass> context, providing access to the api.
What I didn't want to do, is to expose a getter to that ApiContext in the ChefApi or the ChefService. There is nothing special about that generic context (as opposed to the Compute and BlobStore context), so it does not make sense to expose it. This is what motivated this change: since there is no access to the "creator context" from the ChefService or the ChefApi, I changed the entity that gets cached and indexed by jclouds-karaf in the case of Chef. Instead of indexing the ChefService, I index directly the Context. Being the "context" the entity jclouds-karaf works with, makes it unnecessary to provide a "getContext" method in the ChefApi or ChefService, so I preferred this approach to avoid coupling the Chef apis to the jclouds-karaf design. With this change, the karaf service cache are now:
ctx-name1 -> ComputeService1
ctx-name2 -> ComputeService2
ctx-name3 -> BlobStore1
ctx-name4 -> ApiContext<ChefApi> (which can be used to get the ChefService)
I'm not good at explaining things, but I hope the motivation is a bit more clear now :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-99024165
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
Closed #65.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#event-298705307
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Ignasi Barrera <no...@github.com>.
The point is that in order to support N "named services", karaf stores and indexes them by name. This way users can reference them by name and use each service easily.
That name is extracted from the cached object itself in several places along the karaf code. In the case of the ComputeService and BlobStore cases, both have a getContext() method that provides access to the (named) context and its name, so everything works out of the box. jclouds-karaf was build assuming that the services provide access to their creating context, but that was (perhaps) a poor decision based on the fact that all services actually had a specific context. That is no longer true in Chef.
In the case of Chef, neither the ChefApi nor the ChefService provide access to the context and have no notion of a "name". In order to cache only the ChefApi we should introduce a wrapper object that adds a name to it (but that is very similar to directly caching the ApiContext itself), or modify the ChefApi or ChefService in a way that they can provide a name (for example by providing access to the creating context). I would like to avoid doing this too, because it is just coupling the ChefApi and ChefService to a specific design of jclouds-karaf, and that does not look like a good idea.
The key point here is that the services jclouds-karaf provides, have to be "named". And the ChefApi and ChefService alone don't have a name. The context has, so given that they don't provide a direct access to the context, caching the context directly seems the better way to go. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-99202413
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> but I hope the motivation is a bit more clear now
Yes, that makes sense - thank you for the detailed explanation! I guess my question really boils down to "Can we cache ChefApi rather than the context?" - from a quick glance, it seems we only use the context directly in the completers, and we can get the ChefService from the ChefApi.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-99153116
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> @@ -61,7 +65,9 @@ public void start(BundleContext context) throws Exception {
> cacheProviderRegistration = context.registerService(CacheProvider.class.getName(), cacheProvider, new Hashtable<String, Object>());
>
>
> - chefServiceTracker = CacheUtils.createServiceCacheTracker(context, ChefService.class, chefCacheManager);
> + chefServiceTracker = CacheUtils.createServiceCacheTracker(context, new TypeToken<ApiContext<ChefApi>>() {
> + private static final long serialVersionUID = 1L;
[minor] Do we use this or `@SuppressWarnings` elsewhere?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29634669
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> @@ -50,6 +52,10 @@
> public static final String JCLOUDS_CHEF_VALIDATOR_KEY_FILE = "JCLOUDS_CHEF_VALIDATOR_KEY_FILE";
> public static final String JCLOUDS_CHEF_VALIDATOR_CREDENTIAL = "JCLOUDS_CHEF_VALIDATOR_CREDENTIAL";
> public static final String JCLOUDS_CHEF_ENDPOINT = "JCLOUDS_CHEF_ENDPOINT";
> +
> + public static final TypeToken<ApiContext<ChefApi>> CHEF_TOKEN = new TypeToken<ApiContext<ChefApi>>() {
> + private static final long serialVersionUID = 1L;
> It looks cleaner to me this way. WDYT?
I'm fine with either - just wondering what we use elsewhere in jclouds.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29692130
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> - public static ChefService findOrCreateChefService(String api, String name, String clientName, String clientCredential, String clientKeyFile, String validatorName, String validatorCredential, String validatorKeyFile, String endpoint, List<ChefService> chefServices) {
> + public static ApiContext<ChefApi> findOrCreateChefService(String api, String name, String clientName,
> + String clientCredential, String clientKeyFile, String validatorName, String validatorCredential,
> + String validatorKeyFile, String endpoint, List<ApiContext<ChefApi>> chefServices) {
[minor] Indents off?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29634949
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> @@ -192,10 +198,10 @@ public static ChefService getChefService(String id, String api, List<ChefService
> }
>
> if (!Strings.isNullOrEmpty(api)) {
> - ChefService service = null;
> - for (ChefService svc : services) {
> - if (api.equals(svc.getContext().unwrap().getId())) {
> - service = svc;
> + ApiContext<ChefApi> service = null;
[minor] Indent off?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65/files#r29634937
Re: [jclouds-karaf] Removed the uses of the old ChefContext (#65)
Posted by Andrew Phillips <no...@github.com>.
> The key point here is that the services jclouds-karaf provides, have to be "named". And the ChefApi and ChefService alone don't have a name.
That's what I understood too, based on your description (thanks!). I should start by saying that I'm fine with the change as-is and I don't want to spend more of your time on this than necessary, but for me the real question then is: does it make _sense_ for either the ChefApi or the ChefService to be named, or not?
If not, then I agree that creating a `NamedChefApi` decorator simply to make Karaf happy is probably more messing than caching the context directly, even though it currently feels to me a bit that we're caching "the first object we can find that has a name" ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-karaf/pull/65#issuecomment-99301693