[Bug]: Invalid order of -XX properties in KAFKA_JVM_PERFORMANCE_OPTS variable

[jakubmalek]

Bug Description

The ordering of JVM performance options is not guaranteed, due use of unordered Map<String, String> in JvmOptions class.
As a consequence following configuration:

jmxOptions:
  -XX:
    UnlockDiagnosticVMOptions: true
    PrintNMTStatistics: true

can result in JVM startup failure when JVM flags are misordered:
java -XX:+PrintNMTStatistics -XX:+UnlockDiagnosticVMOptions

Error: VM option 'PrintNMTStatistics' is diagnostic and must be enabled via -XX:+UnlockDiagnosticVMOptions.
Error: The unlock option must precede 'PrintNMTStatistics'.
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

Steps to reproduce

Configure jvmOptions with to enable UnlockDiagnosticVMOptions and PrintNMTStatistics flags.

Expected behavior

The ordering of performance options should guaranteed.

Strimzi version

0.31.1

Kubernetes version

1.24.9

Installation method

Helm chart

Infrastructure

Azure

Configuration files and logs

No response

Additional context

No response

[scholzj]

IIRC, ordered maps are not supported by YAML / CRDs. So I think this might not be possible. Have you tried directly defining the environment variable using the .spec.kafka.template section as a String?

[jakubmalek]

That is indeed a good point, I forgot about possible issues with the ordering inside the resulting k8s manifest itself.
I'm not sure how many of those VM -XX options requires specific order, but I think the easiest way out (without breaking changes to CRD) would be to include such options in separated configuration section.

But anyway, was able to get it running with proposed use .spec.kafka.template thanks for the hint @scholzj !

spec:
  template:
    connectContainer:
      env:
        - name: JAVA_TOOL_OPTIONS
          value: -XX:+UnlockDiagnosticVMOptions

[scholzj]

Triaged on 1.6.2023: It might help to use LinkedHashMap on our side instead of HashMap. It has to be investigated if this really helps or if the ordering breaks even before (e.g. in Kubernetes). If that does not help, API changes would be the only option.

[fvaleri]

The -XX property is defined as a JSON schema object, which is parsed as an unordered set of properties mapping a string to a value. As @scholzj said, the ordering information is lost inside Kubernetes, so the only option would be to change the schema to use an array. Alternatively, we can simply document the JAVA_TOOL_OPTIONS workaround.

[scholzj]

Ok, so I guess we can re-triage it next week with this new information. I will update the labels.

[jakubmalek]

One solution that came to my mind is to apply sorting of XX arguments in ModelUtils#jvmPerformanceOptions method. The sorting can be based on a set of "prioritized" parameters like UnlockDiagnosticVMOptions, that should be placed at the beginning.
The simplest solution could include hardcoded list of "prioritized" flags in more elaborate implementation, the list could be configurable by the user - but I'm not sure if it's really needed.

[scholzj]

My understanding is that the options might differ for different versions and JDKs - and likely their dependencies as well. So I wonder if hardcoding such logic might break it for users who use other JDKs. But it would be an option to consider.

[tombentley]

Triaged on 15/06/2023: We should investigate ensuring that UnlockDiagnosticVMOptions is the first argument if it's present. We think this is the only JVM option where the ordering matters, so it doesn't seem like we're setting a difficult-to-maintain precedent in special-casing this one. It might be necessary to do this in the bash script used to start the broker.

[jakubmalek]

I created PR with my proposal of hardcoded priority list used for sorting.
If you prefer to go with bash-script approach feel free to decline it.