[COMMUNITY] [WFCORE-6830] Add four new attributes to the HTTP management interface #6166

darranl · 2024-09-06T14:43:02Z

https://issues.redhat.com/browse/WFCORE-6830

This is a continuation of the addition of the new schema added under:
#6152

https://issues.redhat.com/browse/WFCORE-6829

This is also a continuation of #6004 which is the actual implementation. This PR focuses on adding the four configuration options to the HTTP management interface.

 - backlog
 - no-request-timeout
 - connection-high-water
 - connection-low-water

WildFly Proposal: wildfly/wildfly-proposals#595

Also update the SupportedNamespaceParsingTestCases to include stability level tests.

… configurations. Ideally we should be able to run these domain test with the default stability level.

…this parser.

…so version, URI, and Stability are all accessible.

- backlog - no-request-timeout - connection-high-water - connection-low-water These values could previously be set using system properties, for backwards compatibility the system properties are retained.

… test using attributed defined on the mode.

…ributes in both the standalone.xml and host.xml configuration files.

… add handler. Also log an INFO message that a deprecated system property is in use with info about the alternative.

bstansberry

@darranl I've only got a couple minor things.

bstansberry · 2024-09-16T21:19:57Z

...er/src/main/java/org/jboss/as/controller/management/BaseHttpInterfaceResourceDefinition.java

+
+    public static final SimpleAttributeDefinition NO_REQUEST_TIMEOUT = new SimpleAttributeDefinitionBuilder(ModelDescriptionConstants.NO_REQUEST_TIMEOUT, ModelType.INT, true)
+            .setAllowExpression(true)
+            .setDefaultValue(new ModelNode(60000))


Please add a setMeasurementUnit

This is added, BTW @bstansberry GitHub says FAHRENHEIGHT was your contribution? 😄

Somebody hacked me!

Not guilty! Except of careless cutting and pasting!

https://access.redhat.com/webassets/avalon/d/Red_Hat_JBoss_Operations_Network-3.1-API-en-US/files/domain/index.html?org/rhq/core/domain/measurement/MeasurementUnits.html

bstansberry · 2024-09-16T21:25:33Z

domain-http/interface/src/main/java/org/jboss/as/domain/http/server/ManagementHttpServer.java

@@ -252,6 +252,9 @@ private static ManagementHttpServer create(Builder builder) {
            }
        }

