RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
Please review these tests that were developed to test JVM's container
awareness in Docker environment.
     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
     Webrev:
       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
     Testing:
         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
            Ran the developed tests via jtreg
            Pass

         2. Automated testing system - run these tests
            In progress

Thank you,
Misha

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Bob Vandette
http://cr.openjdk.java.net/~mseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/CPUSetsReader.java.html

Not sure this is a problem but  If you specify --cpuset-cpus 2-3,1 in docker you end up with cpuset.cpus containing 1-3.

Your cpusets test cases are all increasing in order so you won’t hit this issue.

The read function in this file has a hard coded /sys/fs/cgroup/cpuset directory.  This may not be where this
ends up being mounted.  Is this read function even used?


http://cr.openjdk.java.net/~mseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/TestCPUAwareness.java.html

Your test assumes that there are at least two physical processors on the host.  You might want to check first.

 55             testAPCCombo("0,1", 200*1000, 100*1000, 4*1024, 2);
 56             testAPCCombo("0,1", 200*1000, 100*1000, 1*1024, 2)

Everything else looks good,
bob.


> On Nov 1, 2017, at 11:11 PM, mikhailo <[hidden email]> wrote:
>
> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>     Webrev:
>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>     Testing:
>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>            Ran the developed tests via jtreg
>            Pass
>
>         2. Automated testing system - run these tests
>            In progress
>
> Thank you,
> Misha
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Igor Ignatyev
In reply to this post by Mikhailo Seledtsov
Hi Misha,

I have several comments:
1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it should be WB_IsContainerized(JNIEnv*, jobject)
2. CPUSetsReader.java : listToString can be rewritten using stream api as list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
3. in several files, I have noticed some problems w/ indents which make code quite hard to read, for example
>   73         Common.run( Common.newOpts(imageName)
>   74              .addDockerOpts("--memory", valueToSet))
>   75             .shouldMatch("Memory Limit is:.*" + expectedTraceValue);
or
>   96         DockerRunOptions opts = Common.newOpts(imageName, "AttemptOOM")
>   97             .addDockerOpts("--memory", dockerMemLimit, "--memory-swap", dockerMemLimit);
>   98             opts.classParams.add("" + sizeToAllocInMb);

4. TestCPUSets.java : it's better to use Assert.assertEquals(parts.length, 2) than Asserts.assertTrue(parts.length == 2) as the former will also print the actual value of parts.length. so you won't need to have L#103
5. Test*: there is no need to have parentheses in '@requires (docker.support)'
6. Test*: ClassFileInstaller doesn't have to be run by JDK under test, so you can use '@run driver' to run it
7. TestCPUSets.java : L#72, to get a proper new line symbol you should use %n in format string. also System.out::printf seems to be more popular in our code than System.out::format
8. TestCPUSets.java: shouldn't checkResult assert that we found lineMarker?
9. Test*: would you consider adding .shouldHaveExitValue(0) to all Common.run calls which aren't expected to fail, e.g. all test* in TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in TestMemoryAwareness

Thanks,
-- Igor

> On Nov 1, 2017, at 8:11 PM, mikhailo <[hidden email]> wrote:
>
> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>     Webrev:
>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>     Testing:
>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>            Ran the developed tests via jtreg
>            Pass
>
>         2. Automated testing system - run these tests
>            In progress
>
> Thank you,
> Misha
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
In reply to this post by Bob Vandette
Hi Bob,

  Thank you for review. Please see my comments inline:

On 11/3/17, 11:04 AM, Bob Vandette wrote:
> http://cr.openjdk.java.net/~mseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/CPUSetsReader.java.html 
> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/CPUSetsReader.java.html>
>
> Not sure this is a problem but  If you specify --cpuset-cpus 2-3,1 in
> docker you end up with cpuset.cpus containing 1-3.
> Your cpusets test cases are all increasing in order so you won’t hit
> this issue.
>
I did not do the exhaustive test of all combinations of cpu sets. I
tried to cover more common cases while balancing vs complexity and
execution time.
In TestCPUSets.java I read all available cpu sets from the system,
flattened them into a list/set, and then created a container with
a subset of one, a full subset available on the system, and rounded half
of the subset.
If you wish I could file an RFE for 11 to add more corner test cases for
this. Please let me know.
>
> The read function in this file has a hard coded /sys/fs/cgroup/cpuset
> directory.  This may not be where this
> ends up being mounted.  Is this read function even used?
Yes. This function is used from " TestCPUSets.testTheSet()" to read the
sets.
I did design the method and test such that if the file can not be read,
the test case for a given set will be skipped, to avoid false failures.
I did not realize the location for this directory could vary. I can
introduce a property jdk.test.docker.cpuset.location that test users or
test system could specify to point to the location of cpuset directory;
if not specify the default value would be as it is now,
/sys/fs/cgroup/cpuset. Is this reasonable?
>
>
> http://cr.openjdk.java.net/~mseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/TestCPUAwareness.java.html
>
> Your test assumes that there are at least two physical processors on
> the host.  You might want to check first.
>
>  55             testAPCCombo("*0,1",* 200*1000, 100*1000, 4*1024, 2);
>  56             testAPCCombo("*0,1*", 200*1000, 100*1000, 1*1024, 2)
Makes sense. Will do.
>
> Everything else looks good,
> bob.
Thank you,

Misha

>
>
>> On Nov 1, 2017, at 11:11 PM, mikhailo <[hidden email]>
>> wrote:
>>
>> Please review these tests that were developed to test JVM's container
>> awareness in Docker environment.
>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>     Webrev:
>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>     Testing:
>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>            Ran the developed tests via jtreg
>>            Pass
>>
>>         2. Automated testing system - run these tests
>>            In progress
>>
>> Thank you,
>> Misha
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
In reply to this post by Igor Ignatyev
Hi Igor,

   Thank you for reviewing the code.

On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
> Hi Misha,
>
> I have several comments:
> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it should be WB_IsContainerized(JNIEnv*, jobject)
Fixed
> 2. CPUSetsReader.java : listToString can be rewritten using stream api as list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
Thank you. I will try it out.
> 3. in several files, I have noticed some problems w/ indents which make code quite hard to read, for example
>>    73         Common.run( Common.newOpts(imageName)
>>    74              .addDockerOpts("--memory", valueToSet))
>>    75             .shouldMatch("Memory Limit is:.*" + expectedTraceValue);
> or
>>    96         DockerRunOptions opts = Common.newOpts(imageName, "AttemptOOM")
>>    97             .addDockerOpts("--memory", dockerMemLimit, "--memory-swap", dockerMemLimit);
>>    98             opts.classParams.add("" + sizeToAllocInMb);
OK. I will unwind these expressions, and make sure indentation looks good.
> 4. TestCPUSets.java : it's better to use Assert.assertEquals(parts.length, 2) than Asserts.assertTrue(parts.length == 2) as the former will also print the actual value of parts.length. so you won't need to have L#103
OK
> 5. Test*: there is no need to have parentheses in '@requires (docker.support)'
OK
> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under test, so you can use '@run driver' to run it
Will fix.
> 7. TestCPUSets.java : L#72, to get a proper new line symbol you should use %n in format string. also System.out::printf seems to be more popular in our code than System.out::format
OK.
> 8. TestCPUSets.java: shouldn't checkResult assert that we found lineMarker?
Oops. Missed it. Will add code to check it.
> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all Common.run calls which aren't expected to fail, e.g. all test* in TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in TestMemoryAwareness
Common.run() already checks for .shouldHaveExitValue(0) after running
the container.


I will apply feedback from Bob and you, and will post a new webrev.


Thank you,
Misha

>
> Thanks,
> -- Igor
>
>> On Nov 1, 2017, at 8:11 PM, mikhailo<[hidden email]>  wrote:
>>
>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>      Webrev:
>>        Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>        WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>      Testing:
>>          1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>             Ran the developed tests via jtreg
>>             Pass
>>
>>          2. Automated testing system - run these tests
>>             In progress
>>
>> Thank you,
>> Misha
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Bob Vandette
In reply to this post by Mikhailo Seledtsov

> On Nov 6, 2017, at 11:05 PM, Mikhailo Seledtsov <[hidden email]> wrote:
>
> Hi Bob,
>
>  Thank you for review. Please see my comments inline:
>
> On 11/3/17, 11:04 AM, Bob Vandette wrote:
>> http://cr.openjdk.java.net/~mseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/CPUSetsReader.java.html
>>
>> Not sure this is a problem but  If you specify --cpuset-cpus 2-3,1 in docker you end up with cpuset.cpus containing 1-3.
>> Your cpusets test cases are all increasing in order so you won’t hit this issue.
>>
> I did not do the exhaustive test of all combinations of cpu sets. I tried to cover more common cases while balancing vs complexity and execution time.
> In TestCPUSets.java I read all available cpu sets from the system, flattened them into a list/set, and then created a container with
> a subset of one, a full subset available on the system, and rounded half of the subset.
> If you wish I could file an RFE for 11 to add more corner test cases for this. Please let me know.
>>
>> The read function in this file has a hard coded /sys/fs/cgroup/cpuset directory.  This may not be where this
>> ends up being mounted.  Is this read function even used?
> Yes. This function is used from " TestCPUSets.testTheSet()" to read the sets.
> I did design the method and test such that if the file can not be read, the test case for a given set will be skipped, to avoid false failures.
> I did not realize the location for this directory could vary. I can introduce a property jdk.test.docker.cpuset.location that test users or test system could specify to point to the location of cpuset directory; if not specify the default value would be as it is now, /sys/fs/cgroup/cpuset. Is this reasonable?

I think this needs to be more dynamic.  One of my internal systems has, cpuset.cpus is in /cgroup/cpuset.
Who knows how the mach5 systems are configured.

Maybe you can use /proc/self/status and extract Cpus_allowed_list?


>>
>>
>> http://cr.openjdk.java.net/~mseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/TestCPUAwareness.java.html
>>
>> Your test assumes that there are at least two physical processors on the host.  You might want to check first.
>>
>>  55             testAPCCombo("0,1", 200*1000, 100*1000, 4*1024, 2);
>>  56             testAPCCombo("0,1", 200*1000, 100*1000, 1*1024, 2)
> Makes sense. Will do.
>>
>> Everything else looks good,
>> bob.
> Thank you,

Once you get all of your changes done, maybe you can commit them in a local forest and do an hg export of the
changeset.  I can then import it and push it for you with my changes.  Assuming this works, this should preserve
you as the committer and allow the push to have both implementation and tests.

Bob.

