RFR(XS): 8189213: [TESTBUG] Running jtreg tests on machine without docker shows extra message
Bob Vandette
bob.vandette at oracle.com
Tue Nov 7 19:47:52 UTC 2017
> On Nov 7, 2017, at 2:25 PM, mikhailo <mikhailo.seledtsov at oracle.com> wrote:
>
>
>
> On 11/07/2017 09:04 AM, Bob Vandette wrote:
>>> On Nov 6, 2017, at 5:42 PM, mikhailo <mikhailo.seledtsov at oracle.com> 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.
>> Just having checking for the existence of /bin/docker doesn’t means that it’s properly configured for use by any test process.
>> There’s permission setup, possible proxy setup etc. We had issues with our internal testing systems not having the tools we’ve
>> needed and it’s a pain to have to go to each system in order to clear the way to successfully run all the tests.
> Yes, I agree with Bob, it is important to check that the docker is runnable by the test user or test agent.
> Given this, I propose to check 'docker ps' with a small timeout, say 300ms. However, instead of doing 'docker ps', I will do the '<full path>/docker ps' with the docker binary that I have found on a given system.
> This way we run 'docker ps' ONLY if we found it on the system. And, we give a process runner an exact location of the binary which will eliminate a search on the PATH, which in turn allows us to make wait timeout smaller, hence reducing impact/cost on VMProps evaluation.
>
> Does it work for you Bob? David, are you OK with this approach?
I think this is an improvement and I’m ok with this approach but I don’t see that it will save that much time.
Starting docker takes a while (worse the first time). I’ve seen 1sec startup times. Let see what David thinks
of the /tmp caching idea. You’d have to put the user that found and successfully ran docker and have to deal
with multi-process write access to the file.
Bob.
>
>>
>> This is probably breaking new ground, but can we cache the result of “docker ps” in /tmp in order to eliminate the performance
>> overhead for all but the first time a new user runs these tests?
> I try to keep this solution simple. If you and David approve the solution above, I will implement and test it. If we find any problems, either during my testing or during test production, we could look into caching mechanisms.
>
>
> Thank you,
> Misha
>>
>> Bob.
>
>>>
>>>
>>> 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