brooklyn-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rich...@apache.org
Subject [01/16] git commit: Fix MultiLoctionResolver leak
Date Tue, 27 May 2014 11:05:17 GMT
Repository: incubator-brooklyn
Updated Branches:
  refs/heads/feature/policy-persistence d68fac5f9 -> 3b195189d
  refs/heads/master 497399d6a -> 02bf201b6


Fix MultiLoctionResolver leak

- on resolve, was creating sub-locs; these weren’t being cleaned up
  on unmanage(loc) so BasicLocationRegistry.resolveForPeeking failed


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/1be837e0
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/1be837e0
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/1be837e0

Branch: refs/heads/master
Commit: 1be837e03635b4c7d25699ee668909741167059c
Parents: 48df48a
Author: Aled Sage <aled.sage@gmail.com>
Authored: Thu May 22 08:33:47 2014 +0100
Committer: Aled Sage <aled.sage@gmail.com>
Committed: Mon May 26 22:09:29 2014 +0100

----------------------------------------------------------------------
 .../java/brooklyn/entity/basic/Entities.java    | 12 ++++
 .../location/basic/AbstractLocation.java        |  3 +-
 .../location/basic/LocationInternal.java        |  7 +-
 .../location/basic/LocationPredicates.java      | 69 +++++++++++++++++++
 .../location/basic/MultiLocationResolver.java   | 71 ++++++++++++++------
 .../location/basic/LocationPredicatesTest.java  | 68 +++++++++++++++++++
 .../basic/MultiLocationResolverTest.java        | 11 +++
 7 files changed, 216 insertions(+), 25 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1be837e0/core/src/main/java/brooklyn/entity/basic/Entities.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/entity/basic/Entities.java b/core/src/main/java/brooklyn/entity/basic/Entities.java
index 486c79d..2ba6a23 100644
--- a/core/src/main/java/brooklyn/entity/basic/Entities.java
+++ b/core/src/main/java/brooklyn/entity/basic/Entities.java
@@ -742,6 +742,18 @@ public class Entities {
         };
     }
     
+    public static boolean isManaged(Location loc) {
+        ManagementContext mgmt = ((LocationInternal)loc).getManagementContext();
+        return (mgmt != null) && mgmt.isRunning() && mgmt.getLocationManager().isManaged(loc);
+    }
+
+    public static void unmanage(Location loc) {
+        if (isManaged(loc)) {
+            ManagementContext mgmt = ((LocationInternal)loc).getManagementContext();
+            mgmt.getLocationManager().unmanage(loc);
+        }
+    }
+
     /**
      * Registers the given location (and all its children) with the management context. 
      * @throws IllegalStateException if the parent location is not already managed

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1be837e0/core/src/main/java/brooklyn/location/basic/AbstractLocation.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/basic/AbstractLocation.java b/core/src/main/java/brooklyn/location/basic/AbstractLocation.java
index e48f5fa..0e99844 100644
--- a/core/src/main/java/brooklyn/location/basic/AbstractLocation.java
+++ b/core/src/main/java/brooklyn/location/basic/AbstractLocation.java
@@ -199,7 +199,8 @@ public abstract class AbstractLocation implements LocationInternal, HasHostGeoIn
             configBag.putAll(oldConfig);
         }
     }
-    
+
+    @Override
     public ManagementContext getManagementContext() {
         return managementContext;
     }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1be837e0/core/src/main/java/brooklyn/location/basic/LocationInternal.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/basic/LocationInternal.java b/core/src/main/java/brooklyn/location/basic/LocationInternal.java
index ee58df7..ef8d7a2 100644
--- a/core/src/main/java/brooklyn/location/basic/LocationInternal.java
+++ b/core/src/main/java/brooklyn/location/basic/LocationInternal.java
@@ -2,16 +2,17 @@ package brooklyn.location.basic;
 
 import java.util.Map;
 
-import com.google.common.annotations.Beta;
-
 import brooklyn.config.ConfigKey;
 import brooklyn.entity.basic.ConfigKeys;
 import brooklyn.entity.rebind.RebindSupport;
 import brooklyn.entity.rebind.Rebindable;
 import brooklyn.location.Location;
+import brooklyn.management.ManagementContext;
 import brooklyn.mementos.LocationMemento;
 import brooklyn.util.config.ConfigBag;
 
+import com.google.common.annotations.Beta;
+
 /**
  * Information about locations private to Brooklyn.
  */
