RFR(S): 8227122: [TESTBUG] Create Docker sidecar test cases

Tue Jul 16 16:48:46 UTC 2019

Hi Severin, Bob,

    Here is an updated webrev that should address all of your feedback:

     http://cr.openjdk.java.net/~mseledtsov/8227122.01/

To summarize the changes since webrev 00:

      - using 'docker ps' to wait until the "main" container starts

      - removed use of --ipc=shareable (not needed)

      - added comments regarding sharing of /tmp

      - using docker volumes ("--volumes-from") to share /tmp between 
the containers instead of mapping to host directories (avoids potential 
access/permission problems)

      - few other minor changes and cleanup

Testing:

      ran this test on OL 7.6 and on variety of Linux nodes in the lab, 
a number of times - all PASS

See more of comments inline below

On 7/12/19 2:12 AM, Severin Gehwolf wrote:
> Hi Misha,
>
> On Thu, 2019-07-11 at 17:58 -0700, mikhailo.seledtsov at oracle.com wrote:
>> Hi Severin,
>>
>>     Thank you for taking a look at this change.
>>
>> On 7/10/19 10:40 AM, Severin Gehwolf wrote:
>>> Hi Misha,
>>>
>>> On Tue, 2019-07-02 at 15:24 -0700, mikhailo.seledtsov at oracle.com wrote:
>>>> Please review this new test that uses a Docker sidecar pattern to
>>>> manage/monitor JVM running in the main payload container.
>>>>
>>>> Sidecar is a common pattern used in the cloud environments for
>>>> monitoring among other uses. In side car pattern the main
>>>> application/service container that runs the payload is paired with a
>>>> sidecar container. It is achieved by sharing certain namespace
>>>> aspects
>>>> between the two containers such as PID namespace, specific
>>>> sub-directories, IPC and more.
>>>>
>>>> This test implements the following cases:
>>>>      - "jcmd -l" to list java processes running in "main" container
>>>> from
>>>> the "sidecar" container
>>>>      - "jhsdb jinfo" in the sidecar configuration
>>>>      - jcmd <some-command>
>>>>
>>>> This change also builds a basis for more test cases in the future.
>>>>
>>>> Minor changes were done to DockerTestUtils:
>>>>      - changing access to DOCKER_COMMAND constant to public
>>>>      - minor spelling and terminology corrections
>>>>
>>>>
>>>>        JBS: https://bugs.openjdk.java.net/browse/JDK-8227122
>>>>        Webrev: http://cr.openjdk.java.net/~mseledtsov/8227122.00/
>>>>        Testing:
>>>>            1. ran Docker tests on Linux-x64 - PASS
>>>>            2. Running Docker tests in test cluster - in progress
>>>>
>>> // JCMD does not work in sidecar configuration, except for "jcmd -l".
>>> // Including this test case to assist in reproduction of the problem.
>>> // t.assertIsAlive();
>>> // testCase03(mainProcPid);
>>>
>>> FWIW, "jcmd -l" doesn't work in this case either. It only sees itself
>>> as far as I can tell.
>> In my experiment it does work. Here are parts of the test log, first the
>> command that runs jcmd in a sidecar container, then the output of that
>> container:
>>
>> """
>>
>> [COMMAND]
>>
>> /usr/local/bin/docker run --tty=true --rm --cap-add=SYS_PTRACE
>> --sig-proxy=true --pid=container:test-container-main
>> --ipc=container:test-container-main -v
>> /ws/playArea/sidecar-jcmd-8227122/JTwork/scratch/.:/tmp/
>> jdk-internal:test-jfr-jcmd /jdk/bin/jcmd -l
>>
>> [2019-07-12T00:26:29.083764Z] Gathering output for process 8703
>> [ELAPSED: 5 ms]
>> [STDERR]
>>
>> [STDOUT]
>> 1 EventGeneratorLoop 15
>> 23 jdk.jcmd/sun.tools.jcmd.JCmd -l
>>
>> """
>>
>> The output shows 2 processes, one is EventGeneratorLoop with PID of 1
>> (as expected). This is possible because the containers share certain
>> namespaces and mounted volumes in a 'sidecar' configuration. In this
>> case, containers share the PID namespace
>> (--pid=container:test-container-main) and share volumes mounted as
>> "/tmp" inside the container (-v
>> /ws/playArea/sidecar-jcmd-8227122/JTwork/scratch/.:/tmp/)
>>
> Right, sorry. Perhaps this code should get a comment that sharing /tmp
> between sidecar and host container is needed for jvmstat - used
> internally by the attach mechanism - to work. See
> HotSpotAttachProvider.testAttachable():
>
> +        String[] command = new String[] {
> +            DockerTestUtils.DOCKER_COMMAND, "run",
> +            "--tty=true", "--rm",
> +            "--cap-add=SYS_PTRACE", "--sig-proxy=true",
> +            "--pid=container:" + MAIN_CONTAINER_NAME,
> +            "--pid=container:" + MAIN_CONTAINER_NAME,
> +            "--ipc=container:" + MAIN_CONTAINER_NAME,
> +            "-v", WORK_DIR + ":/tmp/",
>
> I believe -XX:+UsePerfData would be in order too as I don't think
> things would work if that default changed.
I have added the comments and added -XX:+UsePerfData for the "main" JVM 
process.
>
>>> What's more, this seems to be a case of AttachListener::is_init_trigger[1] and
>>> VirtualMachineImpl.createAttachFile[2] disagreeing. The former looks in
>>> $(pwd)/.attach_pid<pid> or /tmp/.attach_pid<pid> and the latter creates
>>> it in /proc/<pid>/root/tmp/.attach_pid<ns_pid>.
> This seems to be the cause for why testCase03 doesn't work. Perhaps
> this deserves a bug and I can help fix it.
>
> While looking at that, I discovered what I said below, which is a
> different case I know.
Once these tests are integrated I will file a bug, and can reference the 
test from that bug.
>
>>> There seems to be more issues involved. As attaching to a JVM inside a
>>> container doesn't seem to work from outside which is supposed to be
>>> fixed with JDK-8179498. That alone seems to warrant a bug.
>> You are describing a slightly different use case / pattern, but I agree
>> it does not seem to work. I am happy to hear confirmation of that.
> I was pointing out that JDK-8179498 seems to have regressed. It's
> unrelated but should be taken into account when fixing the above issue.
>
>> The pattern addressed in this test is a side car, where both the
>> observer and observee run in containers; the containers are 'friendly'
>> by sharing certain apsects of namespaces.
> Yes.
>
>> The use case you are describing is somewhat different, if I understand
>> correctly: the observer runs on a host machine, and obsrvee runs in a
>> container. Observer tries to use jcmd to list the java processes running
>> in container(s), and issue commands, but that fails. I can create a bug
>> for that, and a simple test case.
> There should be a bug and a test so that it cannot again regress.
>
> JDK-8193710 is also related, but the fix for that bug didn't have a
> test either :( That's this one which needs fixing:
> https://bugs.openjdk.java.net/browse/JDK-8195809

     JDK-8195809: [TESTBUG] Create tests for JDK-8193710 jps and jcmd -l 
support for Docker containers

     I have assigned it to myself, and will be working on it soon.

Thank you,

Misha

>
>>>       private static DockerThread startMainContainer() throws Exception {
>>>           // start "main" container (the observee)
>>>           DockerRunOptions opts = commonDockerOpts("EventGeneratorLoop");
>>>           opts.addDockerOpts("--cap-add=SYS_PTRACE", "--ipc=shareable")
>>>
>>> Is '--ipc=shareable' really needed? It's not a supported option for my
>>> docker here :-(
>> I have removed the '--ipc=shareable' and the test still works. I think
>> this is extra stuff that is not necessary for this test case, so I will
>> remove it.
> Excellent!
>
>> I will incorporate changes from your and Bob's review, run some testing,
>> and post an updated webrev.
> Thanks,
> Severin
>
>>> [1] http://hg.openjdk.java.net/jdk/jdk/file/ba72dac556c3/src/hotspot/os/linux/attachListener_linux.cpp#l500
>>> [2] http://hg.openjdk.java.net/jdk/jdk/file/ba72dac556c3/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java#l295
>>>