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

PAYARA-3880 Fixed JVM options as semi-colon separated list parameter #4002

Merged
merged 2 commits into from
Jun 5, 2019

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Jun 3, 2019

Description

When JVM options are passed to commands like the create-jvm-options command the options are compacted into a single String with : as delimiter between the options.

Depending on the view of the caller JVM options are lists or maps. Processing is partly as list, partly as maps. Even when the processing assumes maps a caller might conceptually use lists in which case the keys of an input map contain the full option including values, the map entry then is empty.

Dependencies

Lately changes were made in #3887, #3957 and #3536 to address other corner cases. One side effect was #3992.

Reviewer notes

This PR is the attempt to satisfy all requirements (including those of the referenced PRs/JIRA issues) with a general processing function that does not have corner case exceptions as they have shown to cause problems elsewhere.

To allow unit testing of the processing function I had to extract it to a class that is free of static references to facilities that aren't relevant for the test but would require some form of context initialisation. Therefore I extracted it to ResourceUtil.

Testing performed

Similar to the processing function the tests were moved to the appropriate test class for the new location. Existing tests were made into junit tests and extended with further test cases. The tests now cover 100% of the processing function.

For integration I further performed manual testing:

Admin GUI:

  • edit -Xmx512m to -Xmx1024m, save
  • add an option of type key=value, save
  • remove the value part of the added option (keeping the = sign), save

Command line:
Using create-jvm-options as documented here https://docs.payara.fish/documentation/payara-server/server-configuration/jvm-options.html to try same key-values tested in admin GUI.

REST API:
Sending POST or PUT to http://localhost:4848/management/domain/configs/config/server-config/java-config/jvm-options):

  • Header: Accept: application/json
  • Header: X-Requested-By: GlassFish REST HTML interface
  • Header: Content-Type: application/json
  • Body: { "-client": "" } (and other key-values)

The effect needed to be verified in the domain.xml.

Note that REST API POST does replace the config with the posted body while PUT always adds. This is general behaviour that isn't very convenient and might even be considered for change.

Note also that command line strips escaping to early making it ineffective. E.g. -XX\:NewRatio\=2 is stripped to -XX:NewRatio=2 before the parameters are handled so it is split into -XX and NewRatio=2 (which is an illegal option). This is an most likely unreported error preexisting at least since 5.191. According to the documentation (link above) such escaping should have allowed to avoid splitting option values.

Note that adding an options of type key= at the end of the list in the admin GUI can lead to an exception when trying to split encountering an escape at the end of the input. This is most likely a defect in general input processing.

Further note that create-jvm-options does warn about -Xmx or -Xms already being set when adding them anew. This however is only a warning and both values will be contained. Due to the unpredictable nature of the JVM options bag in the config object the order of options can change with every change swapping the option that is effective in case of duplicate "keys".

@jbee jbee added this to the 5.193 milestone Jun 3, 2019
@jbee jbee self-assigned this Jun 3, 2019
@jbee
Copy link
Contributor Author

jbee commented Jun 4, 2019

jenkins test please

Copy link
Contributor

@cubastanley cubastanley left a comment

Choose a reason for hiding this comment

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

Tested locally, works as expected

Copy link

@AlanRoth AlanRoth left a comment

Choose a reason for hiding this comment

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

Tested locally, all good

@jbee
Copy link
Contributor Author

jbee commented Jun 4, 2019

jenkins test please

@Pandrex247
Copy link
Member

I don't know if I did something weird or not, but I managed to set -XX\:NewRatio\=2 from the command line

@jbee
Copy link
Contributor Author

jbee commented Jun 5, 2019

@Pandrex247 Did run commands from shell (asadmin create-jvm-options ...) or first enter into asadmin and then go create-jvm-options? Maybe that can make a difference?

@jbee jbee merged commit 83cb008 into payara:master Jun 5, 2019
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jun 12, 2019
PAYARA-3880 Fixed JVM options as semi-colon separated list parameter
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jun 12, 2019
Merge pull request payara#4002 from jbee/PAYARA-3880-jvm-options

Approved-by: Alan Roth <[email protected]>
Approved-by: Andrew Pielage <[email protected]>
AlanRoth pushed a commit to AlanRoth/Payara that referenced this pull request Aug 13, 2019
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