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

[David Holmes]

On 14/09/2017 2:15 PM, 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().

That's fine it isn't the cwd code path at issue.

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

  294         } catch (IOException x) {
  295             f = new File(tmpdir, fn);
  296             f.createNewFile();
  297         }

This is the backup in case cwd fails for some reason. I have no idea why 
it might fail but either:

a) it can fail in which case I still think the tmpfile case needs to be 
updated; or
b) it can't fail, in which case the code above should be removed along 
with any other code that checks in tmpdir.

Thanks,
David

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