Restore options file and vm support #16543

tajila · 2023-01-11T22:15:34Z

Restore options file and vm support

Introduce options file to provide the ability to supply cmdline options on restore. Initially only properties will be supported. Further support for JVM options will be added later.

Signed-off-by: Tobi Ajila atobia@ca.ibm.com

tajila · 2023-01-12T20:51:53Z

@DanHeidinga Please review these changes

JasonFengJ9 · 2023-01-17T15:30:34Z

jcl/src/openj9.criu/share/classes/org/eclipse/openj9/criu/CRIUSupport.java

+
+		J9InternalCheckpointHookAPI.registerPostRestoreHook(RESTORE_OPTIONS_FILE_PRIORITY,
+				"Restore options via options file:" + optionsFilePath, () -> { //$NON-NLS-1$
+					try (BufferedReader envFileReader = new BufferedReader(new FileReader(optionsFilePath))) {


optionsFileReader fits better here.

JasonFengJ9 · 2023-01-17T15:36:49Z

runtime/criusupport/criusupport.cpp

+	bool result = true;
+	J9JavaVM *vm = currentThread->javaVM;
+	J9JavaVMArgInfoList vmArgumentsList;
+	UDATA ignored;


This variable missed initialization though its value is to be ignored.

runtime/criusupport/criusupport.cpp

JasonFengJ9 · 2023-01-17T16:17:12Z

runtime/vm/vmprops.c

+	for (i = 0; i < initArgs->nOptions; ++i) {
+		char * optionString = initArgs->options[i].optionString;
+
+		if (strncmp("-D", optionString, 2) == 0) {


Suggested change

if (strncmp("-D", optionString, 2) == 0) {

if (0 == strncmp("-D", optionString, 2)) {

JasonFengJ9 · 2023-01-17T16:17:37Z

runtime/vm/vmprops.c

+			UDATA propNameLen = 0;
+
+			propValue = strchr(optionString + 2, '=');
+			if (propValue == NULL) {


Suggested change

if (propValue == NULL) {

if (NULL == propValue) {

JasonFengJ9 · 2023-01-18T13:34:01Z

runtime/criusupport/criusupport.cpp

+		goto done;
+	}
+
+	vm->checkpointState.restoreArgsList = createJvmInitArgs(vm->portLibrary, vm->vmArgsArray->actualVMArgs, &vmArgumentsList, &ignored);


There is no issue at the default mode -XX:+CRIURestoreNonPortableMode here though multiple checkpoints could be prepared w/ a bit of cleanup before creating a new one.

if (NULL != vm->checkpointState.restoreArgsList) { destroyJvmInitArgs(vm->portLibrary, vm->checkpointState.restoreArgsList); }

JasonFengJ9 · 2023-01-18T13:37:08Z

runtime/nls/j9cl/j9jcl.nls

@@ -572,3 +572,11 @@ J9NLS_JCL_CRIU_LOADING_LIBCRIU_FUNCTIONS_FAILED.explanation=CRIUSupport::checkpo
 J9NLS_JCL_CRIU_LOADING_LIBCRIU_FUNCTIONS_FAILED.system_action=The JVM will throw a SystemCheckpointException.
 J9NLS_JCL_CRIU_LOADING_LIBCRIU_FUNCTIONS_FAILED.user_response=Check your installation of criu with `criu check`.
 # END NON-TRANSLATABLE
+
+J9NLS_JCL_CRIU_LOADING_OPTIONS_FILE_FAILED=The JVM could load the options file


It seems a not missed.

JasonFengJ9 · 2023-01-18T13:42:52Z

runtime/nls/j9cl/j9jcl.nls

+# START NON-TRANSLATABLE
+J9NLS_JCL_CRIU_LOADING_OPTIONS_FILE_FAILED.sample_input_1=1
+J9NLS_JCL_CRIU_LOADING_OPTIONS_FILE_FAILED.explanation=CRIUSupport::checkpointJVM failed.
+J9NLS_JCL_CRIU_LOADING_OPTIONS_FILE_FAILED.system_action=The JVM will throw a SystemCheckpointException.


Is this JVMRestoreException according to

if (false == loadRestoreArguements(currentThread, optionsFileChars)) { currentExceptionClass = vm->checkpointState.criuJVMRestoreExceptionClass; nlsMsgFormat = j9nls_lookup_message(J9NLS_DO_NOT_PRINT_MESSAGE_TAG | J9NLS_DO_NOT_APPEND_NEWLINE, J9NLS_JCL_CRIU_LOADING_OPTIONS_FILE_FAILED, NULL); goto wakeJavaThreadsWithExclusiveVMAccess; }

JasonFengJ9 · 2023-01-18T13:46:43Z

runtime/vm/vmprops.c

+					goto fail;
+				}
+			}
+			if (j2seVersion >= J2SE_V11) {


This can be replaced w/ #if JAVA_SPEC_VERSION >= 11.

DanHeidinga

I'm confused by this PR as it appears to be setting the properties both natively and in a Java-level restore hook. What am I missing?

The VM collects the initial set of properties and then, during one of the JCL init stages, hands sets them into the class library code and the class libs become the master copy.

Assuming my knowledge on that is still correct, this PR looks like it needs some updates to the design

jcl/src/openj9.criu/share/classes/org/eclipse/openj9/criu/CRIUSupport.java

DanHeidinga · 2023-01-19T15:26:27Z

jcl/src/openj9.criu/share/classes/org/eclipse/openj9/criu/CRIUSupport.java

+	 */
+	public CRIUSupport registerRestoreOptionsFile(Path optionsFile) {
+		String optionsFilePath = optionsFile.toAbsolutePath().toString();
+		this.optionsFile = new StringBuilder("-Xoptionsfile=").append(optionsFilePath).toString(); //$NON-NLS-1$


Is there a reason to use StringBuilder here? The straightforward "-Xoptionsfile="+optionsFilePath is both shorter and more straightforward given this is code that is likely executed once.

DanHeidinga · 2023-01-19T15:33:30Z

jcl/src/openj9.criu/share/classes/org/eclipse/openj9/criu/CRIUSupport.java

+				"Restore options via options file:" + optionsFilePath, () -> { //$NON-NLS-1$
+					try (BufferedReader optionsFileReader = new BufferedReader(new FileReader(optionsFilePath))) {
+						String entry = null;
+						while ((entry = optionsFileReader.readLine()) != null) {


There's a Stream<String> lines() method on BufferedReader that handles the line reading for you:

br.lines().forEach(.......)

Any luck with Dan's suggestion here?

I need to do an explicit readLine to support line continuations. Documentation indicates this may cause undefined behaviour when used with streams

Why do you need an explicit readLine? Use an extra variable as an accumulator something like this:

String options = null; br.lines().forEach(l -> { l = l.trim(); if (l.endsWith("\\")) { option = l.substring(0, l.length()-1); } else { if (option != null) { l = option.concat(l); option=null; } System.out.println(l); } });

jcl/src/openj9.criu/share/classes/org/eclipse/openj9/criu/CRIUSupport.java

DanHeidinga · 2023-01-19T15:44:00Z

runtime/criusupport/criusupport.cpp

@@ -341,6 +343,38 @@ releaseSafeOrExcusiveVMAccess(J9VMThread *currentThread, J9InternalVMFunctions *
 	}
 }

+static bool
+loadRestoreArguements(J9VMThread *currentThread, const char *optionsFile)


Suggested change

loadRestoreArguements(J9VMThread *currentThread, const char *optionsFile)

loadRestoreArguments(J9VMThread *currentThread, const char *optionsFile)

DanHeidinga · 2023-01-19T15:45:36Z

runtime/criusupport/criusupport.cpp

+	UDATA ignored = 0;
+
+	if (NULL != vm->checkpointState.restoreArgsList) {
+		destroyJvmInitArgs(vm->portLibrary, vm->checkpointState.restoreArgsList);


Is destroy the right choice here? I assume this is handling the case where we're restoring from a previously restored checkpoint? If that's the case, then we should chain the previously set args so we can see all the previous values when servicing an issue

DanHeidinga · 2023-01-19T15:58:14Z

runtime/vm/vmprops.c

+	/* Create the -D properties. This may override any of the writeable properties above.
+	Should the command line override read-only props? */


Formatting:

Suggested change

/* Create the -D properties. This may override any of the writeable properties above.

Should the command line override read-only props? */

/* Create the -D properties. This may override any of the writeable properties above.

* Should the command line override read-only props?

*/

tajila · 2023-01-19T17:10:14Z

I'm confused by this PR as it appears to be setting the properties both natively and in a Java-level restore hook. What am I missing?

We need the System properties to be set in both because the native structure needs to be updated of things like jvmti GetSystemProperties (https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#GetSystemProperties). And the java-level for System::getProperties.

The VM collects the initial set of properties and then, during one of the JCL init stages, hands sets them into the class library code and the class libs become the master copy.

The reason it didnt reuse the init helper is that the native list and the java-level list arent always the same. Calling System.setProperty will not update the native list. So if at restore time I update the native list and use System::getPropertyList to rebuild the java list instead of parsing the file then there is a chance that it will overwrite some updates that were made between checkpoint and startup.

Perhaps thats an argument for not updating the native list at all.

DanHeidinga · 2023-01-19T18:12:03Z

I'm confused by this PR as it appears to be setting the properties both natively and in a Java-level restore hook. What am I missing?

We need the System properties to be set in both because the native structure needs to be updated of things like jvmti GetSystemProperties (https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#GetSystemProperties). And the java-level for System::getProperties.

Interesting. Reading the JVMTI docs, it appears that JVMTI only allows setting vm-properties during the "On_Load" phase though they can be read after that. It also states that the JVMTI view and the System::getProperties view are likely to diverge during execution and that JNI should be used to get the accurate view.

Given that a restore is happening after the "On_Load" phase, I don't think it makes sense to update the VM view of the properties as they're already spec'd to diverge.

The JVMTI docs also state properties set on the command line are included so an argument could be made to add them to the vm-view but I think, as we've previously discussed, that since restore is a restore, not a start, that adding them may break anyone who expects the list to be static after the "On_Load" phase ends.

That's my long way of saying that I think we shouldn't update the VM view as we're not in the "On_Load" phase, it may break anyone relying on the spec behaviour, and philosophically, we're a restore not start. Oh, and it matches the existing spec'd behaviour of the java and vm views diverging.

The VM collects the initial set of properties and then, during one of the JCL init stages, hands sets them into the class library code and the class libs become the master copy.

The reason it didnt reuse the init helper is that the native list and the java-level list arent always the same. Calling System.setProperty will not update the native list. So if at restore time I update the native list and use System::getPropertyList to rebuild the java list instead of parsing the file then there is a chance that it will overwrite some updates that were made between checkpoint and startup.

Perhaps thats an argument for not updating the native list at all.

Another good argument! +1 to not update the native list.

tajila · 2023-01-27T22:44:55Z

jenkins test sanity xlinux jdk17

tajila · 2023-01-27T22:58:11Z

jenkins test sanity xlinux jdk17

JasonFengJ9 · 2023-01-30T14:07:10Z

runtime/criusupport/criusupport.cpp

@@ -638,6 +689,13 @@ Java_org_eclipse_openj9_criu_CRIUSupport_checkpointJVMImpl(JNIEnv *env,
 		/* We can only end up here if the CRIU restore was successful */
 		isAfterCheckpoint = TRUE;

+		if (false == loadRestoreArguments(currentThread, optionsFileChars)) {


Need to check NULL != optionsFile to avoid loadRestoreArguments w/ an empty buffer.

addXOptionsFile handles the empty optionsFileChars. I want to call loadRestoreArguments uncondiontally because Ill have a follow on PR that also looks for options in the env variables (IBM_JAVA_OPTIONS, etc.)

OPENJ9_JAVA_OPTIONS right? =)

runtime/criusupport/criusupport.cpp

DanHeidinga · 2023-01-30T15:32:17Z

runtime/vm/vmprops.c

@@ -530,6 +530,107 @@ getLibSubDir(J9JavaVM *vm, const char *subDir, char **value)
 }

 #define COM_SUN_MANAGEMENT "com.sun.management"
+
+UDATA
+loadCMDLineSystemProperties(J9JavaVM *vm, JavaVMInitArgs *initArgs)


Is this refactoring still needed? I was kind of expecting it to be removed as we agreed to only support the Java-level ::setProperties call.

no Ive removed the change

With CRIU support, options args can be found in the `vm->argsArray` or the `vm->checkpointState.restoreArgsList`. The new helpers allow users to specify which argsArray they want to query. Related to eclipse-openj9#16543 Signed-off-by: Tobi Ajila <atobia@ca.ibm.com>

tajila · 2023-01-30T21:59:29Z

@DanHeidinga @JasonFengJ9 changes are ready for another look

JasonFengJ9

LGTM

DanHeidinga

Apart from the minor NPE comment, this looks good to me

DanHeidinga · 2023-01-31T19:35:38Z

jcl/src/openj9.criu/share/classes/org/eclipse/openj9/criu/CRIUSupport.java

+										propertyValue = propertyValue.substring(0, propertyValue.length() - 1);
+										propertyValue = propertyValue.trim();
+										String nextLine = optionsFileReader.readLine();
+										propertyValue += nextLine.trim();


For a malformed file that has a line continuation mark on the last line, this will throw an NPE. Not sure it's a problem but it might be better to throw a JVMRestoreException in that case. (And the code does explicit null checks above, so likely worth doing here)

I added NPE to the catch block below, this way we also know the cause of the RestoreException

Introduce options file to provide the ability to supply cmdline options on restore. Initially only properties will be supported. Further support for JVM options will be added later. Signed-off-by: Tobi Ajila <atobia@ca.ibm.com>

tajila · 2023-01-31T21:48:48Z

jenkins test sanity xlinux jdk17

tajila · 2023-02-01T15:58:49Z

@DanHeidinga any other concerns

keithc-ca · 2023-02-01T18:49:36Z

jcl/src/openj9.criu/share/classes/org/eclipse/openj9/criu/CRIUSupport.java

+							if (!entry.isEmpty()) {
+								if (entry.startsWith("-D")) { //$NON-NLS-1$
+									String entrySplit[] = entry.split("=", 2); //$NON-NLS-1$
+									String propertyValue = entrySplit[1];


This will throw an exception if the line doesn't contain "="; probably not the best user experience.
Perhaps adding to the catch list is reasonable, but it would be helpful to include some of the troublesome input in the exception thrown there.

tajila force-pushed the criu2 branch 8 times, most recently from 8377116 to ded5726 Compare January 12, 2023 18:52

tajila marked this pull request as ready for review January 12, 2023 20:51

tajila requested a review from DanHeidinga January 12, 2023 20:51

JasonFengJ9 reviewed Jan 17, 2023

View reviewed changes

JasonFengJ9 reviewed Jan 18, 2023

View reviewed changes

tajila force-pushed the criu2 branch 2 times, most recently from 50eadfd to 781d071 Compare January 18, 2023 14:17

DanHeidinga reviewed Jan 19, 2023

View reviewed changes

tajila marked this pull request as draft January 20, 2023 20:02

tajila force-pushed the criu2 branch 11 times, most recently from 7c649f9 to 445db14 Compare January 25, 2023 19:36

tajila force-pushed the criu2 branch from 28b733b to 115fa24 Compare January 27, 2023 22:21

tajila force-pushed the criu2 branch 2 times, most recently from d04ae5b to 3b72514 Compare January 27, 2023 22:54

JasonFengJ9 reviewed Jan 30, 2023

View reviewed changes

tajila force-pushed the criu2 branch from 3b72514 to b291839 Compare January 30, 2023 15:00

DanHeidinga reviewed Jan 30, 2023

View reviewed changes

runtime/criusupport/criusupport.cpp Show resolved Hide resolved

DanHeidinga reviewed Jan 30, 2023

View reviewed changes

tajila mentioned this pull request Jan 30, 2023

Add parameterized varaints for options parsing #16623

Merged

tajila force-pushed the criu2 branch 6 times, most recently from d492452 to 6c66535 Compare January 30, 2023 18:48

JasonFengJ9 approved these changes Jan 31, 2023

View reviewed changes

DanHeidinga approved these changes Jan 31, 2023

View reviewed changes

tajila force-pushed the criu2 branch 2 times, most recently from 336976e to e8a3cb4 Compare January 31, 2023 21:33

Restore options file and vm support

d0a1d42

Introduce options file to provide the ability to supply cmdline options on restore. Initially only properties will be supported. Further support for JVM options will be added later. Signed-off-by: Tobi Ajila <atobia@ca.ibm.com>

tajila force-pushed the criu2 branch from e8a3cb4 to d0a1d42 Compare January 31, 2023 21:43

DanHeidinga merged commit 219f2e8 into eclipse-openj9:master Feb 1, 2023

keithc-ca reviewed Feb 1, 2023

View reviewed changes

keithc-ca mentioned this pull request Feb 1, 2023

Minor CRIU cleanup #16645

Merged

	if (strncmp("-D", optionString, 2) == 0) {
	if (0 == strncmp("-D", optionString, 2)) {

	loadRestoreArguements(J9VMThread currentThread, const char optionsFile)
	loadRestoreArguments(J9VMThread currentThread, const char optionsFile)

		/* Create the -D properties. This may override any of the writeable properties above.
		Should the command line override read-only props? */

Restore options file and vm support #16543

Restore options file and vm support #16543

Conversation

tajila commented Jan 11, 2023

tajila commented Jan 12, 2023

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

DanHeidinga 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

tajila commented Jan 19, 2023

DanHeidinga commented Jan 19, 2023

tajila commented Jan 27, 2023

tajila commented Jan 27, 2023

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

tajila commented Jan 30, 2023

JasonFengJ9 left a comment

Choose a reason for hiding this comment

DanHeidinga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tajila commented Jan 31, 2023

tajila commented Feb 1, 2023

Choose a reason for hiding this comment