RFR(10)(M): 8179498: attach in linux should be relative to /proc/pid/root and namespace aware
Chris Plummer
chris.plummer at oracle.com
Mon Sep 18 18:11:18 UTC 2017
On 9/17/17 5:17 PM, David Holmes wrote:
> 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.
>
Hi David,
> 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.
Ok. I'll make this change.
thanks,
Chris
>
> 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