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.
2. in SATestUtils::canAddPrivileges, could you please add some meaningful message to the RuntimeException at L#102? 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(). 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. > + */ 5. it also might make sense to catch IOException within SATestUtils::checkAttachOk and throw it as Error or RuntimeException. I've briefly looked at all the changed tests and they look good. Thanks, -- Igor > On Mar 12, 2020, at 11:06 PM, Chris Plummer <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 >>>>>>> >>>>>> >>>>> >>>> >>> >> >
