RFR(XS): 8189213: [TESTBUG] Running jtreg tests on machine without docker shows extra message
mikhailo
mikhailo.seledtsov at oracle.com
Mon Nov 6 22:42:02 UTC 2017
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