>
> Misha
>>
>>
>>> On Nov 1, 2017, at 11:11 PM, mikhailo <[hidden email]> wrote:
>>>
>>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>     Webrev:
>>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>>     Testing:
>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>            Ran the developed tests via jtreg
>>>            Pass
>>>
>>>         2. Automated testing system - run these tests
>>>            In progress
>>>
>>> Thank you,
>>> Misha
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov


On 11/07/2017 08:14 AM, Bob Vandette wrote:

>> On Nov 6, 2017, at 11:05 PM, Mikhailo Seledtsov <[hidden email]> wrote:
>>
>> Hi Bob,
>>
>>   Thank you for review. Please see my comments inline:
>>
>> On 11/3/17, 11:04 AM, Bob Vandette wrote:
>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/CPUSetsReader.java.html
>>>
>>> Not sure this is a problem but  If you specify --cpuset-cpus 2-3,1 in docker you end up with cpuset.cpus containing 1-3.
>>> Your cpusets test cases are all increasing in order so you won’t hit this issue.
>>>
>> I did not do the exhaustive test of all combinations of cpu sets. I tried to cover more common cases while balancing vs complexity and execution time.
>> In TestCPUSets.java I read all available cpu sets from the system, flattened them into a list/set, and then created a container with
>> a subset of one, a full subset available on the system, and rounded half of the subset.
>> If you wish I could file an RFE for 11 to add more corner test cases for this. Please let me know.
>>> The read function in this file has a hard coded /sys/fs/cgroup/cpuset directory.  This may not be where this
>>> ends up being mounted.  Is this read function even used?
>> Yes. This function is used from " TestCPUSets.testTheSet()" to read the sets.
>> I did design the method and test such that if the file can not be read, the test case for a given set will be skipped, to avoid false failures.
>> I did not realize the location for this directory could vary. I can introduce a property jdk.test.docker.cpuset.location that test users or test system could specify to point to the location of cpuset directory; if not specify the default value would be as it is now, /sys/fs/cgroup/cpuset. Is this reasonable?
> I think this needs to be more dynamic.  One of my internal systems has, cpuset.cpus is in /cgroup/cpuset.
> Who knows how the mach5 systems are configured.
>
> Maybe you can use /proc/self/status and extract Cpus_allowed_list?
OK. I will change my tests to extract values from /proc/self/status. I
assume this method is more standard and reliable than reading from
/sys/fs/cgroup/cpuset. Right?


Thank you,
Misha

>
>>>
>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00/test/hotspot/jtreg/runtime/containers/docker/TestCPUAwareness.java.html
>>>
>>> Your test assumes that there are at least two physical processors on the host.  You might want to check first.
>>>
>>>   55             testAPCCombo("0,1", 200*1000, 100*1000, 4*1024, 2);
>>>   56             testAPCCombo("0,1", 200*1000, 100*1000, 1*1024, 2)
>> Makes sense. Will do.
>>> Everything else looks good,
>>> bob.
>> Thank you,
> Once you get all of your changes done, maybe you can commit them in a local forest and do an hg export of the
> changeset.  I can then import it and push it for you with my changes.  Assuming this works, this should preserve
> you as the committer and allow the push to have both implementation and tests.
>
> Bob.
>
>> Misha
>>>
>>>> On Nov 1, 2017, at 11:11 PM, mikhailo <[hidden email]> wrote:
>>>>
>>>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>>      Webrev:
>>>>        Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>>>        WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>>>      Testing:
>>>>          1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>             Ran the developed tests via jtreg
>>>>             Pass
>>>>
>>>>          2. Automated testing system - run these tests
>>>>             In progress
>>>>
>>>> Thank you,
>>>> Misha
>>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
In reply to this post by Mikhailo Seledtsov
Here is an update webrev:
     http://cr.openjdk.java.net/~mseledtsov/8189762.02/
I also tried to generate a diff between 01 and 02, but could not. My
script does not seem to work any more, or I am missing something.
Igor, please let me know if you have scripts or command line to generate
incremental webrev (I committed my initial changes locally, then applied
the new changes on top).

Summary for updated webrev.02:
  - implemented review feedback from Bob and Igor
  - added @driver to all tests, since the actual testing happens in the
child docker process; the main is just a test driver
  - configured docker directory for exclusive access by jtreg tests,
since there are potential and actual issues when running these tests
concurrently
  - modified test groups to add a hotspot_docker test group, and to
exclude docker testing from lower tiers
     (once the infra is ready, I plan to add this execution initially at
higher tiers; first will run it for a while as custom runs)

My next steps:
   - rebase to new jdk tree
   - update/merge
   - retest

Thank you,
Misha


On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:

> Hi Igor,
>
>   Thank you for reviewing the code.
>
> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>> Hi Misha,
>>
>> I have several comments:
>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it
>> should be WB_IsContainerized(JNIEnv*, jobject)
> Fixed
>> 2. CPUSetsReader.java : listToString can be rewritten using stream
>> api as
>> list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
> Thank you. I will try it out.
>> 3. in several files, I have noticed some problems w/ indents which
>> make code quite hard to read, for example
>>>    73         Common.run( Common.newOpts(imageName)
>>>    74              .addDockerOpts("--memory", valueToSet))
>>>    75             .shouldMatch("Memory Limit is:.*" +
>>> expectedTraceValue);
>> or
>>>    96         DockerRunOptions opts = Common.newOpts(imageName,
>>> "AttemptOOM")
>>>    97             .addDockerOpts("--memory", dockerMemLimit,
>>> "--memory-swap", dockerMemLimit);
>>>    98             opts.classParams.add("" + sizeToAllocInMb);
> OK. I will unwind these expressions, and make sure indentation looks
> good.
>> 4. TestCPUSets.java : it's better to use
>> Assert.assertEquals(parts.length, 2) than
>> Asserts.assertTrue(parts.length == 2) as the former will also print
>> the actual value of parts.length. so you won't need to have L#103
> OK
>> 5. Test*: there is no need to have parentheses in '@requires
>> (docker.support)'
> OK
>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under
>> test, so you can use '@run driver' to run it
> Will fix.
>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you
>> should use %n in format string. also System.out::printf seems to be
>> more popular in our code than System.out::format
> OK.
>> 8. TestCPUSets.java: shouldn't checkResult assert that we found
>> lineMarker?
> Oops. Missed it. Will add code to check it.
>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all
>> Common.run calls which aren't expected to fail, e.g. all test* in
>> TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in
>> TestMemoryAwareness
> Common.run() already checks for .shouldHaveExitValue(0) after running
> the container.
>
>
> I will apply feedback from Bob and you, and will post a new webrev.
>
>
> Thank you,
> Misha
>>
>> Thanks,
>> -- Igor
>>
>>> On Nov 1, 2017, at 8:11 PM, mikhailo<[hidden email]>  
>>> wrote:
>>>
>>> Please review these tests that were developed to test JVM's
>>> container awareness in Docker environment.
>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>      Webrev:
>>>        Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>>        WB API:
>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>>      Testing:
>>>          1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>             Ran the developed tests via jtreg
>>>             Pass
>>>
>>>          2. Automated testing system - run these tests
>>>             In progress
>>>
>>> Thank you,
>>> Misha
>>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
Here is a differential webrev:
     http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/

Misha

On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:

> Here is an update webrev:
>     http://cr.openjdk.java.net/~mseledtsov/8189762.02/
> I also tried to generate a diff between 01 and 02, but could not. My
> script does not seem to work any more, or I am missing something.
> Igor, please let me know if you have scripts or command line to
> generate incremental webrev (I committed my initial changes locally,
> then applied the new changes on top).
>
> Summary for updated webrev.02:
>  - implemented review feedback from Bob and Igor
>  - added @driver to all tests, since the actual testing happens in the
> child docker process; the main is just a test driver
>  - configured docker directory for exclusive access by jtreg tests,
> since there are potential and actual issues when running these tests
> concurrently
>  - modified test groups to add a hotspot_docker test group, and to
> exclude docker testing from lower tiers
>     (once the infra is ready, I plan to add this execution initially
> at higher tiers; first will run it for a while as custom runs)
>
> My next steps:
>   - rebase to new jdk tree
>   - update/merge
>   - retest
>
> Thank you,
> Misha
>
>
> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>> Hi Igor,
>>
>>   Thank you for reviewing the code.
>>
>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>> Hi Misha,
>>>
>>> I have several comments:
>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it
>>> should be WB_IsContainerized(JNIEnv*, jobject)
>> Fixed
>>> 2. CPUSetsReader.java : listToString can be rewritten using stream
>>> api as
>>> list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>> Thank you. I will try it out.
>>> 3. in several files, I have noticed some problems w/ indents which
>>> make code quite hard to read, for example
>>>>    73         Common.run( Common.newOpts(imageName)
>>>>    74              .addDockerOpts("--memory", valueToSet))
>>>>    75             .shouldMatch("Memory Limit is:.*" +
>>>> expectedTraceValue);
>>> or
>>>>    96         DockerRunOptions opts = Common.newOpts(imageName,
>>>> "AttemptOOM")
>>>>    97             .addDockerOpts("--memory", dockerMemLimit,
>>>> "--memory-swap", dockerMemLimit);
>>>>    98             opts.classParams.add("" + sizeToAllocInMb);
>> OK. I will unwind these expressions, and make sure indentation looks
>> good.
>>> 4. TestCPUSets.java : it's better to use
>>> Assert.assertEquals(parts.length, 2) than
>>> Asserts.assertTrue(parts.length == 2) as the former will also print
>>> the actual value of parts.length. so you won't need to have L#103
>> OK
>>> 5. Test*: there is no need to have parentheses in '@requires
>>> (docker.support)'
>> OK
>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under
>>> test, so you can use '@run driver' to run it
>> Will fix.
>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you
>>> should use %n in format string. also System.out::printf seems to be
>>> more popular in our code than System.out::format
>> OK.
>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found
>>> lineMarker?
>> Oops. Missed it. Will add code to check it.
>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all
>>> Common.run calls which aren't expected to fail, e.g. all test* in
>>> TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in
>>> TestMemoryAwareness
>> Common.run() already checks for .shouldHaveExitValue(0) after running
>> the container.
>>
>>
>> I will apply feedback from Bob and you, and will post a new webrev.
>>
>>
>> Thank you,
>> Misha
>>>
>>> Thanks,
>>> -- Igor
>>>
>>>> On Nov 1, 2017, at 8:11 PM,
>>>> mikhailo<[hidden email]>  wrote:
>>>>
>>>> Please review these tests that were developed to test JVM's
>>>> container awareness in Docker environment.
>>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>>      Webrev:
>>>>        Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>>>        WB API:
>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>>>      Testing:
>>>>          1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>             Ran the developed tests via jtreg
>>>>             Pass
>>>>
>>>>          2. Automated testing system - run these tests
>>>>             In progress
>>>>
>>>> Thank you,
>>>> Misha
>>>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Bob Vandette
Do you still need this function?  I don’t see it being used.

  84     public static String read(String fileName) {
  85         String path = CGROUP_CPUSET_PATH + fileName;
  86         try {
  87             return readLineFromFile(path);
  88         } catch (Exception e) {
  89             System.err.println("Exception reading " + path);
  90             System.err.println("Exception: " + e);
  91         }
  92
  93         return null;
  94     }

