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 > On Mar 15, 2020, at 4:35 PM, Chris Plummer <chris.plum...@oracle.com> wrote: > > Hi Igor, > > Thanks for the review. Here's and updated webrev with all of the suggestions > from you and Serguei: > > http://cr.openjdk.java.net/~cjplummer/8238268/webrev.01/index.html > <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.01/index.html> > > Also some comments inline below. > > On 3/13/20 9:26 AM, Igor Ignatyev wrote: >> 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 >> >> >>> On Mar 12, 2020, at 11:06 PM, Chris Plummer <chris.plum...@oracle.com >>> <mailto:chris.plum...@oracle.com>> wrote: >>> >>> 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 >>> <mailto: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 >>>> >>>>> thanks, >>>>> >>>>> Chris >>>>> >>>>> On 3/11/20 11:20 PM, serguei.spit...@oracle.com >>>>> <mailto:serguei.spit...@oracle.com> wrote: >>>>>> 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: >>>>>>> On 3/10/20 6:07 PM, serguei.spit...@oracle.com >>>>>>> <mailto:serguei.spit...@oracle.com> 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! >>>>>>>> >>>>>>>> A couple of quick nits so far: >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java.udiff.html >>>>>>>> >>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java.udiff.html> >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java.udiff.html >>>>>>>> >>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java.udiff.html> >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java.udiff.html >>>>>>>> >>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java.udiff.html> >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java.udiff.html >>>>>>>> >>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java.udiff.html> >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackMixed.java.udiff.html >>>>>>>> >>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackMixed.java.udiff.html> >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestRevPtrsForInvokeDynamic.java.udiff.html >>>>>>>> >>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestRevPtrsForInvokeDynamic.java.udiff.html> >>>>>>>> import jdk.test.lib.Utils; >>>>>>>> -import jdk.test.lib.Asserts; >>>>>>>> +import jdk.test.lib.SA.SATestUtils; >>>>>>>> Need to swap these exports. >>>>>>>> >>>>>>>> >>>>>>> Ok >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/lib/jdk/test/lib/SA/SATestUtils.java.frames.html >>>>>>>> >>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/lib/jdk/test/lib/SA/SATestUtils.java.frames.html> >>>>>>>> 48 if (SATestUtils.needsPrivileges()) { >>>>>>>> 49 cmdStringList = >>>>>>>> SATestUtils.addPrivileges(cmdStringList); >>>>>>>> The method calls are local, so the class name can be omitted in the >>>>>>>> method names: >>>>>>>> SATestUtils.needsPrivileges and SATestUtils.addPrivileges. >>>>>>> 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 >>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8238268> >>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/ >>>>>>>>> <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 >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
