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