From 2cbb255f8736249a736008ae0caaa45da8bfed83 Mon Sep 17 00:00:00 2001 From: Yeray Borges Date: Wed, 11 Dec 2019 13:12:43 +0000 Subject: [PATCH 1/2] [WFCORE-4774] Avoid circular recursion searching for the capability status --- .../as/controller/CapabilityRegistry.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/controller/src/main/java/org/jboss/as/controller/CapabilityRegistry.java b/controller/src/main/java/org/jboss/as/controller/CapabilityRegistry.java index 87c5c61a6e9..f71d70074e9 100644 --- a/controller/src/main/java/org/jboss/as/controller/CapabilityRegistry.java +++ b/controller/src/main/java/org/jboss/as/controller/CapabilityRegistry.java @@ -352,14 +352,14 @@ public Map getRuntimeStatus(PathAddress address, Im if (size == 0) { result = Collections.emptyMap(); } else { - Set examined = new HashSet<>(); + Set visited = new HashSet<>(); if (size == 1) { CapabilityId id = ids.iterator().next(); - result = Collections.singletonMap(id, getCapabilityStatus(id, examined)); + result = Collections.singletonMap(id, getCapabilityStatus(id, visited)); } else { result = new HashMap<>(size); for (CapabilityId id : ids) { - result.put(id, getCapabilityStatus(id, examined)); + result.put(id, getCapabilityStatus(id, visited)); } } } @@ -369,7 +369,7 @@ public Map getRuntimeStatus(PathAddress address, Im } } - private RuntimeStatus getCapabilityStatus(CapabilityId id, Set examined) { + private RuntimeStatus getCapabilityStatus(CapabilityId id, Set visited) { // This is meant for checking runtime stuff, which should only be for servers or // HC runtime stuff, both of which use CapabilityScope.GLOBAL or HostCapabilityScope. So this assert // is to check that assumption is valid, as further thought is needed if not (e.g. see WFCORE-1710). @@ -385,11 +385,12 @@ private RuntimeStatus getCapabilityStatus(CapabilityId id, Set exa if (reloadCapabilities.contains(id)) { return RuntimeStatus.RELOAD_REQUIRED; } - examined.add(id); } // else defer reload-required check until after we search requirements for restart-required + visited.add(id); + Map dependents = requirements.get(id); - RuntimeStatus result = getDependentCapabilityStatus(dependents, id, examined); + RuntimeStatus result = getDependentCapabilityStatus(dependents, id, visited); // TODO we could also check runtimeOnlyRequirements but it's not clear that's meaningful // If the non-normal runtime-only req has had its cap removed, a RUNTIME step for the dependent // will not see it any more and won't try and integrate. If the req is reload-required but @@ -399,7 +400,7 @@ private RuntimeStatus getCapabilityStatus(CapabilityId id, Set exa if (result != RuntimeStatus.RESTART_REQUIRED) { // Check pending remove requirements dependents = pendingRemoveRequirements.get(id); - RuntimeStatus pending = getDependentCapabilityStatus(dependents, id, examined); + RuntimeStatus pending = getDependentCapabilityStatus(dependents, id, visited); if (pending != RuntimeStatus.NORMAL) { result = pending; } @@ -412,7 +413,7 @@ private RuntimeStatus getCapabilityStatus(CapabilityId id, Set exa return result; } - private RuntimeStatus getDependentCapabilityStatus(Map dependents, CapabilityId requiror, Set examined) { + private RuntimeStatus getDependentCapabilityStatus(Map dependents, CapabilityId requiror, Set visited) { RuntimeStatus result = RuntimeStatus.NORMAL; if (dependents != null) { for (String dependent : dependents.keySet()) { @@ -422,8 +423,8 @@ private RuntimeStatus getDependentCapabilityStatus(Map Date: Fri, 13 Dec 2019 10:15:48 +0000 Subject: [PATCH 2/2] [WFCORE-4774] Add capability test cases when there are circular capabilities and server is either in reload or restart --- .../CapabilityRegistryTestCase.java | 170 +++++++++++++++--- 1 file changed, 141 insertions(+), 29 deletions(-) diff --git a/controller/src/test/java/org/jboss/as/controller/CapabilityRegistryTestCase.java b/controller/src/test/java/org/jboss/as/controller/CapabilityRegistryTestCase.java index 94eeacc77d6..43958d40339 100644 --- a/controller/src/test/java/org/jboss/as/controller/CapabilityRegistryTestCase.java +++ b/controller/src/test/java/org/jboss/as/controller/CapabilityRegistryTestCase.java @@ -21,6 +21,7 @@ import static org.jboss.as.controller.descriptions.ModelDescriptionConstants.ADD; import static org.jboss.as.controller.descriptions.ModelDescriptionConstants.RELOAD; import static org.jboss.as.controller.descriptions.ModelDescriptionConstants.REMOVE; +import static org.jboss.as.controller.descriptions.ModelDescriptionConstants.RESTART; import static org.jboss.as.controller.descriptions.ModelDescriptionConstants.RESULT; import static org.jboss.as.controller.descriptions.ModelDescriptionConstants.STEPS; @@ -33,7 +34,6 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; -import org.jboss.as.controller.capability.Capability; import org.jboss.as.controller.capability.RuntimeCapability; import org.jboss.as.controller.capability.registry.CapabilityId; import org.jboss.as.controller.capability.registry.CapabilityScope; @@ -113,6 +113,7 @@ public void validateParameter(String parameterName, ModelNode value) throws Oper private static PathAddress TEST_ADDRESS5 = PathAddress.pathAddress("subsystem", "broken"); private static PathElement RELOAD_ELEMENT = PathElement.pathElement("subsystem", "reload"); + private static PathElement CIRCULAR_CAP_ELEMENT = PathElement.pathElement("subsystem", "circular"); private static PathElement CHILD_ELEMENT = PathElement.pathElement("child", "test"); private static PathElement GRANDCHILD_ELEMENT = PathElement.pathElement("grandchild", "test"); private static PathElement INFANT_ELEMENT = PathElement.pathElement("infant", "test"); @@ -126,6 +127,31 @@ public void validateParameter(String parameterName, ModelNode value) throws Oper private static PathElement DEP_CAP_ELEMENT = PathElement.pathElement("subsystem", "dep-cap"); private static PathElement DEP_CAP_ELEMENT_2 = PathElement.pathElement("subsystem", "dep-cap-2"); + + private static final RuntimeCapability CIRCULAR_PARENT_CAPABILITY = RuntimeCapability.Builder.of("org.wildfly.test.circular-parent-capability", false, Void.class) + .addRequirements("org.wildfly.test.circular-child-capability") + .build(); + + private static final RuntimeCapability CIRCULAR_CHILD_CAPABILITY = RuntimeCapability.Builder.of("org.wildfly.test.circular-child-capability", false, Void.class) + .addRequirements(CIRCULAR_PARENT_CAPABILITY.getName()) + .build(); + + private static final OperationStepHandler RESTART_HANDLER = new OperationStepHandler() { + @Override + public void execute(OperationContext context, ModelNode operation) throws OperationFailedException { + context.restartRequired(); + context.completeStep(new OperationContext.RollbackHandler() { + + @Override + public void handleRollback(OperationContext context, ModelNode operation) { + context.revertRestartRequired(); + } + }); + } + }; + + private static final OperationDefinition RESTART_DEFINITION = SimpleOperationDefinitionBuilder.of(RESTART, NonResolvingResourceDescriptionResolver.INSTANCE).build(); + private static ResourceDefinition TEST_RESOURCE1 = ResourceBuilder.Factory.create(TEST_ADDRESS1.getElement(0), NonResolvingResourceDescriptionResolver.INSTANCE) .setAddOperation(new AbstractAddStepHandler(new HashSet<>(Arrays.asList(IO_POOL_RUNTIME_CAPABILITY, IO_WORKER_RUNTIME_CAPABILITY)), ad, other)) @@ -207,7 +233,7 @@ protected void performRuntime(OperationContext context, ModelNode operation, Res } } - private static class RuntimeReportRemoveHandler extends AbstractRemoveStepHandler { + private static class RuntimeReportRemoveHandler extends AbstractRemoveStepHandler { private RuntimeReportRemoveHandler(Set caps) { super(caps); } @@ -216,13 +242,19 @@ protected void performRuntime(OperationContext context, ModelNode operation, Mod if (operation.get(RELOAD).asBoolean(false)) { context.reloadRequired(); context.getResult().set(true); + } else if (operation.get(RESTART).asBoolean(false)) { + context.restartRequired(); + context.getResult().set(true); } } @Override protected void recoverServices(OperationContext context, ModelNode operation, ModelNode model) throws OperationFailedException { if (operation.get(RELOAD).asBoolean(false)) { - context.reloadRequired(); + context.revertReloadRequired(); + context.getResult().set(false); + } else if (operation.get(RESTART).asBoolean(false)) { + context.revertRestartRequired(); context.getResult().set(false); } } @@ -255,62 +287,77 @@ public void execute(OperationContext context, ModelNode operation) throws Operat .setRuntimeOnly() .build(); - private static ResourceBuilder createReloadTestResourceBuilder(PathElement address, RuntimeCapability... caps) { - return addReloadTestOps(ResourceBuilder.Factory.create(address, NonResolvingResourceDescriptionResolver.INSTANCE), caps); + private static ResourceBuilder createReloadRestartTestResourceBuilder(PathElement address, RuntimeCapability... caps) { + return addReloadRestartTestOps(ResourceBuilder.Factory.create(address, NonResolvingResourceDescriptionResolver.INSTANCE), caps); } - private static ResourceBuilder addReloadTestOps(ResourceBuilder builder, RuntimeCapability... caps) { + private static ResourceBuilder addReloadRestartTestOps(ResourceBuilder builder, RuntimeCapability... caps) { ResourceBuilder result = builder .addOperation(RELOAD_DEFINITION, RELOAD_HANDLER) + .addOperation(RESTART_DEFINITION, RESTART_HANDLER) .addOperation(RUNTIME_MOD_DEFINITION, RUNTIME_MOD_HANDLER) .addOperation(RUNTIME_ONLY_DEFINITION, RUNTIME_MOD_HANDLER); + if (caps != null && caps.length > 0) { - Set capsSet = new HashSet(Arrays.asList(caps)); - result = result.addCapabilities((Capability[]) caps) + Set capsSet = new HashSet<>(Arrays.asList(caps)); + result = result.addCapabilities(caps) .setAddOperation(new RuntimeReportAddHandler(capsSet)) .setRemoveOperation(new RuntimeReportRemoveHandler(capsSet)); } + else { + result = result.setAddOperation(new RuntimeReportAddHandler(Collections.emptySet())) + .setRemoveOperation(new RuntimeReportRemoveHandler(Collections.emptySet())); + } + return result; } - private static ResourceDefinition RELOAD_PARENT = createReloadTestResourceBuilder(RELOAD_ELEMENT, RELOADED_CAPABILITY) - .pushChild(createReloadTestResourceBuilder(INDEPENDENT_ELEMENT, INDEPENDENT_CAPABILITY)) - .pushChild(createReloadTestResourceBuilder(CHILD_ELEMENT)).pop() + private static ResourceDefinition RELOAD_PARENT = createReloadRestartTestResourceBuilder(RELOAD_ELEMENT, RELOADED_CAPABILITY) + .pushChild(createReloadRestartTestResourceBuilder(INDEPENDENT_ELEMENT, INDEPENDENT_CAPABILITY)) + .pushChild(createReloadRestartTestResourceBuilder(CHILD_ELEMENT)).pop() .pop() - .pushChild(createReloadTestResourceBuilder(UNINCORPORATED_ELEMENT)) + .pushChild(createReloadRestartTestResourceBuilder(UNINCORPORATED_ELEMENT)) .setIncorporatingCapabilities(Collections.emptySet()) - .pushChild(createReloadTestResourceBuilder(CHILD_ELEMENT)).pop() + .pushChild(createReloadRestartTestResourceBuilder(CHILD_ELEMENT)).pop() .pop() - .pushChild(createReloadTestResourceBuilder(CHILD_ELEMENT)) - .pushChild(createReloadTestResourceBuilder(GRANDCHILD_ELEMENT)) - .pushChild(createReloadTestResourceBuilder(INFANT_ELEMENT)) + .pushChild(createReloadRestartTestResourceBuilder(CHILD_ELEMENT)) + .pushChild(createReloadRestartTestResourceBuilder(GRANDCHILD_ELEMENT)) + .pushChild(createReloadRestartTestResourceBuilder(INFANT_ELEMENT)) .pop() .pop() - .pushChild(createReloadTestResourceBuilder(UNINCORPORATED_ELEMENT)) + .pushChild(createReloadRestartTestResourceBuilder(UNINCORPORATED_ELEMENT)) .setIncorporatingCapabilities(Collections.emptySet()) - .pushChild(createReloadTestResourceBuilder(INFANT_ELEMENT)).pop() + .pushChild(createReloadRestartTestResourceBuilder(INFANT_ELEMENT)).pop() .pop() .pop() + + .build(); + + private static ResourceDefinition CIRCULAR_CAP_RESOURCE = createReloadRestartTestResourceBuilder(CIRCULAR_CAP_ELEMENT) + .pushChild(createReloadRestartTestResourceBuilder(INDEPENDENT_ELEMENT, INDEPENDENT_CAPABILITY)).pop() + .pushChild(createReloadRestartTestResourceBuilder(CHILD_ELEMENT, CIRCULAR_PARENT_CAPABILITY)) + .pushChild(createReloadRestartTestResourceBuilder(GRANDCHILD_ELEMENT, CIRCULAR_CHILD_CAPABILITY)).pop() + .pop() .build(); - private static ResourceDefinition HOST_RESOURCE = createReloadTestResourceBuilder(HOST_ELEMENT, HOST_CAPABILITY) - .pushChild(createReloadTestResourceBuilder(CHILD_ELEMENT)).pop() + private static ResourceDefinition HOST_RESOURCE = createReloadRestartTestResourceBuilder(HOST_ELEMENT, HOST_CAPABILITY) + .pushChild(createReloadRestartTestResourceBuilder(CHILD_ELEMENT)).pop() .build(); - private static ResourceDefinition PROFILE_RESOURCE = createReloadTestResourceBuilder(PROFILE_ELEMENT, PROFILE_CAPABILITY) - .pushChild(createReloadTestResourceBuilder(NO_CAP_ELEMENT)) - .pushChild(createReloadTestResourceBuilder(CHILD_ELEMENT)).pop() + private static ResourceDefinition PROFILE_RESOURCE = createReloadRestartTestResourceBuilder(PROFILE_ELEMENT, PROFILE_CAPABILITY) + .pushChild(createReloadRestartTestResourceBuilder(NO_CAP_ELEMENT)) + .pushChild(createReloadRestartTestResourceBuilder(CHILD_ELEMENT)).pop() .pop() .build(); - private static ResourceDefinition NO_CAP_RESOURCE = createReloadTestResourceBuilder(CHILD_ELEMENT) + private static ResourceDefinition NO_CAP_RESOURCE = createReloadRestartTestResourceBuilder(CHILD_ELEMENT) .build(); - private static ResourceDefinition DEP_CAP_RESOURCE = createReloadTestResourceBuilder(DEP_CAP_ELEMENT, DEPENDENT_CAPABILITY) - .pushChild(createReloadTestResourceBuilder(CHILD_ELEMENT, TRANS_DEP_CAPABILITY)).pop() + private static ResourceDefinition DEP_CAP_RESOURCE = createReloadRestartTestResourceBuilder(DEP_CAP_ELEMENT, DEPENDENT_CAPABILITY) + .pushChild(createReloadRestartTestResourceBuilder(CHILD_ELEMENT, TRANS_DEP_CAPABILITY)).pop() .build(); - private static ResourceDefinition DEP_CAP_RESOURCE_2 = createReloadTestResourceBuilder(DEP_CAP_ELEMENT_2, TRANS_DEP_CAPABILITY) + private static ResourceDefinition DEP_CAP_RESOURCE_2 = createReloadRestartTestResourceBuilder(DEP_CAP_ELEMENT_2, TRANS_DEP_CAPABILITY) .build(); private static final AtomicReference readLatchHolder = new AtomicReference<>(); @@ -343,7 +390,7 @@ protected void performRuntime(OperationContext context, ModelNode operation, Res ((context, operation) -> context.getResult().set(true))) .build(); - private static final int RELOAD_CAP_COUNT = 6; + private static final int RELOAD_CAP_COUNT = 8; private ManagementModel managementModel; @@ -368,6 +415,7 @@ protected void initModel(ManagementModel managementModel) { mrr.unregisterSubModel(NO_CAP_RESOURCE.getPathElement()); mrr.unregisterSubModel(DEP_CAP_RESOURCE.getPathElement()); mrr.unregisterSubModel(DEP_CAP_RESOURCE_2.getPathElement()); + mrr.unregisterSubModel(CIRCULAR_CAP_RESOURCE.getPathElement()); }); rootRegistration.registerOperationHandler(SimpleOperationDefinitionBuilder.of("create", NonResolvingResourceDescriptionResolver.INSTANCE).build(), (context, operation) -> { @@ -382,6 +430,7 @@ protected void initModel(ManagementModel managementModel) { mrr.registerSubModel(NO_CAP_RESOURCE); mrr.registerSubModel(DEP_CAP_RESOURCE); mrr.registerSubModel(DEP_CAP_RESOURCE_2); + mrr.registerSubModel(CIRCULAR_CAP_RESOURCE); }); rootRegistration.registerOperationHandler(SimpleOperationDefinitionBuilder.of("root-cap", NonResolvingResourceDescriptionResolver.INSTANCE).build(), new OperationStepHandler() { @@ -400,6 +449,7 @@ public void execute(OperationContext context, ModelNode operation) throws Operat rootRegistration.registerOperationHandler(RELOAD_DEFINITION, RELOAD_HANDLER); rootRegistration.registerOperationHandler(RUNTIME_MOD_DEFINITION, RUNTIME_MOD_HANDLER); rootRegistration.registerOperationHandler(RUNTIME_ONLY_DEFINITION, RUNTIME_MOD_HANDLER); + rootRegistration.registerOperationHandler(RESTART_DEFINITION, RESTART_HANDLER); } @Before @@ -418,7 +468,6 @@ public void checkEmptyRegistry() throws Exception { Assert.assertEquals(expectedCaps(0), capabilityRegistry.getPossibleCapabilities().size()); } - @Test public void testCapabilityPossibleProviders() throws OperationFailedException { Set result = capabilityRegistry.getPossibleProviderPoints(new CapabilityId(TEST_CAPABILITY2.getName(), CapabilityScope.GLOBAL)); @@ -934,6 +983,56 @@ public void testGetCapabilities() throws OperationFailedException { Assert.assertTrue(result.contains("dyn")); } + /** + * Tests that a runtime operation can be done when there is a circular requirements between two capabilities + * and the server is in restart-required state by an independent capability + * @throws OperationFailedException + */ + @Test + public void testRuntimeCircularCapabilitiesInRequiredState() throws OperationFailedException { + runtimeCircularCapabilities(p -> requireRestart(p)); + } + + /** + * Tests that a runtime operation can be done when there is a circular requirements between two capabilities + * and the server is in reload-required state by an independent capability + * @throws OperationFailedException + */ + @Test + public void testRuntimeCircularCapabilitiesInReloadState() throws OperationFailedException { + runtimeCircularCapabilities(p -> requireReload(p)); + } + + private void runtimeCircularCapabilities(ReloadRestartAction action) throws OperationFailedException { + add(CIRCULAR_CAP_ELEMENT); + add(CIRCULAR_CAP_ELEMENT, INDEPENDENT_ELEMENT); + + ModelNode addOp1 = Util.createEmptyOperation("add", PathAddress.pathAddress(CIRCULAR_CAP_ELEMENT, CHILD_ELEMENT)); + ModelNode addOp2 = Util.createEmptyOperation("add", PathAddress.pathAddress(CIRCULAR_CAP_ELEMENT, CHILD_ELEMENT, GRANDCHILD_ELEMENT)); + + ModelNode composite = createOperation(ModelDescriptionConstants.COMPOSITE); + composite.get(STEPS).add(addOp1); + composite.get(STEPS).add(addOp2); + executeCheckNoFailure(composite); + + try { + action.accept(PathAddress.pathAddress(CIRCULAR_CAP_ELEMENT, INDEPENDENT_ELEMENT)); + runtimeCheck(true, CIRCULAR_CAP_ELEMENT, CHILD_ELEMENT, GRANDCHILD_ELEMENT); + runtimeOnlyCheck(true, CIRCULAR_CAP_ELEMENT, CHILD_ELEMENT, GRANDCHILD_ELEMENT); + } finally { + addOp1 = Util.createEmptyOperation("remove", PathAddress.pathAddress(CIRCULAR_CAP_ELEMENT, CHILD_ELEMENT, GRANDCHILD_ELEMENT)); + addOp2 = Util.createEmptyOperation("remove", PathAddress.pathAddress(CIRCULAR_CAP_ELEMENT, CHILD_ELEMENT)); + + composite = createOperation(ModelDescriptionConstants.COMPOSITE); + composite.get(STEPS).add(addOp1); + composite.get(STEPS).add(addOp2); + executeCheckNoFailure(composite); + + remove(CIRCULAR_CAP_ELEMENT, INDEPENDENT_ELEMENT); + remove(CIRCULAR_CAP_ELEMENT); + } + } + private void addRemoveAddTest() throws OperationFailedException { ManagementResourceRegistration registration = managementModel.getRootResourceRegistration().getSubModel(PathAddress.pathAddress(DEP_CAP_ELEMENT)); Assert.assertEquals(1, registration.getCapabilities().size()); @@ -968,6 +1067,14 @@ private void requireReload(PathElement... elements) throws OperationFailedExcept executeCheckNoFailure(Util.createEmptyOperation(RELOAD_DEFINITION.getName(), address)); } + private void requireReload(PathAddress address) throws OperationFailedException { + executeCheckNoFailure(Util.createEmptyOperation(RELOAD_DEFINITION.getName(), address)); + } + + private void requireRestart(PathAddress address) throws OperationFailedException { + executeCheckNoFailure(Util.createEmptyOperation(RESTART_DEFINITION.getName(), address)); + } + private void runtimeCheck(boolean expectResult, PathElement... elements) throws OperationFailedException { runtimeCheck(false, expectResult, elements); } @@ -988,4 +1095,9 @@ private void runtimeCheck(boolean runtimeOnly, boolean expectResult, PathElement Assert.assertFalse(result.toString(), result.isDefined()); } } + + @FunctionalInterface + public interface ReloadRestartAction { + void accept(T t) throws E; + } }