Bob.

> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov <[hidden email]> wrote:
>
> Here is a differential webrev:
>    http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/
>
> Misha
>
> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>> Here is an update webrev:
>>    http://cr.openjdk.java.net/~mseledtsov/8189762.02/
>> I also tried to generate a diff between 01 and 02, but could not. My script does not seem to work any more, or I am missing something.
>> Igor, please let me know if you have scripts or command line to generate incremental webrev (I committed my initial changes locally, then applied the new changes on top).
>>
>> Summary for updated webrev.02:
>> - implemented review feedback from Bob and Igor
>> - added @driver to all tests, since the actual testing happens in the child docker process; the main is just a test driver
>> - configured docker directory for exclusive access by jtreg tests, since there are potential and actual issues when running these tests concurrently
>> - modified test groups to add a hotspot_docker test group, and to exclude docker testing from lower tiers
>>    (once the infra is ready, I plan to add this execution initially at higher tiers; first will run it for a while as custom runs)
>>
>> My next steps:
>>  - rebase to new jdk tree
>>  - update/merge
>>  - retest
>>
>> Thank you,
>> Misha
>>
>>
>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>> Hi Igor,
>>>
>>>  Thank you for reviewing the code.
>>>
>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>> Hi Misha,
>>>>
>>>> I have several comments:
>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it should be WB_IsContainerized(JNIEnv*, jobject)
>>> Fixed
>>>> 2. CPUSetsReader.java : listToString can be rewritten using stream api as list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>> Thank you. I will try it out.
>>>> 3. in several files, I have noticed some problems w/ indents which make code quite hard to read, for example
>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>   75             .shouldMatch("Memory Limit is:.*" + expectedTraceValue);
>>>> or
>>>>>   96         DockerRunOptions opts = Common.newOpts(imageName, "AttemptOOM")
>>>>>   97             .addDockerOpts("--memory", dockerMemLimit, "--memory-swap", dockerMemLimit);
>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>> OK. I will unwind these expressions, and make sure indentation looks good.
>>>> 4. TestCPUSets.java : it's better to use Assert.assertEquals(parts.length, 2) than Asserts.assertTrue(parts.length == 2) as the former will also print the actual value of parts.length. so you won't need to have L#103
>>> OK
>>>> 5. Test*: there is no need to have parentheses in '@requires (docker.support)'
>>> OK
>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under test, so you can use '@run driver' to run it
>>> Will fix.
>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you should use %n in format string. also System.out::printf seems to be more popular in our code than System.out::format
>>> OK.
>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found lineMarker?
>>> Oops. Missed it. Will add code to check it.
>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all Common.run calls which aren't expected to fail, e.g. all test* in TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in TestMemoryAwareness
>>> Common.run() already checks for .shouldHaveExitValue(0) after running the container.
>>>
>>>
>>> I will apply feedback from Bob and you, and will post a new webrev.
>>>
>>>
>>> Thank you,
>>> Misha
>>>>
>>>> Thanks,
>>>> -- Igor
>>>>
>>>>> On Nov 1, 2017, at 8:11 PM, mikhailo<[hidden email]>  wrote:
>>>>>
>>>>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>>>     Webrev:
>>>>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/
>>>>>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/
>>>>>     Testing:
>>>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>>            Ran the developed tests via jtreg
>>>>>            Pass
>>>>>
>>>>>         2. Automated testing system - run these tests
>>>>>            In progress
>>>>>
>>>>> Thank you,
>>>>> Misha
>>>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
I am not using it. I thought it may be helpful if we go back to that
method of extracting cpuset values for some reason.
On the other hand, dead code is not a good thing. So I am a bit undecided.
I will delete it if you think it is better to do so.

Misha

On 11/9/17, 12:00 PM, Bob Vandette wrote:

> Do you still need this function?  I don’t see it being used.
>
>    84     public static String read(String fileName) {
>    85         String path = CGROUP_CPUSET_PATH + fileName;
>    86         try {
>    87             return readLineFromFile(path);
>    88         } catch (Exception e) {
>    89             System.err.println("Exception reading " + path);
>    90             System.err.println("Exception: " + e);
>    91         }
>    92
>    93         return null;
>    94     }
>
> Bob.
>
>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov
>> <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> Here is a differential webrev:
>> http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/ 
>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.01-02/webrev/>
>>
>> Misha
>>
>> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>>> Here is an update webrev:
>>> http://cr.openjdk.java.net/~mseledtsov/8189762.02/ 
>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.02/>
>>> I also tried to generate a diff between 01 and 02, but could not. My
>>> script does not seem to work any more, or I am missing something.
>>> Igor, please let me know if you have scripts or command line to
>>> generate incremental webrev (I committed my initial changes locally,
>>> then applied the new changes on top).
>>>
>>> Summary for updated webrev.02:
>>> - implemented review feedback from Bob and Igor
>>> - added @driver to all tests, since the actual testing happens in
>>> the child docker process; the main is just a test driver
>>> - configured docker directory for exclusive access by jtreg tests,
>>> since there are potential and actual issues when running these tests
>>> concurrently
>>> - modified test groups to add a hotspot_docker test group, and to
>>> exclude docker testing from lower tiers
>>>    (once the infra is ready, I plan to add this execution initially
>>> at higher tiers; first will run it for a while as custom runs)
>>>
>>> My next steps:
>>>  - rebase to new jdk tree
>>>  - update/merge
>>>  - retest
>>>
>>> Thank you,
>>> Misha
>>>
>>>
>>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>>> Hi Igor,
>>>>
>>>>  Thank you for reviewing the code.
>>>>
>>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>>> Hi Misha,
>>>>>
>>>>> I have several comments:
>>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature,
>>>>> it should be WB_IsContainerized(JNIEnv*, jobject)
>>>> Fixed
>>>>> 2. CPUSetsReader.java : listToString can be rewritten using stream
>>>>> api as
>>>>> list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>>> Thank you. I will try it out.
>>>>> 3. in several files, I have noticed some problems w/ indents which
>>>>> make code quite hard to read, for example
>>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>>   75             .shouldMatch("Memory Limit is:.*" +
>>>>>> expectedTraceValue);
>>>>> or
>>>>>>   96         DockerRunOptions opts = Common.newOpts(imageName,
>>>>>> "AttemptOOM")
>>>>>>   97             .addDockerOpts("--memory", dockerMemLimit,
>>>>>> "--memory-swap", dockerMemLimit);
>>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>>> OK. I will unwind these expressions, and make sure indentation
>>>> looks good.
>>>>> 4. TestCPUSets.java : it's better to use
>>>>> Assert.assertEquals(parts.length, 2) than
>>>>> Asserts.assertTrue(parts.length == 2) as the former will also
>>>>> print the actual value of parts.length. so you won't need to have
>>>>> L#103
>>>> OK
>>>>> 5. Test*: there is no need to have parentheses in '@requires
>>>>> (docker.support)'
>>>> OK
>>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under
>>>>> test, so you can use '@run driver' to run it
>>>> Will fix.
>>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you
>>>>> should use %n in format string. also System.out::printf seems to
>>>>> be more popular in our code than System.out::format
>>>> OK.
>>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found
>>>>> lineMarker?
>>>> Oops. Missed it. Will add code to check it.
>>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all
>>>>> Common.run calls which aren't expected to fail, e.g. all test* in
>>>>> TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in
>>>>> TestMemoryAwareness
>>>> Common.run() already checks for .shouldHaveExitValue(0) after
>>>> running the container.
>>>>
>>>>
>>>> I will apply feedback from Bob and you, and will post a new webrev.
>>>>
>>>>
>>>> Thank you,
>>>> Misha
>>>>>
>>>>> Thanks,
>>>>> -- Igor
>>>>>
>>>>>> On Nov 1, 2017, at 8:11 PM,
>>>>>> mikhailo<[hidden email]
>>>>>> <mailto:[hidden email]>>  wrote:
>>>>>>
>>>>>> Please review these tests that were developed to test JVM's
>>>>>> container awareness in Docker environment.
>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>>>>     Webrev:
>>>>>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/ 
>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>       WB API:
>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/ 
>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00.whitebox/>
>>>>>>     Testing:
>>>>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>>>            Ran the developed tests via jtreg
>>>>>>            Pass
>>>>>>
>>>>>>         2. Automated testing system - run these tests
>>>>>>            In progress
>>>>>>
>>>>>> Thank you,
>>>>>> Misha
>>>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Bob Vandette
Since that path is not guaranteed to work, I’d remove CGROUP_CPUSET_PATH and the function.

Bob.

