OpenLiberty / ci.maven

Maven plugins for managing Liberty profile servers #devops
Apache License 2.0
130 stars 91 forks source link

Duplicate jvm.options entries generated from a single -Dliberty.jvm.xyz=123 system property in v3.10 #1778

Closed scottkurz closed 11 months ago

scottkurz commented 11 months ago

Running mvn liberty:run -Dliberty.jvm.debug="-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777" (to attach the debugger to the server) I got an error and noticed the reason was duplicate entries in the generated jvm.options:

$ cat .../jvm.options
# Generated by liberty-maven-plugin
-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777
-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7777

The problem also happens with dev mode.

LIKELY CAUSE

I'm pretty sure the change here is responsible.

In debugging into StartDebugMojoSupport.loadLibertyConfigFromProperties()

    private void loadLibertyConfigFromProperties() {
        loadLibertyConfigFromProperties(project.getProperties());
        loadLibertyConfigFromProperties(System.getProperties());
    }

I can see the problem.

In the good, normal case (e.g. with v3.9) only the second load here, from the System properties, results in the addition of a new entry.

                    case JVM:        jvmMavenProps.add(value);

In the bad case, I get an entry from the 1st load from project.getProperties() and then a 2nd from the System properties. (Because jvm.options are a list, not a key-value map, this doesn't get merged into one entry like the other config types).

If I just run liberty:create there's no problem. If I execute 'run' or 'dev' though, we do a second copyConfigFiles AFTER having executed the new (in v3.10) setContainerEngine() call.

SOLUTION

The answer might require doing something more like a clone of these properties? Seems like we have a test gap too.

cherylking commented 11 months ago

So I understand in this case we shouldn't add two entries for one property, but in most cases I don't think it would cause a failure. This debug setting is probably somewhat special.

For other options, I believe the last one in the generated file will take precedence (and I think we depend on that behavior for when a particular jvm option is specified both in the pom.xml and from the command line). We even have a test case for it showing that both options appear in the generated file, but the one we expect to take precedence appears last.

scottkurz commented 11 months ago

Interesting..yeah from https://stackoverflow.com/questions/2740725/duplicated-java-runtime-options-what-is-the-order-of-preference it does look like the last one wins... and we can see this with -Dliberty.jvm.print="XX:+PrintFlagsFinal".

I guess a case could be made that we could (should?) eliminate exact duplicates. This way, for an arg that couldn't be duplicated, like the jdwp debug thing, you can have it as each of a Maven property & system property. But, you'd still have a similar issue if you, say, used the two properties with different ports in each (so they wouldn't be identical), and that doesn't seem like it'd be that helpful to matter.

cherylking commented 11 months ago

The problem stemmed from code that modified the Properties that were returned from project.getProperties(). That was not the intent of the code. However, I will also change the existing code that adds jvm option entries to the List so that duplicate entries are not added.