[8u] RFR: 8217766: Container Support doesn't work for some Join Controllers combinations

[Andrew Hughes]

On 10:14 Mon 30 Nov     , Severin Gehwolf wrote:
> On Mon, 2020-11-30 at 05:13 +0000, Andrew Hughes wrote:
> > On 15:32 Tue 27 Oct     , Severin Gehwolf wrote:
> > > Hi!
> > > 
> > > Could I please get a review for this 8u backport of 8217766. I see
> > > Oracle backported it too to 8u281. The JDK 11 patch does not apply
> > > cleanly, because a) no unified logging in hotspot in 8u b) split of jdk
> > > and hotspot repositories in 8u.
> > > 
> > > I've manually backported hotspot changes. The JDK changes actually
> > > applied cleanly. While this fixes part of the problem. JDK-8254854 will
> > > be fixed separately and as a follow-up.
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8217766
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8217766/01/
> > > 
> > > Testing: Manually on affected system with join controllers[1].
> > > Container tests on Linux x86_64: no regressions.
> > > 
> > > Thoughts?
> > > 
> > > Thanks,
> > > Severin
> > > 
> > > [1] https://bugs.openjdk.java.net/browse/JDK-8217766?focusedCommentId=14373516&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14373516
> > > 
> > 
> > A blank line (before the mountinfo comment) gets lost in the backport
> > from 11u to 8u, causing the diffs of osContainer_linux.cpp to not line
> > up. You can see this after applying your patch and comparing the two
> > files:
> > 
> > @@ -262,13 +324,14 @@
> >      char tmpcgroups[MAXPATHLEN+1];
> >      char *cptr = tmpcgroups;
> >      char *token;
> > +
> >      // mountinfo format is documented at https://www.kernel.org/doc/Documentation/filesystems/proc.txt
> >      if (sscanf(p, "%*d %*d %*d:%*d %s %s %*[^-]- cgroup %*s %s", tmproot, tmpmount, tmpcgroups) != 3) {
> >        continue;
> >      }
> >      while ((token = strsep(&cptr, ",")) != NULL) {
> >        if (strcmp(token, "memory") == 0) {
> > -        memory = new CgroupSubsystem(tmproot, tmpmount);
> > +        memory = new CgroupMemorySubsystem(tmproot, tmpmount);
> >        } else if (strcmp(token, "cpuset") == 0) {
> >          cpuset = new CgroupSubsystem(tmproot, tmpmount);
> >        } else if (strcmp(token, "cpu") == 0) {
> > 
> > Not only does this make it hard to compare your backport with the
> > original to see what changed, it will cause unnecessary context
> > differences for future backports. Please rectify this.
> 
> Thanks for the review!
> 
> Done:
> https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8217766/jdk8/hotspot/webrev/
> 
> Thanks,
> Severin
> 

Thanks. That looks better.

webrev is still doing something weird. If I apply the 11u patch myself,
I get a clean diff between it and the 11u version:

https://cr.openjdk.java.net/~andrew/openjdk8/8217766/webrev.01/8217766.8u.andrew

but, as soon as I webrev that, I get what you did this time:

https://cr.openjdk.java.net/~andrew/openjdk8/8217766/webrev.01/hotspot.patch

Quite bizarre.

Anyway, if what you have locally is the same as the first of these links,
I'm happy. I thought at first you'd had to add your own logging code, but
it's just the removal that's slightly different. Please flag for approval.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
OpenJDK Package Owner
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222