8294402: Add diagnostic logging to VMProps.checkDockerSupport #12239

mseledts · 2023-01-26T23:23:16Z

Add log() and logToFile() methods to VMProps for diagnostic purposes.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8294402: Add diagnostic logging to VMProps.checkDockerSupport

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Leonid Mesnik (@lmesnik - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12239

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12239.diff

bridgekeeper · 2023-01-26T23:24:16Z

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

openjdk · 2023-01-26T23:25:41Z

@mseledts To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

build
client
compiler
core-libs
hotspot
hotspot-compiler
hotspot-gc
hotspot-jfr
hotspot-runtime
i18n
ide-support
javadoc
jdk
jmx
kulla
net
nio
security
serviceability
shenandoah

mseledts · 2023-01-30T23:57:47Z

/label

openjdk · 2023-01-30T23:59:06Z

@mseledts Usage: /label <add|remove> [label[, label, ...]] or /label [<+|->label[, <+|->label, ...]] where label is an additional classification that should be applied to this PR. These labels are valid:

serviceability
hotspot
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
hotspot-runtime
jmx
build
nio
client
core-libs
compiler
net
hotspot-gc
hotspot-jfr

mseledts · 2023-01-31T00:06:42Z

/label add hotspot

openjdk · 2023-01-31T00:07:35Z

@mseledts
The hotspot label was successfully added.

mlbridge · 2023-01-31T00:11:43Z

Webrevs

test/jtreg-ext/requires/VMProps.java

mseledts · 2023-02-02T03:57:12Z

Alternatively we could keep the conditional guard using the property -Djtreg.log.vmprops. The users will enable this property in the test execution scripts for a test set of interest, such as container testing.

dholmes-ora · 2023-02-02T04:42:26Z

Okay I'm struggling with the best way to handle this. I think the problem is that the code currently contains logging output of different levels (ref unified logging in the VM) but we only have a binary on/off selection mechanism. I don't want to see all the logging all the time - too noisy. But I do want to see the logging of the stdout/stderr from the exec'd process any time it fails.

But I guess we could keep it all conditional on the property and have the task definition for the container tests set the property as suggested.

mseledts · 2023-02-08T17:57:21Z

I will then makes the logging conditional on the property mentioned above. Any test run who wishes additional logging could add/enable this property by the launching script.

dholmes-ora

Looks okay to me. Thanks.

openjdk · 2023-02-09T02:44:15Z

@mseledts 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:

8294402: Add diagnostic logging to VMProps.checkDockerSupport

Reviewed-by: dholmes, lmesnik

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 392 new commits pushed to the master branch:

a917fb3: 7033677: potential cast error in MemberEnter
6120319: 8302226: failure_handler native.core should wait for coredump to finish
fef3eab: 8302734: Parallel: Remove unused LGRPSpace::_invalid_region
ea5bfea: 8302661: Parallel: Remove PSVirtualSpace::is_aligned
edf238b: 8302635: Race condition in HttpBodySubscriberWrapper when cancelling request
cd77fcf: 8290822: C2: assert in PhaseIdealLoop::do_unroll() is subject to undefined behavior
57c9bc3: 8302335: IGV: Bytecode not showing
57fde75: 8302113: Improve CRC32 intrinsic with crypto pmull on AArch64
b8c9d6c: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false
dc55a7f: 8302202: Incorrect desugaring of null-allowed nested patterns
... and 382 more: https://git.openjdk.org/jdk/compare/3be5758bb413fb6b4dc6191d78ca38332d5153f1...master

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 master branch, type /integrate in a new comment.

lmesnik · 2023-02-09T19:08:42Z

test/jtreg-ext/requires/VMProps.java


+    // Returns comma-separated file names for stdout and stderr.
+    private String redirectOutputToLogFile(String msg, ProcessBuilder pb, String fileNameBase) {
+        if (!Boolean.getBoolean("jtreg.log.vmprops")) {


Why not dump output always? It is printed only if exitcode is not zero anyway and doesn't add noise.
However not having it in the case of failure require reproducing the bug.

We have to be consistent. Since we agreed in earlier discussion to use property jtreg.log.vmprops as a guard for logging we should also use it for writing into a file. The user who runs the tests can always specify this property in the test execution script.

If we take the approach of always logging to vmprops.log then this makes sense. If no objections I will always redirect output into a file.

test/jtreg-ext/requires/VMProps.java

lmesnik · 2023-02-09T19:23:10Z

test/jtreg-ext/requires/VMProps.java

+     */
+    protected static void log(String msg) {
+        if (!Boolean.getBoolean("jtreg.log.vmprops")) {
+            return;


Why not always log in to the file like vmprops.log? Also, add try/catch into the call() and redirect file to stderr if something is wrong. And redirect if the property is set for passed tests.

I had a discussion with Leonid in person, and I am OK with such approach. To summarize:

always log to a file (vmprops.log or similar)

this way we always have information to base investigation on if things go wrong

conditionally log into stderr to avoid polluting the output and being too verbose
If I do not hear any objections I will proceed with implementation tomorrow.

lmesnik

just refresh my approval

mseledts · 2023-02-17T18:28:55Z

Thank you David, Leonid and Dan.

/integrate

openjdk · 2023-02-17T19:37:41Z

Going to push as commit 03d613b.
Since your change was applied there have been 393 commits pushed to the master branch:

a263f28: 8302777: CDS should not relocate heap if mapping fails
a917fb3: 7033677: potential cast error in MemberEnter
6120319: 8302226: failure_handler native.core should wait for coredump to finish
fef3eab: 8302734: Parallel: Remove unused LGRPSpace::_invalid_region
ea5bfea: 8302661: Parallel: Remove PSVirtualSpace::is_aligned
edf238b: 8302635: Race condition in HttpBodySubscriberWrapper when cancelling request
cd77fcf: 8290822: C2: assert in PhaseIdealLoop::do_unroll() is subject to undefined behavior
57c9bc3: 8302335: IGV: Bytecode not showing
57fde75: 8302113: Improve CRC32 intrinsic with crypto pmull on AArch64
b8c9d6c: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false
... and 383 more: https://git.openjdk.org/jdk/compare/3be5758bb413fb6b4dc6191d78ca38332d5153f1...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-17T19:38:36Z

@mseledts Pushed as commit 03d613b.

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

mseledts added 5 commits January 24, 2023 19:51

Add diagnostic logging to at-requires VMProps

bb79e90

Allow both absolute and relative path for log file

632432f

More logging for container methods

5deb681

Draft: redirect container ps child output to log file

35afd7c

Simplification and refactoring

bcf4691

mseledts added 3 commits January 30, 2023 10:59

Fixed issues with process out/err redirect

da9c27c

Log to stderr as per discussion

5621686

Minor fixes in printed message

7851e5d

mseledts changed the title ~~DRAFT: 8294402: Add diagnostic logging to VMProps.checkDockerSupport~~ 8294402: Add diagnostic logging to VMProps.checkDockerSupport Jan 30, 2023

openjdk bot added the rfr Pull request is ready for review label Jan 30, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 31, 2023

Conditionally enable VMProps logging

4465bf1

Comment about logging to stdout

881a547

dholmes-ora reviewed Jan 31, 2023

View reviewed changes

test/jtreg-ext/requires/VMProps.java Outdated Show resolved Hide resolved

test/jtreg-ext/requires/VMProps.java Outdated Show resolved Hide resolved

mseledts added 2 commits January 31, 2023 18:31

Updated comment as per review feedback

02d6833

Review feedback: printing stdout and stderr files to log()

b1aa1c4

dholmes-ora reviewed Feb 1, 2023

View reviewed changes

test/jtreg-ext/requires/VMProps.java Outdated Show resolved Hide resolved

dholmes-ora reviewed Feb 1, 2023

View reviewed changes

test/jtreg-ext/requires/VMProps.java Outdated Show resolved Hide resolved

dholmes-ora reviewed Feb 1, 2023

View reviewed changes

test/jtreg-ext/requires/VMProps.java Outdated Show resolved Hide resolved

mseledts added 2 commits February 1, 2023 13:17

Addressing review feedback

ceb0ed6

Removed logging condition as per review feedback

1eef0f6

Re-added conditional logging per review discussion

b85636d

dholmes-ora approved these changes Feb 9, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 9, 2023

lmesnik approved these changes Feb 9, 2023

View reviewed changes

Addressed recent review feedback from Leonid

e1890ea

dholmes-ora approved these changes Feb 17, 2023

View reviewed changes

lmesnik approved these changes Feb 17, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 17, 2023

openjdk bot closed this Feb 17, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 17, 2023

8294402: Add diagnostic logging to VMProps.checkDockerSupport #12239

8294402: Add diagnostic logging to VMProps.checkDockerSupport #12239

Uh oh!

Conversation

mseledts commented Jan 26, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 26, 2023

Uh oh!

openjdk bot commented Jan 26, 2023

Uh oh!

mseledts commented Jan 30, 2023

Uh oh!

openjdk bot commented Jan 30, 2023

Uh oh!

mseledts commented Jan 31, 2023

Uh oh!

openjdk bot commented Jan 31, 2023

Uh oh!

mlbridge bot commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mseledts commented Feb 2, 2023

Uh oh!

dholmes-ora commented Feb 2, 2023

Uh oh!

mseledts commented Feb 8, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmesnik Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

mseledts Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

mseledts Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lmesnik Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

mseledts Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

lmesnik left a comment

Choose a reason for hiding this comment

Uh oh!

mseledts commented Feb 17, 2023

Uh oh!

openjdk bot commented Feb 17, 2023

Uh oh!

openjdk bot commented Feb 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

mseledts commented Jan 26, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 31, 2023 •

edited

Loading

openjdk bot commented Feb 9, 2023 •

edited

Loading