Skip to content

Fix setting swaplimit=true without checking memory.swap.max#42071

Merged
thaJeztah merged 1 commit intomoby:masterfrom
jmguzik:41926-cgroups-swap-memory-max
Jun 5, 2021
Merged

Fix setting swaplimit=true without checking memory.swap.max#42071
thaJeztah merged 1 commit intomoby:masterfrom
jmguzik:41926-cgroups-swap-memory-max

Conversation

@jmguzik
Copy link
Copy Markdown
Contributor

@jmguzik jmguzik commented Feb 24, 2021

- What I did
fixes #41926

- How I did it
Added one condition to moby/pkg/sysinfo/cgroup2_linux.go

- How to verify it
Actually, I wanted to create unit tests for this one. The problem is certain files must be present on FS, so I see 2 solutions:

  1. use mocks (of os.Stat for example)
  2. since part of the path is passed as an argument, choose a different dir for tests (eg /tmp?)

I do not know what is the common practice in the project (1st PR), so I decided just to simply open a PR and ask.

- Description for the changelog

Fixes setting swaplimit=true without checking memory.swap.max

- A picture of a cute animal (not mandatory but encouraged)
Another time :)

@jmguzik
Copy link
Copy Markdown
Contributor Author

jmguzik commented Feb 24, 2021

cc @AkihiroSuda as issue author

AkihiroSuda
AkihiroSuda reviewed Feb 25, 2021
View reviewed changes
Comment thread pkg/sysinfo/cgroup2_linux.go Outdated
AkihiroSuda
AkihiroSuda requested changes Feb 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comment

@jmguzik jmguzik force-pushed the 41926-cgroups-swap-memory-max branch from 99872ec to b7c01c1 Compare February 25, 2021 07:46
jmguzik
jmguzik commented Feb 25, 2021
View reviewed changes
Comment thread pkg/sysinfo/cgroup2_linux.go Outdated
@jmguzik jmguzik requested a review from AkihiroSuda February 25, 2021 07:59
@jmguzik jmguzik force-pushed the 41926-cgroups-swap-memory-max branch from b7c01c1 to c5e1235 Compare February 25, 2021 23:03
jmguzik
jmguzik commented Feb 25, 2021
View reviewed changes
Comment thread pkg/sysinfo/cgroup2_linux.go Outdated
AkihiroSuda
AkihiroSuda requested changes Feb 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but we need to support dind case (g="/", in a cgroup namespace)

Comment thread pkg/sysinfo/cgroup2_linux.go Outdated
AkihiroSuda
AkihiroSuda reviewed Feb 26, 2021
View reviewed changes
Comment thread pkg/sysinfo/cgroup2_linux.go Outdated
@jmguzik jmguzik force-pushed the 41926-cgroups-swap-memory-max branch from c5e1235 to 7c7b50d Compare February 26, 2021 09:33
@jmguzik jmguzik requested a review from AkihiroSuda February 26, 2021 11:11
AkihiroSuda
AkihiroSuda approved these changes Feb 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@jmguzik
Copy link
Copy Markdown
Contributor Author

jmguzik commented Mar 1, 2021

I feel ci problems are not related to this issue, but anyway, rebase?

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Mar 2, 2021

restarted CI

thaJeztah
thaJeztah requested changes Mar 2, 2021
View reviewed changes
Comment thread pkg/sysinfo/cgroup2_linux.go Outdated
Comment thread pkg/sysinfo/cgroup2_linux.go Outdated
Comment thread pkg/sysinfo/cgroup2_linux.go Outdated
@jmguzik jmguzik force-pushed the 41926-cgroups-swap-memory-max branch from 7c7b50d to 09ffefd Compare March 5, 2021 00:04
@jmguzik jmguzik requested a review from thaJeztah March 5, 2021 00:11
@jmguzik
Copy link
Copy Markdown
Contributor Author

jmguzik commented Mar 16, 2021

@thaJeztah @AkihiroSuda how the situation looks like with this change? CI failures are unrelated.

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Apr 7, 2021

@thaJeztah PTAL?