> On Nov 9, 2017, at 3:31 PM, Mikhailo Seledtsov <[hidden email]> wrote:
>
> I am not using it. I thought it may be helpful if we go back to that method of extracting cpuset values for some reason.
> On the other hand, dead code is not a good thing. So I am a bit undecided.
> I will delete it if you think it is better to do so.
>
> Misha
>
> On 11/9/17, 12:00 PM, Bob Vandette wrote:
>>
>> Do you still need this function?  I don’t see it being used.
>>
>>   84     public static String read(String fileName) {
>>   85         String path = CGROUP_CPUSET_PATH + fileName;
>>   86         try {
>>   87             return readLineFromFile(path);
>>   88         } catch (Exception e) {
>>   89             System.err.println("Exception reading " + path);
>>   90             System.err.println("Exception: " + e);
>>   91         }
>>   92
>>   93         return null;
>>   94     }
>>
>> Bob.
>>
>>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>> Here is a differential webrev:
>>>    http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.01-02/webrev/>
>>>
>>> Misha
>>>
>>> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>>>> Here is an update webrev:
>>>>    http://cr.openjdk.java.net/~mseledtsov/8189762.02/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.02/>
>>>> I also tried to generate a diff between 01 and 02, but could not. My script does not seem to work any more, or I am missing something.
>>>> Igor, please let me know if you have scripts or command line to generate incremental webrev (I committed my initial changes locally, then applied the new changes on top).
>>>>
>>>> Summary for updated webrev.02:
>>>> - implemented review feedback from Bob and Igor
>>>> - added @driver to all tests, since the actual testing happens in the child docker process; the main is just a test driver
>>>> - configured docker directory for exclusive access by jtreg tests, since there are potential and actual issues when running these tests concurrently
>>>> - modified test groups to add a hotspot_docker test group, and to exclude docker testing from lower tiers
>>>>    (once the infra is ready, I plan to add this execution initially at higher tiers; first will run it for a while as custom runs)
>>>>
>>>> My next steps:
>>>>  - rebase to new jdk tree
>>>>  - update/merge
>>>>  - retest
>>>>
>>>> Thank you,
>>>> Misha
>>>>
>>>>
>>>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>>>> Hi Igor,
>>>>>
>>>>>  Thank you for reviewing the code.
>>>>>
>>>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>>>> Hi Misha,
>>>>>>
>>>>>> I have several comments:
>>>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it should be WB_IsContainerized(JNIEnv*, jobject)
>>>>> Fixed
>>>>>> 2. CPUSetsReader.java : listToString can be rewritten using stream api as list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>>>> Thank you. I will try it out.
>>>>>> 3. in several files, I have noticed some problems w/ indents which make code quite hard to read, for example
>>>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>>>   75             .shouldMatch("Memory Limit is:.*" + expectedTraceValue);
>>>>>> or
>>>>>>>   96         DockerRunOptions opts = Common.newOpts(imageName, "AttemptOOM")
>>>>>>>   97             .addDockerOpts("--memory", dockerMemLimit, "--memory-swap", dockerMemLimit);
>>>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>>>> OK. I will unwind these expressions, and make sure indentation looks good.
>>>>>> 4. TestCPUSets.java : it's better to use Assert.assertEquals(parts.length, 2) than Asserts.assertTrue(parts.length == 2) as the former will also print the actual value of parts.length. so you won't need to have L#103
>>>>> OK
>>>>>> 5. Test*: there is no need to have parentheses in '@requires (docker.support)'
>>>>> OK
>>>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under test, so you can use '@run driver' to run it
>>>>> Will fix.
>>>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you should use %n in format string. also System.out::printf seems to be more popular in our code than System.out::format
>>>>> OK.
>>>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found lineMarker?
>>>>> Oops. Missed it. Will add code to check it.
>>>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all Common.run calls which aren't expected to fail, e.g. all test* in TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in TestMemoryAwareness
>>>>> Common.run() already checks for .shouldHaveExitValue(0) after running the container.
>>>>>
>>>>>
>>>>> I will apply feedback from Bob and you, and will post a new webrev.
>>>>>
>>>>>
>>>>> Thank you,
>>>>> Misha
>>>>>>
>>>>>> Thanks,
>>>>>> -- Igor
>>>>>>
>>>>>>> On Nov 1, 2017, at 8:11 PM, mikhailo<[hidden email] <mailto:[hidden email]>>  wrote:
>>>>>>>
>>>>>>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762 <https://bugs.openjdk.java.net/browse/JDK-8189762>
>>>>>>>     Webrev:
>>>>>>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00.whitebox/>
>>>>>>>     Testing:
>>>>>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>>>>            Ran the developed tests via jtreg
>>>>>>>            Pass
>>>>>>>
>>>>>>>         2. Automated testing system - run these tests
>>>>>>>            In progress
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Misha
>>>>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
OK, will remove it.

Misha

On 11/9/17, 12:38 PM, Bob Vandette wrote:

> Since that path is not guaranteed to work, I’d remove
> CGROUP_CPUSET_PATH and the function.
>
> Bob.
>
>> On Nov 9, 2017, at 3:31 PM, Mikhailo Seledtsov
>> <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> I am not using it. I thought it may be helpful if we go back to that
>> method of extracting cpuset values for some reason.
>> On the other hand, dead code is not a good thing. So I am a bit
>> undecided.
>> I will delete it if you think it is better to do so.
>>
>> Misha
>>
>> On 11/9/17, 12:00 PM, Bob Vandette wrote:
>>> Do you still need this function?  I don’t see it being used.
>>>
>>>    84     public static String read(String fileName) {
>>>    85         String path = CGROUP_CPUSET_PATH + fileName;
>>>    86         try {
>>>    87             return readLineFromFile(path);
>>>    88         } catch (Exception e) {
>>>    89             System.err.println("Exception reading " + path);
>>>    90             System.err.println("Exception: " + e);
>>>    91         }
>>>    92
>>>    93         return null;
>>>    94     }
>>>
>>> Bob.
>>>
>>>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov
>>>> <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>> Here is a differential webrev:
>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.01-02/webrev/>
>>>>
>>>> Misha
>>>>
>>>> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>>>>> Here is an update webrev:
>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.02/ 
>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.02/>
>>>>> I also tried to generate a diff between 01 and 02, but could not.
>>>>> My script does not seem to work any more, or I am missing something.
>>>>> Igor, please let me know if you have scripts or command line to
>>>>> generate incremental webrev (I committed my initial changes
>>>>> locally, then applied the new changes on top).
>>>>>
>>>>> Summary for updated webrev.02:
>>>>> - implemented review feedback from Bob and Igor
>>>>> - added @driver to all tests, since the actual testing happens in
>>>>> the child docker process; the main is just a test driver
>>>>> - configured docker directory for exclusive access by jtreg tests,
>>>>> since there are potential and actual issues when running these
>>>>> tests concurrently
>>>>> - modified test groups to add a hotspot_docker test group, and to
>>>>> exclude docker testing from lower tiers
>>>>>    (once the infra is ready, I plan to add this execution
>>>>> initially at higher tiers; first will run it for a while as custom
>>>>> runs)
>>>>>
>>>>> My next steps:
>>>>>  - rebase to new jdk tree
>>>>>  - update/merge
>>>>>  - retest
>>>>>
>>>>> Thank you,
>>>>> Misha
>>>>>
>>>>>
>>>>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>>>>> Hi Igor,
>>>>>>
>>>>>>  Thank you for reviewing the code.
>>>>>>
>>>>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>>>>> Hi Misha,
>>>>>>>
>>>>>>> I have several comments:
>>>>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature,
>>>>>>> it should be WB_IsContainerized(JNIEnv*, jobject)
>>>>>> Fixed
>>>>>>> 2. CPUSetsReader.java : listToString can be rewritten using
>>>>>>> stream api as
>>>>>>> list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>>>>> Thank you. I will try it out.
>>>>>>> 3. in several files, I have noticed some problems w/ indents
>>>>>>> which make code quite hard to read, for example
>>>>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>>>>   75             .shouldMatch("Memory Limit is:.*" +
>>>>>>>> expectedTraceValue);
>>>>>>> or
>>>>>>>>   96         DockerRunOptions opts = Common.newOpts(imageName,
>>>>>>>> "AttemptOOM")
>>>>>>>>   97             .addDockerOpts("--memory", dockerMemLimit,
>>>>>>>> "--memory-swap", dockerMemLimit);
>>>>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>>>>> OK. I will unwind these expressions, and make sure indentation
>>>>>> looks good.
>>>>>>> 4. TestCPUSets.java : it's better to use
>>>>>>> Assert.assertEquals(parts.length, 2) than
>>>>>>> Asserts.assertTrue(parts.length == 2) as the former will also
>>>>>>> print the actual value of parts.length. so you won't need to
>>>>>>> have L#103
>>>>>> OK
>>>>>>> 5. Test*: there is no need to have parentheses in '@requires
>>>>>>> (docker.support)'
>>>>>> OK
>>>>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under
>>>>>>> test, so you can use '@run driver' to run it
>>>>>> Will fix.
>>>>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you
>>>>>>> should use %n in format string. also System.out::printf seems to
>>>>>>> be more popular in our code than System.out::format
>>>>>> OK.
>>>>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found
>>>>>>> lineMarker?
>>>>>> Oops. Missed it. Will add code to check it.
>>>>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to
>>>>>>> all Common.run calls which aren't expected to fail, e.g. all
>>>>>>> test* in TestCPUAwareness, testMemorySoftLimit and
>>>>>>> testMemoryLimit in TestMemoryAwareness
>>>>>> Common.run() already checks for .shouldHaveExitValue(0) after
>>>>>> running the container.
>>>>>>
>>>>>>
>>>>>> I will apply feedback from Bob and you, and will post a new webrev.
>>>>>>
>>>>>>
>>>>>> Thank you,
>>>>>> Misha
>>>>>>>
>>>>>>> Thanks,
>>>>>>> -- Igor
>>>>>>>
>>>>>>>> On Nov 1, 2017, at 8:11 PM,
>>>>>>>> mikhailo<[hidden email]
>>>>>>>> <mailto:[hidden email]>>  wrote:
>>>>>>>>
>>>>>>>> Please review these tests that were developed to test JVM's
>>>>>>>> container awareness in Docker environment.
>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>>>>>>     Webrev:
>>>>>>>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/ 
>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>>>       WB API:
>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/ 
>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00.whitebox/>
>>>>>>>>     Testing:
>>>>>>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>>>>>            Ran the developed tests via jtreg
>>>>>>>>            Pass
>>>>>>>>
>>>>>>>>         2. Automated testing system - run these tests
>>>>>>>>            In progress
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Misha
>>>>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Igor Ignatyev
Hi Misha,

a few more comments:
- are you using CPUSetsReader::listToString(boolean abc, List<Integer> list, int maxCount) method? I wasn't able to find any usages of it.
- CPUSetsReader uses Asserts.assertTrue(parts.length == 2) instead of Asserts.assertEQ(parts.length, 2)
- instead of using Stream::collect in CPUSetsReader::readFromProcStatus, you can use Stream::findFirst(). so it'll be smth like

> +        try (Stream<String> stream = Files.lines(Paths.get(path))) {
> +            o = stream
> +                .filter(line -> line.contains(setType))
> +                .findFirst();
> +        } catch (IOException e) {
> +            return null;
> +        }
> +
> +        if (!o.isPresent()) {
> +            return null;
> +        }
> +
> +        String[] parts = o.get().replaceAll("\\s","").split(":");
- you don't need to box int into Integer in TestCPUSets::checkResult L#108:
> Asserts.assertEquals(new Integer(parts.length), new Integer(2));
- IIRC, in order to properly use whitebox API, you have to have both sun.hotspot.WhiteBox and sun.hotspot.WhiteBox$WhiteBoxPermission in BCP, but '@run driver ClassFileInstaller -jar whitebox.jar sun.hotspot.WhiteBox', which is used in all your tests, will put only s.h.WhiteBox in whitebox.jar
- how do you plan to use :hotspot_containers test group? can't you use the test directory instead?

the rest looks good to me.

Thanks,
-- Igor

