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.

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

runc update: fix logic by distinguishing nil from zero #4227

Closed
wants to merge 2 commits into from
Closed

runc update: fix logic by distinguishing nil from zero #4227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d784f69
libct/cg/fs2: skip cpu.weight if cpu.idle is set
kolyshkin Mar 19, 2024
029a767
runc update: fix logic by distinguishing nil from zero
kolyshkin Mar 18, 2024
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
5 changes: 4 additions & 1 deletion libcontainer/cgroups/fs2/cpu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/opencontainers/runc/libcontainer/cgroups/fscommon"
"github.com/opencontainers/runc/libcontainer/configs"
"github.com/sirupsen/logrus"
)

func isCpuSet(r *configs.Resources) bool {
Expand All @@ -30,7 +31,9 @@ func setCpu(dirPath string, r *configs.Resources) error {

// NOTE: .CpuShares is not used here. Conversion is the caller's responsibility.
if r.CpuWeight != 0 {
if err := cgroups.WriteFile(dirPath, "cpu.weight", strconv.FormatUint(r.CpuWeight, 10)); err != nil {
if r.CPUIdle != nil && *r.CPUIdle != 0 {
logrus.Warn("CPU weight can't be set when CPU idle is enabled -- ignoring")
} else if err := cgroups.WriteFile(dirPath, "cpu.weight", strconv.FormatUint(r.CpuWeight, 10)); err != nil {
return err
}
}
Expand Down
55 changes: 53 additions & 2 deletions tests/integration/update.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ function setup() {
fi
fi

# try to remove memory limit
# Remove memory (and swap) limit.
runc update test_update --memory -1
[ "$status" -eq 0 ]

Expand Down Expand Up @@ -332,7 +332,7 @@ EOF
check_cpu_shares 100
}

@test "cpu burst" {
@test "set and update cpu burst" {
[ $EUID -ne 0 ] && requires rootless_cgroup
requires cgroups_cpu_burst

Expand All @@ -344,11 +344,62 @@ EOF
[ "$status" -eq 0 ]
check_cpu_burst 500000

# Check that updating cpu period does not resets CPU burst.
runc update test_update --cpu-period 800000
[ "$status" -eq 0 ]
check_cpu_burst 500000

runc update test_update --cpu-period 900000 --cpu-burst 0
[ "$status" -eq 0 ]
check_cpu_burst 0
}

# https://github.com/opencontainers/runc/issues/4225
@test "updating memory does not reset cpu period with no quota" {
[ $EUID -ne 0 ] && requires rootless_cgroup

update_config '.linux.resources.cpu |= {}'

runc run -d --console-socket "$CONSOLE_SOCKET" test_update
[ "$status" -eq 0 ]
check_cpu_quota -1 100000 "infinity"

runc update test_update --cpu-period 900000
[ "$status" -eq 0 ]
check_cpu_quota -1 900000 "infinity"

runc update test_update --memory 500M
[ "$status" -eq 0 ]
check_cpu_quota -1 900000 "infinity"
}

# https://github.com/opencontainers/runc/issues/4225
@test "updating memory and period resets cpu quota" {
[ $EUID -ne 0 ] && requires rootless_cgroup

update_config '.linux.resources.cpu |= {}'

runc run -d --console-socket "$CONSOLE_SOCKET" test_update
[ "$status" -eq 0 ]
check_cpu_quota -1 100000 "infinity"

runc update test_update --cpu-period 900000 --cpu-quota 504000
[ "$status" -eq 0 ]
check_cpu_quota 504000 900000 "560ms"

runc update test_update --cpu-period 900000
[ "$status" -eq 0 ]
check_cpu_quota 504000 900000 "560ms"

runc update test_update --memory 500M
[ "$status" -eq 0 ]
check_cpu_quota 504000 900000 "560ms"

runc update test_update --cpu-period 900000
[ "$status" -eq 0 ]
check_cpu_quota 504000 900000 "560ms"
}

@test "set cpu period with no quota" {
[ $EUID -ne 0 ] && requires rootless_cgroup

Expand Down
172 changes: 83 additions & 89 deletions update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,6 @@ import (
"github.com/urfave/cli"
)

func i64Ptr(i int64) *int64 { return &i }
func u64Ptr(i uint64) *uint64 { return &i }
func u16Ptr(i uint16) *uint16 { return &i }
func boolPtr(b bool) *bool { return &b }

var updateCommand = cli.Command{
Name: "update",
Usage: "update container resource constraints",
Expand Down Expand Up @@ -147,30 +142,10 @@ other options are ignored.
}

r := specs.LinuxResources{
Memory: &specs.LinuxMemory{
Limit: i64Ptr(0),
Reservation: i64Ptr(0),
Swap: i64Ptr(0),
Kernel: i64Ptr(0),
KernelTCP: i64Ptr(0),
CheckBeforeUpdate: boolPtr(false),
},
CPU: &specs.LinuxCPU{
Shares: u64Ptr(0),
Quota: i64Ptr(0),
Burst: u64Ptr(0),
Period: u64Ptr(0),
RealtimeRuntime: i64Ptr(0),
RealtimePeriod: u64Ptr(0),
Cpus: "",
Mems: "",
},
BlockIO: &specs.LinuxBlockIO{
Weight: u16Ptr(0),
},
Pids: &specs.LinuxPids{
Limit: 0,
},
Memory: &specs.LinuxMemory{},
CPU: &specs.LinuxCPU{},
BlockIO: &specs.LinuxBlockIO{},
Pids: &specs.LinuxPids{},
}

config := container.Config()
Expand All @@ -196,7 +171,8 @@ other options are ignored.
}
} else {
if val := context.Int("blkio-weight"); val != 0 {
r.BlockIO.Weight = u16Ptr(uint16(val))
v := uint16(val)
r.BlockIO.Weight = &v
}
if val := context.String("cpuset-cpus"); val != "" {
r.CPU.Cpus = val
Expand All @@ -209,50 +185,48 @@ other options are ignored.
if err != nil {
return fmt.Errorf("invalid value for cpu-idle: %w", err)
}
r.CPU.Idle = i64Ptr(idle)
r.CPU.Idle = &idle
}

for _, pair := range []struct {
opt string
dest *uint64
dest **uint64
}{
{"cpu-burst", r.CPU.Burst},
{"cpu-period", r.CPU.Period},
{"cpu-rt-period", r.CPU.RealtimePeriod},
{"cpu-share", r.CPU.Shares},
{"cpu-burst", &r.CPU.Burst},
{"cpu-period", &r.CPU.Period},
{"cpu-rt-period", &r.CPU.RealtimePeriod},
{"cpu-share", &r.CPU.Shares},
} {
if val := context.String(pair.opt); val != "" {
var err error
*pair.dest, err = strconv.ParseUint(val, 10, 64)
v, err := strconv.ParseUint(val, 10, 64)
if err != nil {
return fmt.Errorf("invalid value for %s: %w", pair.opt, err)
}
*pair.dest = &v
}
}
for _, pair := range []struct {
opt string
dest *int64
dest **int64
}{
{"cpu-quota", r.CPU.Quota},
{"cpu-rt-runtime", r.CPU.RealtimeRuntime},
{"cpu-quota", &r.CPU.Quota},
{"cpu-rt-runtime", &r.CPU.RealtimeRuntime},
} {
if val := context.String(pair.opt); val != "" {
var err error
*pair.dest, err = strconv.ParseInt(val, 10, 64)
v, err := strconv.ParseInt(val, 10, 64)
if err != nil {
return fmt.Errorf("invalid value for %s: %w", pair.opt, err)
}
*pair.dest = &v
}
}
for _, pair := range []struct {
opt string
dest *int64
dest **int64
}{
{"memory", r.Memory.Limit},
{"memory-swap", r.Memory.Swap},
{"kernel-memory", r.Memory.Kernel}, //nolint:staticcheck // Ignore SA1019. Need to keep deprecated package for compatibility.
{"kernel-memory-tcp", r.Memory.KernelTCP},
{"memory-reservation", r.Memory.Reservation},
{"memory", &r.Memory.Limit},
{"memory-swap", &r.Memory.Swap},
{"memory-reservation", &r.Memory.Reservation},
} {
if val := context.String(pair.opt); val != "" {
var v int64
Expand All @@ -265,60 +239,80 @@ other options are ignored.
} else {
v = -1
}
*pair.dest = v
*pair.dest = &v
}
}
// Ignored (to be removed) options.
for _, opt := range []string{
"kernel-memory",
"kernel-memory-tcp",
} {
if val := context.String(opt); val != "" {
logrus.Warnf("Setting --%s is ignored and will be removed", opt)
}
}

r.Pids.Limit = int64(context.Int("pids-limit"))
}

if *r.Memory.Kernel != 0 || *r.Memory.KernelTCP != 0 { //nolint:staticcheck // Ignore SA1019. Need to keep deprecated package for compatibility.
logrus.Warn("Kernel memory settings are ignored and will be removed")
}
// Merge the new changes from r into existing config.Cgroups.Resources
// (loaded from state.json in getContainer). This is needed because some
// parameters are inter-dependent and thus cgroup drivers need to know
// the old/current values.

// Update the values
config.Cgroups.Resources.BlkioWeight = *r.BlockIO.Weight
if r.BlockIO.Weight != nil {
config.Cgroups.Resources.BlkioWeight = *r.BlockIO.Weight
}

// Setting CPU quota and period independently does not make much sense,
// but historically runc allowed it and this needs to be supported
// to not break compatibility.
//
// For systemd cgroup drivers to set CPU quota/period correctly,
// it needs to know both values. For fs2 cgroup driver to be compatible
// with the fs driver, it also needs to know both values.
//
// Here in update, previously set values are available from config.
// If only one of {quota,period} is set and the other is not, leave
// the unset parameter at the old value (don't overwrite config).
p, q := *r.CPU.Period, *r.CPU.Quota
if (p == 0 && q == 0) || (p != 0 && q != 0) {
// both values are either set or unset (0)
config.Cgroups.Resources.CpuPeriod = p
config.Cgroups.Resources.CpuQuota = q
} else {
// one is set and the other is not
if p != 0 {
// set new period, leave quota at old value
config.Cgroups.Resources.CpuPeriod = p
} else if q != 0 {
// set new quota, leave period at old value
config.Cgroups.Resources.CpuQuota = q
}
// to not break compatibility. Cgroup drivers implement default values
// for both period and quota.
if r.CPU.Period != nil {
config.Cgroups.Resources.CpuPeriod = *r.CPU.Period
}
if r.CPU.Quota != nil {
config.Cgroups.Resources.CpuQuota = *r.CPU.Quota
}

config.Cgroups.Resources.CpuBurst = r.CPU.Burst
config.Cgroups.Resources.CpuShares = *r.CPU.Shares
// CpuWeight is used for cgroupv2 and should be converted
config.Cgroups.Resources.CpuWeight = cgroups.ConvertCPUSharesToCgroupV2Value(*r.CPU.Shares)
config.Cgroups.Resources.CpuRtPeriod = *r.CPU.RealtimePeriod
config.Cgroups.Resources.CpuRtRuntime = *r.CPU.RealtimeRuntime
if r.CPU.Burst != nil {
config.Cgroups.Resources.CpuBurst = r.CPU.Burst
}
if r.CPU.Shares != nil {
config.Cgroups.Resources.CpuShares = *r.CPU.Shares
// CpuWeight is used for cgroupv2 and should be converted
config.Cgroups.Resources.CpuWeight = cgroups.ConvertCPUSharesToCgroupV2Value(*r.CPU.Shares)
}
if r.CPU.RealtimePeriod != nil {
config.Cgroups.Resources.CpuRtPeriod = *r.CPU.RealtimePeriod
}
if r.CPU.RealtimeRuntime != nil {
config.Cgroups.Resources.CpuRtRuntime = *r.CPU.RealtimeRuntime
}
config.Cgroups.Resources.CpusetCpus = r.CPU.Cpus
config.Cgroups.Resources.CpusetMems = r.CPU.Mems
config.Cgroups.Resources.Memory = *r.Memory.Limit
config.Cgroups.Resources.CPUIdle = r.CPU.Idle
config.Cgroups.Resources.MemoryReservation = *r.Memory.Reservation
config.Cgroups.Resources.MemorySwap = *r.Memory.Swap
config.Cgroups.Resources.MemoryCheckBeforeUpdate = *r.Memory.CheckBeforeUpdate
if r.Memory.Limit != nil {
config.Cgroups.Resources.Memory = *r.Memory.Limit
// For compatibility with cgroup v1, set swap to unlimited in case
// the memory is set to unlimited, and swap is not explicitly set,
// treating the request as "set both memory and swap to unlimited".
if *r.Memory.Limit == -1 && r.Memory.Swap == nil {
swap := int64(-1)
r.Memory.Swap = &swap
}
}
if r.CPU.Idle != nil {
config.Cgroups.Resources.CPUIdle = r.CPU.Idle
}
if r.Memory.Reservation != nil {
config.Cgroups.Resources.MemoryReservation = *r.Memory.Reservation
}
if r.Memory.Swap != nil {
config.Cgroups.Resources.MemorySwap = *r.Memory.Swap
}
if r.Memory.CheckBeforeUpdate != nil {
config.Cgroups.Resources.MemoryCheckBeforeUpdate = *r.Memory.CheckBeforeUpdate
}
config.Cgroups.Resources.PidsLimit = r.Pids.Limit
config.Cgroups.Resources.Unified = r.Unified

Expand Down
Loading