RFR: JDK-8215544: SA: Modify ClhsdbLauncher to add sudo privileges to enable MacOS tests on Mach5

[Jini George]

Thanks, Goetz for letting me know of the failure and thanks, David for 
the suggestion wrt SkippedException. Goetz, I will send you a patch 
off-list, and it would be great if you could test it again for me.

Thanks,
Jini.

On 1/20/2019 4:18 AM, David Holmes wrote:
> On 18/01/2019 11:30 pm, Lindenmaier, Goetz wrote:
>> Hi Jini,
>>
>> we see test failing after this change arrived deeper in our 
>> infrastructure.
>> On a linuxx86_64 docker container it does not work.
>>
>> This is because you now throw Error() if shouldSAAttach() returns false.
>> I think you need this:
>> --- a/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java    Thu 
>> Jan 03 17:30:03 2019 +0100
>> +++ b/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java    Fri 
>> Jan 18 14:25:05 2019 +0100
>> @@ -190,8 +190,9 @@
>>                      needPrivileges = true;
>>                  }
>>               } else {
>> -                System.out.println("SA attach not expected to work. 
>> Insufficient privileges.");
>> -                throw new Error("Cannot attach.");
>> +                // Silently skip the test if we don't have enough 
>> permissions to attach
>> +                System.out.println("SA attach not expected to work - 
>> test skipped.");
>> +                return null;
> 
> We should be able to use SkippedException for this case.
> 
> David
> 
>>               }
>>           }
>>
>> This is because canPtraceAttachLinux() in Platform.java returns false.
>>
>> What do you think?
>>
>> Best regards,
>>    Goetz.
>>
>>
>>
>>> -----Original Message-----
>>> From: serviceability-dev 
>>> <serviceability-dev-bounces at openjdk.java.net> On
>>> Behalf Of Jini George
>>> Sent: Montag, 14. Januar 2019 06:08
>>> To: Sharath Ballal <sharath.ballal at oracle.com>; JC Beyler
>>> <jcbeyler at google.com>; serviceability-dev at openjdk.java.net
>>> Subject: Re: RFR: JDK-8215544: SA: Modify ClhsdbLauncher to add sudo
>>> privileges to enable MacOS tests on Mach5
>>>
>>> Thank you very much, Sharath, for the review. My comments inline.
>>>
>>> On 1/12/2019 3:38 PM, Sharath Ballal wrote:
>>>> Hi Jini,
>>>>
>>>> ClhsdbLauncher.java
>>>> 1. Is the platform check not required in case of core files ?
>>> No, it is not needed. The permission issues don't exist.
>>>
>>>> -        if (!Platform.shouldSAAttach()) {
>>>> -            // Silently skip the test if we don't have enough 
>>>> permissions to attach
>>>> -            System.out.println("SA attach not expected to work - 
>>>> test skipped.");
>>>> -            return null;
>>>> -        }
>>>> -
>>>>
>>>> 2. Nit: extra line at 135
>>>
>>> Done.
>>>
>>> Thanks,
>>> Jini.
>>>>
>>>>
>>>> Thanks,
>>>> Sharath
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Jini George
>>>> Sent: Friday, January 11, 2019 9:46 AM
>>>> To: JC Beyler; serviceability-dev at openjdk.java.net
>>>> Subject: Re: RFR: JDK-8215544: SA: Modify ClhsdbLauncher to add sudo
>>> privileges to enable MacOS tests on Mach5
>>>>
>>>> Thanks again, JC! :-)
>>>>
>>>> Folks, Could I please get one more review on this ?
>>>>
>>>> -Jini.
>>>>
>>>> On 1/11/2019 9:38 AM, JC Beyler wrote:
>>>>> Hi Jini,
>>>>>
>>>>> Looks good to me now :) Thanks for handling the comments :) Jc
>>>>>
>>>>>
>>>>> On Tue, Jan 8, 2019 at 8:11 PM Jini George <jini.george at oracle.com
>>>>> <mailto:jini.george at oracle.com>> wrote:
>>>>>
>>>>>       Thank you very much for the review, JC. My comments inline:
>>>>>
>>>>>       On 1/8/2019 12:22 AM, JC Beyler wrote:
>>>>>        >
>>>>>        >
>>>>>
>>> http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/hotspot/jtreg/s 
>>>
>>> erviceability/sa/ClhsdbLauncher.java.udiff.html
>>>>>        >    +                // by appending 'sudo' to it. Check 
>>>>> now if sudo
>>>>>        > passes in this
>>>>>        >    +                // environment with a simple 'echo' 
>>>>> command.
>>>>>        >       -> Really shouldn't provide implementation details 
>>>>> about
>>>>>       what is
>>>>>        > being done by that method; if we change that, this comment 
>>>>> will
>>>>>       rot :-)
>>>>>        > canAddPrivileges is explicit enough in my mind
>>>>>
>>>>>       Done. I have removed the comment.
>>>>>
>>>>>        >
>>>>>        >
>>>>>
>>>>> http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/lib/jdk/tes
>>>>> t/lib/SA/SATestUtils.java.html
>>>>>
>>>>>        >
>>>>>        >      -> You put some System.out.println; are they intended to
>>>>>       still be
>>>>>        > there or were they for debug?
>>>>>
>>>>>       I guess you meant this one:
>>>>>       System.out.println("Adding 'sudo -E' to the command.");
>>>>>
>>>>>       This one is meant to be there.
>>>>>
>>>>>        >
>>>>>        >
>>>>>        >    77         for (String val: cmdStringList) {
>>>>>        >    78             outStringList.add(val);
>>>>>        >    79         }
>>>>>        >
>>>>>        >
>>>>>        > Couldn't we use addAll ?
>>>>>
>>>>>       Done.
>>>>>       The modified webrev is at:
>>>>>
>>>>>       http://cr.openjdk.java.net/~jgeorge/8215544/webrev.04/index.html
>>>>>
>>>>>       Thanks,
>>>>>       Jini.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> -- 
>>>>>
>>>>> Thanks,
>>>>> Jc