> On Nov 9, 2017, at 1:04 PM, Mikhailo Seledtsov <[hidden email]> wrote:
>
> OK, will remove it.
>
> Misha
>
> On 11/9/17, 12:38 PM, Bob Vandette wrote:
>>
>> Since that path is not guaranteed to work, I’d remove CGROUP_CPUSET_PATH and the function.
>>
>> Bob.
>>
>>> On Nov 9, 2017, at 3:31 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>> I am not using it. I thought it may be helpful if we go back to that method of extracting cpuset values for some reason.
>>> On the other hand, dead code is not a good thing. So I am a bit undecided.
>>> I will delete it if you think it is better to do so.
>>>
>>> Misha
>>>
>>> On 11/9/17, 12:00 PM, Bob Vandette wrote:
>>>>
>>>> Do you still need this function?  I don’t see it being used.
>>>>
>>>>   84     public static String read(String fileName) {
>>>>   85         String path = CGROUP_CPUSET_PATH + fileName;
>>>>   86         try {
>>>>   87             return readLineFromFile(path);
>>>>   88         } catch (Exception e) {
>>>>   89             System.err.println("Exception reading " + path);
>>>>   90             System.err.println("Exception: " + e);
>>>>   91         }
>>>>   92
>>>>   93         return null;
>>>>   94     }
>>>>
>>>> Bob.
>>>>
>>>>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>
>>>>> Here is a differential webrev:
>>>>>    http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.01-02/webrev/>
>>>>>
>>>>> Misha
>>>>>
>>>>> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>>>>>> Here is an update webrev:
>>>>>>    http://cr.openjdk.java.net/~mseledtsov/8189762.02/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.02/>
>>>>>> I also tried to generate a diff between 01 and 02, but could not. My script does not seem to work any more, or I am missing something.
>>>>>> Igor, please let me know if you have scripts or command line to generate incremental webrev (I committed my initial changes locally, then applied the new changes on top).
>>>>>>
>>>>>> Summary for updated webrev.02:
>>>>>> - implemented review feedback from Bob and Igor
>>>>>> - added @driver to all tests, since the actual testing happens in the child docker process; the main is just a test driver
>>>>>> - configured docker directory for exclusive access by jtreg tests, since there are potential and actual issues when running these tests concurrently
>>>>>> - modified test groups to add a hotspot_docker test group, and to exclude docker testing from lower tiers
>>>>>>    (once the infra is ready, I plan to add this execution initially at higher tiers; first will run it for a while as custom runs)
>>>>>>
>>>>>> My next steps:
>>>>>>  - rebase to new jdk tree
>>>>>>  - update/merge
>>>>>>  - retest
>>>>>>
>>>>>> Thank you,
>>>>>> Misha
>>>>>>
>>>>>>
>>>>>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>>>>>> Hi Igor,
>>>>>>>
>>>>>>>  Thank you for reviewing the code.
>>>>>>>
>>>>>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>>>>>> Hi Misha,
>>>>>>>>
>>>>>>>> I have several comments:
>>>>>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it should be WB_IsContainerized(JNIEnv*, jobject)
>>>>>>> Fixed
>>>>>>>> 2. CPUSetsReader.java : listToString can be rewritten using stream api as list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>>>>>> Thank you. I will try it out.
>>>>>>>> 3. in several files, I have noticed some problems w/ indents which make code quite hard to read, for example
>>>>>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>>>>>   75             .shouldMatch("Memory Limit is:.*" + expectedTraceValue);
>>>>>>>> or
>>>>>>>>>   96         DockerRunOptions opts = Common.newOpts(imageName, "AttemptOOM")
>>>>>>>>>   97             .addDockerOpts("--memory", dockerMemLimit, "--memory-swap", dockerMemLimit);
>>>>>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>>>>>> OK. I will unwind these expressions, and make sure indentation looks good.
>>>>>>>> 4. TestCPUSets.java : it's better to use Assert.assertEquals(parts.length, 2) than Asserts.assertTrue(parts.length == 2) as the former will also print the actual value of parts.length. so you won't need to have L#103
>>>>>>> OK
>>>>>>>> 5. Test*: there is no need to have parentheses in '@requires (docker.support)'
>>>>>>> OK
>>>>>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under test, so you can use '@run driver' to run it
>>>>>>> Will fix.
>>>>>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you should use %n in format string. also System.out::printf seems to be more popular in our code than System.out::format
>>>>>>> OK.
>>>>>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found lineMarker?
>>>>>>> Oops. Missed it. Will add code to check it.
>>>>>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all Common.run calls which aren't expected to fail, e.g. all test* in TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in TestMemoryAwareness
>>>>>>> Common.run() already checks for .shouldHaveExitValue(0) after running the container.
>>>>>>>
>>>>>>>
>>>>>>> I will apply feedback from Bob and you, and will post a new webrev.
>>>>>>>
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Misha
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> -- Igor
>>>>>>>>
>>>>>>>>> On Nov 1, 2017, at 8:11 PM, mikhailo<[hidden email] <mailto:[hidden email]>>  wrote:
>>>>>>>>>
>>>>>>>>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762 <https://bugs.openjdk.java.net/browse/JDK-8189762>
>>>>>>>>>     Webrev:
>>>>>>>>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>>>>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00.whitebox/>
>>>>>>>>>     Testing:
>>>>>>>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>>>>>>            Ran the developed tests via jtreg
>>>>>>>>>            Pass
>>>>>>>>>
>>>>>>>>>         2. Automated testing system - run these tests
>>>>>>>>>            In progress
>>>>>>>>>
>>>>>>>>> Thank you,
>>>>>>>>> Misha
>>>>>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
Hi Igor, Bob,

   Webrev containing latest changes based on feedback from you:
   Diff from 02:  http://cr.openjdk.java.net/~mseledtsov/8189762.02-03/
   Full:                http://cr.openjdk.java.net/~mseledtsov/8189762.03/


Thank you,
Misha


On 11/14/17, 4:00 PM, Igor Ignatyev wrote:

> Hi Misha,
>
> a few more comments:
> - are you using CPUSetsReader::listToString(boolean abc, List<Integer>
> list, int maxCount) method? I wasn't able to find any usages of it.
> - CPUSetsReader uses Asserts.assertTrue(parts.length == 2) instead of
> Asserts.assertEQ(parts.length, 2)
> - instead of using Stream::collect in
> CPUSetsReader::readFromProcStatus, you can use Stream::findFirst(). so
> it'll be smth like
>> +        try (Stream<String>  stream = Files.lines(Paths.get(path))) {
>> +            o = stream
>> +                .filter(line ->  line.contains(setType))
>> +                .findFirst();
>> +        } catch (IOException e) {
>> +            return null;
>> +        }
>> +
>> +        if (!o.isPresent()) {
>> +            return null;
>> +        }
>> +
>> +        String[] parts = o.get().replaceAll("\\s","").split(":");
> - you don't need to box int into Integer in TestCPUSets::checkResult
> L#108:
>> Asserts.assertEquals(new Integer(parts.length), new Integer(2));
> - IIRC, in order to properly use whitebox API, you have to have
> both sun.hotspot.WhiteBox and sun.hotspot.WhiteBox$WhiteBoxPermission
> in BCP, but '@run driver ClassFileInstaller -jar whitebox.jar
> sun.hotspot.WhiteBox', which is used in all your tests, will put only
> s.h.WhiteBox in whitebox.jar
> - how do you plan to use :hotspot_containers test group? can't you use
> the test directory instead?
>
> the rest looks good to me.
>
> Thanks,
> -- Igor
>
>> On Nov 9, 2017, at 1:04 PM, Mikhailo Seledtsov
>> <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> OK, will remove it.
>>
>> Misha
>>
>> On 11/9/17, 12:38 PM, Bob Vandette wrote:
>>> Since that path is not guaranteed to work, I’d remove
>>> CGROUP_CPUSET_PATH and the function.
>>>
>>> Bob.
>>>
>>>> On Nov 9, 2017, at 3:31 PM, Mikhailo Seledtsov
>>>> <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>> I am not using it. I thought it may be helpful if we go back to
>>>> that method of extracting cpuset values for some reason.
>>>> On the other hand, dead code is not a good thing. So I am a bit
>>>> undecided.
>>>> I will delete it if you think it is better to do so.
>>>>
>>>> Misha
>>>>
>>>> On 11/9/17, 12:00 PM, Bob Vandette wrote:
>>>>> Do you still need this function?  I don’t see it being used.
>>>>>
>>>>>    84     public static String read(String fileName) {
>>>>>    85         String path = CGROUP_CPUSET_PATH + fileName;
>>>>>    86         try {
>>>>>    87             return readLineFromFile(path);
>>>>>    88         } catch (Exception e) {
>>>>>    89             System.err.println("Exception reading " + path);
>>>>>    90             System.err.println("Exception: " + e);
>>>>>    91         }
>>>>>    92
>>>>>    93         return null;
>>>>>    94     }
>>>>>
>>>>> Bob.
>>>>>
>>>>>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov
>>>>>> <[hidden email]
>>>>>> <mailto:[hidden email]>> wrote:
>>>>>>
>>>>>> Here is a differential webrev:
>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/ 
>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.01-02/webrev/>
>>>>>>
>>>>>> Misha
>>>>>>
>>>>>> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>>>>>>> Here is an update webrev:
>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.02/ 
>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.02/>
>>>>>>> I also tried to generate a diff between 01 and 02, but could
>>>>>>> not. My script does not seem to work any more, or I am missing
>>>>>>> something.
>>>>>>> Igor, please let me know if you have scripts or command line to
>>>>>>> generate incremental webrev (I committed my initial changes
>>>>>>> locally, then applied the new changes on top).
>>>>>>>
>>>>>>> Summary for updated webrev.02:
>>>>>>> - implemented review feedback from Bob and Igor
>>>>>>> - added @driver to all tests, since the actual testing happens
>>>>>>> in the child docker process; the main is just a test driver
>>>>>>> - configured docker directory for exclusive access by jtreg
>>>>>>> tests, since there are potential and actual issues when running
>>>>>>> these tests concurrently
>>>>>>> - modified test groups to add a hotspot_docker test group, and
>>>>>>> to exclude docker testing from lower tiers
>>>>>>>    (once the infra is ready, I plan to add this execution
>>>>>>> initially at higher tiers; first will run it for a while as
>>>>>>> custom runs)
>>>>>>>
>>>>>>> My next steps:
>>>>>>>  - rebase to new jdk tree
>>>>>>>  - update/merge
>>>>>>>  - retest
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Misha
>>>>>>>
>>>>>>>
>>>>>>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>>>>>>> Hi Igor,
>>>>>>>>
>>>>>>>>  Thank you for reviewing the code.
>>>>>>>>
>>>>>>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>>>>>>> Hi Misha,
>>>>>>>>>
>>>>>>>>> I have several comments:
>>>>>>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect
>>>>>>>>> signature, it should be WB_IsContainerized(JNIEnv*, jobject)
>>>>>>>> Fixed
>>>>>>>>> 2. CPUSetsReader.java : listToString can be rewritten using
>>>>>>>>> stream api as
>>>>>>>>> list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>>>>>>> Thank you. I will try it out.
>>>>>>>>> 3. in several files, I have noticed some problems w/ indents
>>>>>>>>> which make code quite hard to read, for example
>>>>>>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>>>>>>   75             .shouldMatch("Memory Limit is:.*" +
>>>>>>>>>> expectedTraceValue);
>>>>>>>>> or
>>>>>>>>>>   96         DockerRunOptions opts =
>>>>>>>>>> Common.newOpts(imageName, "AttemptOOM")
>>>>>>>>>>   97             .addDockerOpts("--memory", dockerMemLimit,
>>>>>>>>>> "--memory-swap", dockerMemLimit);
>>>>>>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>>>>>>> OK. I will unwind these expressions, and make sure indentation
>>>>>>>> looks good.
>>>>>>>>> 4. TestCPUSets.java : it's better to use
>>>>>>>>> Assert.assertEquals(parts.length, 2) than
>>>>>>>>> Asserts.assertTrue(parts.length == 2) as the former will also
>>>>>>>>> print the actual value of parts.length. so you won't need to
>>>>>>>>> have L#103
>>>>>>>> OK
>>>>>>>>> 5. Test*: there is no need to have parentheses in '@requires
>>>>>>>>> (docker.support)'
>>>>>>>> OK
>>>>>>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK
>>>>>>>>> under test, so you can use '@run driver' to run it
>>>>>>>> Will fix.
>>>>>>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol
>>>>>>>>> you should use %n in format string. also System.out::printf
>>>>>>>>> seems to be more popular in our code than System.out::format
>>>>>>>> OK.
>>>>>>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we
>>>>>>>>> found lineMarker?
>>>>>>>> Oops. Missed it. Will add code to check it.
>>>>>>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to
>>>>>>>>> all Common.run calls which aren't expected to fail, e.g. all
>>>>>>>>> test* in TestCPUAwareness, testMemorySoftLimit and
>>>>>>>>> testMemoryLimit in TestMemoryAwareness
>>>>>>>> Common.run() already checks for .shouldHaveExitValue(0) after
>>>>>>>> running the container.
>>>>>>>>
>>>>>>>>
>>>>>>>> I will apply feedback from Bob and you, and will post a new webrev.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Misha
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> -- Igor
>>>>>>>>>
>>>>>>>>>> On Nov 1, 2017, at 8:11 PM,
>>>>>>>>>> mikhailo<[hidden email]
>>>>>>>>>> <mailto:[hidden email]>>  wrote:
>>>>>>>>>>
>>>>>>>>>> Please review these tests that were developed to test JVM's
>>>>>>>>>> container awareness in Docker environment.
>>>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>>>>>>>>     Webrev:
>>>>>>>>>>       Tests:
>>>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00/ 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>>>>>       WB API:
>>>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/ 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00.whitebox/>
>>>>>>>>>>     Testing:
>>>>>>>>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>>>>>>>            Ran the developed tests via jtreg
>>>>>>>>>>            Pass
>>>>>>>>>>
>>>>>>>>>>         2. Automated testing system - run these tests
>>>>>>>>>>            In progress
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Misha
>>>>>>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Igor Ignatyev
looks good to me

