8377562: [CRaC] Add better logging to RemoteJmxTest

[TimPushkin]

Makes RemoteJmxTest log stderr of the child process. Refactors CRaC tests to make coordination with exec process simpler and more robust.

---

Progress

• Change must not contain extraneous whitespace

Issue

• JDK-8377562: [CRaC] Add better logging to RemoteJmxTest (Task - P3)

Reviewers

• Radim Vansa (@rvansa - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/crac.git pull/295/head:pull/295
$ git checkout pull/295

Update a local copy of the PR:
$ git checkout pull/295
$ git pull https://git.openjdk.org/crac.git pull/295/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 295

View PR using the GUI difftool:
$ git pr show -t 295

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/295.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back tpushkin! A progress list of the required criteria for merging this PR into crac will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@TimPushkin This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8377562: [CRaC] Add better logging to RemoteJmxTest

Reviewed-by: rvansa

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the crac branch:

• 49b02b4: 8378259: [CRaC] Tests using CRIU fail in GitHub Actions

Please see this link for an up-to-date comparison between the source branch of this pull request and the crac branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the crac branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 02: Full - Incremental (3135c6ae)
• 01: Full - Incremental (c355f325)
• 00: Full (c86bc70b)

[rvansa]

I had just a quick peek on the changes, but while at it, could you consider to always capture the output (but tee it into stderr/stdout as well)?

Also, I don't like the .doRestore() and .doRestoreToAnalyze() too much: what about replacing the latter with .expect(outputAnalyzer -> ...).doRestore()?

[TimPushkin]

Ok, I'll think of a better interface.

BTW, the test failures on linux-x64 are not from this change. I cannot reproduce them locally but in CI they also occur on the main branch: https://github.com/TimPushkin/crac/actions/runs/21992337569. Looks like something is wrong with the linux-x64 environment in CI.

[TimPushkin]

The remaining test failures should be fixed by #296

[TimPushkin]

@rvansa

I had just a quick peek on the changes, but while at it, could you consider to always capture the output (but tee it into stderr/stdout as well)?

Done. I was a bit hesitant because without capturing the output would still be redirected to test process'es stdout (i.e. it would be visible in the test crash log) which should be more efficient than capturing+printing. But since capturing is actually the default it is probably efficient enough for tests and we get uniform formatting this way.

Also, I don't like the .doRestore() and .doRestoreToAnalyze() too much: what about replacing the latter with .expect(outputAnalyzer -> ...).doRestore()?

I considered this but I personally do not like the alternatives:

1. .expect(outputAnalyzer -> ...).doRestore() — firstly, outputAnalyzer handling is typically unique to a specific process invocation, so if there are several doCheckpoint(), doRestore(), doPlain() calls each will need to have its own expect, i.e. the lambda feels more like an argument for a specific process invocation call. Secondly, with lambdas the syntax is more awkward: I prefer doRestoreToAnalyze().expectA().expectB() over .expect(o -> o.expectA().expectB()).doRestore(), and if we need to save the analyzer for later it would be var out = doRestoreToAnalyze() vs OutputAnalyzer out; b.expect(o -> { out = o; }).doRestore().
2. doRestore(outputAnalyzer -> ...) — solves the first issue above but the second one stands, plus we'd need to do doRestore(null) (or add doRestore() { doRestore(null) }) when no analysis is needed.
3. Now that we always capture output we could always return OutputAnalyzer but constructing one is not completely free (two List<String>s need to be concatenated into two long strings) and we would need to add a parameter or another method anyway to determine whether waitForSuccess() is needed inside doRestore.

I am not completely against (2) and can go that way if you prefer, but to me doRestoreToAnalyze() seems to provide a better syntax.

[TimPushkin]

I wondered why CRIU tests suddenly started passing even though #296 is not included here yet. Turns out they reverted the CI's kernel update that caused the issue.

Doesn't change anything for us, just noting.

[rvansa]

If it were not for the default check of the exit status I would go with 3) (return OA from doRestore() & friends) but while the toAnalyze suffix still seems awkward to me ('analysis' is rather pompous word for string match) I won't have objections.

Minor comments below.

[TimPushkin]

/integrate

[openjdk]

Going to push as commit 056fceb.
Since your change was applied there has been 1 commit pushed to the crac branch:

• 49b02b4: 8378259: [CRaC] Tests using CRIU fail in GitHub Actions

Your commit was automatically rebased without conflicts.

[openjdk]

@TimPushkin Pushed as commit 056fceb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.