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

[David Holmes]

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. ??

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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
> 
>