-- Igor

> On Nov 15, 2017, at 1:37 PM, Mikhailo Seledtsov <[hidden email]> wrote:
>
> Hi Igor, Bob,
>
>   Webrev containing latest changes based on feedback from you:
>   Diff from 02:  http://cr.openjdk.java.net/~mseledtsov/8189762.02-03/ <http://cr.openjdk.java.net/~mseledtsov/8189762.02-03/>
>   Full:                http://cr.openjdk.java.net/~mseledtsov/8189762.03/ <http://cr.openjdk.java.net/~mseledtsov/8189762.03/>
>
>
> Thank you,
> Misha
>
>
> On 11/14/17, 4:00 PM, Igor Ignatyev wrote:
>>
>> Hi Misha,
>>
>> a few more comments:
>> - are you using CPUSetsReader::listToString(boolean abc, List<Integer> list, int maxCount) method? I wasn't able to find any usages of it.
>> - CPUSetsReader uses Asserts.assertTrue(parts.length == 2) instead of Asserts.assertEQ(parts.length, 2)
>> - instead of using Stream::collect in CPUSetsReader::readFromProcStatus, you can use Stream::findFirst(). so it'll be smth like
>>> +        try (Stream<String> stream = Files.lines(Paths.get(path))) {
>>> +            o = stream
>>> +                .filter(line -> line.contains(setType))
>>> +                .findFirst();
>>> +        } catch (IOException e) {
>>> +            return null;
>>> +        }
>>> +
>>> +        if (!o.isPresent()) {
>>> +            return null;
>>> +        }
>>> +
>>> +        String[] parts = o.get().replaceAll("\\s","").split(":");
>> - you don't need to box int into Integer in TestCPUSets::checkResult L#108:
>>> Asserts.assertEquals(new Integer(parts.length), new Integer(2));
>> - IIRC, in order to properly use whitebox API, you have to have both sun.hotspot.WhiteBox and sun.hotspot.WhiteBox$WhiteBoxPermission in BCP, but '@run driver ClassFileInstaller -jar whitebox.jar sun.hotspot.WhiteBox', which is used in all your tests, will put only s.h.WhiteBox in whitebox.jar
>> - how do you plan to use :hotspot_containers test group? can't you use the test directory instead?
>>
>> the rest looks good to me.
>>
>> Thanks,
>> -- Igor
>>
>>> On Nov 9, 2017, at 1:04 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>> OK, will remove it.
>>>
>>> Misha
>>>
>>> On 11/9/17, 12:38 PM, Bob Vandette wrote:
>>>>
>>>> Since that path is not guaranteed to work, I’d remove CGROUP_CPUSET_PATH and the function.
>>>>
>>>> Bob.
>>>>
>>>>> On Nov 9, 2017, at 3:31 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>
>>>>> I am not using it. I thought it may be helpful if we go back to that method of extracting cpuset values for some reason.
>>>>> On the other hand, dead code is not a good thing. So I am a bit undecided.
>>>>> I will delete it if you think it is better to do so.
>>>>>
>>>>> Misha
>>>>>
>>>>> On 11/9/17, 12:00 PM, Bob Vandette wrote:
>>>>>>
>>>>>> Do you still need this function?  I don’t see it being used.
>>>>>>
>>>>>>   84     public static String read(String fileName) {
>>>>>>   85         String path = CGROUP_CPUSET_PATH + fileName;
>>>>>>   86         try {
>>>>>>   87             return readLineFromFile(path);
>>>>>>   88         } catch (Exception e) {
>>>>>>   89             System.err.println("Exception reading " + path);
>>>>>>   90             System.err.println("Exception: " + e);
>>>>>>   91         }
>>>>>>   92
>>>>>>   93         return null;
>>>>>>   94     }
>>>>>>
>>>>>> Bob.
>>>>>>
>>>>>>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>>>
>>>>>>> Here is a differential webrev:
>>>>>>>    http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.01-02/webrev/>
>>>>>>>
>>>>>>> Misha
>>>>>>>
>>>>>>> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>>>>>>>> Here is an update webrev:
>>>>>>>>    http://cr.openjdk.java.net/~mseledtsov/8189762.02/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.02/>
>>>>>>>> I also tried to generate a diff between 01 and 02, but could not. My script does not seem to work any more, or I am missing something.
>>>>>>>> Igor, please let me know if you have scripts or command line to generate incremental webrev (I committed my initial changes locally, then applied the new changes on top).
>>>>>>>>
>>>>>>>> Summary for updated webrev.02:
>>>>>>>> - implemented review feedback from Bob and Igor
>>>>>>>> - added @driver to all tests, since the actual testing happens in the child docker process; the main is just a test driver
>>>>>>>> - configured docker directory for exclusive access by jtreg tests, since there are potential and actual issues when running these tests concurrently
>>>>>>>> - modified test groups to add a hotspot_docker test group, and to exclude docker testing from lower tiers
>>>>>>>>    (once the infra is ready, I plan to add this execution initially at higher tiers; first will run it for a while as custom runs)
>>>>>>>>
>>>>>>>> My next steps:
>>>>>>>>  - rebase to new jdk tree
>>>>>>>>  - update/merge
>>>>>>>>  - retest
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Misha
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>>>>>>>> Hi Igor,
>>>>>>>>>
>>>>>>>>>  Thank you for reviewing the code.
>>>>>>>>>
>>>>>>>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>>>>>>>> Hi Misha,
>>>>>>>>>>
>>>>>>>>>> I have several comments:
>>>>>>>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it should be WB_IsContainerized(JNIEnv*, jobject)
>>>>>>>>> Fixed
>>>>>>>>>> 2. CPUSetsReader.java : listToString can be rewritten using stream api as list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>>>>>>>> Thank you. I will try it out.
>>>>>>>>>> 3. in several files, I have noticed some problems w/ indents which make code quite hard to read, for example
>>>>>>>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>>>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>>>>>>>   75             .shouldMatch("Memory Limit is:.*" + expectedTraceValue);
>>>>>>>>>> or
>>>>>>>>>>>   96         DockerRunOptions opts = Common.newOpts(imageName, "AttemptOOM")
>>>>>>>>>>>   97             .addDockerOpts("--memory", dockerMemLimit, "--memory-swap", dockerMemLimit);
>>>>>>>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>>>>>>>> OK. I will unwind these expressions, and make sure indentation looks good.
>>>>>>>>>> 4. TestCPUSets.java : it's better to use Assert.assertEquals(parts.length, 2) than Asserts.assertTrue(parts.length == 2) as the former will also print the actual value of parts.length. so you won't need to have L#103
>>>>>>>>> OK
>>>>>>>>>> 5. Test*: there is no need to have parentheses in '@requires (docker.support)'
>>>>>>>>> OK
>>>>>>>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under test, so you can use '@run driver' to run it
>>>>>>>>> Will fix.
>>>>>>>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you should use %n in format string. also System.out::printf seems to be more popular in our code than System.out::format
>>>>>>>>> OK.
>>>>>>>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found lineMarker?
>>>>>>>>> Oops. Missed it. Will add code to check it.
>>>>>>>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all Common.run calls which aren't expected to fail, e.g. all test* in TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in TestMemoryAwareness
>>>>>>>>> Common.run() already checks for .shouldHaveExitValue(0) after running the container.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I will apply feedback from Bob and you, and will post a new webrev.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thank you,
>>>>>>>>> Misha
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> -- Igor
>>>>>>>>>>
>>>>>>>>>>> On Nov 1, 2017, at 8:11 PM, mikhailo<[hidden email] <mailto:[hidden email]>>  wrote:
>>>>>>>>>>>
>>>>>>>>>>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>>>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762 <https://bugs.openjdk.java.net/browse/JDK-8189762>
>>>>>>>>>>>     Webrev:
>>>>>>>>>>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>>>>>>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00.whitebox/>
>>>>>>>>>>>     Testing:
>>>>>>>>>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>>>>>>>>            Ran the developed tests via jtreg
>>>>>>>>>>>            Pass
>>>>>>>>>>>
>>>>>>>>>>>         2. Automated testing system - run these tests
>>>>>>>>>>>            In progress
>>>>>>>>>>>
>>>>>>>>>>> Thank you,
>>>>>>>>>>> Misha
>>>>>>>>>>>
>>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Bob Vandette
In reply to this post by Mikhailo Seledtsov
Looks good to me.

