Skip to content

Update spec to master, switch to int64 for memory limits #1495

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.

Sign up for GitHub

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

Merged
crosbymichael merged 3 commits into from
Jun 27, 2017
Merged

Update spec to master, switch to int64 for memory limits #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8ef602
Update to runtime spec 198f23f827eea397d4331d7eb048d9d4c7ff7bee
justincormack Jun 24, 2017
e114618
Remove Platform as no longer in OCI spec
justincormack Jun 24, 2017
3d9074e
Update memory specs to use int64 not uint64
justincormack Jun 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 17 additions & 19 deletions libcontainer/cgroups/fs/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,22 +73,22 @@ func EnableKernelMemoryAccounting(path string) error {
// until a limit is set on the cgroup and limit cannot be set once the
// cgroup has children, or if there are already tasks in the cgroup.
for _, i := range []int64{1, -1} {
if err := setKernelMemory(path, uint64(i)); err != nil {
if err := setKernelMemory(path, i); err != nil {
return err
}
}
return nil
}

func setKernelMemory(path string, kernelMemoryLimit uint64) error {
func setKernelMemory(path string, kernelMemoryLimit int64) error {
if path == "" {
return fmt.Errorf("no such directory for %s", cgroupKernelMemoryLimit)
}
if !cgroups.PathExists(filepath.Join(path, cgroupKernelMemoryLimit)) {
// kernel memory is not enabled on the system so we should do nothing
return nil
}
if err := ioutil.WriteFile(filepath.Join(path, cgroupKernelMemoryLimit), []byte(strconv.FormatUint(kernelMemoryLimit, 10)), 0700); err != nil {
if err := ioutil.WriteFile(filepath.Join(path, cgroupKernelMemoryLimit), []byte(strconv.FormatInt(kernelMemoryLimit, 10)), 0700); err != nil {
// Check if the error number returned by the syscall is "EBUSY"
// The EBUSY signal is returned on attempts to write to the
// memory.kmem.limit_in_bytes file if the cgroup has children or
Expand All @@ -106,14 +106,12 @@ func setKernelMemory(path string, kernelMemoryLimit uint64) error {
}

func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error {
ulimited := -1

// If the memory update is set to uint64(-1) we should also
// set swap to uint64(-1), it means unlimited memory.
if cgroup.Resources.Memory == uint64(ulimited) {
// Only set swap if it's enbled in kernel
// If the memory update is set to -1 we should also
// set swap to -1, it means unlimited memory.
if cgroup.Resources.Memory == -1 {
// Only set swap if it's enabled in kernel
if cgroups.PathExists(filepath.Join(path, cgroupMemorySwapLimit)) {
cgroup.Resources.MemorySwap = uint64(ulimited)
cgroup.Resources.MemorySwap = -1
}
}

Expand All @@ -128,29 +126,29 @@ func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error {
// When update memory limit, we should adapt the write sequence
// for memory and swap memory, so it won't fail because the new
// value and the old value don't fit kernel's validation.
if cgroup.Resources.MemorySwap == uint64(ulimited) || memoryUsage.Limit < cgroup.Resources.MemorySwap {
if err := writeFile(path, cgroupMemorySwapLimit, strconv.FormatUint(cgroup.Resources.MemorySwap, 10)); err != nil {
if cgroup.Resources.MemorySwap == -1 || memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) {
Copy link
Contributor

@tiborvass tiborvass Jun 24, 2017

Choose a reason for hiding this comment

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

memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) is weird

Copy link
Contributor

@tiborvass tiborvass Jun 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think I understand what's going on: since memoryUsage.Limit is a uint64 (since read from fs), we have to choose between casting that to int64 (no problem since getMemory should not return numbers bigger than 2^63-1), or casting an int64 that's different from -1, to uint64. Would be great to add a comment about the casting.

Which brings up the question: have we ensured that the int64s are never < -1 ?

Copy link
Contributor

@hqhq hqhq Jun 24, 2017

Choose a reason for hiding this comment

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

-1 should be the only valid negative value, but I don't think we should ensure that in runc, cgroup API will return "invalid argument" in this case anyway.

if err := writeFile(path, cgroupMemorySwapLimit, strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
return err
}
if err := writeFile(path, cgroupMemoryLimit, strconv.FormatUint(cgroup.Resources.Memory, 10)); err != nil {
if err := writeFile(path, cgroupMemoryLimit, strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
return err
}
} else {
if err := writeFile(path, cgroupMemoryLimit, strconv.FormatUint(cgroup.Resources.Memory, 10)); err != nil {
if err := writeFile(path, cgroupMemoryLimit, strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
return err
}
if err := writeFile(path, cgroupMemorySwapLimit, strconv.FormatUint(cgroup.Resources.MemorySwap, 10)); err != nil {
if err := writeFile(path, cgroupMemorySwapLimit, strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
return err
}
}
} else {
if cgroup.Resources.Memory != 0 {
if err := writeFile(path, cgroupMemoryLimit, strconv.FormatUint(cgroup.Resources.Memory, 10)); err != nil {
if err := writeFile(path, cgroupMemoryLimit, strconv.FormatInt(cgroup.Resources.Memory, 10)); err != nil {
return err
}
}
if cgroup.Resources.MemorySwap != 0 {
if err := writeFile(path, cgroupMemorySwapLimit, strconv.FormatUint(cgroup.Resources.MemorySwap, 10)); err != nil {
if err := writeFile(path, cgroupMemorySwapLimit, strconv.FormatInt(cgroup.Resources.MemorySwap, 10)); err != nil {
return err
}
}
Expand All @@ -171,13 +169,13 @@ func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error {
}

if cgroup.Resources.MemoryReservation != 0 {
if err := writeFile(path, "memory.soft_limit_in_bytes", strconv.FormatUint(cgroup.Resources.MemoryReservation, 10)); err != nil {
if err := writeFile(path, "memory.soft_limit_in_bytes", strconv.FormatInt(cgroup.Resources.MemoryReservation, 10)); err != nil {
return err
}
}

if cgroup.Resources.KernelMemoryTCP != 0 {
if err := writeFile(path, "memory.kmem.tcp.limit_in_bytes", strconv.FormatUint(cgroup.Resources.KernelMemoryTCP, 10)); err != nil {
if err := writeFile(path, "memory.kmem.tcp.limit_in_bytes", strconv.FormatInt(cgroup.Resources.KernelMemoryTCP, 10)); err != nil {
return err
}
}
Expand Down
10 changes: 5 additions & 5 deletions libcontainer/configs/cgroup_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,19 @@ type Resources struct {
Devices []*Device `json:"devices"`

// Memory limit (in bytes)
Memory uint64 `json:"memory"`
Memory int64 `json:"memory"`

// Memory reservation or soft_limit (in bytes)
MemoryReservation uint64 `json:"memory_reservation"`
MemoryReservation int64 `json:"memory_reservation"`

// Total memory usage (memory + swap); set `-1` to enable unlimited swap
MemorySwap uint64 `json:"memory_swap"`
MemorySwap int64 `json:"memory_swap"`

// Kernel memory limit (in bytes)
KernelMemory uint64 `json:"kernel_memory"`
KernelMemory int64 `json:"kernel_memory"`

// Kernel memory limit for TCP use (in bytes)
KernelMemoryTCP uint64 `json:"kernel_memory_tcp"`
KernelMemoryTCP int64 `json:"kernel_memory_tcp"`

// CPU shares (relative weight vs. other containers)
CpuShares uint64 `json:"cpu_shares"`
Expand Down
5 changes: 0 additions & 5 deletions libcontainer/specconv/example.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package specconv

import (
"os"
"runtime"
"strings"

"github.com/opencontainers/runtime-spec/specs-go"
Expand All @@ -15,10 +14,6 @@ func sPtr(s string) *string { return &s }
func Example() *specs.Spec {
return &specs.Spec{
Version: specs.Version,
Platform: specs.Platform{
OS: runtime.GOOS,
Arch: runtime.GOARCH,
},
Root: specs.Root{
Path: "rootfs",
Readonly: true,
Expand Down
14 changes: 0 additions & 14 deletions spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"io/ioutil"
"os"
"runtime"

"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/specconv"
Expand Down Expand Up @@ -131,9 +130,6 @@ func loadSpec(cPath string) (spec *specs.Spec, err error) {
if err = json.NewDecoder(cf).Decode(&spec); err != nil {
return nil, err
}
if err = validatePlatform(&spec.Platform); err != nil {
return nil, err
}
return spec, validateProcessSpec(spec.Process)
}

Expand All @@ -148,13 +144,3 @@ func createLibContainerRlimit(rlimit specs.LinuxRlimit) (configs.Rlimit, error)
Soft: rlimit.Soft,
}, nil
}

func validatePlatform(platform *specs.Platform) error {
if platform.OS != runtime.GOOS {
return fmt.Errorf("target os %s mismatch with current os %s", platform.OS, runtime.GOOS)
}
if platform.Arch != runtime.GOARCH {
return fmt.Errorf("target arch %s mismatch with current arch %s", platform.Arch, runtime.GOARCH)
}
return nil
}
14 changes: 7 additions & 7 deletions update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,11 @@ other options are ignored.

r := specs.LinuxResources{
Memory: &specs.LinuxMemory{
Limit: u64Ptr(0),
Reservation: u64Ptr(0),
Swap: u64Ptr(0),
Kernel: u64Ptr(0),
KernelTCP: u64Ptr(0),
Limit: i64Ptr(0),
Reservation: i64Ptr(0),
Swap: i64Ptr(0),
Kernel: i64Ptr(0),
KernelTCP: i64Ptr(0),
},
CPU: &specs.LinuxCPU{
Shares: u64Ptr(0),
Expand Down Expand Up @@ -213,7 +213,7 @@ other options are ignored.
}
for _, pair := range []struct {
opt string
dest *uint64
dest *int64
}{
{"memory", r.Memory.Limit},
{"memory-swap", r.Memory.Swap},
Expand All @@ -232,7 +232,7 @@ other options are ignored.
} else {
v = -1
}
*pair.dest = uint64(v)
*pair.dest = v
}
}
r.Pids.Limit = int64(context.Int("pids-limit"))
Expand Down
2 changes: 1 addition & 1 deletion vendor.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# OCI runtime-spec. When updating this, make sure you use a version tag rather
# than a commit ID so it's much more obvious what version of the spec we are
# using.
github.com/opencontainers/runtime-spec 239c4e44f2a612ed85f6db9c66247aa33f437e91
github.com/opencontainers/runtime-spec 198f23f827eea397d4331d7eb048d9d4c7ff7bee
# Core libcontainer functionality.
github.com/mrunalp/fileutils ed869b029674c0e9ce4c0dfa781405c2d9946d08
github.com/opencontainers/selinux v1.0.0-rc1
Expand Down
23 changes: 6 additions & 17 deletions vendor/github.com/opencontainers/runtime-spec/specs-go/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.