@@ -50,4 +51,6 @@ public interface LocationInternal extends Location, Rebindable {
 
     @Override
     RebindSupport<LocationMemento> getRebindSupport();
+    
+    ManagementContext getManagementContext();
 }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1be837e0/core/src/main/java/brooklyn/location/basic/LocationPredicates.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/basic/LocationPredicates.java b/core/src/main/java/brooklyn/location/basic/LocationPredicates.java
new file mode 100644
index 0000000..572ec53
--- /dev/null
+++ b/core/src/main/java/brooklyn/location/basic/LocationPredicates.java
@@ -0,0 +1,69 @@
+package brooklyn.location.basic;
+
+import javax.annotation.Nullable;
+
+import brooklyn.config.ConfigKey;
+import brooklyn.config.ConfigKey.HasConfigKey;
+import brooklyn.entity.basic.Entities;
+import brooklyn.location.Location;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Predicate;
+
+public class LocationPredicates {
+
+    public static <T> Predicate<Location> idEqualTo(final T val) {
+        return new Predicate<Location>() {
+            @Override
+            public boolean apply(@Nullable Location input) {
+                return Objects.equal(input.getId(), val);
+            }
+        };
+    }
+    
+    public static <T> Predicate<Location> displayNameEqualTo(final T val) {
+        return new Predicate<Location>() {
+            @Override
+            public boolean apply(@Nullable Location input) {
+                return Objects.equal(input.getDisplayName(), val);
+            }
+        };
+    }
+    
+    public static <T> Predicate<Location> configEqualTo(final ConfigKey<T>
configKey, final T val) {
+        return new Predicate<Location>() {
+            @Override
+            public boolean apply(Location input) {
+                return Objects.equal(input.getConfig(configKey), val);
+            }
+        };
+    }
+
+    public static <T> Predicate<Location> configEqualTo(final HasConfigKey<T>
configKey, final T val) {
+        return new Predicate<Location>() {
+            @Override
+            public boolean apply(Location input) {
+                return Objects.equal(input.getConfig(configKey), val);
+            }
+        };
+    }
+
+    public static <T> Predicate<Location> isChildOf(final Location parent) {
+        return new Predicate<Location>() {
+            @Override
+            public boolean apply(@Nullable Location input) {
+                return Objects.equal(input.getParent(), parent);
+            }
+        };
+    }
+
+    public static <T> Predicate<Location> managed() {
+        return new Predicate<Location>() {
+            @Override
+            public boolean apply(@Nullable Location input) {
+                return input != null && Entities.isManaged(input);
+            }
+        };
+    }
+
+}

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1be837e0/core/src/main/java/brooklyn/location/basic/MultiLocationResolver.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/basic/MultiLocationResolver.java b/core/src/main/java/brooklyn/location/basic/MultiLocationResolver.java
index d0ffed7..de9a964 100644
--- a/core/src/main/java/brooklyn/location/basic/MultiLocationResolver.java
+++ b/core/src/main/java/brooklyn/location/basic/MultiLocationResolver.java
@@ -8,19 +8,27 @@ import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import brooklyn.entity.basic.AbstractEntity;
+import brooklyn.entity.basic.Entities;
 import brooklyn.location.Location;
 import brooklyn.location.LocationRegistry;
 import brooklyn.location.LocationResolver;
 import brooklyn.location.LocationSpec;
 import brooklyn.management.ManagementContext;
 import brooklyn.util.collections.MutableMap;
