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

[Mikhailo Seledtsov]

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.

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