RFR: 8352926: New test TestDockerMemoryMetricsSubgroup.java fails [v5]

[PAWAN CHAWDHARY]

On Thu, 15 May 2025 16:21:56 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> PAWAN CHAWDHARY has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove unused import
>
> test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 73:
> 
>> 71:             return;
>> 72:         }
>> 73:         if (IS_DOCKER && ContainerRuntimeVersionTestUtils.DOCKER_VERSION_20_10_0.compareTo(ContainerRuntimeVersionTestUtils.getContainerRuntimeVersion()) > 0) {
> 
> Better to replace this with 
> `isContainerVersionSupported()`
> ...
> and implement all logic in the the ContainerRuntimeVersionTestUtils

created checkContainerVersionSupported()

> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 38:
> 
>> 36:     private final int micro;
>> 37:     private static final ContainerRuntimeVersionTestUtils DEFAULT = new ContainerRuntimeVersionTestUtils(0, 0, 0);
>> 38:     public static final ContainerRuntimeVersionTestUtils DOCKER_VERSION_20_10_0 = new ContainerRuntimeVersionTestUtils(20, 10, 0);
> 
> Please add comment about meaning of the version or even better rename 
> DOCKER_MINIMAL_SUPPORTED_VERSION = ....

updated name to  DOCKER_MINIMAL_SUPPORTED_VERSION_CGROUPNS and PODMAN_MINIMAL_SUPPORTED_VERSION_CGROUPNS

> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 53:
> 
>> 51:         } else if (this.major < other.major) {
>> 52:             return -1;
>> 53:         } else { // equal major
> 
> no need to add `else {` here

updated

> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 58:
> 
>> 56:             } else if (this.minor < other.minor) {
>> 57:                 return -1;
>> 58:             } else { // equal majors and minors
> 
> no need to add `else {` here

updated

> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 81:
> 
>> 79:         } catch (Exception e) {
>> 80:             System.out.println("Failed to parse container runtime version: " + version);
>> 81:             return DEFAULT;
> 
> Any reason to don't fail here?

updated

> test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 94:
> 
>> 92:         } catch (Exception e) {
>> 93:             System.out.println(Container.ENGINE_COMMAND + " --version command failed. Returning null");
>> 94:             return null;
> 
> Any reason to don't fail here?

updated

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099494838
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099496846
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099494004
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099494142
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099497419
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2099497254