+import brooklyn.util.exceptions.Exceptions;
 import brooklyn.util.text.KeyValueParser;
 import brooklyn.util.text.StringEscapes.JavaStringEscapes;
 
 import com.google.common.collect.Lists;
 
 public class MultiLocationResolver implements LocationResolver {
-    
+
+    private static final Logger LOG = LoggerFactory.getLogger(MultiLocationResolver.class);
+
     private static final String MULTI = "multi";
     
     private static final Pattern PATTERN = Pattern.compile("(" + MULTI + "|" + MULTI.toUpperCase()
+ ")" + ":" + "\\((.*)\\)$");
@@ -65,29 +73,48 @@ public class MultiLocationResolver implements LocationResolver {
         // TODO do we need to pass location flags etc into the children to ensure they are
inherited?
         List<Location> targets = Lists.newArrayList();
         Object targetSpecs = locationArgs.remove("targets");
-        if (targetSpecs instanceof String) {
-            for (String targetSpec : JavaStringEscapes.unwrapJsonishListIfPossible((String)targetSpecs))
{
-                targets.add(managementContext.getLocationRegistry().resolve(targetSpec));
-            }
-        } else if (targetSpecs instanceof Iterable) {
-            for (Object targetSpec: (Iterable<?>)targetSpecs) {
-                if (targetSpec instanceof String) {
-                    targets.add(managementContext.getLocationRegistry().resolve((String)targetSpec));
-                } else {
-                    Set<?> keys = ((Map<?,?>)targetSpec).keySet();
-                    if (keys.size()!=1) 
-                        throw new IllegalArgumentException("targets supplied to MultiLocation
must be a list of single-entry maps (got map of size "+keys.size()+": "+targetSpec+")");
-                    Object key = keys.iterator().next();
-                    Object flagsS = ((Map<?,?>)targetSpec).get(key);
-                    targets.add(managementContext.getLocationRegistry().resolve((String)key,
(Map<?,?>)flagsS));
+        try {
+            if (targetSpecs instanceof String) {
+                for (String targetSpec : JavaStringEscapes.unwrapJsonishListIfPossible((String)targetSpecs))
{
+                    targets.add(managementContext.getLocationRegistry().resolve(targetSpec));
+                }
+            } else if (targetSpecs instanceof Iterable) {
+                for (Object targetSpec: (Iterable<?>)targetSpecs) {
+                    if (targetSpec instanceof String) {
+                        targets.add(managementContext.getLocationRegistry().resolve((String)targetSpec));
+                    } else {
+                        Set<?> keys = ((Map<?,?>)targetSpec).keySet();
+                        if (keys.size()!=1) 
+                            throw new IllegalArgumentException("targets supplied to MultiLocation
must be a list of single-entry maps (got map of size "+keys.size()+": "+targetSpec+")");
+                        Object key = keys.iterator().next();
+                        Object flagsS = ((Map<?,?>)targetSpec).get(key);
+                        targets.add(managementContext.getLocationRegistry().resolve((String)key,
(Map<?,?>)flagsS));
+                    }
                 }
+            } else throw new IllegalArgumentException("targets must be supplied to MultiLocation,
either as string spec or list of single-entry maps each being a location spec");
+            
+            MultiLocation result = managementContext.getLocationManager().createLocation(LocationSpec.create(MultiLocation.class)
+                    .configure(flags)
+                    .configure("subLocations", targets)
+                    .configure(LocationConfigUtils.finalAndOriginalSpecs(spec, locationFlags,
globalProperties, namedLocation)));
+
+            // TODO Important workaround for BasicLocationRegistry.resolveForPeeking.
+            // That creates a location (from the resolver) and immediately unmanages it.
+            // The unmanage *must* remove all the targets created here; otherwise we have
a leak.
+            // Adding the targets as children achieves this.
+            for (Location target : targets) {
+                target.setParent(result);
             }
-        } else throw new IllegalArgumentException("targets must be supplied to MultiLocation,
either as string spec or list of single-entry maps each being a location spec");
-        
-        return managementContext.getLocationManager().createLocation(LocationSpec.create(MultiLocation.class)
-                .configure(flags)
-                .configure("subLocations", targets)
-                .configure(LocationConfigUtils.finalAndOriginalSpecs(spec, locationFlags,
globalProperties, namedLocation)));
+            return result;
+
+        } catch (Exception e) {
+            // Must clean up after ourselves: don't leak sub-locations on error
+            if (LOG.isDebugEnabled()) LOG.debug("Problem resolving MultiLocation; cleaning
up any sub-locations and rethrowing: "+e);
+            for (Location target : targets) {
+                Entities.unmanage(target);
+            }
+            throw Exceptions.propagate(e);
+        }
     }
 
     @Override

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1be837e0/core/src/test/java/brooklyn/location/basic/LocationPredicatesTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/location/basic/LocationPredicatesTest.java b/core/src/test/java/brooklyn/location/basic/LocationPredicatesTest.java
new file mode 100644
index 0000000..287adc0
--- /dev/null
+++ b/core/src/test/java/brooklyn/location/basic/LocationPredicatesTest.java
@@ -0,0 +1,68 @@
+package brooklyn.location.basic;
+
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import brooklyn.entity.basic.Entities;
+import brooklyn.management.internal.LocalManagementContext;
+import brooklyn.test.entity.TestEntity;
+
+public class LocationPredicatesTest {
+
+    private LocalManagementContext managementContext;
+    private LocalhostMachineProvisioningLocation loc;
+    private SshMachineLocation childLoc;
+    
+    @BeforeMethod(alwaysRun=true)
+    public void setUp() throws Exception {
+        managementContext = new LocalManagementContext();
+        loc = (LocalhostMachineProvisioningLocation) managementContext.getLocationRegistry().resolve("localhost:(name=mydisplayname)");
+        childLoc = loc.obtain();
+    }
+
+    @AfterMethod(alwaysRun=true)
+    public void tearDown() throws Exception {
+        if (managementContext != null) Entities.destroyAll(managementContext);
+    }
+    
+    @Test
+    public void testIdEqualTo() throws Exception {
+        assertTrue(LocationPredicates.idEqualTo(loc.getId()).apply(loc));
+        assertFalse(LocationPredicates.idEqualTo("wrongid").apply(loc));
+    }
+    
+    @Test
+    public void testConfigEqualTo() throws Exception {
+        loc.setConfig(TestEntity.CONF_NAME, "myname");
+        assertTrue(LocationPredicates.configEqualTo(TestEntity.CONF_NAME, "myname").apply(loc));
+        assertFalse(LocationPredicates.configEqualTo(TestEntity.CONF_NAME, "wrongname").apply(loc));
+    }
+    
+    @Test
+    public void testDisplayNameEqualTo() throws Exception {
+        assertTrue(LocationPredicates.displayNameEqualTo("mydisplayname").apply(loc));
+        assertFalse(LocationPredicates.displayNameEqualTo("wrongname").apply(loc));
+    }
+    
+    @Test
+    public void testIsChildOf() throws Exception {
+        assertTrue(LocationPredicates.isChildOf(loc).apply(childLoc));
+        assertFalse(LocationPredicates.isChildOf(loc).apply(loc));
+        assertFalse(LocationPredicates.isChildOf(childLoc).apply(loc));
+    }
+    
+    @Test
+    public void testManaged() throws Exception {
+        // TODO get exception in LocalhostMachineProvisioningLocation.removeChild because
childLoc is "in use";
+        // this happens from the call to unmanage(loc), which first unmanaged the children.
+        loc.release(childLoc);
+        
+        assertTrue(LocationPredicates.managed().apply(loc));
+        Entities.unmanage(loc);
+        assertFalse(LocationPredicates.managed().apply(loc));
+    }
+}

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1be837e0/core/src/test/java/brooklyn/location/basic/MultiLocationResolverTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/location/basic/MultiLocationResolverTest.java b/core/src/test/java/brooklyn/location/basic/MultiLocationResolverTest.java
index 075cab1..841c174 100644
--- a/core/src/test/java/brooklyn/location/basic/MultiLocationResolverTest.java
+++ b/core/src/test/java/brooklyn/location/basic/MultiLocationResolverTest.java
@@ -1,6 +1,7 @@
 package brooklyn.location.basic;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertTrue;
 import static org.testng.Assert.fail;
 
@@ -30,10 +31,12 @@ import brooklyn.util.collections.MutableMap;
 import brooklyn.util.exceptions.Exceptions;
 
 import com.google.common.base.Objects;
+import com.google.common.base.Optional;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 
 public class MultiLocationResolverTest {
 
@@ -65,6 +68,14 @@ public class MultiLocationResolverTest {
     }
 
     @Test
+    public void testCleansUpOnInvalidTarget() {
+        assertThrowsNoSuchElement("multi:(targets=\"localhost:(name=testCleansUpOnInvalidTarget),thisNamedLocationDoesNotExist\")");
+        Optional<Location> subtarget = Iterables.tryFind(managementContext.getLocationManager().getLocations(),
LocationPredicates.displayNameEqualTo("testCleansUpOnInvalidTarget"));
+        assertFalse(subtarget.isPresent(), "subtarget="+subtarget);
+    }
+
+
+    @Test
     public void testResolvesSubLocs() {
         assertMultiLocation(resolve("multi:(targets=localhost)"), 1, ImmutableList.of(Predicates.instanceOf(LocalhostMachineProvisioningLocation.class)));
         assertMultiLocation(resolve("multi:(targets=\"localhost,localhost\")"), 2, Collections.nCopies(2,
Predicates.instanceOf(LocalhostMachineProvisioningLocation.class)));


Mime
View raw message