|
Hi Chris,
+1
No need in another webrev.
Thanks,
Serguei
On 3/16/20 17:14, Igor Ignatyev wrote:
Hi
Chris,
does canPtraceAttachLinux have to be public?
otherwise, looks good to me.
-- Igor
Hi Serguei and Igor,
New webrev:
http://cr.openjdk.java.net/~cjplummer/8238268/webrev.02/index.html
Only files changed were Platform.java and
SATestUtils.java.
-Moved canPtraceAttachLinux() from Platform.java to
SATestUtils.java
-Changed Platform.canPtraceAttachLinux() reference in
SATestUtils.java to just be canPtraceAttachLinux().
-Had to change userName.equals("root") reference in
canPtraceAttachLinux() to Platform.isRoot(). Probably
should have been that way in the first place.
-Made some adjustments to the imports
thanks,
Chris
On 3/16/20 12:13 PM, Igor Ignatev wrote:
On 3/16/20 11:26,
Chris Plummer wrote:
I had to make
another change. TestMutuallyExclusivePlatformPredicates.java
failed when I ran tier 3. I had fixed it a
long while back due to Platform.shouldSAAttach()
being removed, but there were more changes
to Platform.java after that that I didn't
account for. isRoot() was added and
canPtraceAttrachLinux() was made public. So
this is what the diff looks like now:
---
a/test/hotspot/jtreg/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java
+++
b/test/hotspot/jtreg/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java
@@ -51,9 +51,9 @@
VM_TYPE("isClient", "isServer",
"isMinimal", "isZero", "isEmbedded"),
MODE("isInt", "isMixed", "isComp"),
IGNORED("isEmulatedClient",
"isDebugBuild", "isFastDebugBuild",
- "isSlowDebugBuild", "hasSA",
"shouldSAAttach", "isTieredSupported",
+ "isSlowDebugBuild", "hasSA",
"canPtraceAttachLinux", "isTieredSupported",
"areCustomLoadersSupportedForCDS",
"isDefaultCDSArchiveSupported",
- "isSignedOSX");
+ "isSignedOSX", "isRoot");
However, I'm thinking maybe I should just move
canPtraceAttachLinux() to SATestUtils.java
since that's the only user, and it is an SA
specific API. What do you think?
The approach to localize canPtraceAttachLinux() in
SATestUtils.java sounds right to me if it is an SA
specific API.
+1
— Igor
Thanks,
Serguei
Hi Chris,
Looks good.
Thank you for update!
Thanks,
Serguei
On 3/15/20 17:47, Chris Plummer wrote:
I changed
them all to "SA Attach" and grepped to
make sure there are no other occurrences
of "SA attach".
thanks,
Chris
On 3/15/20 4:49 PM, Igor Ignatyev wrote:
Hi Chris,
looks good, thanks!
one minor nit,
in SATestUtils::skipIfCannotAttach,
you have two exception messages which
start with 'SA attach not expected to
work.', and one w/ 'SA Attach not
expected to work.' (w/ Attach instead
of attach), it'd be nicer to have them
uniform.
Cheers,
-- Igor
HI Chris,
overall looks
good to me, a few comments
though:
1. since you
removed
vm.hasSAandCanAttach
from VMProps, you also
need to remove it from all
TEST.ROOT files which
mention it
(test/jdk/TEST.ROOT
and test/hotspot/jtreg/TEST.ROOT)
so people won't be
confused by undefined
property and jtreg will be
able to properly report
invalid usages of it if
any.
Ok, but it's unclear to me
what requires.properties is
even for, and what is the
impact of extra or missing
properties. What kind of test
would catch these errors?
jtreg uses 'requires.properties' as
a list of extra variables for
@require expressions, if a test uses
a name which isn't
in 'requires.properties' (or known
to jtreg), jtreg won't execute such
test and will set its status to
Error w/ 'invalid name ...' message.
2. in
SATestUtils::canAddPrivileges,
could you please add some
meaningful message to the
RuntimeException at L#102?
Ok.
throw new
RuntimeException("sudo process
interrupted", e);
3.
SATestUtils::checkAttachOk
method name is somewhat
misleading (hence you had
to put comment every time
you used it), I'd
recommend you to rename to
smth like
skipIfCannotAttach().
Ok, but I still left the
comment in place.
4. in
SATestUtils::checkAttachOk's
javadoc, it would be
better to use @throws tag
like:
+ /**
+ * Checks if SA
Attach is expected to
work.
+. * @throws
SkippedException ifSA
Attach is not expected
to work.
+ */
Ok.
5. it also
might make sense to
catch IOException within
SATestUtils::checkAttachOk
and throw it as Error or
RuntimeException.
Ok.
I've briefly
looked at all the changed
tests and they look good.
Thanks!
Chris
Thanks,
-- Igor
Hi
Serguei,
Thanks for the
review!
Can I get one
more reviewer
please?
thanks,
Chris
On 3/12/20 12:06
AM, serguei.spit...@oracle.com
wrote:
Hi
Chris,
On 3/12/20
00:03, Chris
Plummer wrote:
Hi
Serguei,
That check
used to be in
Platform.shouldSAAttach(), which essentially was moved to
SATestUtils.checkAttachOk()
and reworked
some. It was
necessary in
Platform.shouldSAAttach()
since that was
used to
evaluation
vm.hasSAandCanAttach
(which is now
gone). When I
moved
everything to
SATestUtils.checkAttachOk(), I recall thinking it wasn't really
necessary
since all
tests that
call it should
have @require
vm.hasSA, but
left it in
anyway just to
be extra safe.
I'm still
inclined to
just leave it
in, but would
not be opposed
to removing
it.
I agree, it is
more safe to
keep it, at list
for now.
Thanks,
Serguei
Hi
Chris,
I've made
another pass
today.
It looks good
to me.
I have just
one minor
questions.
There is some
overlap
between the requires
vm.hasSA check
and
checkAttachOk:
+ public static void checkAttachOk() throws IOException {
+ if (!Platform.hasSA()) {
+ throw new SkippedException("SA not supported.");
+ }
In
the former
case, the test
is not run but
in the latter
the
SkippedException
is thrown.
As I see, all
tests with the
checkAttachOk
call use requires
vm.hasSA as
well.
It can be that
the first
check
"if
(!Platform.hasSA())"
in
the
checkAttachOk
is redundant.
It
is okay and
more safe in
general but
generates
little
confusion.
I'm okay if
you don't do
anything with
this but
wanted to know
your view.
Thanks,
Serguei
On 3/10/20
18:57, Chris
Plummer wrote:
Hi
Chris,
Overall, this
looks as a
right
direction to
me while it is
not easy to
verify all the
details.
Yes, there are
a lot of tests
with quite a
few different
types of
changes. I did
a lot of
testing and
verified that
when the tests
pass, they
pass for the
right reasons
(really ran
the test,
skipped due to
lack of
privileges, or
skipped due to
running signed
on OSX 10.14
or later). I
also verified
locally
running as
root, running
with a cached
sudo, and
running
without sudo.
I'll make
another pass
tomorrow.
Thanks!
Ok
Ok
94 try {
95 if (echoProcess.waitFor(60, TimeUnit.SECONDS) == false) {
96 // Due to using the "-n" option, sudo should complete almost immediately. 60 seconds
97 // is more than generous. If it didn't complete in that time, something went very wrong.
98 echoProcess.destroyForcibly();
99 throw new RuntimeException("Timed out waiting for sudo to execute.");
100 }
101 } catch (InterruptedException e) {
102 throw new RuntimeException(e);
103 }
The lines
101/103 are
misaligned.
Ok.
Thanks,
Serguei
Thanks,
Chris
On 3/9/20
19:29, Chris
Plummer wrote:
Hi,
Please help
review the
following:
https://bugs.openjdk.java.net/browse/JDK-8238268
http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/
I'll try to
give enough
background
first to make
it easier to
understand the
changes. On
OSX you must
run SA tests
that attach to
a live process
as root or
using sudo.
For example:
sudo make
run-test
TEST=serviceability/sa/ClhsdbJstackXcompStress.java
Whether
running as
root or under
sudo, the
check to allow
the test to
run is done
with:
private
static boolean
canAttachOSX()
{
return
userName.equals("root");
}
Any test using
"@requires
vm.hasSAandCanAttach"
must pass this
check via
Platform.shouldSAAttach(),
which for OSX
returns:
return
canAttachOSX()
&&
!isSignedOSX();
So if running
as root the
"@requires
vm.hasSAandCanAttach"
passes,
otherwise it
does not.
However, using
a root login
to run tests
is not a very
desirable, nor
is issuing a
"sudo make
run-test" (any
created file
ends up with
root
ownership).
Because of
this support
was previously
added for just
running the
attaching
process using
sudo, not the
entire test.
This was only
done for the
20 or so tests
that use
ClhsdbLauncher.
These tests
use "@requires
vm.hasSA", and
then while
running the
test will do a
"sudo" check
if
canAttachOSX()
returns false:
if
(!Platform.shouldSAAttach())
{
if
(Platform.isOSX()) {
if
(Platform.isSignedOSX()) {
throw new SkippedException("SA attach not expected
to work. JDK
is signed.");
} else if
(SATestUtils.canAddPrivileges()) {
needPrivileges = true;
}
}
if
(!needPrivileges) {
// Skip the
test if we
don't have
enough
permissions to
attach
// and cannot
add
privileges.
throw new
SkippedException(
"SA attach not expected to work. Insufficient
privileges.");
}
}
So basically
it does a
runtime check
of
vm.hasSAandCanAttach,
and if it
fails then
checks if
running with
sudo will
work. This
allows for
either a
passwordless
sudo to be
used when
running
clhsdb, or for
the user to be
prompted for
the sudo
password (note
I've remove
support for
the latter
with my
changes).
That brings us
to the CR that
is being
fixed.
ClhsdbLauncher
tests support
sudo and will
therefore run
with our CI
testing on
OSX, but the
25 or so tests
that use
"@requires
vm.hasSAandCanAttach"
do not, and
therefore are
never run with
our CI OSX
testing. The
changes in
this webrev
fix that.
There are two
possible
approaches to
the fix. One
is having the
check for sudo
be done as
part of the
vm.hasSAandCanAttach
evaluation.
The other
approach is to
do the check
in the test at
runtime
similar to how
ClhsdbLauncher
currently
does. This
would mean
just using
"@requires
vm.hasSA" for
all the tests
instead of
"@requires
vm.hasSAandCanAttach".
I chose the
later because
there is an
advantage to
throwing
SkippedException
rather than
just silently
skipping the
test using
@requires. The
advantage is
that mdash
tells you how
many tests
were skipped,
and when you
hover over the
reason you can
see the
SkippedException
message, which
will
differentiate
between
reasons like
the JDK was
signed or
there are
insufficient
privileges. If
all the
checking was
done by the
vm.hasSAandCanAttach
evaluation,
you would not
know why the
test wasn't
run.
The "support"
related
changes made
are all in the
following 3
files. The
rest of the
changes are in
the tests:
test/jtreg-ext/requires/VMProps.java
test/lib/jdk/test/lib/Platform.java
test/lib/jdk/test/lib/SA/SATestUtils.java
You'll noticed
that one
change I made
to the sudo
support in
SATestUtils.canAddPrivileges()
is to make
sudo
non-interactive,
which means no
password
prompt. So
that means
either the
user does not
require a
password, or
the
credentials
have been
cached.
Otherwise the
sudo check
will fail. On
most platforms
if you execute
a sudo
command, the
credentials
are cached for
5 minutes. So
if your user
is not setup
for
passwordless
sudo, then a
sudo command
can be issued
before running
the tests, and
will likely
remain cached
until the test
is run. The
reason for
using
passwordless
is because
prompting in
the middle of
running tests
can be
confusing (you
usually walk
way once
launching the
tests and miss
the prompt
anyway), and
avoids
unnecessary
delays in
automated
testing due to
waiting for
the password
prompt to
timeout (it
used to wait 1
minute).
There are
essentially 3
types of tests
that SA Attach
to a process,
each needing a
slightly
different fix:
1. Tests that
directly
launch a
jdk.hotspot.agent
class, such as
TestClassDump.java. They need to call
SATestUtils.checkAttachOk() to verify that attaching will be possible,
and then
SATestUtils.addPrivilegesIfNeeded(pb) to get the sudo command added if
needed.They
also need to
switch from
using
hasSAandCanAttach
to using
hasSA.
2. Tests that
launch command
line tools
such has
jhsdb. They
need to call
SATestUtils.checkAttachOk()
to verify that
attaching will
be possible,
and then
SATestUtils.createProcessBuilder() to create a process that will be
launched using
sudo if
necessary.They
also need to
switch from
using
hasSAandCanAttach
to using
hasSA.
3. Tests that
use
ClhsdbLauncher.
They already
use hasSA
instead of
hasSAandCanAttach,
and rely on
ClhsdbLauncher
to do check at
runtime if
attaching will
work, so for
the most part
all the these
tests are
unchanged.
ClhsdbLauncher
was modified
to take
advantage of
the new
SATestUtils.createProcessBuilder() and SATestUtils.checkAttachOk() APIs.
Some tests
required
special
handling:
test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java
test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java
- These two
tests SA
Attach to a
core file, not
to a process,
so only need
hasSA,
not
hasSAandCanAttach.
No other
changes were
needed.
test/hotspot/jtreg/serviceability/sa/ClhsdbFindPC.java
- The output
should never
be null. If
the test was
skipped due to
lack of
privileges,
you
would never
get to this
section of the
test.
test/hotspot/jtreg/serviceability/sa/TestClhsdbJstackLock.java
test/hotspot/jtreg/serviceability/sa/TestIntConstant.java
test/hotspot/jtreg/serviceability/sa/TestPrintMdo.java
test/hotspot/jtreg/serviceability/sa/TestType.java
test/hotspot/jtreg/serviceability/sa/TestUniverse.java
- These are
ClhsdbLauncher
tests, so they
should have
been using
hasSA instead
of
hasSAandCanAttachin
the first
place. No
other changes
were needed.
test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java
test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java
test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java
- These tests
used to
"@require mac"
but seem run
fine on OSX,
so I removed
this
requirement.
test/jdk/sun/tools/jhsdb/BasicLauncherTest.java
- This test
had a runtime
check to not
run on OSX due
to not having
core file
stack
walking
support.
However, this
tests always
attaches to a
process, not a
core file,
and seems to
run just fine
on OSX.
test/jdk/sun/tools/jstack/DeadlockDetectionTest.java
- I changed
the test to
throw a
SkippedException
if it gets the
unexpected
error code
rather than
just println.
And a few
other
miscellaneous
changes not
already
covered:
test/lib/jdk/test/lib/Platform.java
- Made
canPtraceAttachLinux()
public so it
can be called
from
SATestUtils.
-
vm.hasSAandCanAttach
is now gone.
thanks,
Chris
|
