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, [email protected] 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
