New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WFCORE-3751 DefaultCapabilityReferenceRecorder generates wrong dependent name if corresponding RuntimeCapability uses a dynamic name mapper #3216
Conversation
Core - Full Integration Build 7111 outcome was UNKNOWN using a merge of 3d2eeb7 |
*/ | ||
String getDynamicDependentName(PathAddress currentAddress) { | ||
@Deprecated String getDynamicDependentName(PathAddress currentAddress) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a (package) method. If it is no longer used, we can delete it instead of deprecating it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. I'll remove it.
dependentName = baseDependentName; | ||
} | ||
RuntimeCapability<?> dependentCapability = this.getDependentCapability(context); | ||
String dependentName = (dependentCapability.isDynamicallyNamed() ? dependentCapability.fromBaseCapability(context.getCurrentAddress()) : dependentCapability).getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main change is here.
Before, we were checking if the dependent capability was dynamic (according to the author) to be able to build its name.
Now, we fetch the dependent RuntimeCapability
according to the current OperationContext MRR and build its name according to the MRR capabilities.
It is similar to the ContextDependencyRecorder implementation (although its getDependentCapability
impl differs a bit as it checks there is a single registered capability).
My concern is the warning in ContextDependencyRecorder
javadoc:
CapabilityReferenceRecorder that determines the dependent capability from the OperationContext. This assumes that the resource registration associated with currently executing step will expose a capability set including one and only one capability. This recorder cannot be used with attributes associated with resources that do not meet this requirement.
Your change to the DefaultCapabilityReferenceRecorder generalises the requirement of the ContextDependencyRecorder as the MRR can contain several capabilities and you only use the one corresponding to the baseDependentName
.
Is my understanding correct?
Is there still a sense to have separate DefaultCapabilityReferenceRecorder and ContextDependencyRecorder as they now both require a Context MRR to work.
We could have a special case for attribute-based capability but that's a special case of your general DefaultCapabilityReferenceRecorder now, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your understanding is correct. In retrospect, DefaultCapabilityReferenceRecorder should probably extend ContextDependencyRecorder, and just override its logic for generating the dependent name (i.e. from a specific baseDependentName). Done.
Is there still a sense to have separate DefaultCapabilityReferenceRecorder and ContextDependencyRecorder
A valid question. Personally, I didn't even know that ContextDependencyRecorder existed until this week :) - so I don't know how many people are using it.
ContextDependencyRecorder requires fewer parameters, which is nice, but it can't be used if the resource defines multiple capabilities.
I suspect it is worth having both - but we should at least consolidate the implementations.
Another logical implementation is one that accepts the RuntimeCapability itself (this is how the CapabilityReferenceRecorder in wildfly-clustering-common works), which simplifies the getDependentCapability(...) even further (and without an exception case), but that is probably out of scope for this fix.
We could have a special case for attribute-based capability
What do you mean by attribute-based capability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - maybe you mean CompositeAttributeDependencyRecorder?
return capability; | ||
} | ||
} | ||
throw ControllerLogger.ROOT_LOGGER.unknownCapability(this.baseDependentName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pass the OperationContext address to the exception so that the users can identify which resource is causing the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could - but unless the same capability is defined by multiple resources (which is rare), the problematic resource is usually obvious.
@@ -73,7 +73,7 @@ | |||
public static final SimpleAttributeDefinition SECURITY_DOMAIN = new SimpleAttributeDefinitionBuilder(ModelDescriptionConstants.SECURITY_DOMAIN, ModelType.STRING, false) | |||
.setMinSize(1) | |||
.setFlags(AttributeAccess.Flag.RESTART_RESOURCE_SERVICES) | |||
.setCapabilityReference(SECURITY_DOMAIN_CAPABILITY, MANAGEMENT_IDENTITY_CAPABILITY, false) | |||
.setCapabilityReference(SECURITY_DOMAIN_CAPABILITY, MANAGEMENT_IDENTITY_CAPABILITY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
juste a note that I much prefer to let the CapabilityReference determines where the dependent capability is dynamic or not according to the registered RuntimeCapability instead to have to specify it myself when I set the capability reference :)
.setAddRestartLevel(OperationEntry.Flag.RESTART_NONE) | ||
.setRemoveRestartLevel(OperationEntry.Flag.RESTART_RESOURCE_SERVICES) | ||
.setCapabilities(MANAGEMENT_IDENTITY_RUNTIME_CAPABILITY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is your change equivalent?
Capability added to the ReloadRequiredRemoveStepHandler will be handled in org.jboss.as.controller.AbstractRemoveStepHandler#recordCapabilitiesAndRequirements that does a whole lot more that when you set it on the resource definition where it is handled in org.jboss.as.controller.SimpleResourceDefinition#registerCapabilities.
I agree with your change in principle as I think the capabilities belongs to the resource and not to its add/remove operation. However, are we missing some process from org.jboss.as.controller.AbstractRemoveStepHandler#recordCapabilitiesAndRequirements in doing so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is your change equivalent?
Yes - the resulting behavior is exactly the same.
See: https://github.com/wildfly/wildfly-core/blob/master/controller/src/main/java/org/jboss/as/controller/AbstractRemoveStepHandler.java#L185
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, right.
So my question is not specific to your PR but more generally why have we 2 different ways to specify capabilities (either in add/remove handler or in the resource definition)?
In any case, your PR is fine wrt that issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmesnil That's just a matter of code history; in the initial cap/req code there was no MRR.getCapabilities(); the relevant caps were stored in the 'capabilities' field in the add/remove handlers.
@@ -120,6 +120,10 @@ | |||
<artifactId>junit</artifactId> | |||
<scope>test</scope> | |||
</dependency> | |||
<dependency> | |||
<groupId>org.mockito</groupId> | |||
<artifactId>mockito-core</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be in <scope>test</scope>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops - yes, absolutely.
@@ -518,6 +522,10 @@ public void execute(ModelNode operation, OperationMessageHandler handler, ProxyO | |||
private OperationStepHandler nextStep; | |||
protected MockOperationContext(final Resource root, final boolean booting, final PathAddress operationAddress) { | |||
super(root, booting, operationAddress); | |||
Set<RuntimeCapability> capabilities = new HashSet<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That goes back to my concern in the DefaultCapabilityReferenceRecorder
implementation that now requires to get info from the MRR.
I am not sure if that's a constraint that will always be there. (it used to be absent in this test fixture for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supplying capabilities to the add/remove operation handler is redundant. As I understand it, not registering a capability with the MMR is a bug.
@pferraro After finishing my review, your PR looks right except for the added requirement to always get RuntimeCapability info from the MRR in the DefaultCapabilityReferenceRecorder implementation. @ehsavoie @bstansberry Do you have an use case where this would not be the case? The tests were such a case but I don't know if that was by design or just because it was not needed to get info from the MRR. |
@jmesnil The main way the cap would not be in the MRR is if people relied on the old code that passed the relevant caps to the Abstract[Add|Remove]StepHandler constructor and didn't record the cap with the MRR. That would largely be a bug, as not recording with the MRR means the possible capability info is not available. I don't think it's done in WF code but it might be done in downstream projects that haven't kept up with best practices. I say "largely a bug" because we've contemplated having a capability only registered if an attribute is set to a particular value. For example jts=true in subsystem=transactions, when we get around to doing caps in that subsystem. But:
@pferraro if we do this we need to deprecate all the add/remove handler constructors in the controller module that accept a RuntimeCapability param. Their presence obscures that the caps to register really should come from the MRR. Finally, I think this will avoid breaking any downstream code that isn't properly registering the cap with the MRR: |
Core - Full Integration Build 7247 outcome was UNKNOWN using a merge of e70c52f |
AbstractBoottimeAddStepHandler, ReloadRequiredAddStepHandler, ReloadRequiredRemoveStepHandler , ServiceRemoveStepHandler constructors with capabilities should be deprecated too. |
Core - Full Integration Build 7484 outcome was FAILURE using a merge of dd1af51 Failed tests
|
…es that use a dynamic name mapper
…abilityReference(...) method.
…rs that specify capabilities.
@jmesnil I've cherry-picked it into this PR. |
https://issues.jboss.org/browse/WFCORE-3751