[WFCORE-6843] Logging a WARN if a deployment's runtime name doesn't have an extension #6340

khermano · 2025-02-14T08:14:00Z

Issue: https://issues.redhat.com/browse/WFCORE-6843

yersan

@khermano Check the "Check non-i18n logging" failure, you have to use the proper log classes and add the message using @message, see for example DomainControllerLogger, otherwise your warning messages won't be i18n

khermano · 2025-02-17T08:00:03Z

Thank you for your very quick response and sorry for the late response, I overlooked that. I will look at it now. Thanks for the information. :)

server/src/main/java/org/jboss/as/server/logging/ServerLogger.java

...-controller/src/main/java/org/jboss/as/domain/controller/logging/DomainControllerLogger.java

...va/org/jboss/as/domain/controller/operations/deployment/ServerGroupDeploymentAddHandler.java

yersan · 2025-02-24T09:26:05Z

server/src/main/java/org/jboss/as/server/deployment/DeploymentHandlerUtil.java

+                    if (!deploymentUnitName.contains(".")) {
+                        DEPLOYMENT_NAMECHECK_LOGGER.deploymentsRuntimeNameWithoutExtension(managementName, deploymentUnitName);
+                    }


One last detail, we need to do the same for the other methods as redeploy, replace

The same applies to Domain Mode handlers, we need to cover the redeploy and replace

Hi @yersan,
I looked at it, and I pushed my changes, but I would like to ask about redeploy in domain mode.
I am not entirely sure if I got that right, but I assume that in domain mode, when talking about redeployment, we mean operation deployment-overlay, and it has to do with DomainDeploymentOverlayRedeployLinksHandler, right?
If that is the case, I think it is not possible to change the runtime name of the deployment, or am I mistaken?

when talking about redeployment, we mean operation deployment-overlay, and it has to do with DomainDeploymentOverlayRedeployLinksHandler, right?

It is not related to the overlay functionality. We are talking here about the redeploy operation under /server-group=*/deployment=* resource. The handler is ServerGroupDeploymentRedeployHandler but according to the code, it seems that the operations there are redirected directly to the managed servers, so I think we are not going to be able to do something easy with it. Being a redeploy that does not change the runtime name, I think we will be ok with it. In addition, the server handler will be already prepared to log the trace in the managed server JVM, so, that's fine with me.

…ave an extension

yersan

@khermano I took anothe look today, domain mode is a bit more tricky, I've added some comments related.

In addition, you also have to check the ServerGroupDeploymentReplaceHandler, which is in charge of /server-group=*:replace-deployment operation and acepts a runtime-name as an attribute.

yersan · 2025-03-11T17:30:20Z

...va/org/jboss/as/domain/controller/operations/deployment/ServerGroupDeploymentAddHandler.java

