RFR(XS): 8189213: [TESTBUG] Running jtreg tests on machine without docker shows extra message

David Holmes david.holmes at oracle.com
Tue Nov 7 01:34:19 UTC 2017


Hi Misha,

This seems reasonable.

Thanks,
David

On 7/11/2017 8:42 AM, mikhailo wrote:
> Hi David,
> 
> 
> On 11/05/2017 07:28 PM, David Holmes wrote:
>> Hi Misha,
>>
>> On 4/11/2017 10:21 AM, Mikhailo Seledtsov wrote:
>>> Hi David,
>>>
>>> Updated webrev: http://cr.openjdk.java.net/~mseledtsov/8189213.01/
>>>
>>> Please see my comments below.
>>>
>>> On 11/2/17, 6:02 PM, David Holmes wrote:
>>>> Hi Misha,
>>>>
>>>> On 3/11/2017 10:22 AM, mikhailo wrote:
>>>>> Hi David,
>>>>>
>>>>>    Thank you for taking a look at this change.
>>>>>
>>>>>
>>>>> On 11/02/2017 04:47 PM, David Holmes wrote:
>>>>>> Hi Misha,
>>>>>>
>>>>>> I don't see anything else in VMProps that writes anything out so 
>>>>>> to me the fix is to delete the System.err.println completely.
>>>>> Removal is an option, and seems a simpler one. However, we will 
>>>>> loose the diagnostic information.
>>>>> On the other hand, if an issue arises on a specific host or system, 
>>>>> I can patch this file with extra trace statements, and diagnose the 
>>>>> failure that way.
>>>>> If you think removing the print statement is a better option, and I 
>>>>> do not hear objections or other opinions, I can just remove the 
>>>>> print statement.
>>>>
>>>> Delete please :)
>>>>
>>> Done.
>>>>>> That said I did not look at this in the past but I feel the whole 
>>>>>> docker test here is not really appropriate/suitable to always be 
>>>>>> run on linux-x64. It has to exec another process and wait up to 10 
>>>>>> seconds to get a result! Can't this actually be done via a 
>>>>>> WhiteBox check once Bob's container support updates are in place?
>>>>> Actually, WhiteBox is not necessary for such check. The ability to 
>>>>> run docker on a given host/node can be determined in a body of a 
>>>>> jtreg test, via a test utility. In fact, that was my original 
>>>>> intent. When this change was presented for a discussion, this 
>>>>> aspect was debated and reviewers have reached a consensus to do 
>>>>> such check in VMProps.java. We mainly discussed within VM SQE team. 
>>>>> The main argument was that using @requires is a more proper way of 
>>>>> "skipping" the test(s) on unsupported environments, rather then 
>>>>> checking this in the body of the test and then returning. When 
>>>>> using @requires, the test execution system will report the test as 
>>>>> "not run". When skipping the test using "return" from the body of a 
>>>>> test, the execution system will report test as "ran" and passed; 
>>>>> jtreg does not yet support the "skipped" state.
>>>>>
>>>>> Unfortunately, the way our "@requires" mechanism works is that it 
>>>>> runs each time prior to starting a test run, even if the test being 
>>>>> run is not concerned with a specific property. It was my concern 
>>>>> that this could be a burden on the test run startup, but I 
>>>>> eventually agreed to the opinion of other reviewers.
>>>>>
>>>>> If you have a strong opinion or concern about this, let me know. 
>>>>> However, I am not sure what is the best process to use to overturn 
>>>>> a decision on a prior review or design discussion. I guess one 
>>>>> option is to file a bug or an RFE, and bring it up for a discussion.
>>>>
>>>> Yes let's file a bug/RFE. VMProps can use WhiteBox and I think using 
>>>> WhiteBox connected to Bob's new container support code is better 
>>>> than exec'ing a separate process (which I'm not even sure what it 
>>>> does).
>>> I have filed a bug: "JDK-8190730 - [TESTBUG] Calling 'docker ps' from 
>>> VMProps.java to determine presence of docker engine is deemed too heavy"
>>>
>>> Also, in current webrev, I though I could do something quick and easy 
>>> to address your concerns, before we discuss and agree on a final 
>>> solution.
>>> I have timed running "docker ps" on 2 different hosts, 30 times each. 
>>> I saw a spread between 20ms to 150ms, mostly centered around 30ms. 
>>> Hence, I reduced the timeout in the VMProps.java from 10 sec to 
>>> 500ms. I thought 500 ms gives enough time cushion for slower test 
>>> nodes/hosts.
>>
>> I think a slowness would arise on systems that do not have docker 
>> installed. If I time "docker ps" on my system I get
>>
>>  > time docker ps
>> The program 'docker' is currently not installed.  To run 'docker' 
>> please ask your administrator to install the package 'docker'
>>
>> real    0m4.103s
>> user    0m0.044s
>> sys     0m0.036s
>>
>> but the second call is much quicker:
>>
>>  > time docker ps
>> The program 'docker' is currently not installed.  To run 'docker' 
>> please ask your administrator to install the package 'docker'
>>
>> real    0m0.078s
>> user    0m0.052s
>> sys     0m0.012s
>>
>> It may well depend on the user's path. If "docker" is present early in 
>> the path then it will be found and executed quickly, but if not 
>> present, or if further down the path it may take a while to occur.
>>
>> So I think the timeout change, as a short term proposal, may be more 
>> risky and so not worthwhile.
>>
>> I'd prefer to see the whole "docker ps" disappear altogether. Such an 
>> approach is not scalable if there may be different container systems.
> I have discussed this topic again with Igor and Leonid, and explained 
> your concerns.
> 
> 
> We came up with alternative ideas:
> 
> 1. VMProps.java will check the existence of docker on a given test 
> machine. We will look at "/bin/docker", "/usr/bin/docker", 
> "/usr/local/bin/docker"
>      For now we only support Linux x64. If any of these files exist, we 
> return 'true' for a corresponding at-requires property, otherwise false.
> 
> 2. In case someone wishes to specify a custom location for a docker 
> binary, we thought we could use a property for that:
>       jdk.test.lib.docker.path=""
>      If this property is specified, VMProps.java will check the docker 
> at that location.
> 
> 3. No more "docker ps" execution in VMProps.java.
> 
> Please let me know what you think.
> 
> 
> 
> Thank you,
> Misha
> 
>>
>> Thanks,
>> David
>>
>>> Let me know what you think. I can revert this part of the change or 
>>> reduce the timeout value a bit more.
>>>
>>> Thank you,
>>> Misha
>>>>
>>>>>
>>>>> For this change under review, if I do not hear opinions from other 
>>>>> reviewers, I can simply delete the print statement.
>>>>
>>>> Sounds good to me.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>
>>>>> Thank you,
>>>>> Misha
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 3/11/2017 7:03 AM, mikhailo wrote:
>>>>>>> Please review this simple change that makes the error message in 
>>>>>>> jtreg-ext/requires/VMProps.java conditional
>>>>>>> on java property "vmprops.trace.verbose". W/o this condition an 
>>>>>>> error message is printed each time any jtreg
>>>>>>> test is ran, including non-docker tests.
>>>>>>>
>>>>>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8189213
>>>>>>>      Webrev: http://cr.openjdk.java.net/~mseledtsov/8189213.00/
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ======= Background and details:
>>>>>>> VMProps.java is called for each test run of jtreg (be it a run 
>>>>>>> containing a singe test or multiple tests).
>>>>>>> VMProps evaluates the @requires properties. Therefore, in case of 
>>>>>>> this bug, any time a user runs any jtreg VM test
>>>>>>> on a machine w/o an operational docker engine, this error will 
>>>>>>> pop up.
>>>>>>> The fix makes printing of this error message conditional, off by 
>>>>>>> default. The message can be turend on
>>>>>>> via a property vmprops.trace.verbose when detailed diagnostic 
>>>>>>> info is desired.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ======== Testing:
>>>>>>> Local testing:
>>>>>>>    W/o docker present:
>>>>>>>      jtreg <arbitrary-jtreg-test>
>>>>>>>        No error message
>>>>>>>      jtreg -Dvmprops.trace.verbose=true <arbitrary-jtreg-test>
>>>>>>>        Detailed error message
>>>>>>>      jtreg DockerBasicTest.java
>>>>>>>        Test skipped
>>>>>>>
>>>>>>>    With docker installed but no permissions:
>>>>>>>      jtreg <arbitrary-jtreg-test>
>>>>>>>        No error message
>>>>>>>      jtreg -Dvmprops.trace.verbose=true <arbitrary-jtreg-test>
>>>>>>>        Detailed error message
>>>>>>>      jtreg DockerBasicTest.java
>>>>>>>        Test skipped
>>>>>>>
>>>>>>>    With docker installed and operational:
>>>>>>>      jtreg <arbitrary-jtreg-test>
>>>>>>>        No error message
>>>>>>>      jtreg -Dvmprops.trace.verbose=true <arbitrary-jtreg-test>
>>>>>>>        No error message
>>>>>>>      jtreg DockerBasicTest.java
>>>>>>>        Test passed
>>>>>>>
>>>>>>> Testing via automated distributed test system:
>>>>>>>    - tier1
>>>>>>>    - docker tests
>>>>>>>    In progress
>>>>>>>
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Misha
>>>>>>>
>>>>>
> 


More information about the hotspot-runtime-dev mailing list