Repository: brooklyn-server Updated Branches: refs/heads/master 6dafb3fae -> e601350d7
BROOKLYN-600: cleanup entities on deploy-failure Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/a87e6692 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/a87e6692 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/a87e6692 Branch: refs/heads/master Commit: a87e6692a54a10d389165fd82c93b3fd2f4ccc03 Parents: 6dafb3f Author: Aled Sage <aled.s...@gmail.com> Authored: Thu Sep 6 12:38:05 2018 +0100 Committer: Aled Sage <aled.s...@gmail.com> Committed: Thu Sep 6 12:38:05 2018 +0100 ---------------------------------------------------------------------- .../mgmt/internal/EntityManagerInternal.java | 8 ++ .../core/mgmt/internal/LocalEntityManager.java | 43 +++++++++ .../internal/NonDeploymentEntityManager.java | 6 +- .../core/objs/proxy/InternalEntityFactory.java | 15 +++- .../core/entity/proxying/EntityManagerTest.java | 27 ++++++ .../brooklyn/core/mgmt/DeployFailureTest.java | 95 +++++++++++++++++++- 6 files changed, 191 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/a87e6692/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EntityManagerInternal.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EntityManagerInternal.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EntityManagerInternal.java index 9d0ab49..4e6adb9 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EntityManagerInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EntityManagerInternal.java @@ -39,4 +39,12 @@ public interface EntityManagerInternal extends EntityManager, BrooklynObjectMana */ @Beta <T extends Entity> T createEntity(EntitySpec<T> spec, Optional<String> entityId); + + /** + * Similar to {@link #unmanage(Entity)}, but used to discard partially constructed entities. + * + * @throws IllegalStateException if the entity, or any of its descendents are already managed. + */ + @Beta + void discardPremanaged(Entity e); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/a87e6692/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalEntityManager.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalEntityManager.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalEntityManager.java index f09ddab..35ab40b 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalEntityManager.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalEntityManager.java @@ -23,9 +23,11 @@ import static com.google.common.base.Preconditions.checkNotNull; import java.lang.reflect.Proxy; import java.util.Collection; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.Stack; import java.util.WeakHashMap; import java.util.concurrent.ConcurrentMap; import java.util.regex.Pattern; @@ -460,7 +462,48 @@ public class LocalEntityManager implements EntityManagerInternal { public void unmanage(final Entity e, final ManagementTransitionMode mode) { unmanage(e, mode, false); } + + @Override + public void discardPremanaged(final Entity e) { + if (e == null) return; + if (!isRunning()) return; + + Set<String> todiscard = new LinkedHashSet<>(); + Stack<Entity> tovisit = new Stack<>(); + Set<Entity> visited = new LinkedHashSet<>(); + + tovisit.push(e); + + while (!tovisit.isEmpty()) { + Entity next = tovisit.pop(); + visited.add(next); + for (Entity child : next.getChildren()) { + if (!visited.contains(child)) { + tovisit.push(child); + } + } + + if (isManaged(next)) { + throw new IllegalStateException("Cannot discard entity "+e+" because it or a descendent is already managed ("+next+")"); + } + Entity realNext = deproxyIfNecessary(next); + String id = next.getId(); + Entity realFound = preRegisteredEntitiesById.get(id); + if (realFound == null) preManagedEntitiesById.get(id); + + if (realFound != null && realFound != realNext) { + throw new IllegalStateException("Cannot discard pre-managed entity "+e+" because it or a descendent's id ("+id+") clashes with a different entity (given "+next+" but found "+realFound+")"); + } + todiscard.add(id); + } + + for (String id : todiscard) { + preRegisteredEntitiesById.remove(id); + preManagedEntitiesById.remove(id); + } + } + private void unmanage(final Entity e, ManagementTransitionMode mode, boolean hasBeenReplaced) { if (shouldSkipUnmanagement(e)) return; final ManagementTransitionInfo info = new ManagementTransitionInfo(managementContext, mode); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/a87e6692/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentEntityManager.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentEntityManager.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentEntityManager.java index 8289ab7..9a739ea 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentEntityManager.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentEntityManager.java @@ -190,6 +190,11 @@ public class NonDeploymentEntityManager implements EntityManagerInternal { throw new IllegalStateException("Non-deployment context "+this+" is not valid for this operation: cannot unmanage "+e); } + @Override + public void discardPremanaged(Entity e) { + throw new IllegalStateException("Non-deployment context "+this+" is not valid for this operation: cannot discardPremanaged "+e); + } + private boolean isInitialManagementContextReal() { return (initialManagementContext != null && !(initialManagementContext instanceof NonDeploymentManagementContext)); } @@ -202,5 +207,4 @@ public class NonDeploymentEntityManager implements EntityManagerInternal { throw new IllegalStateException("Non-deployment context "+this+" (with no initial management context supplied) is not valid for this operation."); } } - } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/a87e6692/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java index 9953e8c..28208cc 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java @@ -48,6 +48,7 @@ import org.apache.brooklyn.core.entity.EntityInternal; import org.apache.brooklyn.core.mgmt.BrooklynTags; import org.apache.brooklyn.core.mgmt.BrooklynTags.NamedStringTag; import org.apache.brooklyn.core.mgmt.BrooklynTaskTags; +import org.apache.brooklyn.core.mgmt.internal.EntityManagerInternal; import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; @@ -163,7 +164,19 @@ public class InternalEntityFactory extends InternalFactory { Map<String,EntitySpec<?>> specsByEntityId = MutableMap.of(); T entity = createEntityAndDescendantsUninitialized(spec, entityId, entitiesByEntityId, specsByEntityId); - initEntityAndDescendants(entity.getId(), entitiesByEntityId, specsByEntityId); + try { + initEntityAndDescendants(entity.getId(), entitiesByEntityId, specsByEntityId); + } catch (RuntimeException e) { + Exceptions.propagateIfFatal(e); + log.info("Failed to initialise entity "+entity+" and its descendants - unmanaging and propagating original exception: "+Exceptions.collapseText(e)); + try { + ((EntityManagerInternal)managementContext.getEntityManager()).discardPremanaged(entity); + } catch (Exception e2) { + Exceptions.propagateIfFatal(e2); + log.info("Failed to unmanage entity "+entity+" and its descendants, after failure to initialise (rethrowing original exception)", e2); + } + throw e; + } return entity; } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/a87e6692/core/src/test/java/org/apache/brooklyn/core/entity/proxying/EntityManagerTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/entity/proxying/EntityManagerTest.java b/core/src/test/java/org/apache/brooklyn/core/entity/proxying/EntityManagerTest.java index 7d21071..125bb29 100644 --- a/core/src/test/java/org/apache/brooklyn/core/entity/proxying/EntityManagerTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/entity/proxying/EntityManagerTest.java @@ -45,6 +45,7 @@ import org.apache.brooklyn.core.test.entity.TestEntity; import org.apache.brooklyn.core.test.entity.TestEntityImpl; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.MutableMap; +import org.apache.brooklyn.util.text.Identifiers; import org.apache.brooklyn.util.time.Duration; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -137,6 +138,32 @@ public class EntityManagerTest extends BrooklynAppUnitTestSupport { assertSame(Entities.deproxy(postApp), origDeproxiedApp); } + @Test + public void testDiscardPremanaged() { + String id = Identifiers.makeRandomId(12); + TestEntityImpl entity = mgmt.getEntityFactory().constructEntity(TestEntityImpl.class, ImmutableList.of(TestEntity.class), id); + assertTrue(((LocalEntityManager)entityManager).isKnownEntityId(id)); + assertFalse(Entities.isManaged(entity)); + + ((EntityManagerInternal)entityManager).discardPremanaged(entity); + assertFalse(((LocalEntityManager)entityManager).isKnownEntityId(id)); + assertFalse(Entities.isManaged(entity)); + } + + + @Test + public void testDiscardPremanagedFailsIfManaged() { + try { + ((EntityManagerInternal)entityManager).discardPremanaged(app); + Asserts.shouldHaveFailedPreviously(); + } catch (IllegalStateException e) { + Asserts.expectedFailureContains(e, "Cannot discard", "it or a descendent is already managed"); + } + + // Should have had no effect + Entities.isManaged(app); + } + // See https://issues.apache.org/jira/browse/BROOKLYN-352 // Before the fix, 250ms was sufficient to cause the ConcurrentModificationException @Test http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/a87e6692/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java index f5a1de5..b9b5bb0 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java @@ -18,28 +18,85 @@ */ package org.apache.brooklyn.core.mgmt; +import static org.testng.Assert.assertFalse; + +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.Set; + import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntitySpec; +import org.apache.brooklyn.api.entity.ImplementedBy; +import org.apache.brooklyn.config.ConfigKey; +import org.apache.brooklyn.core.config.ConfigKeys; +import org.apache.brooklyn.core.config.ConstraintViolationException; import org.apache.brooklyn.core.mgmt.internal.LocalEntityManager; import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport; import org.apache.brooklyn.core.test.entity.TestEntity; import org.apache.brooklyn.core.test.entity.TestEntityImpl; import org.apache.brooklyn.entity.stock.BasicApplication; import org.apache.brooklyn.test.Asserts; +import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import com.google.common.base.Predicates; + public class DeployFailureTest extends BrooklynAppUnitTestSupport { + private static final Set<Entity> constructedEntities = Collections.synchronizedSet(new LinkedHashSet<>()); + private LocalEntityManager entityManager; @BeforeMethod(alwaysRun=true) @Override public void setUp() throws Exception { + constructedEntities.clear(); super.setUp(); entityManager = (LocalEntityManager) mgmt.getEntityManager(); } + @AfterMethod(alwaysRun=true) + @Override + public void tearDown() throws Exception { + try { + super.tearDown(); + } finally { + constructedEntities.clear(); + } + } + + @Test + public void testConfigConstraintViolation() throws Exception { + try { + mgmt.getEntityManager().createEntity(EntitySpec.create(BasicApplication.class) + .child(EntitySpec.create(TestEntityWithConfigConstraint.class))); + Asserts.shouldHaveFailedPreviously(); + } catch (ConstraintViolationException e) { + // Check gives nice exception + Asserts.expectedFailureContains(e, "myNonNullKey", "Predicates.notNull()"); + } + + // Should have disposed of entities that failed to be created + assertEntitiesNotKnown(constructedEntities); + } + + @Test + public void testFailedInit() throws Exception { + try { + mgmt.getEntityManager().createEntity(EntitySpec.create(BasicApplication.class) + .child(EntitySpec.create(TestEntity.class) + .impl(TestEntityFailingInitImpl.class))); + Asserts.shouldHaveFailedPreviously(); + } catch (Exception e) { + // Check gives nice exception + Asserts.expectedFailureContains(e, "Simulating failure in entity init"); + } + + // Should have disposed of entities that failed to be created + assertEntitiesNotKnown(constructedEntities); + } + @Test public void testFailedGetParent() throws Exception { entityManager.getAllEntitiesInApplication(app); @@ -50,15 +107,51 @@ public class DeployFailureTest extends BrooklynAppUnitTestSupport { .impl(TestEntityFailingGetParentImpl.class))); Asserts.shouldHaveFailedPreviously(); } catch (ClassCastException e) { + // TODO Should give nicer exception Asserts.expectedFailureContains(e, "cannot be cast", "WrongParentEntity"); } // This should continue to work, after the failed entity-deploy above // See https://issues.apache.org/jira/browse/BROOKLYN-599 entityManager.getAllEntitiesInApplication(app); + + // Should have disposed of entities that failed to be created + assertEntitiesNotKnown(constructedEntities); } - public static class TestEntityFailingGetParentImpl extends TestEntityImpl { + private void assertEntitiesNotKnown(Iterable<Entity> entities) { + for (Entity entity : constructedEntities) { + if (entity.getId() != null) { + assertFalse(entityManager.isKnownEntityId(entity.getId()), "entity="+entity); + } + } + } + + public static class TestEntityRecordingConstructionImpl extends TestEntityImpl { + public TestEntityRecordingConstructionImpl() { + constructedEntities.add(this); + } + } + + @ImplementedBy(TestEntityWithConfigConstraintImpl.class) + public static interface TestEntityWithConfigConstraint extends TestEntity { + ConfigKey<String> NON_NULL_KEY = ConfigKeys.builder(String.class) + .name("myNonNullKey") + .constraint(Predicates.notNull()) + .build(); + } + + public static class TestEntityWithConfigConstraintImpl extends TestEntityRecordingConstructionImpl implements TestEntityWithConfigConstraint { + } + + public static class TestEntityFailingInitImpl extends TestEntityRecordingConstructionImpl { + @Override + public void init() { + throw new RuntimeException("Simulating failure in entity init()"); + } + } + + public static class TestEntityFailingGetParentImpl extends TestEntityRecordingConstructionImpl { private static interface WrongParentEntity extends Entity {} @Override
