[crac] RFR: 8350845: [CRaC] Support C/R engines in form of a library [v2]

[Radim Vansa]

On Tue, 4 Mar 2025 08:54:45 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

>> Adds support for C/R engines implemented in form of dynamic libraries.
>> 
>> In the proposed API the engine controls the set of the configurable options — they are mainly passed by the user via the new `CRaCEngineOptions` VM option, but some may also be passed by the VM. Other VM and/or engine implementations may extend the API via "extensions".
>> 
>> The JVM is supposed to know what configuration options and API extensions are supported by calling `can_configure` and `get_extension`.
>> 
>> The user is supposed to know which options they can pass by asking the engine with `-XX:CRaCEngineOptions=help` (support for this is engine-dependent). Example of how this looks for bundled engines:
>> 
>> $ java -XX:CRaCEngineOptions=help
>> crexec - pseudo-CRaC-engine used to relay data from JVM to a "real" engine implemented as an executable (instead of a library). The engine executable is expected to have CRaC-CRIU-like CLI. Support of the configuration options also depends on the engine executable.
>> 
>> Configuration options:
>> * keep_running=<true/false> (default: false) - keep the process running after the checkpoint or kill it.
>> * direct_map=<true/false> (default: false) - on restore, map process data directly from saved files. This may speedup the restore but the resulting process will not be the same as before the checkpoint.
>> * args=<string> (default: "") - free space-separated arguments passed directly to the engine executable, e.g. "--arg1 --arg2 --arg3".
>> 
>> 
>> Notable related changes included in the patch:
>> - The ability to pass arguments to C/R engine was removed from `CRaCEngine` because it can now be implemented through `CRaCEngineOptions`. E.g. `-XX:CRaCEngine=criu,--verbose` is now `-XX:CRaCEngine=criu -XX:CRaCEngineOptions=args=--verbose`.
>> - `CRaCEngine` (as well as the new `CRaCEngineOptions`) VM Option is now not updated in the restored VM. This seems more correct because engine path and options will very likely differ on restore and on checkpoint, so it is not obvious how to combine them — just replacing won't always work. Until now the updates were ignored by the CRaC implementation anyway, so this should only be visible by inspecting the options through JMX.
>> - `CRaCRestoreFrom` is now considered set (and thus an attempt to restore is made) only if it is set to a non-empty value, i.e. `-XX:CRaCRestoreFrom=""` won't trigger a (failing) restore attempt anymore. This is to be consistent with how `-XX:CRaCCheckpointTo=""` does not trigger a chec...
>
> Timofei Pushkin has updated the pull request incrementally with five additional commits since the last revision:
> 
>  - Fix platform-specific issues
>  - Rewrite crexec on C++
>  - Change engine options help
>  - Move engine wrapper into its own files
>  - Address review comments

src/hotspot/share/runtime/crac.cpp line 299:

> 297:   }
> 298:   if (status == CracEngine::ApiStatus::UNSUPPORTED) {
> 299:     if (printf("Selected CRaC engine does not provide information about itself.\n") < 0) {

Don't you think that testing `printf` return value is excessive? (unless you expect problems...)

src/hotspot/share/runtime/crac.cpp line 310:

> 308:   const char *description = engine.description();
> 309:   if (description == nullptr) {
> 310:     log_error(crac)("CRaC engine failed to provide its textual description");

API says that returning `nullptr` is a valid behaviour - shouldn't log error (maybe just print some placeholder text). The same below.

src/hotspot/share/runtime/crac.cpp line 492:

> 490: 
> 491:   // Note that this is a local, i.e. the handle will be destroyed if we fail to restore
> 492:   auto engine = CracEngine(CRaCRestoreFrom);

A nitpick/question: why do you use this syntax that does an extra copy rather than

CracEngine engine(CRaCRestoreFrom);

?

src/hotspot/share/runtime/globals.hpp line 1964:

> 1962:   product_pd(ccstr, CRaCEngine, RESTORE_SETTABLE,                           \
> 1963:       "Path or name of a program or a shared library implementing "         \
> 1964:       "checkpoint and restore. On restore this option applies only to "     \

I would even say that the CREngine must match on checkpoint and restore.

test/jdk/jdk/crac/CracEngineOptionsTest.java line 90:

> 88:                     "CRaC engine option: 'keep_running' = 'true'",
> 89:                     "CRaC engine option: 'args' = '-v -v -v -v'",
> 90:                     "CRaC engine option: 'keep_running' = 'false'");

I think that providing the same option twice should result in warning, at least.

-------------

PR Review Comment: https://git.openjdk.org/crac/pull/207#discussion_r1979362701
PR Review Comment: https://git.openjdk.org/crac/pull/207#discussion_r1979361623
PR Review Comment: https://git.openjdk.org/crac/pull/207#discussion_r1979394252
PR Review Comment: https://git.openjdk.org/crac/pull/207#discussion_r1979410150
PR Review Comment: https://git.openjdk.org/crac/pull/207#discussion_r1979431335