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

Tue Nov 7 04:06:53 UTC 2017

Thank you David,

I will make changes and post an updated webrev.

Misha

On 11/6/17, 5:34 PM, David Holmes wrote:
> 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
>>>>>>>>
>>>>>>
>>