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

[David Holmes]

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.

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