RFR [XS] : 8229182: runtime/containers/docker/TestMemoryAwareness.java test fails on SLES12

[Langer, Christoph]

Hi Matthias,

overall the patch looks good to me (and proves to solve the test issue in our nightlies).

I have some minor nits/room for cleanup:

test/lib/jdk/test/lib/containers/docker/DockerRunOptions.java:
- Copyright year should be updated
- Line 38, initialization of ArrayList: This could be written as new ArrayList< >();, e.g.
public ArrayList<String> javaOptsAppended = new ArrayList< >();
Maybe you can update the initializations in line 34, 36 and 40 as well.

test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java (as you are touching it):
line 43 " import jdk.test.lib.process.ProcessTools;" can be removed
line 48 " private static final String FS = File.separator;" can be removed

Don't need to see a new webrev for that.

Thanks & Best regards
Christoph

> -----Original Message-----
> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
> Baesken, Matthias
> Sent: Freitag, 30. August 2019 15:44
> To: Mikhailo Seledtsov <mikhailo.seledtsov at oracle.com>
> Cc: hotspot-dev at openjdk.java.net
> Subject: RE: RFR [XS] : 8229182:
> runtime/containers/docker/TestMemoryAwareness.java test fails on SLES12
> 
> Hello ,  here is a  new revision  that  retains  the javaTest options and  just
> resets Xmx .
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8229182.4/
> 
> I checked your proposal to use
> jdk.test.lib.Utils.getFilteredTestJavaOpts(String...  filters)   ; unfortunately  it
> does not really  do what I want .
> This  returns me a copy  of the  filtered TestJavaOpts .
> 
> But in DockerTestUtils.java   buildJavaCommand   we   still use the  real
> (unfiltered) options , unless  we set    opts.appendTestJavaOptions    to false
> .
> And I think it was stated  in the review  that  it isn’t  wanted to  set
> opts.appendTestJavaOptions    to false  (because this  would  remove    not
> only  the unwanted  Xmx  in our case but more options that might be needed
> ).
> 
> So    8229182.4   allows   to  intentionally  set   another Xmx   AFTER   what
> Utils.getTestJavaOpts   ,  this is what we want in this test  and it should be
> compatible   for other users  of    DockerTestUtils.java  .
> 
> Best regards, Matthias
> 
> 
> 
> > -----Original Message-----
> > From: Mikhailo Seledtsov <mikhailo.seledtsov at oracle.com>
> > Sent: Dienstag, 27. August 2019 22:55
> > To: Baesken, Matthias <matthias.baesken at sap.com>
> > Cc: Bob Vandette <bob.vandette at oracle.com>; hotspot-
> > dev at openjdk.java.net
> > Subject: Re: RFR [XS] : 8229182:
> > runtime/containers/docker/TestMemoryAwareness.java test fails on
> SLES12
> >
> > FYI: you could use jdk.test.lib.Utils.getFilteredTestJavaOpts(String...
> > filters) for this purpose.
> >
> > On 8/27/19, 10:15 AM, Mikhailo Seledtsov wrote:
> > > I agree with Bob. It seems this is exactly what we wish in this case:
> > > filter out/eliminate some options that interfere with the test, while
> > > retaining other options and passing them to the JVM running inside a
> > > container.
> > >
> > >
> > > Thank you,
> > > Misha
> > >
> > > On 8/27/19, 8:42 AM, Bob Vandette wrote:
> > >> This is an isolated problem and I don’t like the idea of adding a new
> > >> option just for this case.
> > >>
> > >> What if we parse the options and eliminated the general -Xmx option
> > >> if one was specified
> > >> by the test?  After all, this is the intention of adding a test
> > >> specific heap limit.
> > >>
> > >> Bob.
> > >>
> > >>
> > >>
> > >>> On Aug 27, 2019, at 10:33 AM, Severin
> Gehwolf<sgehwolf at redhat.com>
> > >>> wrote:
> > >>>
> > >>> On Tue, 2019-08-27 at 14:20 +0000, Baesken, Matthias wrote:
> > >>>> Hello  ,   my suggestions was " appendJavaOpts "   :
> > >>>>
> > >>> OK, got it now. Looking at the webrev this wasn't clear as it didn't do
> > >>> that.
> > >>>
> > >>>>      In case we want to be safe,  I suggest to
> > >>>> add  an  additional  method   " appendJavaOpts "
> > >>>>
> > >>>>      opts.appendJavaOpts("-Xmx" + javaHeapSize);
> > >>>>
> > >>>>      to DockerRunOptions ;  appendJavaOpts   would append  the
> > >>>> options  after  the  testJavaOpts    (so Xmx can be overwritten ).
> > >>>>
> > >>>> But if  overrideJavaOpts    is preferred  that fine with me too ...
> > >>> I'd prefer a more telling name than "append". I'd ask myself when
> > >>> trying to use it: Does it append to regular java options? How is it
> > >>> different from addJavaOpts()? You get the idea. I'd prefer
> > >>> overrideJavaOpts for that reason.
> > >>>
> > >>> Perhaps addOverrideOpts()? Either way, the javadoc should make it
> > clear
> > >>> what adding options there means.
> > >>>
> > >>> Thanks,
> > >>> Severin
> > >>>
> > >>>> Best regards, Matthias
> > >>>>
> > >>>>
> > >>>>> On Tue, 2019-08-27 at 13:49 +0000, Baesken, Matthias wrote:
> > >>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8229182.3/
> > >>>>> OK, so, in summary, the constraints are as follows:
> > >>>>>
> > >>>>> * We don't want to turn of appending global test java options as
> > >>>>> these
> > >>>>>    could possibly contain local options to a specific test
> > >>>>> environment.
> > >>>>>    It would be a surprise if docker tests wouldn't have them.
> > >>>>> * Just setting Java options is not enough as the intention is to
> > >>>>> set
> > >>>>>    them for the Docker test, but could potentially be overridden by
> > >>>>>    global test options (this issue).
> > >>>>>
> > >>>>> Given we'd like to keep that general design, maybe it would make
> > >>>>> sense
> > >>>>> to create a 3'rd, separate set of flags, neither java options nor
> > >>>>> test
> > >>>>> options would manage to override. Call it "overrideJavaOpts" or
> > >>>>> some
> > >>>>> such. At least that would make it clear that these options if set
> > >>>>> have
> > >>>>> the final say.
> > >>>>>
> > >>>>> Thoughts?
> > >>>>>
> > >>>>> Thanks,
> > >>>>> Severin