+        ROOT_LOGGER.debugf("Resource Constraints backlog=%d, noRequestTimeout=%d, connectionHighWater=%d, connectionLowWater=%d",


"Resource Constraints" is pretty vague in a message from a broadly scoped thing like ROOT_LOGGER.

@bstansberry although this is ROOT_LOGGER the category logged at is actually "org.jboss.as.domain.http.api.undertow" - I could change the phrasing slightly to something like "Concurrent connection limits" but I think the log category does cover that this is the Undertow HTTP API.

Actually may go for "HTTP Management API Connection Constraints"

Added, I went for the second option to add some clarity.

Oh sorry; for some reason I thought this was in controller or server or something. Anyway, the change is an improvement.

yersan · 2024-09-17T07:48:31Z

testsuite/domain/src/test/resources/base-server-config.xml

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>

-<server xmlns="urn:jboss:domain:20.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:jboss:domain:20.0 wildfly-config_20_0.xsd">
+<server xmlns="urn:jboss:domain:community:20.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:jboss:domain:community:20.0 wildfly-config_community_20_0.xsd">


I think our last agreement was not to have community schemas in these configuration files. If we have specific community stability level tests, we should reload the server into that stability level before running those tests. Otherwise, we will need to convert these configuration files back into default schema when creating the product branches and we want to avoid that.

@yersan FYI we have the following Jira issue to come back and follow up on this https://issues.redhat.com/browse/WFCORE-6972 this is why the commit message says it is a temporary change. I have bumped the follow up Jira issue to Critical so we can come back to these as these configuration files already contain community schema references so this is not something new.

Added to this this is becoming a real rabbit hole which is intertwined by tests @ehsavoie is also evolving further so I think we need to get this PR in and Emmanuel's PR in then come back and fix up this testsuite.

There is also further information on #6153 regarding the types of failure we see if we try to just use the default stability level.

FYI this was my closing conclusion on the chat thread on the 2nd Sept:

On the original discussion for this thread, as these tests are also being refactored for their own feature promotion I think I am going to take a step back on that and temporarily just add another community namespace to the configuration schema just so I can progress with my feature.

Once my feature and Emmanual's promotion is ready we can look at how to combine at the end to complete some of the thoughts here.

Added to this this is becoming a real rabbit hole which is intertwined by tests @ehsavoie is also evolving further so I think we need to get this PR in and Emmanuel's PR in then come back and fix up this testsuite.

Ok, got it, I mistakenly thought that the changes here were to accommodate the ManagementInterfaceResourcesTestCase, but they are just because after incorporating the community schema we need to bypass the flaws in the ManagementReadXmlTestCase test case, which is not able to work with schemas in default stability level.

However, independently of that, since this ManagementInterfaceResourcesTestCase now is testing changes that are only in community stability level, should not be already prepared to reload the server into the expected stability level? That would make it to skip these tests if the community stability level is not available and the server was provisioned to default stability level.

I am fine if we postpone this to later, but just wanted to crosscheck if you agreed with that plan.

FYI I just added an inline comment where the test checks the attributes are available to decide if that part of the test should run / be skipped.

bstansberry · 2024-09-17T18:37:50Z

I've merged the creaper upgrade in full WF and manually started new runs of the 3 "Full integration" jobs that failed due to the need for that.

…meout.

bstansberry

@darranl You've addressed my concerns, but from the CI results it seems the changed ManagmentInterfaceResourcesTestCase is unstable.

…ection constraints for the HTTP management API.

darranl · 2024-09-19T09:52:00Z

The test failures are odd, checking the line numbers that is actually the existing version of the test failing not the new management model configured version.

darranl · 2024-09-19T10:51:42Z

...rc/test/java/org/wildfly/core/test/standalone/mgmt/ManagementInterfaceResourcesTestCase.java

+                cli.sendLine(String.format(SYSTEM_PROPERTY_ADD, CONNECTION_LOW_WATER_PROPERTY, 3));
+                cli.sendLine(String.format(SYSTEM_PROPERTY_ADD, NO_REQUEST_TIMEOUT_PROPERTY, noRequestTimeout));
+            } else {
+                cli.sendLine(String.format(HTTP_INTERFACE_READ_ATTRIBUTE, BACKLOG_ATTRIBUTE), true);


@yersan FYI at this point we do not need to reload the server for the test as our testsuites already run the server at the default stability level of the feature pack so here it is already running at the community level.

This is going to be a part of a bigger challenge, our WildFly and WildFly Core testsuites are going to need to remain runnable with the default configuration for the feature pack under test and for both these projects that will mean we will have community schemas in the configuration and we can't assume to run using the stability level of default.

To bridge this gap I have added this test to check if the attribute is present in the management model, if the attribute does not exist we assume we are running at a stability level that does not support it and skip that part of the test.

This is going to be a part of a bigger challenge, our WildFly and WildFly Core testsuites are going to need to remain runnable with the default configuration for the feature pack under test and for both these projects that will mean we will have community schemas in the configuration and we can't assume to run using the stability level of default.

@darranl My understanding about the initial approach to challenge this was that community schemas were not going to be present (they were merged but the plan is to rollback them) and any test that is not meant to run for "default" stability level should reload the server to the stability level they need.

So, hence, if we provision to "community" stability level, the test will reload the server to the "community" stability level (if it is not already running on it) and the test will run. If we provision to "default" then the test will be skipped because the server will not support a reload to "community" stability level. This approach is what I had in mind that we were going to follow. If that is not correct or we require a different approach we should discuss what to do in Zulip.

To bridge this gap I have added this test to check if the attribute is present in the management model

Ok, that's another approach we can use for this test.

@yersan this test is unrelated to the issues with the existing schemas, this is a test that already exists in the manualmode testsuite and already tests using a server provisioned at the default stability level of the feature pack i.e. community.

@darranl I think we are talking about different things. Now, with your latest changes skipping the test if the attribute is not present, the potential issue this PR could have introduced when we are converting to product branches is gone. That's fine. However, I was talking about what was the approach we planned to follow to face these cases: reload the server to the desired stability level if that stability level is not "default".

@yersan I have not added anything to this test to get the server to run at the Commnity stability level, in this module the server already runs at the default stability level of the feature pack there is nothing to need a reload to community as it is already at that stability level.

I have not added anything to this test to get the server to run at the Commnity stability level, in this module the server already runs at the default stability level of the feature pack there is nothing to need a reload to community as it is already at that stability level.

@darranl Without your change checking whether the attribute is present, we should have tweaked this test as soon as we provision the server to "default" stability level to create the product branches. However, the approach we defined to face this case was to reload the server to the desired stability level if that stability level is not "default". This PR is now approaching this problem differently, which is fine, but I only wanted to explain what we planned to do.

@yersan I am still not following the place reloading to the community stability level would have a place on that workflow.

In this repository the server is provisioned using the community stability level and runs using the community stability level - reloading to the community stability level would be a noop as we end up where we started.

Once we are in conversion branches and beyond the default stability level doesn't just become the default stability level it becomes the only stability level so the reload to community again serves no purpose.

@darranl Yes, let me try to explain this. If we use our current tasks to reload to a desired stability level, see https://issues.redhat.com/browse/WFCORE-6728:

The reload will be a noop if the target server is already running on the desired stability level; so in the end there won't be any overhead if we try to reload to "community" stability level a server that is already running at "community" level. Hence, in a nonproduct branch, the test will be executed as we were "not doing" a reload.

If the server does not support the desired stability level, for example, at product branches reload to "community" stability level won't be possible because the running server will only support "default" stability level; the test case will be ignored.

The final effect will be that the same code will make the test be ignored at product branches but be executed in nonproduct branches. Test configuration files should be kept using "default" stability level schemas. This "default" schemas can be read by a server that is running on "community" stability level or in "default" stability level.

Ignoring the test if the attributes don't exist is also a valid approach, in the end, what we should use will depend a lot on the test case itself. Having specific tests for community-only tests could make the code easier to promote to a higher stability level, since it promotion would be removing the task that reloads the server to the desired stability level, but I guess that should be done on more complex test cases. Its implementation is more complex ... and I don't know yet if it will cover all the cases or if we are missing something in that puzzle.

@yersan thank you for the clarification, I understand where you are coming from. I can move to that pattern in a follow up PR.

We maybe should add something to the table in the wildfly-proposals repo for the feature process describing the testing requirements (I need to check what we already have).

I think we should probably avoid going into the reload detail too much but I can see we should add some guidance discussing that all new tests should use the appropriate server setup task for the target stability level.

We maybe should add something to the table in the wildfly-proposals repo for the feature process describing the testing requirements (I need to check what we already have).

yes, that maybe could help

bstansberry · 2024-09-19T14:34:37Z

...rc/test/java/org/wildfly/core/test/standalone/mgmt/ManagementInterfaceResourcesTestCase.java

+                    cli.sendLine(String.format(SYSTEM_PROPERTY_REMOVE, CONNECTION_LOW_WATER_PROPERTY));
+                    cli.sendLine(String.format(SYSTEM_PROPERTY_REMOVE, NO_REQUEST_TIMEOUT_PROPERTY));
+                } else {
+                    cli.sendLine(String.format(HTTP_INTERFACE_UNDEFINE_ATTRIBUTE, BACKLOG_ATTRIBUTE));


I think this needs handling similar to the L163/172 block. In default stability this will fail and the controller.stop will not be called.

Ah good catch.

@bstansberry actual if you scroll up slightly to line 166 I do call controller.stop() the moment the decision is made to not proceed with that test.

Ok, and you return.

…tInterfaceResourcesTestCase from the bootable jar profile as it relies on admin mode.

There is already a mechaninsm to skip the test on product branches

yersan · 2024-09-20T15:00:51Z

Thanks @darranl @bstansberry @pedro-hos

darranl added 8 commits September 2, 2024 14:00

[WFCORE-6829] Add a new 'urn:jboss:domain:community:20.0' schema.

5cddd78

Also update the SupportedNamespaceParsingTestCases to include stability level tests.

[WFCORE-6829] Temporarily add community qualifier to some domain test…

1be5d5c

… configurations. Ideally we should be able to run these domain test with the default stability level.

[WFCORE-6829] Fork StandaloneXml_18 to 20 as changes are planned for …

a3e8817

…this parser.

[WFCORE-6829] Pass VersionedNamespace into ManagementXmlReaderWriter …

be14f48

…so version, URI, and Stability are all accessible.

[WFCORE-6830] Add four new attributes to the HTTP management interface:

986b12e

- backlog - no-request-timeout - connection-high-water - connection-low-water These values could previously be set using system properties, for backwards compatibility the system properties are retained.

[WFCORE-6830] Add XML handling for the four new management attributes.

768fc44

[WFCORE-6830] Update the ManagementInterfaceResourcesTestCase to also…

c74b661

… test using attributed defined on the mode.

[WFCORE-6830] Add test cases for handling the new community level att…

47e39e5

…ributes in both the standalone.xml and host.xml configuration files.

darranl added Feature This PR adds a new feature to WildFly missing-reqs This PR is missing external requirements before it can be merged labels Sep 6, 2024

[WFCORE-6830] Move checking of the legacy system property to the base…

8d4c67f

… add handler. Also log an INFO message that a deprecated system property is in use with info about the alternative.

darranl marked this pull request as ready for review September 15, 2024 14:38

darranl mentioned this pull request Sep 15, 2024

[Community] [WFLY-19715] Add documentaion for the new attributes added to control resource constraints for the HTTP management interface. wildfly/wildfly#18193

Merged

bstansberry requested changes Sep 16, 2024

View reviewed changes

yersan previously requested changes Sep 17, 2024

View reviewed changes

wildfly deleted a comment from wildfly-ci Sep 17, 2024

[WFCORE-6830] Add a MeasurementUnit of MilliSeconds for no-request-ti…

6486a31

…meout.

bstansberry reviewed Sep 18, 2024

View reviewed changes

[WFCORE-6830] Adjust the DEBUG message to indicate this is about conn…

d61bb68

…ection constraints for the HTTP management API.

darranl force-pushed the WFCORE-6830 branch from 10702fb to d61bb68 Compare September 19, 2024 09:44

darranl commented Sep 19, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

darranl added Critical 26.x labels Sep 19, 2024

bstansberry requested changes Sep 19, 2024

View reviewed changes

[WFCORE-6820] Exclude org.wildfly.core.test.standalone.mgmt.Managemen…

2a1ce05

…tInterfaceResourcesTestCase from the bootable jar profile as it relies on admin mode.

This comment was marked as outdated.

Sign in to view

bstansberry approved these changes Sep 19, 2024

View reviewed changes

yersan added ready-for-merge This PR is ready to be merged and fulfills all requirements and removed ready-for-merge This PR is ready to be merged and fulfills all requirements labels Sep 20, 2024

yersan added ready-for-merge This PR is ready to be merged and fulfills all requirements and removed missing-reqs This PR is missing external requirements before it can be merged labels Sep 20, 2024

yersan merged commit 42536dd into wildfly:main Sep 20, 2024
12 checks passed

[COMMUNITY] [WFCORE-6830] Add four new attributes to the HTTP management interface #6166

[COMMUNITY] [WFCORE-6830] Add four new attributes to the HTTP management interface #6166

Conversation

darranl commented Sep 6, 2024 • edited by yersan Loading

bstansberry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstansberry commented Sep 17, 2024

bstansberry left a comment

Choose a reason for hiding this comment

darranl commented Sep 19, 2024

Choose a reason for hiding this comment

yersan Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

yersan commented Sep 20, 2024

darranl commented Sep 6, 2024 •

edited by yersan

Loading

yersan Sep 19, 2024 •

edited

Loading