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
|
