[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: [PATCH] attach in linux should be relative to /proc/pid/root and namespace aware
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2017-08-22 18:37:14
Message-ID: b0f0fde0-84d6-1ace-2cf6-731dc0085693 () oracle ! com
[Download RAW message or body]

Ping!

Unless I hear back otherwise, I'll attempt to add the extra exception 
handling, finish up testing (sans running all tests under docker), and 
send out a review.

thanks,

Chris

On 8/17/17 5:15 PM, Chris Plummer wrote:
> Hi TJ,
>
> JDK-8179498 is assigned to me. I've been playing around with your 
> changes to make sure they work fine, and plan on sponsoring the 
> commit. I did run into one issue. It ended up being user error, but 
> highlights a gap in the jcmd docker support. More specifically, "jcmd 
> -l" and "jcmd <MainClass>" don't work. jcmd on the host has no 
> visibility into the list of JVM pid's running on the docker. Normally 
> this list is obtained by looking in /tmp/hsperfdata_<user>/* to find 
> the list of JVM pid's for each user. However, since the docker maps to 
> a different /tmp, jcmd on the host does not see these tmp files for 
> docker JVMs. Thus you first need to find the pid of the JVM running on 
> the docker using ps on the host, and then run jcmd using "jcmd <pid>". 
> Because of this for quite a while I thought your changes were not 
> working, but when I dug more into the attach API and learned how "jcmd 
> -l" discovers the list of JVM pid's, I realized my error, and 
> everything was fine when I specified a PID. This should probably be 
> documented somewhere, at the very lead in the release notes for 10.
>
> Regarding any cleanup of the changes, the only thing I noted was the 
> following comment from Erik and your reply:
>
>> On Wed, May 03, 2017 at 03:14:29PM +0200, Erik Gahlin wrote:
>> >/I noticed thatgetNamespacePid throws IOException and 
>> InvalidPathException. />/Perhaps we want to catch those, so we can 
>> default to the original pid if />/there is a I/O related problem. /
>> That seems reasonable, I'll add that.
> Is there a webrev that has this change? The only one I have access to 
> is the following that David setup before this change was suggested:
>
> http://cr.openjdk.java.net/~dholmes/8179498/webrev/
>
> As for testing, I've verified that at least jcmd works between a 
> docker container and its host, and I will verify that these changes 
> don't break anything when the tool and the target JVM are on the same 
> host. What will be a challenge is verifying that all our tests pass 
> when the target JVM is in a docker container. I assume the tests 
> simply spawn a JVM process to attach to, so default behavior is always 
> same host. It is likely far from trivial to modify them to spawn the 
> process in a docker. Any suggestions?
>
> thanks,
>
> Chris


[prev in list] [next in list] [prev in thread] [next in thread]