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

[mikhailo]

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
>>>>>>
>>>>