Bob.


> On Nov 15, 2017, at 4:37 PM, Mikhailo Seledtsov <[hidden email]> wrote:
>
> Hi Igor, Bob,
>
>   Webrev containing latest changes based on feedback from you:
>   Diff from 02:  http://cr.openjdk.java.net/~mseledtsov/8189762.02-03/ <http://cr.openjdk.java.net/~mseledtsov/8189762.02-03/>
>   Full:                http://cr.openjdk.java.net/~mseledtsov/8189762.03/ <http://cr.openjdk.java.net/~mseledtsov/8189762.03/>
>
>
> Thank you,
> Misha
>
>
> On 11/14/17, 4:00 PM, Igor Ignatyev wrote:
>>
>> Hi Misha,
>>
>> a few more comments:
>> - are you using CPUSetsReader::listToString(boolean abc, List<Integer> list, int maxCount) method? I wasn't able to find any usages of it.
>> - CPUSetsReader uses Asserts.assertTrue(parts.length == 2) instead of Asserts.assertEQ(parts.length, 2)
>> - instead of using Stream::collect in CPUSetsReader::readFromProcStatus, you can use Stream::findFirst(). so it'll be smth like
>>> +        try (Stream<String> stream = Files.lines(Paths.get(path))) {
>>> +            o = stream
>>> +                .filter(line -> line.contains(setType))
>>> +                .findFirst();
>>> +        } catch (IOException e) {
>>> +            return null;
>>> +        }
>>> +
>>> +        if (!o.isPresent()) {
>>> +            return null;
>>> +        }
>>> +
>>> +        String[] parts = o.get().replaceAll("\\s","").split(":");
>> - you don't need to box int into Integer in TestCPUSets::checkResult L#108:
>>> Asserts.assertEquals(new Integer(parts.length), new Integer(2));
>> - IIRC, in order to properly use whitebox API, you have to have both sun.hotspot.WhiteBox and sun.hotspot.WhiteBox$WhiteBoxPermission in BCP, but '@run driver ClassFileInstaller -jar whitebox.jar sun.hotspot.WhiteBox', which is used in all your tests, will put only s.h.WhiteBox in whitebox.jar
>> - how do you plan to use :hotspot_containers test group? can't you use the test directory instead?
>>
>> the rest looks good to me.
>>
>> Thanks,
>> -- Igor
>>
>>> On Nov 9, 2017, at 1:04 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>> OK, will remove it.
>>>
>>> Misha
>>>
>>> On 11/9/17, 12:38 PM, Bob Vandette wrote:
>>>>
>>>> Since that path is not guaranteed to work, I’d remove CGROUP_CPUSET_PATH and the function.
>>>>
>>>> Bob.
>>>>
>>>>> On Nov 9, 2017, at 3:31 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>
>>>>> I am not using it. I thought it may be helpful if we go back to that method of extracting cpuset values for some reason.
>>>>> On the other hand, dead code is not a good thing. So I am a bit undecided.
>>>>> I will delete it if you think it is better to do so.
>>>>>
>>>>> Misha
>>>>>
>>>>> On 11/9/17, 12:00 PM, Bob Vandette wrote:
>>>>>>
>>>>>> Do you still need this function?  I don’t see it being used.
>>>>>>
>>>>>>   84     public static String read(String fileName) {
>>>>>>   85         String path = CGROUP_CPUSET_PATH + fileName;
>>>>>>   86         try {
>>>>>>   87             return readLineFromFile(path);
>>>>>>   88         } catch (Exception e) {
>>>>>>   89             System.err.println("Exception reading " + path);
>>>>>>   90             System.err.println("Exception: " + e);
>>>>>>   91         }
>>>>>>   92
>>>>>>   93         return null;
>>>>>>   94     }
>>>>>>
>>>>>> Bob.
>>>>>>
>>>>>>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>>>
>>>>>>> Here is a differential webrev:
>>>>>>>    http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.01-02/webrev/>
>>>>>>>
>>>>>>> Misha
>>>>>>>
>>>>>>> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>>>>>>>> Here is an update webrev:
>>>>>>>>    http://cr.openjdk.java.net/~mseledtsov/8189762.02/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.02/>
>>>>>>>> I also tried to generate a diff between 01 and 02, but could not. My script does not seem to work any more, or I am missing something.
>>>>>>>> Igor, please let me know if you have scripts or command line to generate incremental webrev (I committed my initial changes locally, then applied the new changes on top).
>>>>>>>>
>>>>>>>> Summary for updated webrev.02:
>>>>>>>> - implemented review feedback from Bob and Igor
>>>>>>>> - added @driver to all tests, since the actual testing happens in the child docker process; the main is just a test driver
>>>>>>>> - configured docker directory for exclusive access by jtreg tests, since there are potential and actual issues when running these tests concurrently
>>>>>>>> - modified test groups to add a hotspot_docker test group, and to exclude docker testing from lower tiers
>>>>>>>>    (once the infra is ready, I plan to add this execution initially at higher tiers; first will run it for a while as custom runs)
>>>>>>>>
>>>>>>>> My next steps:
>>>>>>>>  - rebase to new jdk tree
>>>>>>>>  - update/merge
>>>>>>>>  - retest
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Misha
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>>>>>>>> Hi Igor,
>>>>>>>>>
>>>>>>>>>  Thank you for reviewing the code.
>>>>>>>>>
>>>>>>>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>>>>>>>> Hi Misha,
>>>>>>>>>>
>>>>>>>>>> I have several comments:
>>>>>>>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect signature, it should be WB_IsContainerized(JNIEnv*, jobject)
>>>>>>>>> Fixed
>>>>>>>>>> 2. CPUSetsReader.java : listToString can be rewritten using stream api as list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>>>>>>>> Thank you. I will try it out.
>>>>>>>>>> 3. in several files, I have noticed some problems w/ indents which make code quite hard to read, for example
>>>>>>>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>>>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>>>>>>>   75             .shouldMatch("Memory Limit is:.*" + expectedTraceValue);
>>>>>>>>>> or
>>>>>>>>>>>   96         DockerRunOptions opts = Common.newOpts(imageName, "AttemptOOM")
>>>>>>>>>>>   97             .addDockerOpts("--memory", dockerMemLimit, "--memory-swap", dockerMemLimit);
>>>>>>>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>>>>>>>> OK. I will unwind these expressions, and make sure indentation looks good.
>>>>>>>>>> 4. TestCPUSets.java : it's better to use Assert.assertEquals(parts.length, 2) than Asserts.assertTrue(parts.length == 2) as the former will also print the actual value of parts.length. so you won't need to have L#103
>>>>>>>>> OK
>>>>>>>>>> 5. Test*: there is no need to have parentheses in '@requires (docker.support)'
>>>>>>>>> OK
>>>>>>>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK under test, so you can use '@run driver' to run it
>>>>>>>>> Will fix.
>>>>>>>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol you should use %n in format string. also System.out::printf seems to be more popular in our code than System.out::format
>>>>>>>>> OK.
>>>>>>>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we found lineMarker?
>>>>>>>>> Oops. Missed it. Will add code to check it.
>>>>>>>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0) to all Common.run calls which aren't expected to fail, e.g. all test* in TestCPUAwareness, testMemorySoftLimit and testMemoryLimit in TestMemoryAwareness
>>>>>>>>> Common.run() already checks for .shouldHaveExitValue(0) after running the container.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I will apply feedback from Bob and you, and will post a new webrev.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thank you,
>>>>>>>>> Misha
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> -- Igor
>>>>>>>>>>
>>>>>>>>>>> On Nov 1, 2017, at 8:11 PM, mikhailo<[hidden email] <mailto:[hidden email]>>  wrote:
>>>>>>>>>>>
>>>>>>>>>>> Please review these tests that were developed to test JVM's container awareness in Docker environment.
>>>>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762 <https://bugs.openjdk.java.net/browse/JDK-8189762>
>>>>>>>>>>>     Webrev:
>>>>>>>>>>>       Tests: http://cr.openjdk.java.net/~mseledtsov/8189762.00/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>>>>>>       WB API: http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/ <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00.whitebox/>
>>>>>>>>>>>     Testing:
>>>>>>>>>>>         1. Locally: Linux-x64, docker engine version: 17.06.2-ce
>>>>>>>>>>>            Ran the developed tests via jtreg
>>>>>>>>>>>            Pass
>>>>>>>>>>>
>>>>>>>>>>>         2. Automated testing system - run these tests
>>>>>>>>>>>            In progress
>>>>>>>>>>>
>>>>>>>>>>> Thank you,
>>>>>>>>>>> Misha
>>>>>>>>>>>
>>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8189762: [TESTBUG] Create tests for JDK-8146115 container awareness and resource configuration

Mikhailo Seledtsov
Bob, Igor,

   Thank you for review.

Misha

On 11/15/17, 3:12 PM, Bob Vandette wrote:

