Skip to content
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-6731] Expose supported stability levels via the management API #5908

Closed
wants to merge 1 commit into from

Conversation

yersan
Copy link
Collaborator

@yersan yersan commented Mar 19, 2024

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Mar 19, 2024
@yersan yersan requested a review from pferraro March 20, 2024 10:02
Copy link
Contributor

@pferraro pferraro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor suggestions only.

@@ -485,6 +487,7 @@ public HostControllerEnvironment(Map<String, String> hostSystemProperties, boole
this.processType = processType;

this.stability = getEnumProperty(hostSystemProperties, STABILITY, this.productConfig.getDefaultStability());
this.stabilities = Collections.unmodifiableSet(productConfig.getStabilitySet());
Copy link
Contributor

@pferraro pferraro Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this set unmodifiable is the right thing to do, but better to do this directly in ProductConfig: https://github.com/wildfly/wildfly-core/blob/main/version/src/main/java/org/jboss/as/version/ProductConfig.java#L97
That way we prevent anyone from modifying this value, and every process env does not need to perform the same wrapping.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pferraro Done

* @return a set of stability levels
*/
public Set<Stability> getStabilities() {
return this.stabilities;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... that way this method (and t can just delegate to ProductConfig.getStabilities().

I would also consider defining this method in ProcessEnvironment to enforce consistency across subclasses.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also consider defining this method in ProcessEnvironment to enforce consistency across subclasses.

@pferraro @yersan This is a good idea but I think it's out of scope for this JIRA. Just because when I look at ProcessEnvironment it's an abstract class (not an interface) shares a lot of functionality with both of its subclasses, but stores and exposes no state itself and thus forces both subclasses to do so. If we're going to change that for this we should revisit all of it, which is out of scope.

I have no idea why it's done this way. My only guess is ProcessEnvironment was originally stateless and over time the two subclasses evolved independently and the kind of refactoring I'm discussing here was never in scope. :)

I filed https://issues.redhat.com/browse/WFCORE-6760

@darranl darranl requested a review from hpehl March 20, 2024 13:25
@darranl
Copy link
Contributor

darranl commented Mar 20, 2024

@hpehl Could you please also review, this exposed information will be useful to management clients.

Copy link
Contributor

@hpehl hpehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, LGTM. Thanks @yersan

AIUI, this PR adds two properties to the management model:

  1. current stability level
  2. possible stability levels

Some questions:

  • Can the user change the stability level at runtime (e.g., using an operation), or is this only possible at startup time when using a command-line option/system property?
  • How can I, as a client, detect whether a specific resource, operation, or attribute is only supported for a specific stability level?

@yersan
Copy link
Collaborator Author

yersan commented Mar 22, 2024

@hpehl thanks for reviewing it.

Can the user change the stability level at runtime (e.g., using an operation), or is this only possible at startup time when using a command-line option/system property?

There is an ongoing Feature that would allow restarting the server at a different stability level: #5895

How can I, as a client, detect whether a specific resource, operation, or attribute is only supported for a specific stability level?

When you describe a resource/operation, now the resources provide a field that specifies the higher stability lever where this resource is available. This attribute name is "stability".

If the resource / attribute / operation is not suitable to run in the current server stability level, then the resource will not be available.

For example, if the operation A is configured to work on "preview" stability level, it won't be available if you run the server in "community" stability level. "community" has a higher stability level than "preview".

The operation A should report "preview" as its higher stability level where it can work, and you have to run the server in preview to be able to describe it.

Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yersan Just a couple minor points.

@@ -304,6 +304,7 @@ public ProcessType getProcessType() {
private final boolean startGracefully;
private final GitRepository repository;
private final Stability stability;
private final Set<Stability> stabilities;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field shouldn't be needed as this.productConfig can be called to get the stabilities set.

@@ -234,6 +235,7 @@ public class HostControllerEnvironment extends ProcessEnvironment {

private final RunningMode initialRunningMode;
private final Stability stability;
private final Set<Stability> stabilities;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field shouldn't be needed as this.productConfig can be called to get the stabilities set.

@bstansberry
Copy link
Contributor

Superseded by #5917 which resolves the conflict and deals with my minor requests.

@bstansberry
Copy link
Contributor

Thanks @yersan @pferraro and @hpehl

@yersan yersan deleted the WFCORE-6731 branch April 1, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants