rootless: fix getCurrentOOMScoreAdj (fix rootless docker in kubernetes) by AkihiroSuda · Pull Request #42189 · moby/moby

AkihiroSuda · 2021-03-23T09:26:38Z

- What I did

Fix #40068: rootless docker in kubernetes: "getting the final child's pid from pipe caused \"EOF\": unknown"

- How I did it

Fixed getCurrentOOMScoreAdj() to trim "\n" before calling strconv.Atoi.

- How to verify it
rootless.yaml:

apiVersion: v1
kind: Pod
metadata:
  name: rootless
spec:
  containers:
    # dind:local is from "docker:20.10.5-dind" but the following files are updated:
    # - /usr/local/bin/dockerd     (for this PR)
    # - /usr/local/bin/containerd* (for https://github.com/containerd/containerd/pull/4845)
    - image: dind:local
      imagePullPolicy: Never
      name: rootless
      env:
        - name: DOCKER_HOST
          value: unix:///run/user/1000/docker.sock
      securityContext:
        privileged: true

$ kind create cluster
$ kubectl apply -f rootless.yaml
$ kubectl exec rootless -- docker run hello-world
...
Hello from Docker!

- Description for the changelog

Fix `rootless docker in kubernetes: "getting the final child's pid from pipe caused "EOF": unknown"

- A picture of a cute animal (not mandatory but encouraged)
🐧

thaJeztah

Wondering; if the issue is that we're trying to set an OOM score that's out of range; should we check limits in toRootless, and clip to max / min (-1000 .... 1000)?

moby/rootless/specconv/specconv_linux.go

Lines 70 to 72 in 46cdcd2

    
           if spec.Process.OOMScoreAdj != nil && *spec.Process.OOMScoreAdj < currentOOMScoreAdj { 
        
           	*spec.Process.OOMScoreAdj = currentOOMScoreAdj 
        
           }

thaJeztah · 2021-03-23T12:50:45Z

Do we know under what circumstances this fails? Would it be an os.IsNotExist()? (in that case, should we just ignore?)

Never happens, IIUC

thaJeztah · 2021-03-23T12:56:56Z

Should we log the value before trimming? (also simplifies the code a bit)

Suggested change

s := string(b)

i, err := strconv.Atoi(strings.TrimSpace(s))

if err != nil {

logrus.WithError(err).Warn("failed to parse /proc/self/oom_score_adj (%q)", s)

i, err := strconv.Atoi(strings.TrimSpace(string(b)))

if err != nil {

logrus.WithError(err).Warn("failed to parse /proc/self/oom_score_adj (%q)", b)

Already logging untrimmed string

oh! 🤦 yes, it's just the extra variable to convert []byte to string

AkihiroSuda · 2021-03-23T13:05:09Z

No, not out of range.

Docker always attempts to launch a container with oomScoreAdj=0, but the daemon's oomScoreAdj is set to 1000 by Kube, so we need to adjust the oomScoreAdj of the container being created from 0 to 1000.

`getCurrentOOMScoreAdj()` was broken because `strconv.Atoi()` was called without trimming "\n". Fix issue 40068: `rootless docker in kubernetes: "getting the final child's pid from pipe caused \"EOF\": unknown" Signed-off-by: Akihiro Suda <[email protected]>

thaJeztah · 2021-03-24T08:45:23Z

Docker always attempts to launch a container with oomScoreAdj=0

