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-6815][Community] Utility to reload a Host Controller to a different stab… #570

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

yersan
Copy link
Contributor

@yersan yersan commented May 13, 2024

…ility level in the testsuite

Jira issue: https://issues.redhat.com/browse/WFCORE-6815

@yersan yersan requested a review from kabir May 13, 2024 16:24
@yersan
Copy link
Contributor Author

yersan commented May 20, 2024

Hello @kabir , any chance you can take a look at this one as SME for a community feature?
@jamezp Would you agree to be the external third-party contributor and review this one from your side?

@jamezp
Copy link
Member

jamezp commented May 20, 2024

@yersan Yes, but as an FYI I'm on PTO for the a week starting Wednesday so if this is time sensitive, I might not be that helpful :)

@yersan
Copy link
Contributor Author

yersan commented Jun 4, 2024

Hi @kabir / @jamezp Any chance to look at this proposal?

The lack of ability to segregate the tests is blocking work for the conversion branches, since we need to properly avoid the execution of tests that are not meant for the default stability level. If possible, I would like to unlock this. Thanks!

- The operation will be registered at stability level 'community' at present.
- The operation is based on the reload operation, and adds a parameter called `stability`. This parameter takes the stability level we want to reload the server to.
- The operation also supports all the parameters supported by the standard reload operation, included the restart-servers parameter.
- Restarting a host controller with a different stability level without restarting the servers to get the value of the new stability level is not an useful operation, for simplicity and consistency with the reload operation, the restart-servers parameter will be allowed.
Copy link
Member

Choose a reason for hiding this comment

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

Should this say the "restart-servers parameter will not be allowed"? I'm asking because the first part of the sentence makes it not sound useful.

If the wording is intended, is the host controller meant to act as the minimum stability level for servers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamezp

Should this say the "restart-servers parameter will not be allowed"?

It is currently allowed, but it does not make sense to reload the host to a certain stability level and not restart your servers. If you do that, you could have your host running at one stability level, but your server running at another. I think that could be a problem. Maybe the simplest approach here would be to remove the restart-servers attribute from the reload-enhanced operation and restart the servers always when this operation is used.

I initially wanted to avoid these differences and assume you should know what you are doing when using the reload-enhanced, after all it is for our testsuite, That would bring more flexibility, but maybe that's not a good idea. WDYT?

is the host controller meant to act as the minimum stability level for servers?

The Host Controller sends its stability level to the managed servers with the managed server boot command, that's all. It dictates what stability level the managed server will run once it gets restarted. So, if you do not restart your servers, you could have the Host Controller running on preview and the managed servers still on community, I guess that could be problematic somehow, but I'm not sure.

Maybe we should start as restrictive as we can, e.g. remove the restart servers from reload enhanced and always restart them to avoid problems, and if there is a need, open it to allow restarting the server to get more flexibility.

Copy link
Member

Choose a reason for hiding this comment

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

@yersan IMO then I think the restart-servers attribute should be removed from the new operation. I can understand not wanting the difference, However, attributes on operations that don't exist are ignored so it won't really hurt anything and makes it safer I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamezp I've removed the restart-servers attribute from the reload-enhanced operation. Let me know how it looks like now

@yersan yersan force-pushed the WFCORE-6815 branch 2 times, most recently from 041b5ea to a3c59e4 Compare June 7, 2024 09:07
@yersan
Copy link
Contributor Author

yersan commented Jun 10, 2024

@kabir Let me know if there is anything else to add here, otherwise, approve if you agree, thanks!

@yersan
Copy link
Contributor Author

yersan commented Jun 17, 2024

@kabir Friendly remainder, this one is waiting for your approval to get it merged, please take a look, thanks!

@bstansberry bstansberry changed the title [WFCORE-6815][Community] Utility to reload domain to a different stab… [WFCORE-6815][Community] Utility to reload a Host Controller to a different stab… Jul 1, 2024
@bstansberry bstansberry merged commit 4e874fb into wildfly:main Jul 2, 2024
1 check passed
@bstansberry
Copy link
Contributor

Thanks @yersan, @kabir and @jamezp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants