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

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.

One minor nit: DockerTestUtils.java does not need "import java.util.Map;" (no need to post updated webrev for this change)


Thank you,

Misha


More thoughts?

Thanks,
Severin

Thanks,
-- Igor

On Jul 16, 2019, at 5:36 AM, Severin Gehwolf <sgehw...@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.seledt...@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

Reply via email to