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

David Holmes david.holmes at oracle.com
Mon Sep 18 00:17:08 UTC 2017 

Hi Chris,

On 15/09/2017 2:19 PM, Chris Plummer wrote:
> On 9/14/17 9:56 AM, Chris Plummer wrote:
>> 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).
> Here's an updated webrev with the tmpdir fix in createAttachFile():
> 
> http://cr.openjdk.java.net/~cjplummer/8179498/webrev.02/webrev_jdk/
> 
> I ran all the same tests again, including testing with docker. To make 
> sure it was hitting the tmpdir code, I forced the cwd code to error out 
> by making it use cwdX instead.
> 
> There are also two other changes to fix an issue I noticed when you 
> provide a bad pid. You are suppose to get an error message like this:
> 
> java.io.IOException: No such process
>      at jdk.attach/sun.tools.attach.VirtualMachineImpl.sendQuitTo(Native 
> Method)
>      at 
> jdk.attach/sun.tools.attach.VirtualMachineImpl.<init>(VirtualMachineImpl.java:80) 
> 
>      at 
> jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) 
> 
>      at 
> jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) 
> 
>      at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:114)
>      at jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:98)
> 
> However, I was seeing the following due to the docker related changes:
> 
> java.nio.file.NoSuchFileException: /proc/7777/status
>      at 
> java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92) 
> 
>      at 
> java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111) 
> 
>      at 
> java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116) 
> 
>      at 
> java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:215) 
> 
>      at java.base/java.nio.file.Files.newByteChannel(Files.java:369)
>      at java.base/java.nio.file.Files.newByteChannel(Files.java:415)
>      at 
> java.base/java.nio.file.spi.FileSystemProvider.newInputStream(FileSystemProvider.java:384) 
> 
>      at java.base/java.nio.file.Files.newInputStream(Files.java:154)
>      at java.base/java.nio.file.Files.newBufferedReader(Files.java:2830)
>      at java.base/java.nio.file.Files.readAllLines(Files.java:3260)
>      at 
> jdk.attach/sun.tools.attach.VirtualMachineImpl.getNamespacePid(VirtualMachineImpl.java:334) 
> 
>      at 
> jdk.attach/sun.tools.attach.VirtualMachineImpl.<init>(VirtualMachineImpl.java:71) 
> 
>      at 
> jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) 
> 
>      at 
> jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) 
> 
>      at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:114)
>      at jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:98)
> 
> I fixed getNamespacePid() to validate that /proc/<pid>/status exists 
> before trying to process it. If it doesn't, it just returns pid rather 
> than trying to find ns_pid. However, that led to:
> 
> java.io.IOException: No such file or directory
>      at java.base/java.io.UnixFileSystem.createFileExclusively(Native 
> Method)
>      at java.base/java.io.File.createNewFile(File.java:1024)
>      at 
> jdk.attach/sun.tools.attach.VirtualMachineImpl.createAttachFile(VirtualMachineImpl.java:300) 
> 
>      at 
> jdk.attach/sun.tools.attach.VirtualMachineImpl.<init>(VirtualMachineImpl.java:78) 
> 
>      at 
> jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) 
> 
>      at 
> jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) 
> 
>      at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:114)
>      at jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:98)
> 
> This is because createAttachFile() first tried to create the file in 
> /proc/<pid>/cwd, and after that failed with IOException (which is 
> caught), it tried in /proc/<pid>/root/tmp, which fails with the above 
> uncaught IOException. I changed the code to only try 
> /proc/<pid>/root/tmp if pid != ns_pid (which means either it is a docker 
> situation and the pid is valid). Otherwise it reverts to the old 
> behavior just using /tmp.

This all seems okay.

One suggestion:

  359         } catch (NumberFormatException | IOException x) {
  360             throw new AttachNotSupportedException("Unable to parse 
namespace");
  361         }

Set "x" as the cause of the AttachNotSupportedException before throwing 
it. That will give more diagnostics if we ever see this exception.

Thans,
David

> thanks,
> 
> Chris
>>
>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>
>>
> 
>

More information about the serviceability-dev mailing list