You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2016/03/17 16:18:31 UTC
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-server/pull/70
fix location leak - use LocationSpec wherever possible
This improves how locations are handled internally, switching the resolvers to return `LocationSpec` rather than `Location` instances which may or may not be managed.
This allows us to fix a leak if catalog items include a location block (and includes a test for that, /cc @bostko ). It also simplifies a number of things internally. There is quite a bit more we'd like to do at some point (/cc @aledsage):
* have locations come entirely from the `TypeRegistry` (ie the catalog), doing away with `LocationDefinition` and `LocationRegistry`
* treat `Location` instances *as* `Entity` instances
However those are bigger changes, and this PR addresses the immediate pain points and makes the code quite a bit tidier.
There are a huge number of tests changed so to facilitate review the main important commits are:
* https://github.com/apache/brooklyn-server/commit/2876a9e5884332c38cdd2f631d0d221de2de35c9 pass location *specs* to entity spec (preferred over location instances)
* https://github.com/apache/brooklyn-server/commit/f755c25d2e66fa93a4a5f9f29ab8edc8d1c5be48 resolvers create location *specs* instead of location instances -- this breaks Resolver compatibility, mostly pretty easy to fix
* https://github.com/apache/brooklyn-server/commit/a3d74701fc37ae9b3daae15865510dfb1b94e327 special flag where a spec needs to do something special to create the instance
(beta, used only for PortForwardManager)
* https://github.com/apache/brooklyn-server/commit/a75e8f7d1b02a8e094f631a82e1cc6ddc4340fec add catalog info to the object returned via the legacy `/v1/locations` endpoint to facilitate switching to `/v1/catalog/locations` in future, and notes
Builds on #62.
Note also that this breaks backwards compatibility if someone *implements* `LocationResolver`. A note should be added in the release notes when this is merged.
/cc @grkvlt does clocker provide a `LocationResolver`? if so it will need changed to return a `LocationSpec` not a `Location` instance. if it is not a straightforward change ping me and I can help; this PR adds a "special constructor" facility which might apply if a `LocationSpec` doesn't work there
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server fix-location-leak
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/70.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #70
----
commit d332bcc4d60868736716990660f388c648dd8546
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-14T16:53:54Z
does not create a localhost named location any longer. the wizard (coming soon) will do this for us.
commit bcf4d9b8abb5a1bc79fe52b295dde6e16beadb7b
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-14T19:12:11Z
Merge branch 'master' into no-localhost-default
commit 40fd76d8124b4f570082f72d3c576b2db05dedf1
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-15T12:10:55Z
add more tests for the number of locations being managed
including a failing test for the case where a blueprint added to catalog includes a location
commit 515af88879f541e28c907237c7c28a817d0113d0
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-15T12:16:32Z
misc deprecation updates
commit 2876a9e5884332c38cdd2f631d0d221de2de35c9
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-15T13:27:35Z
support location spec on entity spec, and deprecate methods which take concrete objects
commit d2c5cc81d1ac784f21c5aa0ebff365c0da6b85b6
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-15T13:28:15Z
update most tests to pass specs when configuring entity spec
commit f755c25d2e66fa93a4a5f9f29ab8edc8d1c5be48
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-16T20:43:56Z
first pass rewriting LocationRegistry and resolvers to return specs
commit 3f1e4c61780e67c972ad6ba4785ed5537bbe9108
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-16T20:44:31Z
update tests to use LocationRegistry spec methods
the CatalogYamlLocationTest.testLocationPartOfBlueprintDoesntLeak recently introduced to highlight the failure is now fixed,
although there are still port-manager-related failures
commit a3d74701fc37ae9b3daae15865510dfb1b94e327
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-17T11:18:22Z
allow a special constructor to be given to specs (beta, needed for PortForwardManager)
this fixes the port-manager-related failures
commit a75e8f7d1b02a8e094f631a82e1cc6ddc4340fec
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-17T15:09:19Z
expand REST API to include catalog info, and update REST API for cleaned up LocationSpec access
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/70#discussion_r56633147
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/proxy/SpecialBrooklynObjectConstructor.java ---
@@ -0,0 +1,80 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.core.objs.proxy;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec;
+import org.apache.brooklyn.api.location.Location;
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+
+import com.google.common.annotations.Beta;
+import com.google.common.reflect.TypeToken;
+
+/** Marker interface for use when a spec needs something special to create the object under management.
+ * Set the {@link Config#SPECIAL_CONSTRUCTOR} config key in the spec pointing to the class;
+ * it will be instantiated and {@link #create(Class, AbstractBrooklynObjectSpec)} invoked to create the class.
+ * The type of course must be consistent with the spec.
+ * <p>
+ * This entire mechanism is beta; it is introduced for a few special cases where some type of dynamicism is currently required.
+ * In most cases the {@link AbstractBrooklynObjectSpec} should correspond directly to the object created.
+ * Some use cases are:
+ * <li>PortForwarderManager, shared global record
+ * <li>Choosing a different entity type smartly based on the location (e.g. deploying to PaaS vs IaaS)
+ * <li>Referencing an existing location (optionally, and there are MAJOR issues with this in general; see below)
+ * <li>Clocker (?) - newly defined docker locations (although these may all point at entities)
+ * <p>
+ * See implementations of this interface.
+ * <p>
+ * Note that the caller may typically do additional configuration on the resulting object,
+ * such as setting a catalog item ID etc. That behaviour may merit adjustment and is subject to change.
+ * <p>
+ * Where this returns an existing item we must be VERY careful around *unmanaging*,
+ * both when this reference is unmanaged (but the original is still in use)
+ * and when the target is unmanaged (but this reference is still in use).
+ * <p>
+ * Some possible improvements to the simple approach here are:
+ * <li>Return a delegate impl of the {@link Location} or {@link Entity} interface pointing at the target.
+ * (This would work fine for sensors/effectors but it would fail any java type checks or casts.
+ * We could switch to a brooklyn-managed type system, which would also allow for explicit yaml inheritance,
+ * but that's not a small task.)
+ * <li>Return a java reflections proxy impl (we have this for entity already;
+ * but for many Location types interfaces are not currently defined;
+ * adding them would be nice to make Locations more like Entities)
+ * <li>Require the appropriate {@link Entity} or {@link Location} type, on a case-by-case basis,
+ * where the lifecycle is as normal, and the logic for initializing and delegating is done
+ * in the init. (Possibly this is better than the current implementation!? It is certainly
+ * simpler than allowing multiple links to existing items, and would be feasible in many
+ * cases, e.g. a `same-machine-as` type (assuming we made SshMachineEntity an interface);
+ * but it would not allow choosing an entity type smartly based on location.)
+ */
+@Beta
+public interface SpecialBrooklynObjectConstructor {
--- End diff --
Yep - when we better understand how this should be used I agree. For now I thought something lightweight, and beta, is better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/70#discussion_r56623807
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocationManagerInternal.java ---
@@ -19,10 +19,18 @@
package org.apache.brooklyn.core.mgmt.internal;
import org.apache.brooklyn.api.location.Location;
+import org.apache.brooklyn.api.location.LocationSpec;
import org.apache.brooklyn.api.mgmt.LocationManager;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
public interface LocationManagerInternal extends LocationManager, BrooklynObjectManagerInternal<Location> {
+ /** Indicates that a call to {@link #createLocation(LocationSpec)} should create a location instance not
+ * linked to the management context. This allows clients to peek at the result but not to use it. */
+ public static final ConfigKey<Boolean> CREATE_UNMANAGED = ConfigKeys.newBooleanConfigKey("brooklyn.internal.location.createUnmanaged",
--- End diff --
Do we still need this, after introducing the spec resolver?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/70#discussion_r56623544
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/proxy/SpecialBrooklynObjectConstructor.java ---
@@ -0,0 +1,80 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.core.objs.proxy;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec;
+import org.apache.brooklyn.api.location.Location;
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+
+import com.google.common.annotations.Beta;
+import com.google.common.reflect.TypeToken;
+
+/** Marker interface for use when a spec needs something special to create the object under management.
+ * Set the {@link Config#SPECIAL_CONSTRUCTOR} config key in the spec pointing to the class;
+ * it will be instantiated and {@link #create(Class, AbstractBrooklynObjectSpec)} invoked to create the class.
+ * The type of course must be consistent with the spec.
+ * <p>
+ * This entire mechanism is beta; it is introduced for a few special cases where some type of dynamicism is currently required.
+ * In most cases the {@link AbstractBrooklynObjectSpec} should correspond directly to the object created.
+ * Some use cases are:
+ * <li>PortForwarderManager, shared global record
+ * <li>Choosing a different entity type smartly based on the location (e.g. deploying to PaaS vs IaaS)
+ * <li>Referencing an existing location (optionally, and there are MAJOR issues with this in general; see below)
+ * <li>Clocker (?) - newly defined docker locations (although these may all point at entities)
+ * <p>
+ * See implementations of this interface.
+ * <p>
+ * Note that the caller may typically do additional configuration on the resulting object,
+ * such as setting a catalog item ID etc. That behaviour may merit adjustment and is subject to change.
+ * <p>
+ * Where this returns an existing item we must be VERY careful around *unmanaging*,
+ * both when this reference is unmanaged (but the original is still in use)
+ * and when the target is unmanaged (but this reference is still in use).
+ * <p>
+ * Some possible improvements to the simple approach here are:
+ * <li>Return a delegate impl of the {@link Location} or {@link Entity} interface pointing at the target.
+ * (This would work fine for sensors/effectors but it would fail any java type checks or casts.
+ * We could switch to a brooklyn-managed type system, which would also allow for explicit yaml inheritance,
+ * but that's not a small task.)
+ * <li>Return a java reflections proxy impl (we have this for entity already;
+ * but for many Location types interfaces are not currently defined;
+ * adding them would be nice to make Locations more like Entities)
+ * <li>Require the appropriate {@link Entity} or {@link Location} type, on a case-by-case basis,
+ * where the lifecycle is as normal, and the logic for initializing and delegating is done
+ * in the init. (Possibly this is better than the current implementation!? It is certainly
+ * simpler than allowing multiple links to existing items, and would be feasible in many
+ * cases, e.g. a `same-machine-as` type (assuming we made SshMachineEntity an interface);
+ * but it would not allow choosing an entity type smartly based on location.)
+ */
+@Beta
+public interface SpecialBrooklynObjectConstructor {
--- End diff --
Don't like `Special` in the name. This is essentially a factory for the BrooklynObject instances - could just call it `BrooklynObjectFactory` (or some derivate including spec in the name). Same for the key.
Also deserves an entry in the spec builder, similar to `.impl`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/70#discussion_r56633321
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocationManagerInternal.java ---
@@ -19,10 +19,18 @@
package org.apache.brooklyn.core.mgmt.internal;
import org.apache.brooklyn.api.location.Location;
+import org.apache.brooklyn.api.location.LocationSpec;
import org.apache.brooklyn.api.mgmt.LocationManager;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
public interface LocationManagerInternal extends LocationManager, BrooklynObjectManagerInternal<Location> {
+ /** Indicates that a call to {@link #createLocation(LocationSpec)} should create a location instance not
+ * linked to the management context. This allows clients to peek at the result but not to use it. */
+ public static final ConfigKey<Boolean> CREATE_UNMANAGED = ConfigKeys.newBooleanConfigKey("brooklyn.internal.location.createUnmanaged",
--- End diff --
Yes we do in a few places, and probably will always, because some info is only available on the Location, not on the LocationSpec, and callers may want to know that. (I suppose those places could unmanage it explicitly but why the overhead.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/70#issuecomment-197996668
looks like generated code has changed; will push update shortly.
it should be reviewable for the most part without that
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/brooklyn-server/pull/70#issuecomment-198262951
Great set of changes. Good to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/70#discussion_r56633005
--- Diff: api/src/main/java/org/apache/brooklyn/api/location/LocationRegistry.java ---
@@ -86,25 +94,62 @@
public boolean canMaybeResolve(String spec);
/** As {@link #resolve(String, Boolean, Map)}, but unwrapped
- * @throws NoSuchElementException if the spec cannot be resolved */
+ * @throws NoSuchElementException if the spec cannot be resolved
+ * @deprecated since 0.9.0 use {@link #getLocationSpec(String, Map)} and then manage it as needed*/
+ @SuppressWarnings("rawtypes")
+ @Deprecated
public Location resolve(String spec, @Nullable Map locationFlags);
/**
- * As {@link #resolve(String)} but takes collections (of strings or locations)
+ * As {@link #getLocationManaged(String)} but takes collections (of strings or locations)
* <p>
* Expects a collection of elements being individual location spec strings or locations,
* and returns a list of resolved (newly created and managed) locations.
* <p>
* From 0.7.0 this no longer flattens lists (nested lists are disallowed)
* or parses comma-separated elements (they are resolved as-is)
- */
+ * @deprecated since 0.9.0 use {@link #getListOfLocationsManaged(Object)} */
+ @Deprecated
public List<Location> resolve(Iterable<?> spec);
/** Takes a string, interpreted as a comma-separated (or JSON style, when you need internal double quotes or commas) list;
* or a list, passed to {@link #resolve(Iterable)}; or null/empty (empty list),
- * and returns a list of resolved (created and managed) locations */
+ * and returns a list of resolved (created and managed) locations
+ * @deprecated since 0.9.0 use {@link #getListOfLocationsManaged(Object)} */
+ @Deprecated
public List<Location> resolveList(Object specList);
+
+ /** Create a {@link LocationSpec} representing the given spec string such as a named location
+ * or using a resolver prefix such as jclouds:aws-ec2.
+ * This can then be inspected, assigned to an {@link EntitySpec},
+ * or passed to {@link LocationManager#createLocation(LocationSpec)} to create directly.
+ * (For that last case, common in tests, see {@link #getLocationManaged(String)}.) */
+ public Maybe<LocationSpec<? extends Location>> getLocationSpec(String spec);
+ /** As {@link #getLocationSpec(String)} but also setting the given flags configured on the resulting spec. */
+ public Maybe<LocationSpec<? extends Location>> getLocationSpec(String spec, Map<?,?> locationFlags);
+
+ /** As {@link #getLocationSpec(String)} where the caller has a {@link LocationDefinition}. */
+ public Maybe<LocationSpec<? extends Location>> getLocationSpec(LocationDefinition ld);
+ /** As {@link #getLocationSpec(String,Map)} where the caller has a {@link LocationDefinition}. */
+ public Maybe<LocationSpec<? extends Location>> getLocationSpec(LocationDefinition ld, Map<?,?> locationFlags);
+ /** A combination of {@link #getLocationSpec(String)} then {@link LocationManager#createLocation(LocationSpec)},
+ * mainly for use in tests or specialised situations where a managed location is needed directly.
+ * The caller is responsible for ensuring that the resulting {@link Location}
+ * is cleaned up, ie removed from management via {@link LocationManager#unmanage(Location)} directly or linking it to
+ * an {@link Entity} or another {@link Location} which will unmanage it. */
+ public Location getLocationManaged(String spec);
--- End diff --
I thought so too at first but it is otherwise an ugly syntax to create location from spec string (get spec object then pass to location manager) and hundreds of places it should sit. I think longer term this method should sit on the `TypeRegistry` as a convenience so defer that sweeping change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/70#issuecomment-197930928
(That can be a separate PR.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/70#issuecomment-198008891
fixed, ready for review
very good if this, and the ui feature it enables https://github.com/apache/brooklyn-ui/issues/18 can make it into 090 /cc @rdowner
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/70#discussion_r56661929
--- Diff: api/src/main/java/org/apache/brooklyn/api/location/LocationRegistry.java ---
@@ -86,25 +94,62 @@
public boolean canMaybeResolve(String spec);
/** As {@link #resolve(String, Boolean, Map)}, but unwrapped
- * @throws NoSuchElementException if the spec cannot be resolved */
+ * @throws NoSuchElementException if the spec cannot be resolved
+ * @deprecated since 0.9.0 use {@link #getLocationSpec(String, Map)} and then manage it as needed*/
+ @SuppressWarnings("rawtypes")
+ @Deprecated
public Location resolve(String spec, @Nullable Map locationFlags);
/**
- * As {@link #resolve(String)} but takes collections (of strings or locations)
+ * As {@link #getLocationManaged(String)} but takes collections (of strings or locations)
* <p>
* Expects a collection of elements being individual location spec strings or locations,
* and returns a list of resolved (newly created and managed) locations.
* <p>
* From 0.7.0 this no longer flattens lists (nested lists are disallowed)
* or parses comma-separated elements (they are resolved as-is)
- */
+ * @deprecated since 0.9.0 use {@link #getListOfLocationsManaged(Object)} */
+ @Deprecated
public List<Location> resolve(Iterable<?> spec);
/** Takes a string, interpreted as a comma-separated (or JSON style, when you need internal double quotes or commas) list;
* or a list, passed to {@link #resolve(Iterable)}; or null/empty (empty list),
- * and returns a list of resolved (created and managed) locations */
+ * and returns a list of resolved (created and managed) locations
+ * @deprecated since 0.9.0 use {@link #getListOfLocationsManaged(Object)} */
+ @Deprecated
--- End diff --
Nice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/70
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/70#issuecomment-198284683
great comments - thanks. will merge as soon as https://github.com/apache/brooklyn-ui/pull/17 lands.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/70#discussion_r56625151
--- Diff: api/src/main/java/org/apache/brooklyn/api/location/LocationRegistry.java ---
@@ -86,25 +94,62 @@
public boolean canMaybeResolve(String spec);
/** As {@link #resolve(String, Boolean, Map)}, but unwrapped
- * @throws NoSuchElementException if the spec cannot be resolved */
+ * @throws NoSuchElementException if the spec cannot be resolved
+ * @deprecated since 0.9.0 use {@link #getLocationSpec(String, Map)} and then manage it as needed*/
+ @SuppressWarnings("rawtypes")
+ @Deprecated
public Location resolve(String spec, @Nullable Map locationFlags);
/**
- * As {@link #resolve(String)} but takes collections (of strings or locations)
+ * As {@link #getLocationManaged(String)} but takes collections (of strings or locations)
* <p>
* Expects a collection of elements being individual location spec strings or locations,
* and returns a list of resolved (newly created and managed) locations.
* <p>
* From 0.7.0 this no longer flattens lists (nested lists are disallowed)
* or parses comma-separated elements (they are resolved as-is)
- */
+ * @deprecated since 0.9.0 use {@link #getListOfLocationsManaged(Object)} */
+ @Deprecated
public List<Location> resolve(Iterable<?> spec);
/** Takes a string, interpreted as a comma-separated (or JSON style, when you need internal double quotes or commas) list;
* or a list, passed to {@link #resolve(Iterable)}; or null/empty (empty list),
- * and returns a list of resolved (created and managed) locations */
+ * and returns a list of resolved (created and managed) locations
+ * @deprecated since 0.9.0 use {@link #getListOfLocationsManaged(Object)} */
+ @Deprecated
public List<Location> resolveList(Object specList);
+
+ /** Create a {@link LocationSpec} representing the given spec string such as a named location
+ * or using a resolver prefix such as jclouds:aws-ec2.
+ * This can then be inspected, assigned to an {@link EntitySpec},
+ * or passed to {@link LocationManager#createLocation(LocationSpec)} to create directly.
+ * (For that last case, common in tests, see {@link #getLocationManaged(String)}.) */
+ public Maybe<LocationSpec<? extends Location>> getLocationSpec(String spec);
+ /** As {@link #getLocationSpec(String)} but also setting the given flags configured on the resulting spec. */
+ public Maybe<LocationSpec<? extends Location>> getLocationSpec(String spec, Map<?,?> locationFlags);
+
+ /** As {@link #getLocationSpec(String)} where the caller has a {@link LocationDefinition}. */
+ public Maybe<LocationSpec<? extends Location>> getLocationSpec(LocationDefinition ld);
+ /** As {@link #getLocationSpec(String,Map)} where the caller has a {@link LocationDefinition}. */
+ public Maybe<LocationSpec<? extends Location>> getLocationSpec(LocationDefinition ld, Map<?,?> locationFlags);
+ /** A combination of {@link #getLocationSpec(String)} then {@link LocationManager#createLocation(LocationSpec)},
+ * mainly for use in tests or specialised situations where a managed location is needed directly.
+ * The caller is responsible for ensuring that the resulting {@link Location}
+ * is cleaned up, ie removed from management via {@link LocationManager#unmanage(Location)} directly or linking it to
+ * an {@link Entity} or another {@link Location} which will unmanage it. */
+ public Location getLocationManaged(String spec);
--- End diff --
Personal preference to not mixing responsibilities. Now that this API is dealing with specs better leave it to utility classes to do the delegation to `LocationManager`. Same for following methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin closed the pull request at:
https://github.com/apache/brooklyn-server/pull/70
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/70#discussion_r56624484
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/proxy/SpecialBrooklynObjectConstructor.java ---
@@ -0,0 +1,80 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.core.objs.proxy;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec;
+import org.apache.brooklyn.api.location.Location;
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+
+import com.google.common.annotations.Beta;
+import com.google.common.reflect.TypeToken;
+
+/** Marker interface for use when a spec needs something special to create the object under management.
+ * Set the {@link Config#SPECIAL_CONSTRUCTOR} config key in the spec pointing to the class;
+ * it will be instantiated and {@link #create(Class, AbstractBrooklynObjectSpec)} invoked to create the class.
+ * The type of course must be consistent with the spec.
+ * <p>
+ * This entire mechanism is beta; it is introduced for a few special cases where some type of dynamicism is currently required.
+ * In most cases the {@link AbstractBrooklynObjectSpec} should correspond directly to the object created.
+ * Some use cases are:
+ * <li>PortForwarderManager, shared global record
+ * <li>Choosing a different entity type smartly based on the location (e.g. deploying to PaaS vs IaaS)
+ * <li>Referencing an existing location (optionally, and there are MAJOR issues with this in general; see below)
+ * <li>Clocker (?) - newly defined docker locations (although these may all point at entities)
+ * <p>
+ * See implementations of this interface.
+ * <p>
+ * Note that the caller may typically do additional configuration on the resulting object,
+ * such as setting a catalog item ID etc. That behaviour may merit adjustment and is subject to change.
+ * <p>
+ * Where this returns an existing item we must be VERY careful around *unmanaging*,
+ * both when this reference is unmanaged (but the original is still in use)
+ * and when the target is unmanaged (but this reference is still in use).
+ * <p>
+ * Some possible improvements to the simple approach here are:
+ * <li>Return a delegate impl of the {@link Location} or {@link Entity} interface pointing at the target.
+ * (This would work fine for sensors/effectors but it would fail any java type checks or casts.
+ * We could switch to a brooklyn-managed type system, which would also allow for explicit yaml inheritance,
+ * but that's not a small task.)
+ * <li>Return a java reflections proxy impl (we have this for entity already;
+ * but for many Location types interfaces are not currently defined;
+ * adding them would be nice to make Locations more like Entities)
+ * <li>Require the appropriate {@link Entity} or {@link Location} type, on a case-by-case basis,
+ * where the lifecycle is as normal, and the logic for initializing and delegating is done
+ * in the init. (Possibly this is better than the current implementation!? It is certainly
+ * simpler than allowing multiple links to existing items, and would be feasible in many
+ * cases, e.g. a `same-machine-as` type (assuming we made SshMachineEntity an interface);
+ * but it would not allow choosing an entity type smartly based on location.)
+ */
+@Beta
+public interface SpecialBrooklynObjectConstructor {
+
+ /** Method to implement to create the given type from the given spec. */
+ public <T> T create(ManagementContext mgmt, Class<T> type, AbstractBrooklynObjectSpec<?,?> spec);
+
+ public static class Config {
+ @Beta @SuppressWarnings("serial")
+ public static ConfigKey<Class<? extends SpecialBrooklynObjectConstructor>> SPECIAL_CONSTRUCTOR =
--- End diff --
If this is to be used from java code only this would better be an object instead of the class. But no reason why not pass it in yaml, so can stay as is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/70#issuecomment-197930852
Having looked at https://github.com/apache/incubator-brooklyn/pull/1124 I think @bostko is right, this does not address a leak where a `Location` is created and passed to an `Entity` as a `start` argument.
Feels like we should do further work to accept `LocationSpec` objects -- or strings etc -- as arguments to `start(...)`, and record explicitly whether an entity is creating the locations (eg using new relationships support -- if that is persisted?). /cc @bostko @aledsage @neykov
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
GitHub user ahgittin reopened a pull request:
https://github.com/apache/brooklyn-server/pull/70
fix location leak - use LocationSpec wherever possible
This improves how locations are handled internally, switching the resolvers to return `LocationSpec` rather than `Location` instances which may or may not be managed.
This allows us to fix a leak if catalog items include a location block (and includes a test for that, /cc @bostko ). It also simplifies a number of things internally. There is quite a bit more we'd like to do at some point (/cc @aledsage):
* have locations come entirely from the `TypeRegistry` (ie the catalog), doing away with `LocationDefinition` and `LocationRegistry`
* treat `Location` instances *as* `Entity` instances
However those are bigger changes, and this PR addresses the immediate pain points and makes the code quite a bit tidier.
There are a huge number of tests changed so to facilitate review the main important commits are:
* https://github.com/apache/brooklyn-server/commit/2876a9e5884332c38cdd2f631d0d221de2de35c9 pass location *specs* to entity spec (preferred over location instances)
* https://github.com/apache/brooklyn-server/commit/f755c25d2e66fa93a4a5f9f29ab8edc8d1c5be48 resolvers create location *specs* instead of location instances -- this breaks Resolver compatibility, mostly pretty easy to fix
* https://github.com/apache/brooklyn-server/commit/a3d74701fc37ae9b3daae15865510dfb1b94e327 special flag where a spec needs to do something special to create the instance
(beta, used only for PortForwardManager)
* https://github.com/apache/brooklyn-server/commit/a75e8f7d1b02a8e094f631a82e1cc6ddc4340fec add catalog info to the object returned via the legacy `/v1/locations` endpoint to facilitate switching to `/v1/catalog/locations` in future, and notes
Builds on #62.
Note also that this breaks backwards compatibility if someone *implements* `LocationResolver`. A note should be added in the release notes when this is merged.
/cc @grkvlt does clocker provide a `LocationResolver`? if so it will need changed to return a `LocationSpec` not a `Location` instance. if it is not a straightforward change ping me and I can help; this PR adds a "special constructor" facility which might apply if a `LocationSpec` doesn't work there
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server fix-location-leak
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/70.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #70
----
commit d332bcc4d60868736716990660f388c648dd8546
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-14T16:53:54Z
does not create a localhost named location any longer. the wizard (coming soon) will do this for us.
commit bcf4d9b8abb5a1bc79fe52b295dde6e16beadb7b
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-14T19:12:11Z
Merge branch 'master' into no-localhost-default
commit 40fd76d8124b4f570082f72d3c576b2db05dedf1
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-15T12:10:55Z
add more tests for the number of locations being managed
including a failing test for the case where a blueprint added to catalog includes a location
commit 515af88879f541e28c907237c7c28a817d0113d0
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-15T12:16:32Z
misc deprecation updates
commit 2876a9e5884332c38cdd2f631d0d221de2de35c9
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-15T13:27:35Z
support location spec on entity spec, and deprecate methods which take concrete objects
commit d2c5cc81d1ac784f21c5aa0ebff365c0da6b85b6
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-15T13:28:15Z
update most tests to pass specs when configuring entity spec
commit 60f470d3b82b8f0d7fec686a0c005627910e100e
Author: Aled Sage <al...@gmail.com>
Date: 2016-03-17T06:48:35Z
Fix SshMachineLocation cleanup
Previously if a temporary location was created (i.e. using
CREATE_UNMANAGED=true) then the cleanup task would execute immediately
and would close the ssh-connections-pool (potentially while the other
thread tried to execute something).
commit f755c25d2e66fa93a4a5f9f29ab8edc8d1c5be48
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-16T20:43:56Z
first pass rewriting LocationRegistry and resolvers to return specs
commit 3f1e4c61780e67c972ad6ba4785ed5537bbe9108
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-16T20:44:31Z
update tests to use LocationRegistry spec methods
the CatalogYamlLocationTest.testLocationPartOfBlueprintDoesntLeak recently introduced to highlight the failure is now fixed,
although there are still port-manager-related failures
commit a3d74701fc37ae9b3daae15865510dfb1b94e327
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-17T11:18:22Z
allow a special constructor to be given to specs (beta, needed for PortForwardManager)
this fixes the port-manager-related failures
commit b05185eb59dcd6ba2c4f4110ca9a4557f6b8ee0e
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-17T15:09:19Z
expand REST API to include catalog info, and update REST API for cleaned up LocationSpec access
commit dfcc6dfc4fd6a6878008e1d31d2409269dce532c
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-17T18:42:10Z
Merge remote-tracking branch 'upstream/master' into fix-location-leak
commit 41a8d66db06e6b5b97b68d5890dc0a5f68980733
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-17T18:42:16Z
This closes #67
commit 138e0f66397f57779dc2eb64b14f60081aa2496d
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-03-17T18:45:09Z
Merge branch 'master' into fix-location-leak
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: fix location leak - use LocationSpec...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/70#issuecomment-198162264
new unrelated failure, not very enlightening logs:
```
2016-03-17 19:52:16,292 INFO Starting RestMockSimpleEntity{id=fh3xdiFS} on machine SshMachineLocation[localhost:null@jenkins-ubuntu-1404-4gb-560/10.176.64.103:22(id=xOhYUjcS)]
2016-03-17 19:52:26,329 INFO TESTNG FAILED: "Surefire test" - org.apache.brooklyn.rest.resources.UsageResourceTest.testGetApplicationUsage() finished in 10239 ms
java.lang.AssertionError: task ftMVXJTj not finished expected [true] but found [false]
at org.apache.brooklyn.rest.resources.UsageResourceTest.waitForTask(UsageResourceTest.java:351)
at org.apache.brooklyn.rest.resources.UsageResourceTest.createApp(UsageResourceTest.java:309)
at org.apache.brooklyn.rest.resources.UsageResourceTest.testGetApplicationUsage(UsageResourceTest.java:161)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---