Hi Misha, On Wed, 2019-07-17 at 10:22 -0700, 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. > > 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. > > 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> > > > > 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 > > > > > >