On 7/17/19 11:37 AM, Igor Ignatyev wrote:
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 <sgehw...@redhat.com
<mailto:sgehw...@redhat.com>> wrote:
Hi Misha,
On Wed, 2019-07-17 at 10:22 -0700,mikhailo.seledt...@oracle.com
<mailto:mikhailo.seledt...@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?
Now that more container technologies are coming online we could consider
more generic names for these properties/variables.
Here are some thoughts:
- container.support (CONTAINER_COMMAND) - may be too generic
- linux.container.support (LINUX_CONTAINER_COMMAND) - more narrow
- even more narrow/specific: oci.container.support
(OCI_CONTAINER_COMMAND)
OCI in this case is " Open Container Initiative", ( Linux
Foundation project to design open standards for Linux Container technology)
I believe both Docker and Podman are OCI compliant.
However, I would recommend to do this work as part of a new RFE. If we
agree, I will create an RFE, and we can continue discussion in the
context of that RFE.
Thank you,
Misha
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 <sgehw...@redhat.com
<mailto: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
<mailto: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
