RFR(10)(M): 8179498: attach in linux should be relative to /proc/pid/root and namespace aware

Thu Sep 14 16:56:42 UTC 2017

On 9/14/17 1:45 AM, David Holmes wrote:
> On 14/09/2017 5:53 PM, Dmitry Samersoff wrote:
>> Chris,
>>
>>> So now .attach_pid<pid> is always created in cwd as you can see in
>>> createAttachFile(), although AttachListener::is_init_trigger() does
>>> check tmp, but only after cwd.
>>
>>> ***getNamespacePid - ns_pid: 125
>>> ***findSocketFile - f: /proc/24445/root/tmp/.java_pid125
>>> ***createAttachFile - path: /proc/24445/cwd/.attach_pid125
>>
>> Could we always use tmp ?
>>
>> IMHO cwd is not a right choice for such kind of files, it should be
>> either $HOME or tmp.
>
> And we hardwired /tmp and stopped using cwd under
>
> https://bugs.openjdk.java.net/browse/JDK-7132199
>
> So I'm a bit confused as to how this has evolved back into using cwd. ??
Yeah, I had this backwards with my earlier comment. Before JDK-7132199 
we actually do it the way we do now, trying cwd first, and then tmp if 
it fails. JDK-7132199 made it only use tmp, but only for 
findSocketFile(). createAttachFile() still tries cwd first and then tmp, 
and I see nothing in the history to indicate this was ever changed, 
other than to force the location of tmp to /tmp with JDK-6950927.

So the question is do we get rid of the cwd support and always use tmp? 
If yes, I think it's best not to do that as part of this CR. I'd rather 
just add the docker /tmp support to createAttachFile() now, and have a 
separate CR deal with removing all cwd support (or maybe even push 
changes for it before the docker support fix).

thanks,

Chris
>
> David
>
>
>> -Dmitry
>>
>> On 14.09.2017 07:15, Chris Plummer wrote:
>>> On 9/13/17 9:00 PM, David Holmes wrote:
>>>> Hi Chris,
>>>>
>>>> On 14/09/2017 1:03 PM, Chris Plummer wrote:
>>>>> Hi David,
>>>>>
>>>>> On 9/13/17 5:12 PM, David Holmes wrote:
>>>>>> Hi Chris,
>>>>>>
>>>>>> On 14/09/2017 8:23 AM, Chris Plummer wrote:
>>>>>>> I could use one more reviewer.
>>>>>>
>>>>>> Generally this seems okay to me.
>>>>>>
>>>>>> One query though ... in createAttachFile don't you need to alter the
>>>>>> tmpdir using part in a similar manner to how findSocketFile was
>>>>>> modified?
>>>>> The fix in findSocketFile is not just to make sure the client uses
>>>>> the correct pid in the .java_pid file files, but also (as you point
>>>>> out) to make sure that the client properly references the target
>>>>> jvm's tmp directory when accessing the .java_pid file. findSocketFile
>>>>> is a little
>>>>
>>>> I presume you mean createAttachFile there.
>>> Yes.
>>>>
>>>>> different. You still have to map to the proper from pid to ns_pid
>>>>> when referencing the .attach_pid file, but you don't have the /tmp
>>>>> mount point differences to deal with. /proc/<pid>/cwd should work
>>>>> even if the pid is for a docker. You don't even have to map to the
>>>>> pid as the docker sees it. /proc/<pid>/cwd from the client's POV
>>>>> should be the same as /proc/<ns_pid>/cwd from the target JVM's POV.
>>>>
>>>> Sorry but I don't follow. If findSocketFile has to look in
>>>> /proc/<pid>/root/<tmpdir> for the socket file, why does the
>>>> createAttachFile not also have to write the attach file into
>>>> /proc/<pid>/root/<tmpdir> ?? In both cases it needs to find the tmpdir
>>>> of the target process.
>>> Fortunately I have some old printlns that might help:
>>>
>>> ***getNamespacePid - ns_pid: 125
>>> ***findSocketFile - f: /proc/24445/root/tmp/.java_pid125
>>> ***createAttachFile - path: /proc/24445/cwd/.attach_pid125
>>>
>>> So this is a case where the real pid is 24445, but the namespace pid in
>>> the docker is 125. The docker can (and does) reference
>>> /tmp/.java_pid125, but the client needs to reference
>>> /proc/24445/root/tmp/.java_pid125 to get to the same file. For
>>> .attach_pid125, the client can get to it through
>>> /proc/24445/cwd/.attach_pid125, and the docker process will look in cwd
>>> for the file. This is done in AttachListener::is_init_trigger().
>>>
>>> BTW, comments like the following are no longer correct due to 
>>> JDK-7132199:
>>>
>>>      // "/tmp" is used as a global well-known location for the files
>>>      // .java_pid<pid>. and .attach_pid<pid>. It is important that this
>>>      // location is the same for all processes, otherwise the tools
>>>      // will not be able to find all Hotspot processes.
>>>      // Any changes to this needs to be synchronized with HotSpot.
>>>      private static final String tmpdir = "/tmp";
>>>
>>> So now .attach_pid<pid> is always created in cwd as you can see in
>>> createAttachFile(), although AttachListener::is_init_trigger() does
>>> check tmp, but only after cwd.
>>>
>>> thanks,
>>>
>>> Chris
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>>
>>>>>> Minor note - you can collapse your catch blocks into 1 using
>>>>>> something like
>>>>>>
>>>>>> ������� } catch (NumberFormatException | IOException x) {
>>>>>> ������������ throw new AttachNotSupportedException("Unable to parse
>>>>>> namespace");
>>>>>> ������� }
>>>>> I'll make that change.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 9/11/17 8:03 PM, Chris Plummer wrote:
>>>>>>>> Ok, I will. Thanks.
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>> On 9/11/17 6:13 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>> Hi Chris,
>>>>>>>>>
>>>>>>>>> This looks good to me.
>>>>>>>>>
>>>>>>>>> I'm not sure if all the nsk.aod and the AttachOnDemand tests from
>>>>>>>>> the nsk.jvmti are run in the hotspot tier1, 2, and 3 tests.
>>>>>>>>> It makes sense to double-check it.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 9/10/17 20:34, Chris Plummer wrote:
>>>>>>>>>> [re-sending - sent to wrong alias first time]
>>>>>>>>>>
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> Please review the following:
>>>>>>>>>>
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8179498
>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8179498/webrev.00/webrev_jdk/ 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The CR has the relevant details. Some previous discussions can
>>>>>>>>>> be found here:
>>>>>>>>>>
>>>>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-April/021237.html 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-May/021249.html 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-August/021679.html 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Testing with docker has been limited to just making sure jcmd
>>>>>>>>>> now works with the docker setup I was provided. I currently
>>>>>>>>>> don't see how we can run our existing tests in a way that would
>>>>>>>>>> test the docker support without doing some rewriting of the 
>>>>>>>>>> tests.
>>>>>>>>>>
>>>>>>>>>> I also ran all our hotspot tier1, 2, and 3 tests, along with
>>>>>>>>>> jdk/test/tools and jdk/test/sun/tools tests to make sure
>>>>>>>>>> existing functionality is not broken with these changes.
>>>>>>>>>>
>>>>>>>>>> thanks,
>>>>>>>>>>
>>>>>>>>>> Chris
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>>