Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cgroupv2: set mem+swap to max if memory is set to max #2285

Closed
wants to merge 1 commit into from

Conversation

kolyshkin
Copy link
Contributor

In cgroupv1, command runc update $ID --memory -1 sets both
memory and memory+swap to -1 (aka unlimited). This was introduced
by commit 18ebc51 (Reset Swap when memory is set to unlimited,
Oct 19 2016).

This is not the case for cgroupv2. Fix it.

References:

In cgroupv1, command `runc update $ID --memory -1` sets both
memory and memory+swap to -1 (aka unlimited). This was introduced
by commit 18ebc51 (Reset Swap when memory is set to unlimited,
Oct 19 2016).

This is not the case for cgroupv2. Fix it.

References:
 - opencontainers#1127
 - moby/moby#22578

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

The motivation for this PR is to make cgroupv2 behavior in this regard compatible with v1. Whether this v1 behavior is good or not I can't yet assess. Even after reading all the discussions linked above, I still do not understand whether it is useful to have unlimited RAM but limited RAM+swap. Probably not, but I'm not quite sure.

@AkihiroSuda
Copy link
Member

Do we need this for systemd driver as well?

@AkihiroSuda
Copy link
Member

cc @giuseppe

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Apr 2, 2020

Do we need this for systemd driver as well?

@AkihiroSuda well, systemd driver

  1. does not set MemorySwapMax= for systemd unit via dbus (yet? I only found it today and need to take a closer look), it is only set via fsManager.
  2. does not changes the systemd unit properties on update (IOW the Set() method only uses fs2 to update the limits).

I found both of these items today, and they both probably need to be taken care of (and I will file issues and try to fix this), but as of now, this PR fixes the issue for both fs2 and systemd drivers.

}

if memSwap != "" {
if err := fscommon.WriteFile(dirPath, "memory.swap.max", memSwap); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

to keep the same cgroup v1 behaviour (as the config.json file is configured for), we need to set the memory swap to requested_memory_limit - requested_swap_limit. Since on cgroup v1 the swap limit accounts also for the memory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, in this case maybe we don't need to touch swap at all, and this PR should be closed, right?

Copy link
Member

Choose a reason for hiding this comment

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

yes setting the memory.max should be enough on cgroup v2 as the swap max should not have any effect on it.

Copy link
Member

Choose a reason for hiding this comment

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

but since the PR is open, I think we could use it for setting the swap limit as it is expected to work on cgroup v1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #2288

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Apr 2, 2020

  1. does not set MemorySwapMax= for systemd unit via dbus (yet? I only found it today and need to take a closer look), it is only set via fsManager.

Created #2288

  1. does not changes the systemd unit properties on update (IOW the Set() methodonly uses fs2 to update the limits).

Filed #2287

@kolyshkin
Copy link
Contributor Author

This PR is wrong. Closed in favor of #2288

@kolyshkin kolyshkin closed this Apr 7, 2020
@kolyshkin kolyshkin deleted the cgroupv2-mem-swap branch May 20, 2020 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants