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
|
