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: fix setting MemorySwap #2288

Merged
merged 1 commit into from
Apr 8, 2020

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Apr 2, 2020

The resources.MemorySwap field from OCI is memory+swap, while cgroupv2
has a separate swap limit, so subtract memory from the limit (and make
sure values are set and sane).

Make sure to set MemorySwapMax for systemd, too. Since systemd does not
have MemorySwapMax for cgroupv1, it is only needed for v2 driver.

  • v2: return -1 on any negative value, add unit test
  • v3: treat any negative value other than -1 as error

@kolyshkin kolyshkin changed the title cgroupv2: set MemorySwapMax= for systemd [WIP/RFC] cgroupv2: set MemorySwapMax= for systemd Apr 2, 2020
@kolyshkin
Copy link
Contributor Author

@giuseppe pointed out that in v1 this is mem+swap while in v2 this is swap only, so this might be wrong. Will take another look tomorrow.

@kolyshkin kolyshkin changed the title [WIP/RFC] cgroupv2: set MemorySwapMax= for systemd cgroupv2: fix setting MemorySwap Apr 7, 2020
@kolyshkin
Copy link
Contributor Author

@AkihiroSuda @giuseppe @mrunalp PTAL

// ConvertMemorySwapToCgroupV2Value converts MemorySwap value for cgroup v2
// drivers. Since Resources.MemorySwap is define as memory+swap combined,
// while in cgroup v2 swap is a separate value, a conversion is needed.
func ConvertMemorySwapToCgroupV2Value(memorySwap, memory int64) (int64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add UT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 8, 2020

LGTM

Approved with PullApprove

The resources.MemorySwap field from OCI is memory+swap, while cgroupv2
has a separate swap limit, so subtract memory from the limit (and make
sure values are set and sane).

Make sure to set MemorySwapMax for systemd, too. Since systemd does not
have MemorySwapMax for cgroupv1, it is only needed for v2 driver.

[v2: return -1 on any negative value, add unit test]
[v3: treat any negative value other than -1 as error]

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

I was wrong in treating any negative value as "max". In fact only -1 should be treated as "max", and all other negative values are invalid.

Patch updated.

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 8, 2020

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael
Copy link
Member

crosbymichael commented Apr 8, 2020

LGTM

Approved with PullApprove

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