> Looks good to me.
>
> Bob.
>
>
>> On Nov 15, 2017, at 4:37 PM, Mikhailo Seledtsov
>> <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> Hi Igor, Bob,
>>
>>   Webrev containing latest changes based on feedback from you:
>>   Diff from 02: http://cr.openjdk.java.net/~mseledtsov/8189762.02-03/
>>   Full: http://cr.openjdk.java.net/~mseledtsov/8189762.03/
>>
>>
>> Thank you,
>> Misha
>>
>>
>> On 11/14/17, 4:00 PM, Igor Ignatyev wrote:
>>> Hi Misha,
>>>
>>> a few more comments:
>>> - are you using CPUSetsReader::listToString(boolean abc,
>>> List<Integer> list, int maxCount) method? I wasn't able to find any
>>> usages of it.
>>> - CPUSetsReader uses Asserts.assertTrue(parts.length == 2) instead
>>> of Asserts.assertEQ(parts.length, 2)
>>> - instead of using Stream::collect in
>>> CPUSetsReader::readFromProcStatus, you can use Stream::findFirst().
>>> so it'll be smth like
>>>> +        try (Stream<String>  stream = Files.lines(Paths.get(path))) {
>>>> +            o = stream
>>>> +                .filter(line ->  line.contains(setType))
>>>> +                .findFirst();
>>>> +        } catch (IOException e) {
>>>> +            return null;
>>>> +        }
>>>> +
>>>> +        if (!o.isPresent()) {
>>>> +            return null;
>>>> +        }
>>>> +
>>>> +        String[] parts = o.get().replaceAll("\\s","").split(":");
>>> - you don't need to box int into Integer in TestCPUSets::checkResult
>>> L#108:
>>>> Asserts.assertEquals(new Integer(parts.length), new Integer(2));
>>> - IIRC, in order to properly use whitebox API, you have to have
>>> both sun.hotspot.WhiteBox and
>>> sun.hotspot.WhiteBox$WhiteBoxPermission in BCP, but '@run driver
>>> ClassFileInstaller -jar whitebox.jar sun.hotspot.WhiteBox', which is
>>> used in all your tests, will put only s.h.WhiteBox in whitebox.jar
>>> - how do you plan to use :hotspot_containers test group? can't you
>>> use the test directory instead?
>>>
>>> the rest looks good to me.
>>>
>>> Thanks,
>>> -- Igor
>>>
>>>> On Nov 9, 2017, at 1:04 PM, Mikhailo Seledtsov
>>>> <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>> OK, will remove it.
>>>>
>>>> Misha
>>>>
>>>> On 11/9/17, 12:38 PM, Bob Vandette wrote:
>>>>> Since that path is not guaranteed to work, I’d remove
>>>>> CGROUP_CPUSET_PATH and the function.
>>>>>
>>>>> Bob.
>>>>>
>>>>>> On Nov 9, 2017, at 3:31 PM, Mikhailo Seledtsov
>>>>>> <[hidden email]
>>>>>> <mailto:[hidden email]>> wrote:
>>>>>>
>>>>>> I am not using it. I thought it may be helpful if we go back to
>>>>>> that method of extracting cpuset values for some reason.
>>>>>> On the other hand, dead code is not a good thing. So I am a bit
>>>>>> undecided.
>>>>>> I will delete it if you think it is better to do so.
>>>>>>
>>>>>> Misha
>>>>>>
>>>>>> On 11/9/17, 12:00 PM, Bob Vandette wrote:
>>>>>>> Do you still need this function?  I don’t see it being used.
>>>>>>>
>>>>>>>    84     public static String read(String fileName) {
>>>>>>>    85         String path = CGROUP_CPUSET_PATH + fileName;
>>>>>>>    86         try {
>>>>>>>    87             return readLineFromFile(path);
>>>>>>>    88         } catch (Exception e) {
>>>>>>>    89             System.err.println("Exception reading " + path);
>>>>>>>    90             System.err.println("Exception: " + e);
>>>>>>>    91         }
>>>>>>>    92
>>>>>>>    93         return null;
>>>>>>>    94     }
>>>>>>>
>>>>>>> Bob.
>>>>>>>
>>>>>>>> On Nov 9, 2017, at 2:15 PM, Mikhailo Seledtsov
>>>>>>>> <[hidden email]
>>>>>>>> <mailto:[hidden email]>> wrote:
>>>>>>>>
>>>>>>>> Here is a differential webrev:
>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.01-02/webrev/ 
>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.01-02/webrev/>
>>>>>>>>
>>>>>>>> Misha
>>>>>>>>
>>>>>>>> On 11/8/17, 5:26 PM, Mikhailo Seledtsov wrote:
>>>>>>>>> Here is an update webrev:
>>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.02/ 
>>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.02/>
>>>>>>>>> I also tried to generate a diff between 01 and 02, but could
>>>>>>>>> not. My script does not seem to work any more, or I am missing
>>>>>>>>> something.
>>>>>>>>> Igor, please let me know if you have scripts or command line
>>>>>>>>> to generate incremental webrev (I committed my initial changes
>>>>>>>>> locally, then applied the new changes on top).
>>>>>>>>>
>>>>>>>>> Summary for updated webrev.02:
>>>>>>>>> - implemented review feedback from Bob and Igor
>>>>>>>>> - added @driver to all tests, since the actual testing happens
>>>>>>>>> in the child docker process; the main is just a test driver
>>>>>>>>> - configured docker directory for exclusive access by jtreg
>>>>>>>>> tests, since there are potential and actual issues when
>>>>>>>>> running these tests concurrently
>>>>>>>>> - modified test groups to add a hotspot_docker test group, and
>>>>>>>>> to exclude docker testing from lower tiers
>>>>>>>>>    (once the infra is ready, I plan to add this execution
>>>>>>>>> initially at higher tiers; first will run it for a while as
>>>>>>>>> custom runs)
>>>>>>>>>
>>>>>>>>> My next steps:
>>>>>>>>>  - rebase to new jdk tree
>>>>>>>>>  - update/merge
>>>>>>>>>  - retest
>>>>>>>>>
>>>>>>>>> Thank you,
>>>>>>>>> Misha
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 11/6/17, 9:20 PM, Mikhailo Seledtsov wrote:
>>>>>>>>>> Hi Igor,
>>>>>>>>>>
>>>>>>>>>>  Thank you for reviewing the code.
>>>>>>>>>>
>>>>>>>>>> On 11/6/17, 4:31 PM, Igor Ignatyev wrote:
>>>>>>>>>>> Hi Misha,
>>>>>>>>>>>
>>>>>>>>>>> I have several comments:
>>>>>>>>>>> 1. whitebox.cpp : WB_IsContainerized has an incorrect
>>>>>>>>>>> signature, it should be WB_IsContainerized(JNIEnv*, jobject)
>>>>>>>>>> Fixed
>>>>>>>>>>> 2. CPUSetsReader.java : listToString can be rewritten using
>>>>>>>>>>> stream api as
>>>>>>>>>>> list.stream().limit(maxCount).map(Object::toString).collect(Collectors.joining(","))
>>>>>>>>>> Thank you. I will try it out.
>>>>>>>>>>> 3. in several files, I have noticed some problems w/ indents
>>>>>>>>>>> which make code quite hard to read, for example
>>>>>>>>>>>>   73         Common.run( Common.newOpts(imageName)
>>>>>>>>>>>>   74              .addDockerOpts("--memory", valueToSet))
>>>>>>>>>>>>   75             .shouldMatch("Memory Limit is:.*" +
>>>>>>>>>>>> expectedTraceValue);
>>>>>>>>>>> or
>>>>>>>>>>>>   96         DockerRunOptions opts =
>>>>>>>>>>>> Common.newOpts(imageName, "AttemptOOM")
>>>>>>>>>>>>   97             .addDockerOpts("--memory", dockerMemLimit,
>>>>>>>>>>>> "--memory-swap", dockerMemLimit);
>>>>>>>>>>>>   98             opts.classParams.add("" + sizeToAllocInMb);
>>>>>>>>>> OK. I will unwind these expressions, and make sure
>>>>>>>>>> indentation looks good.
>>>>>>>>>>> 4. TestCPUSets.java : it's better to use
>>>>>>>>>>> Assert.assertEquals(parts.length, 2) than
>>>>>>>>>>> Asserts.assertTrue(parts.length == 2) as the former will
>>>>>>>>>>> also print the actual value of parts.length. so you won't
>>>>>>>>>>> need to have L#103
>>>>>>>>>> OK
>>>>>>>>>>> 5. Test*: there is no need to have parentheses in '@requires
>>>>>>>>>>> (docker.support)'
>>>>>>>>>> OK
>>>>>>>>>>> 6. Test*: ClassFileInstaller doesn't have to be run by JDK
>>>>>>>>>>> under test, so you can use '@run driver' to run it
>>>>>>>>>> Will fix.
>>>>>>>>>>> 7. TestCPUSets.java : L#72, to get a proper new line symbol
>>>>>>>>>>> you should use %n in format string. also System.out::printf
>>>>>>>>>>> seems to be more popular in our code than System.out::format
>>>>>>>>>> OK.
>>>>>>>>>>> 8. TestCPUSets.java: shouldn't checkResult assert that we
>>>>>>>>>>> found lineMarker?
>>>>>>>>>> Oops. Missed it. Will add code to check it.
>>>>>>>>>>> 9. Test*: would you consider adding .shouldHaveExitValue(0)
>>>>>>>>>>> to all Common.run calls which aren't expected to fail, e.g.
>>>>>>>>>>> all test* in TestCPUAwareness, testMemorySoftLimit and
>>>>>>>>>>> testMemoryLimit in TestMemoryAwareness
>>>>>>>>>> Common.run() already checks for .shouldHaveExitValue(0) after
>>>>>>>>>> running the container.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I will apply feedback from Bob and you, and will post a new
>>>>>>>>>> webrev.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Misha
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> -- Igor
>>>>>>>>>>>
>>>>>>>>>>>> On Nov 1, 2017, at 8:11 PM,
>>>>>>>>>>>> mikhailo<[hidden email]
>>>>>>>>>>>> <mailto:[hidden email]>>  wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Please review these tests that were developed to test JVM's
>>>>>>>>>>>> container awareness in Docker environment.
>>>>>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8189762
>>>>>>>>>>>>     Webrev:
>>>>>>>>>>>>       Tests:
>>>>>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00/ 
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00/>
>>>>>>>>>>>>       WB API:
>>>>>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8189762.00.whitebox/ 
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Emseledtsov/8189762.00.whitebox/>
>>>>>>>>>>>>     Testing:
>>>>>>>>>>>>         1. Locally: Linux-x64, docker engine version:
>>>>>>>>>>>> 17.06.2-ce
>>>>>>>>>>>>            Ran the developed tests via jtreg
>>>>>>>>>>>>            Pass
>>>>>>>>>>>>
>>>>>>>>>>>>         2. Automated testing system - run these tests
>>>>>>>>>>>>            In progress
>>>>>>>>>>>>
>>>>>>>>>>>> Thank you,
>>>>>>>>>>>> Misha
>>>>>>>>>>>>
>>>>>>>
>>>>>
>>>
>