Skip to content

cgroupv2: fix setting MemorySwap#2288

Merged
crosbymichael merged 1 commit intoopencontainers:masterfrom
kolyshkin:mem-swap
Apr 8, 2020
Merged

cgroupv2: fix setting MemorySwap#2288
crosbymichael merged 1 commit intoopencontainers:masterfrom
kolyshkin:mem-swap

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.

4 participants