-
Notifications
You must be signed in to change notification settings - Fork 467
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-6221 Add formal experimental/preview mode to management model #5413
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Just a couple minor comments. Overall though I think this looks good.
controller/src/main/java/org/jboss/as/controller/SimpleOperationDefinitionBuilder.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
controller/src/main/java/org/jboss/as/controller/xml/VersionedURN.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/jboss/as/server/ServerEnvironment.java
Outdated
Show resolved
Hide resolved
5ab65b2
to
3649807
Compare
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.
Hi @pferraro, I did another review. Regarding domain mode, we also need to reject registrations of host controllers that are not running at the same FeatureStream level as the domain controller.
I think Zulip could be better to discuss this, so I'll add the comments there
process-controller/src/main/java/org/jboss/as/process/CommandLineArgumentUsageImpl.java
Outdated
Show resolved
Hide resolved
process-controller/src/main/java/org/jboss/as/process/logging/ProcessLogger.java
Outdated
Show resolved
Hide resolved
controller/src/main/java/org/jboss/as/controller/extension/ExtensionRegistry.java
Outdated
Show resolved
Hide resolved
host-controller/src/main/java/org/jboss/as/host/controller/HostControllerEnvironment.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/jboss/as/server/ServerEnvironment.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
8dc3916
to
4591954
Compare
Core -> Full Integration Build 13184 outcome was FAILURE using a merge of 4591954 Failed tests
|
4591954
to
5fea659
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
* @param registration the child registration of this registry that should no longer be available | ||
* @param descriptionProvider provider for descriptions of the additional attributes or child types | ||
* | ||
* @return a resource registration which may be used to add attributes, operations and sub-models |
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 should note that it may return null.
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.
Fixed
@@ -127,6 +133,7 @@ private HostInfo(final ModelNode hostInfo, DomainHostExcludeRegistry hostIgnoreR | |||
productVersion = hostInfo.hasDefined(PRODUCT_VERSION) ? hostInfo.require(PRODUCT_VERSION).asString() : null; | |||
remoteConnectionId = hostInfo.hasDefined(RemoteDomainConnectionService.DOMAIN_CONNECTION_ID) | |||
? hostInfo.get(RemoteDomainConnectionService.DOMAIN_CONNECTION_ID).asLong() : null; | |||
this.stability = Optional.ofNullable(hostInfo.get(ModelDescriptionConstants.STABILITY).asStringOrNull()).map(Stability::valueOf).orElse(productConfig.getDefaultStability()); |
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 could use a comment explaining why orElse(productConfig.getDefaultStability()) is ok.
I think it is, but I'm tired and can't articulate why I think so. But in any case a future maintainer likely won't easily understand.
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.
Added clarifying comment.
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.
AIUI this basically means if the DC is running at a level other than its default, then a legacy HC can't join.
That's ok, as it's conservative, but otoh I think it should work fine. Anything on the DC that's not at 'default' stability must be associated with a later API version than whatever is on the legacy HC, so it would have to be transformed back to that legacy version.
This is what I was too tired to articulate yesterday. :) Thanks for adding the comment. I wrote the above two paragraphs for the sake of discussion in case I'm thinking about it wrong.
logging/pom.xml
Outdated
<groupId>*</groupId> | ||
<artifactId>*</artifactId> | ||
</exclusion> | ||
</exclusions> | ||
</dependency> |
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 changes to this file seem odd, since there are no other changes to this module.
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.
See: e081907
@@ -366,6 +367,12 @@ public static ServerEnvironmentWrapper determineEnvironment(String[] args, Prope | |||
} else { | |||
gitBranch = arg.substring(idx + 1); | |||
} | |||
} else if (arg.startsWith(CommandLineConstants.STABILITY)) { |
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.
For now, if ProductConfig.getDefaultStability() == DEFAULT this should fail. A similar check can be done in CommandLineArgumentUsageImpl to guard allowing the argument.
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.
Fixed. Also removed from command line usage, when stability has only 1 possible value.
@@ -445,6 +446,12 @@ public static HostControllerEnvironmentWrapper determineEnvironment(String[] arg | |||
} else if (arg.equals(CommandLineConstants.SECMGR)) { | |||
// Enable the security manager | |||
securityManagerEnabled = true; | |||
} else if (arg.startsWith(CommandLineConstants.STABILITY)) { |
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.
See my comments in server module Main.
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.
Fixed. Also removed from command line usage, when stability has only 1 possible value.
@pferraro I've made some minor comments but in general looks good. |
This comment was marked as outdated.
This comment was marked as outdated.
@@ -127,6 +133,7 @@ private HostInfo(final ModelNode hostInfo, DomainHostExcludeRegistry hostIgnoreR | |||
productVersion = hostInfo.hasDefined(PRODUCT_VERSION) ? hostInfo.require(PRODUCT_VERSION).asString() : null; | |||
remoteConnectionId = hostInfo.hasDefined(RemoteDomainConnectionService.DOMAIN_CONNECTION_ID) | |||
? hostInfo.get(RemoteDomainConnectionService.DOMAIN_CONNECTION_ID).asLong() : null; | |||
this.stability = Optional.ofNullable(hostInfo.get(ModelDescriptionConstants.STABILITY).asStringOrNull()).map(Stability::valueOf).orElse(productConfig.getDefaultStability()); |
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.
AIUI this basically means if the DC is running at a level other than its default, then a legacy HC can't join.
That's ok, as it's conservative, but otoh I think it should work fine. Anything on the DC that's not at 'default' stability must be associated with a later API version than whatever is on the legacy HC, so it would have to be transformed back to that legacy version.
This is what I was too tired to articulate yesterday. :) Thanks for adding the comment. I wrote the above two paragraphs for the sake of discussion in case I'm thinking about it wrong.
host-controller/src/main/java/org/jboss/as/host/controller/Main.java
Outdated
Show resolved
Hide resolved
Thanks @pferraro |
https://issues.redhat.com/browse/WFCORE-6221
This pull request formalizes the concept of a quality level in the WildFly kernel based on the general strategy described in WFCORE-6221.
In general, a subsystem associate features within its management model with a specific Quality. During management registration, only those features enabled by the quality level of the server will be registered.
See the test case here for an example of how I imagined this might work: https://github.com/pferraro/wildfly-core/tree/WFCORE-6221/subsystem-test/tests/src/test/java/org/jboss/as/subsystem/test/quality
Please scrutinize. Tell me what you like and do not like.