From e751a168dca2aaf05d12bcbea2f6c55dbeb84c2c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 9 Jun 2020 19:19:46 -0700 Subject: [PATCH] cgroups/systemd: add setting CPUQuotaPeriod prop For some reason, runc systemd drivers (both v1 and v2) never set systemd unit property named `CPUQuotaPeriod` (known as `CPUQuotaPeriodUSec` on dbus and in `systemctl show` output). Set it, and add a check to all the integration tests. The check is less than trivial because, when not set, the value is shown as "infinity" but when set to the same (default) value, shown as "100ms", so in case we expect 100ms (period = 100000 us), we have to _also_ check for "infinity". [v2: add systemd version checks since CPUQuotaPeriod requires v242+] Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/systemd/common.go | 50 +++++++++++++++++++++++--- libcontainer/cgroups/systemd/v1.go | 25 +++++++------ libcontainer/cgroups/systemd/v2.go | 25 +++++++------ tests/integration/helpers.bash | 21 ++++++++--- tests/integration/update.bats | 12 ++++++- 5 files changed, 96 insertions(+), 37 deletions(-) diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go index 5c00c0bb..cdf656b8 100644 --- a/libcontainer/cgroups/systemd/common.go +++ b/libcontainer/cgroups/systemd/common.go @@ -5,6 +5,7 @@ import ( "fmt" "math" "os" + "strconv" "strings" "sync" "time" @@ -21,6 +22,10 @@ var ( connOnce sync.Once connDbus *systemdDbus.Conn connErr error + + versionOnce sync.Once + version int + versionErr error ) // NOTE: This function comes from package github.com/coreos/go-systemd/util @@ -351,21 +356,56 @@ func stopUnit(dbusConnection *systemdDbus.Conn, unitName string) error { return nil } -func addCpuQuota(properties *[]systemdDbus.Property, quota int64, period uint64) { +func systemdVersion(conn *systemdDbus.Conn) (int, error) { + versionOnce.Do(func() { + version = -1 + verStr, err := conn.GetManagerProperty("Version") + if err != nil { + versionErr = err + return + } + + // verStr is like "v245.4-1.fc32" (including quotes) + if !strings.HasPrefix(verStr, `"v`) { + versionErr = fmt.Errorf("can't parse version %s", verStr) + return + } + // remove `"v` prefix and everything after the first dot + ver, err := strconv.Atoi(strings.SplitN(verStr[2:], ".", 2)[0]) + if err != nil { + versionErr = err + } + version = ver + }) + + return version, versionErr +} + +func addCpuQuota(conn *systemdDbus.Conn, properties *[]systemdDbus.Property, quota int64, period uint64) { + if period != 0 { + // systemd only supports CPUQuotaPeriodUSec since v242 + sdVer, err := systemdVersion(conn) + if err != nil { + logrus.Warnf("systemdVersion: %s", err) + } else if sdVer >= 242 { + *properties = append(*properties, + newProp("CPUQuotaPeriodUSec", period)) + } + } if quota != 0 || period != 0 { // corresponds to USEC_INFINITY in systemd cpuQuotaPerSecUSec := uint64(math.MaxUint64) if quota > 0 { - // systemd converts CPUQuotaPerSecUSec (microseconds per CPU second) to CPUQuota - // (integer percentage of CPU) internally. This means that if a fractional percent of - // CPU is indicated by Resources.CpuQuota, we need to round up to the nearest - // 10ms (1% of a second) such that child cgroups can set the cpu.cfs_quota_us they expect. if period == 0 { // assume the default kernel value of 100000 us (100 ms), same for v1 and v2. // v1: https://www.kernel.org/doc/html/latest/scheduler/sched-bwc.html and // v2: https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html period = 100000 } + // systemd converts CPUQuotaPerSecUSec (microseconds per CPU second) to CPUQuota + // (integer percentage of CPU) internally. This means that if a fractional percent of + // CPU is indicated by Resources.CpuQuota, we need to round up to the nearest + // 10ms (1% of a second) such that child cgroups can set the cpu.cfs_quota_us they expect. cpuQuotaPerSecUSec = uint64(quota*1000000) / period if cpuQuotaPerSecUSec%10000 != 0 { cpuQuotaPerSecUSec = ((cpuQuotaPerSecUSec / 10000) + 1) * 10000 diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index b3e91f30..c4d5e526 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -68,7 +68,7 @@ var legacySubsystems = subsystemSet{ &fs.NameGroup{GroupName: "name=systemd"}, } -func genV1ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) { +func genV1ResourcesProperties(c *configs.Cgroup, conn *systemdDbus.Conn) ([]systemdDbus.Property, error) { var properties []systemdDbus.Property r := c.Resources @@ -88,7 +88,7 @@ func genV1ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) newProp("CPUShares", r.CpuShares)) } - addCpuQuota(&properties, r.CpuQuota, r.CpuPeriod) + addCpuQuota(conn, &properties, r.CpuQuota, r.CpuPeriod) if r.BlkioWeight != 0 { properties = append(properties, @@ -165,7 +165,11 @@ func (m *legacyManager) Apply(pid int) error { properties = append(properties, newProp("DefaultDependencies", false)) - resourcesProperties, err := genV1ResourcesProperties(c) + dbusConnection, err := getDbusConnection(false) + if err != nil { + return err + } + resourcesProperties, err := genV1ResourcesProperties(c, dbusConnection) if err != nil { return err } @@ -180,10 +184,6 @@ func (m *legacyManager) Apply(pid int) error { } } - dbusConnection, err := getDbusConnection(false) - if err != nil { - return err - } if err := startUnit(dbusConnection, unitName, properties); err != nil { return err } @@ -368,7 +368,11 @@ func (m *legacyManager) Set(container *configs.Config) error { if m.cgroups.Paths != nil { return nil } - properties, err := genV1ResourcesProperties(container.Cgroups) + dbusConnection, err := getDbusConnection(false) + if err != nil { + return err + } + properties, err := genV1ResourcesProperties(container.Cgroups, dbusConnection) if err != nil { return err } @@ -393,11 +397,6 @@ func (m *legacyManager) Set(container *configs.Config) error { logrus.Infof("freeze container before SetUnitProperties failed: %v", err) } - dbusConnection, err := getDbusConnection(false) - if err != nil { - _ = m.Freeze(targetFreezerState) - return err - } if err := dbusConnection.SetUnitProperties(getUnitName(container.Cgroups), true, properties...); err != nil { _ = m.Freeze(targetFreezerState) return err diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index d11b8abb..c44d7734 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -34,7 +34,7 @@ func NewUnifiedManager(config *configs.Cgroup, path string, rootless bool) cgrou } } -func genV2ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) { +func genV2ResourcesProperties(c *configs.Cgroup, conn *systemdDbus.Conn) ([]systemdDbus.Property, error) { var properties []systemdDbus.Property r := c.Resources @@ -72,7 +72,7 @@ func genV2ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) newProp("CPUWeight", r.CpuWeight)) } - addCpuQuota(&properties, r.CpuQuota, r.CpuPeriod) + addCpuQuota(conn, &properties, r.CpuQuota, r.CpuPeriod) if r.PidsLimit > 0 || r.PidsLimit == -1 { properties = append(properties, @@ -136,17 +136,17 @@ func (m *unifiedManager) Apply(pid int) error { properties = append(properties, newProp("DefaultDependencies", false)) - resourcesProperties, err := genV2ResourcesProperties(c) + dbusConnection, err := getDbusConnection(m.rootless) + if err != nil { + return err + } + resourcesProperties, err := genV2ResourcesProperties(c, dbusConnection) if err != nil { return err } properties = append(properties, resourcesProperties...) properties = append(properties, c.SystemdProps...) - dbusConnection, err := getDbusConnection(m.rootless) - if err != nil { - return err - } if err := startUnit(dbusConnection, unitName, properties); err != nil { return errors.Wrapf(err, "error while starting unit %q with properties %+v", unitName, properties) } @@ -289,7 +289,11 @@ func (m *unifiedManager) GetStats() (*cgroups.Stats, error) { } func (m *unifiedManager) Set(container *configs.Config) error { - properties, err := genV2ResourcesProperties(m.cgroups) + dbusConnection, err := getDbusConnection(m.rootless) + if err != nil { + return err + } + properties, err := genV2ResourcesProperties(m.cgroups, dbusConnection) if err != nil { return err } @@ -314,11 +318,6 @@ func (m *unifiedManager) Set(container *configs.Config) error { logrus.Infof("freeze container before SetUnitProperties failed: %v", err) } - dbusConnection, err := getDbusConnection(m.rootless) - if err != nil { - _ = m.Freeze(targetFreezerState) - return err - } if err := dbusConnection.SetUnitProperties(getUnitName(m.cgroups), true, properties...); err != nil { _ = m.Freeze(targetFreezerState) return errors.Wrap(err, "error while setting unit properties") diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index 5db99320..7cf3580a 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -105,6 +105,16 @@ function runc_rootless_cgroup() { update_config '.linux.resources += {"memory":{},"cpu":{},"blockio":{},"pids":{}}' $bundle } +# Returns systemd version as a number (-1 if systemd is not enabled/supported). +function systemd_version() { + if [ -n "${RUNC_USE_SYSTEMD}" ]; then + systemctl --version | awk '/^systemd / {print $2; exit}' + return + fi + + echo "-1" +} + function init_cgroup_paths() { # init once test -n "$CGROUP_UNIFIED" && return @@ -180,15 +190,16 @@ function check_cgroup_value() { # Helper to check a value in systemd. function check_systemd_value() { [ -z "${RUNC_USE_SYSTEMD}" ] && return - source=$1 + local source=$1 [ "$source" = "unsupported" ] && return - expected="$2" - user="" + local expected="$2" + local expected2="$3" + local user="" [ $(id -u) != "0" ] && user="--user" current=$(systemctl show $user --property $source $SD_UNIT_NAME | awk -F= '{print $2}') - echo "systemd $source: current $current !? $expected" - [ "$current" = "$expected" ] + echo "systemd $source: current $current !? $expected $expected2" + [ "$current" = "$expected" ] || [ -n "$expected2" -a "$current" = "$expected2" ] } # Helper function to set a resources limit diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 261ed99c..72902f4a 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -250,8 +250,18 @@ function check_cpu_quota() { check_cgroup_value "cpu.cfs_quota_us" $quota check_cgroup_value "cpu.cfs_period_us" $period fi - # systemd value is the same for v1 and v2 + # systemd values are the same for v1 and v2 check_systemd_value "CPUQuotaPerSecUSec" $sd_quota + + # CPUQuotaPeriodUSec requires systemd >= v242 + [ "$(systemd_version)" -lt 242 ] && return + + local sd_period=$(( period/1000 ))ms + [ "$sd_period" = "1000ms" ] && sd_period="1s" + local sd_infinity="" + # 100ms is the default value, and if not set, shown as infinity + [ "$sd_period" = "100ms" ] && sd_infinity="infinity" + check_systemd_value "CPUQuotaPeriodUSec" $sd_period $sd_infinity } function check_cpu_shares() {