8374364: TestCgroupMetrics.java fails when cpuset unmounted

[sendaoYan]

Hi all,

Test jdk/internal/platform/cgroup/TestCgroupMetrics.java report fails when the cpuset unmounted. The details shows in jbs issue. I think it's better to check the 'cpu period' or 'cpuset' available or not before run the releated tests.

Change has been verified locally. Test-fix only, no risk.

---

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-8374364: TestCgroupMetrics.java fails when cpuset unmounted (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28996

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back syan! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@sendaoYan The following labels will be automatically applied to this pull request:

• core-libs
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (8e2ee419)
• 03: Full - Incremental (068b1591)
• 02: Full - Incremental (1be8d17a)
• 01: Full - Incremental (fe094b55)
• 00: Full (9411a6e7)

[DamonFool]

So MetricsTester.main(args) requires the docker service, right?

Maybe it would be better to add this check in a place likeMetricsTester, where it requires docker service directly.

[sendaoYan]

So MetricsTester.main(args) requires the docker service, right?

Maybe it would be better to add this check in a place likeMetricsTester, where it requires docker service directly.

Only jdk/internal/platform/cgroup/TestCgroupMetrics.java observed fails when docker service unavailable. There are total two tests invoke MetricsTester. The other test jdk/internal/platform/docker/TestSystemMetrics.java already call DockerTestUtils.checkCanTestDocker() before start test. So i think it's better to leave this check inside test, same like to jdk/internal/platform/docker/TestSystemMetrics.java.

[DamonFool]

So i think it's better to leave this check inside test, same like to jdk/internal/platform/docker/TestSystemMetrics.java.

I don't think so.

Just imagine someone may call MetricsTester.main(args) again in the future, how can you prevent him/her not forgetting adding the check before the call?

That's why I would suggest adding it in the place where it requires docker directly, not the place indirectly.

[openjdk]

@sendaoYan hotspot-runtime has been added to this pull request based on files touched in new commit(s).

[sendaoYan]

That's why I would suggest adding it in the place where it requires docker directly, not the place indirectly.

Fixed.

[sendaoYan]

/issue JDK-8374364

[openjdk]

@sendaoYan This issue is referenced in the PR title - it will now be updated.

[dholmes-ora]

A couple of comments but I'm not sure what the right approach is here. Seems inconsistent to use SkippedException in one place, but not others.

[dholmes-ora]

Note sure this indicates a "fail" as such.

[sendaoYan]

The getCpuPeriod call getLongValue to read property value from file /sys/fs/cgroup/cpu,cpuacct/system.slice/sshd.service/cpu.cfs_period_us. The getLongValue will return CgroupSubsystem.LONG_RETVAL_UNLIMITED if it can not open the file. So I think it's better to skip this sub-test when there is no /sys/fs/cgroup/cpu,cpuacct/system.slice/sshd.service/cpu.cfs_period_us file.

[sendaoYan]

A couple of comments but I'm not sure what the right approach is here. Seems inconsistent to use SkippedException in one place, but not others.

This PR use SkippedException instead of report test pass. It's unrelated to the subject, It just fix it by the way