@@ -94,6 +95,9 @@ static void validateRuntimeNames(String deploymentName, OperationContext context
            Resource root = context.readResourceFromRoot(PathAddress.EMPTY_ADDRESS);
            ModelNode domainDeployment = root.getChild(PathElement.pathElement(DEPLOYMENT, deploymentName)).getModel();
            String runtimeName = getRuntimeName(deploymentName, deployment, domainDeployment);
+            if (!runtimeName.contains(".")) {
+                DEPLOYMENT_NAMECHECK_LOGGER.deploymentsRuntimeNameWithoutExtension(deploymentName, runtimeName);
+            }


As it is now, this log warning will be invoked twice per each deploy operation. Notice that this method, validateRuntimeNames, is invoked from two different handlers that are executed together when we deploy in domain mode. The deploy operation in domain mode consists of an add operation followed by a deploy. Here, ServerGroupDeploymentDeployHandler and ServerGroupDeploymentAddHandler are both involved during the deployment, and both are calling validateRuntimeNames.

We have to avoid logging the warning twice. In addition, since this is also dependent on the affected server group, we have to include the server group name as part of the log warn message.

yersan · 2025-03-11T18:04:28Z

server/src/main/java/org/jboss/as/server/deployment/DeploymentHandlerUtil.java

+                    if (!deploymentUnitName.contains(".")) {
+                        DEPLOYMENT_NAMECHECK_LOGGER.deploymentsRuntimeNameWithoutExtension(managementName, deploymentUnitName);
+                    }


when talking about redeployment, we mean operation deployment-overlay, and it has to do with DomainDeploymentOverlayRedeployLinksHandler, right?

It is not related to the overlay functionality. We are talking here about the redeploy operation under /server-group=*/deployment=* resource. The handler is ServerGroupDeploymentRedeployHandler but according to the code, it seems that the operations there are redirected directly to the managed servers, so I think we are not going to be able to do something easy with it. Being a redeploy that does not change the runtime name, I think we will be ok with it. In addition, the server handler will be already prepared to log the trace in the managed server JVM, so, that's fine with me.

khermano · 2025-03-27T09:09:07Z

Hello @yersan ,
I looked into it and I changed the warning (the commit is not here yet) so it also contains the name of the server group, but I am not sure about removing one of the warnings.
I know it looks like a mistake if the same warning is printed twice in the row, but it's because it is associated with two different operations - ADD and DEPLOY, and I do not think I can remove it from either of them without risking in some cases the runtime name will not be checked at all.
Should I try to implement some counter or check the output or something?
Regarding REPLACE operation, the runtime name attribute in this operation exists, but I can't see it used anywhere. Even when I tried to use this operation with a new runtime-name, the operation for replacement was successful but the new runtime name was never used.
I would suggest that I will aplicate the check anyway and create an issue regarding this unused attribute. What do you think? Or is there something that I overlooked regarding all of this?
Thank you in advance for your help and I am really sorry that it took so long for me to look into it.

yersan · 2025-04-01T10:10:55Z

@khermano:

I know it looks like a mistake if the same warning is printed twice in the row, but it's because it is associated with two different operations - ADD and DEPLOY, and I do not think I can remove it from either of them without risking in some cases the runtime name will not be checked at all.

The Add operation adds the deployment as a server group resource, but the actual deployment on the servers will occur only if you add the resource with enable=true. I think that's the key to distinguishing when we have to show the log trace: the add handler should display the log only if it is executed with enable=true. The deploy handler should always display the log.

Should I try to implement some counter or check the output or something?

You shouldn't, those approaches tend to be more fragile and complex to cope with all the cases, in addition, we do not keep any memory facility in the operation context, and the handlers should work independently.

Regarding REPLACE operation, the runtime name attribute in this operation exists, but I can't see it used anywhere. Even when I tried to use this operation with a new runtime-name, the operation for replacement was successful but the new runtime name was never used.
I would suggest that I will aplicate the check anyway and create an issue regarding this unused attribute. What do you think? Or is there something that I overlooked regarding all of this?

Ideally, we have to create an issue describing what you have found with the replace operation, could you do that? it should have a short description and how to reproduce it.

WFCORE-6843 is an enhancement, and the one you found is a bug. The bug should be blocking the enhancement, we should fix the bug first, and then add the WFCORE-6843 changes on top of the changes that fix the bug. That can be done in a single PR with two commits, or fix the bug, wait until gets resolved and then continue with WFCORE-6843

github-actions · 2025-05-17T01:01:20Z

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

yersan requested changes Feb 14, 2025

View reviewed changes

khermano force-pushed the WFCORE-6843_from_main branch from 2f2cb16 to ac61cc4 Compare February 17, 2025 11:18

yersan requested changes Feb 17, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

khermano force-pushed the WFCORE-6843_from_main branch from ac61cc4 to dac24f1 Compare February 18, 2025 11:51

This comment was marked as outdated.

Sign in to view

yersan requested changes Feb 24, 2025

View reviewed changes

[WFCORE-6843] Logging a WARN if a deployment's runtime name doesn't h…

ef17283

…ave an extension

khermano force-pushed the WFCORE-6843_from_main branch from dac24f1 to ef17283 Compare March 4, 2025 08:02

yersan requested changes Mar 11, 2025

View reviewed changes

github-actions bot added the Stale label May 17, 2025

Not complete changes

fef761f

khermano marked this pull request as draft June 10, 2025 12:03

github-actions bot removed the Stale label Jun 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WFCORE-6843] Logging a WARN if a deployment's runtime name doesn't have an extension #6340

[WFCORE-6843] Logging a WARN if a deployment's runtime name doesn't have an extension #6340

Uh oh!

khermano commented Feb 14, 2025

Uh oh!

yersan left a comment

Uh oh!

khermano commented Feb 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

yersan Feb 24, 2025 •

edited

Loading

Uh oh!

khermano Mar 4, 2025

Uh oh!

yersan Mar 11, 2025

Uh oh!

yersan left a comment

Uh oh!

yersan Mar 11, 2025

Uh oh!

yersan Mar 11, 2025

Uh oh!

khermano commented Mar 27, 2025

Uh oh!

yersan commented Apr 1, 2025

Uh oh!

github-actions bot commented May 17, 2025

Uh oh!

Uh oh!

[WFCORE-6843] Logging a WARN if a deployment's runtime name doesn't have an extension #6340

Are you sure you want to change the base?

[WFCORE-6843] Logging a WARN if a deployment's runtime name doesn't have an extension #6340

Uh oh!

Conversation

khermano commented Feb 14, 2025

Uh oh!

yersan left a comment

Choose a reason for hiding this comment

Uh oh!

khermano commented Feb 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

yersan Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khermano Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

yersan Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

yersan left a comment

Choose a reason for hiding this comment

Uh oh!

yersan Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

yersan Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

khermano commented Mar 27, 2025

Uh oh!

yersan commented Apr 1, 2025

Uh oh!

github-actions bot commented May 17, 2025

Uh oh!

Uh oh!

yersan Feb 24, 2025 •

edited

Loading