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