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