@jmguzik jmguzik force-pushed the 41926-cgroups-swap-memory-max branch from 09ffefd to eded8e4 Compare April 12, 2021 19:34
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Apr 27, 2021

ping @thaJeztah @tiborvass @cpuguy83

cpuguy83
cpuguy83 approved these changes May 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented May 28, 2021

s390x failures are unrelated.
Merging.
https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-42071/16/tests

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented May 28, 2021 •
edited
Loading

CI seems to have to be green 😞

@thaJeztah Can we relax "Merging is blocked" restriction?

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jun 1, 2021

@jmguzik Could you rebase? It will perhaps make Ci green.

@thaJeztah thaJeztah mentioned this pull request Jun 1, 2021
Closed
19 tasks
@jmguzik jmguzik force-pushed the 41926-cgroups-swap-memory-max branch from eded8e4 to 7ef6ece Compare June 3, 2021 20:54
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 5, 2021

I kicked CI

Failures:

TestConcurrencyNoWait tracked by #42458

=== RUN   TestConcurrencyNoWait
--- FAIL: TestConcurrencyNoWait (0.13s)
    iptables_test.go:217:  (iptables failed: iptables -t nat -A DOCKEREST -p tcp -d 192.168.1.1 --dport 1234 -j DNAT --to-destination 172.17.0.1:4321 ! -i lo: Another app is currently holding the xtables lock. Perhaps you want to use the -w option?
         (exit status 4))

TestNetworkDBIslands tracked by #42459

=== RUN   TestNetworkDBIslands
...
networkdb_test.go:864: timeout hit after 2m0s: node5:Waiting for all nodes to cleanly leave, left: 2, failed nodes: 1

TestPortMappingV6Config: created tracking issue #42468

=== RUN   TestPortMappingV6Config
time="2021-06-04T05:10:59Z" level=warning msg="Running modprobe bridge br_netfilter failed with message: , error: exec: \"modprobe\": executable file not found in $PATH"
time="2021-06-04T05:10:59Z" level=warning msg="running inside docker container, ignoring missing kernel params: open /proc/sys/net/bridge/bridge-nf-call-iptables: no such file or directory"
time="2021-06-04T05:10:59Z" level=warning msg="bridge store not initialized. kv object docker/network/v1.0/bridge/dummy/ is not added to the store"
time="2021-06-04T05:10:59Z" level=warning msg="bridge store not initialized. kv object docker/network/v1.0/bridge-endpoint/ep1/ is not added to the store"
time="2021-06-04T05:11:00Z" level=warning msg="bridge store not initialized. kv object docker/network/v1.0/bridge-endpoint/ep1/ is not added to the store"
--- FAIL: TestPortMappingV6Config (0.92s)
    port_mapping_test.go:157: Failed to store the port bindings into the sandbox info. Found: [{udp 172.19.0.11 400 0.0.0.0 54000 54000} {tcp 172.19.0.11 500 0.0.0.0 65000 65000} {sctp 172.19.0.11 500 0.0.0.0 65000 65000}]

thaJeztah
thaJeztah approved these changes Jun 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (sorry, looks like I forgot to submit)

I created #42452 to discuss/track follow-ups on the libcontainer/cgroups discussion

@thaJeztah thaJeztah added this to the 21.xx milestone Jun 5, 2021
@thaJeztah thaJeztah mentioned this pull request Jun 5, 2021
Merged
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 5, 2021

All green, except for the TestNetworkDBIslands test, which is known to be flaky; merging this one; thanks!

@thaJeztah thaJeztah merged commit 8dbd90e into moby:master Jun 5, 2021
@AkihiroSuda AkihiroSuda mentioned this pull request Jun 7, 2021
Merged
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jun 7, 2021

Cherry-pick: #42479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

No one assigned

Labels

area/cgroup2 cgroup v2 kind/bugfix PR's that fix bugs process/cherry-picked status/4-merge

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

[cgroup2] docker info: SwapLimit = true is set without checking

4 participants

@jmguzik @thaJeztah @AkihiroSuda @cpuguy83