RFR: 8227642: [TESTBUG] Make docker tests podman compatible

[Igor Ignatyev]

Hi Severin,

the updated webrev looks good to me, please see a couple comments below.

Cheers,
-- Igor

> On Jul 17, 2019, at 10:34 AM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> 
> Hi Misha,
> 
> On Wed, 2019-07-17 at 10:22 -0700, mikhailo.seledtsov at oracle.com <mailto:mikhailo.seledtsov at oracle.com> wrote:
>> Hi Severin,
>> 
>> On 7/17/19 5:44 AM, Severin Gehwolf wrote:
>>> Hi Igor, Misha,
>>> 
>>> On Tue, 2019-07-16 at 11:49 -0700, Igor Ignatyev wrote:
>>>> Hi Severin,
>>>> 
>>>> I don't think that tests (or test libraries for that matter) should
>>>> be responsible for setting correct PATH value, it should be a part of
>>>> host configuration procedure (tests can/should check that all
>>>> required bins are available though). in other words, I'd prefer if
>>>> you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and
>>>> TestJFREvents. the rest looks good to me.
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/02/webrev/
>>> 
>>> No more additions to PATH are being done.
>>> 
>>> I've discovered that VMProps.java which defines "docker.required", used
>>> the "docker" binary even for podman test runs. This ended up not
>>> running most of the tests even with -Djdk.test.docker.command=podman
>>> specified.
>> Good catch.
should we rename docker.support and DOCKER_COMMAND to something more abstract?

>>> I've fixed that by moving DOCKER_COMMAND to Platform.java so
>>> that it can be used in both places.
>> 
>> Sounds good to me.
>> 
>> (of course, the alternative would be to import 
>> jdk.test.lib.containers.docker.DockerTestUtils into VMProps.java -- not 
>> sure if there are any potential problems doing it this way)
> 
> I've tried that but for some reason this was a problem and VMProps
> failed to compile. I don't know exactly how those jtreg extensions work
> and went with the Platform approach. Hope that's OK.

all files needed for VMProps (or other @requires expression class) have to be listed in requires.extraPropDefns or requires.extraPropDefns.bootlibs property in TEST.ROOT file in all the test suites which use these extensions. we are trying to be very cautious in what is used by VMProps (directly and indirectly) so these lists won't grow and we won't require any modules other than java.base, given DockerTestUtils has dependencies on a number of other library classes, the Platform approach is much better from that point of view.

> 
>>> Testing: Container tests with docker daemon running on Linux x86_64,
>>> container tests without docker daemon running (podman is daemon-less)
>>> via the podman binary on Linux x86_64 (with -e:PATH). All pass.
>> 
>> Sounds good.
>> 
>> 
>> Overall looks good to me.
> 
> Thanks for the review!
> 
>> One minor nit: DockerTestUtils.java does not need "import 
>> java.util.Map;" (no need to post updated webrev for this change)
> 
> OK, good catch. Fixed locally.
> 
> Thanks,
> Severin
> 
>> 
>> Thank you,
>> 
>> Misha
>> 
>>> More thoughts?
>>> 
>>> Thanks,
>>> Severin
>>> 
>>>> Thanks,
>>>> -- Igor
>>>> 
>>>>> On Jul 16, 2019, at 5:36 AM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> I believe I still need a *R*eviewer for this. Any takers?
>>>>> 
>>>>> Thanks,
>>>>> Severin
>>>>> 
>>>>> On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledtsov at oracle.com wrote:
>>>>>> Hi Severin,
>>>>>> 
>>>>>>   The change looks good to me. Thank you for adding support for Podman
>>>>>> container technology.
>>>>>> 
>>>>>> Testing: I ran both HotSpot and JDK container tests with your patch;
>>>>>> tests executed on Oracle Linux 7.6 using default container engine (Docker):
>>>>>> 
>>>>>>     test/hotspot/jtreg/containers/   AND
>>>>>> test/jdk/jdk/internal/platform/docker/
>>>>>> 
>>>>>> All PASS
>>>>>> 
>>>>>> 
>>>>>> Thanks,
>>>>>> 
>>>>>> Misha
>>>>>> 
>>>>>> 
>>>>>> On 7/12/19 11:08 AM, Severin Gehwolf wrote:
>>>>>>> Hi,
>>>>>>> 
>>>>>>> There is an alternative container engine which is being used by Fedora
>>>>>>> and RHEL 8, called podman[1]. It's mostly compatible with docker. It
>>>>>>> looks like OpenJDK docker tests can be made podman compatible with a
>>>>>>> few little tweaks. One "interesting" one is to not assert "Successfully
>>>>>>> built" in the build output but only rely on the exit code, which seems
>>>>>>> to be OK for my testing. Interestingly the test would be skipped in
>>>>>>> that case.
>>>>>>> 
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
>>>>>>> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/
>>>>>>> 
>>>>>>> Adjustments I've done:
>>>>>>>  * Don't assert "Successfully built" in image build output[2].
>>>>>>>  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
>>>>>>>    to work which is in /usr/sbin on Fedora
>>>>>>>  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
>>>>>>>    to be equal to the previous value. I've found those counters to be
>>>>>>>    slowly increasing, which made the tests unreliable.
>>>>>>> 
>>>>>>> Testing:
>>>>>>> 
>>>>>>> Running docker tests with docker as engine. Did the same with podman as
>>>>>>> engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
>>>>>>> passed (non-trivially).
>>>>>>> 
>>>>>>> Thoughts?
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Severin
>>>>>>> 
>>>>>>> [1] https://podman.io/
>>>>>>> [2] Image builds with podman look
>>>>>>>     like ("COMMIT" over "Successfully built"):
>>>>>>> STEP 1: FROM fedora:29
>>>>>>> STEP 2: RUN dnf install -y java-11-openjdk-devel &&     dnf clean all
>>>>>>> --> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
>>>>>>> STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
>>>>>>> 269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
>>>>>>> STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
>>>>>>> STEP 5: COMMIT fedora-metrics-11
>>>>>>> d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e