I recall changing the daemon itself to not adjust if no custom oom-score is set (#41527); in this case, the daemon runs inside a container with an OOM-score "X", but the daemon itself does not have an --oom-score-adj set (so we don't adjust); shouldn't we do the same for containers running in that environment? i.e., we didn't adjust their score, so inherit the current value (which on a host would be "don't adjust", but in this container would be "x")? I would consider the oom-score to be "relative to" the current environment. Would that work?

AkihiroSuda · 2021-03-24T08:53:40Z

I think that's possible by changing OomScoreAdj int to OomScoreAdj *int for allowing nil:

moby/api/types/container/host_config.go

Line 415 in dea989e

OomScoreAdj int // Container preference for OOM-killing

But that doesn't work with existing client implementations.

So I suggest merging this PR, and looking into possibility of the API change in a separate PR.

thaJeztah · 2021-03-24T14:02:23Z

But that doesn't work with existing client implementations.
So I suggest merging this PR, and looking into possibility of the API change in a separate PR.

Yes, we can fix that in a follow-up; I went looking for the original commit that added --oom-score-adj; d3af7f2 (#16277). From that PR it looks like we just added 0 as default; given that that is the default value (and not a choice of the user), and effectively should mean "don't adjust", I think we may even be able to change without changing it to a pointer (just consider 0 to mean "don't adjust"); I see we currently don't have an option to update the value in docker update, so from that perspective, it should not be ambiguous as well (if 0 could mean "reset to default" - not sure if even possible)

thaJeztah

LGTM

thaJeztah · 2021-03-24T14:03:14Z

@cpuguy83 PTAL

AkihiroSuda added area/rootless Rootless Mode kind/bugfix PR's that fix bugs process/cherry-pick labels Mar 23, 2021

This was referenced Mar 23, 2021

rootless docker in kubernetes: "getting the final child's pid from pipe caused \"EOF\"": unknown" #40068

Closed

[Feature request][rootless] Support crun with the default installation #42160

Closed

AkihiroSuda changed the title ~~rootless: fix getCurrentOOMScoreAdj~~ rootless: fix getCurrentOOMScoreAdj (fix rootless docker in kubernetes) Mar 23, 2021

thaJeztah requested changes Mar 23, 2021

View reviewed changes

AkihiroSuda force-pushed the specconv-fix-trimspace branch from da44f09 to d6ddfb6 Compare March 24, 2021 05:42

thaJeztah approved these changes Mar 24, 2021

View reviewed changes

AkihiroSuda requested review from kolyshkin and tonistiigi March 30, 2021 09:38

tiborvass approved these changes Apr 1, 2021

View reviewed changes

tiborvass merged commit e19ca14 into moby:master Apr 1, 2021

thaJeztah mentioned this pull request Apr 1, 2021

[20.10 backport] rootless: fix getCurrentOOMScoreAdj #42236

Merged

AkihiroSuda mentioned this pull request Apr 1, 2021

[20.10 backport] rootless: fix getCurrentOOMScoreAdj (fix rootless docker in kubernetes) #42237

Closed

AkihiroSuda added process/cherry-picked and removed process/cherry-pick labels Apr 1, 2021

thaJeztah added this to the 21.xx milestone Apr 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rootless: fix getCurrentOOMScoreAdj (fix rootless docker in kubernetes)#42189

rootless: fix getCurrentOOMScoreAdj (fix rootless docker in kubernetes)#42189
tiborvass merged 1 commit intomoby:masterfrom
AkihiroSuda:specconv-fix-trimspace

AkihiroSuda commented Mar 23, 2021 •

edited

Loading

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah Mar 23, 2021

Uh oh!

AkihiroSuda Mar 23, 2021

Uh oh!

thaJeztah Mar 23, 2021

Uh oh!

AkihiroSuda Mar 23, 2021

Uh oh!

thaJeztah Mar 23, 2021

Uh oh!

AkihiroSuda commented Mar 23, 2021

Uh oh!

thaJeztah commented Mar 24, 2021

Uh oh!

AkihiroSuda commented Mar 24, 2021

Uh oh!

thaJeztah commented Mar 24, 2021

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if spec.Process.OOMScoreAdj != nil && *spec.Process.OOMScoreAdj < currentOOMScoreAdj {
	*spec.Process.OOMScoreAdj = currentOOMScoreAdj
	}

-	s := string(b)
-	i, err := strconv.Atoi(strings.TrimSpace(s))
-	if err != nil {
-		logrus.WithError(err).Warn("failed to parse /proc/self/oom_score_adj (%q)", s)
+	i, err := strconv.Atoi(strings.TrimSpace(string(b)))
+	if err != nil {
+		logrus.WithError(err).Warn("failed to parse /proc/self/oom_score_adj (%q)", b)

Conversation

AkihiroSuda commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Mar 23, 2021

Uh oh!

thaJeztah commented Mar 24, 2021

Uh oh!

AkihiroSuda commented Mar 24, 2021

Uh oh!

thaJeztah commented Mar 24, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AkihiroSuda commented Mar 23, 2021 •

edited

Loading