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

Add mems, cpus check and move common codes into utils #344

Closed
wants to merge 2 commits into from
Closed

Add mems, cpus check and move common codes into utils #344

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
ef48dee
generate: add param check for mems and cpus
Mar 10, 2017
c94e16e
move common codes to uitls
Mar 10, 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
39 changes: 39 additions & 0 deletions cmd/oci-runtime-tool/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
rspec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/runtime-tools/generate"
"github.com/opencontainers/runtime-tools/generate/seccomp"
"github.com/opencontainers/runtime-tools/utils"
"github.com/urfave/cli"
)

Expand Down Expand Up @@ -495,6 +496,9 @@ func setupSpec(g *generate.Generator, context *cli.Context) error {
}

if context.IsSet("linux-cpus") {
if err := utils.UnitListValid(context.String("linux-cpus")); err != nil {
return err
}
g.SetLinuxResourcesCPUCpus(context.String("linux-cpus"))
}

Expand All @@ -517,6 +521,9 @@ func setupSpec(g *generate.Generator, context *cli.Context) error {
}

if context.IsSet("linux-mems") {
if err := utils.UnitListValid(context.String("linux-mems")); err != nil {
return err
}
g.SetLinuxResourcesCPUMems(context.String("linux-mems"))
}

Expand Down Expand Up @@ -662,6 +669,38 @@ func parseConsoleSize(consoleSize string) (uint, uint, error) {
return uint(width), uint(height), nil
}

func uintListValid(val string) error {
if val == "" {
return nil
}

split := strings.Split(val, ",")
errInvalidFormat := fmt.Errorf("invalid format: %s", val)

for _, r := range split {
if !strings.Contains(r, "-") {
_, err := strconv.Atoi(r)
if err != nil {
return errInvalidFormat
}
} else {
split := strings.SplitN(r, "-", 2)
min, err := strconv.Atoi(split[0])
if err != nil {
return errInvalidFormat
}
max, err := strconv.Atoi(split[1])
if err != nil {
return errInvalidFormat
}
if max < min {
return errInvalidFormat
}
}
}
return nil
}

func parseIDMapping(idms string) (uint32, uint32, uint32, error) {
idm := strings.Split(idms, ":")
if len(idm) != 3 {
Expand Down
7 changes: 2 additions & 5 deletions cmd/runtimetest/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

"github.com/opencontainers/runtime-tools/cmd/runtimetest/mount"
rfc2119 "github.com/opencontainers/runtime-tools/error"
"github.com/opencontainers/runtime-tools/utils"
"github.com/opencontainers/runtime-tools/validate"
)

Expand Down Expand Up @@ -162,11 +163,7 @@ func validateLinuxProcess(spec *rspec.Spec) error {
}

func validateCapabilities(spec *rspec.Spec) error {
last := capability.CAP_LAST_CAP
// workaround for RHEL6 which has no /proc/sys/kernel/cap_last_cap
if last == capability.Cap(63) {
last = capability.CAP_BLOCK_SUSPEND
}
last := utils.LastCap()

processCaps, err := capability.NewPid(0)
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion generate/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

rspec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/runtime-tools/generate/seccomp"
"github.com/opencontainers/runtime-tools/utils"
"github.com/opencontainers/runtime-tools/validate"
"github.com/syndtr/gocapability/capability"
)
Expand Down Expand Up @@ -949,7 +950,7 @@ func (g *Generator) SetupPrivileged(privileged bool) {
if privileged { // Add all capabilities in privileged mode.
var finalCapList []string
for _, cap := range capability.List() {
if g.HostSpecific && cap > validate.LastCap() {
if g.HostSpecific && cap > utils.LastCap() {
continue
}
finalCapList = append(finalCapList, fmt.Sprintf("CAP_%s", strings.ToUpper(cap.String())))
Expand Down
59 changes: 59 additions & 0 deletions utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package utils

import (
"fmt"
"strconv"
"strings"

"github.com/syndtr/gocapability/capability"
)

// LastCap return last cap of system
func LastCap() capability.Cap {
last := capability.CAP_LAST_CAP
// hack for RHEL6 which has no /proc/sys/kernel/cap_last_cap
if last == capability.Cap(63) {
last = capability.CAP_BLOCK_SUSPEND
}

return last
}

// UnitListValid checks strings whether is valid for
// cpuset.cpus and cpuset.mems, duplicates are allowed
// Supported formats:
// 1
// 0-3
// 0-2,1,3
// 0-2,1-3,4
Copy link
Contributor

Choose a reason for hiding this comment

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

These validity checks are not backed by the spec, and since opencontainers/runtime-spec#780 landed I think the spec is unlikely to add them in the future. Do runtime-tools maintainers intend to reach past runtime-spec and enforce limits that have been silently punted to the kernel (runtime-spec doesn't even have “MUST be a valid value for kernel interface [link]”)? If so, we'll need a validation level for “kernel limits not covered by runtime-spec”. If not, I think we can drop UnitListValid.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I don't know the history very well. Why do we remove these limits? These parameters are very related with kernel, if they are not accepted by kernel, that makes no sense, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we remove these limits? These parameters are very related with kernel, if they are not accepted by kernel, that makes no sense, right?

My impression is that runtime-spec maintainers feel that inlining kernel limits isn't particularly useful. The spec itself has no opinion on these limits, and just punts to the kernel (although I think the wording for the punts still needs work, e.g. opencontainers/runtime-spec#746).

But you could write a config with foo-bar as your linux.resources.cpu.cpus value, it would be valid vs. runtime-spec, and if you had a very strange kernel it would actually work for creating a new container. As far as runtime-spec (and our spec-targeting validation here) is concerned, that would be fine. Luckily, the kernel APIs are fairly stable, so config-authors don't have to worry too much about which kernel they're targeting when they figure out these values. And again, if we want to inline these checks with a “kernel limits not covered by runtime-spec” level, I'm fine with that.

func UnitListValid(val string) error {
if val == "" {
return nil
}

split := strings.Split(val, ",")
errInvalidFormat := fmt.Errorf("invalid format: %s", val)

for _, r := range split {
if !strings.Contains(r, "-") {
_, err := strconv.Atoi(r)
if err != nil {
return errInvalidFormat
}
} else {
split := strings.SplitN(r, "-", 2)
min, err := strconv.Atoi(split[0])
if err != nil {
return errInvalidFormat
}
max, err := strconv.Atoi(split[1])
if err != nil {
return errInvalidFormat
}
if max < min {
return errInvalidFormat
}
}
}
return nil
}
29 changes: 17 additions & 12 deletions validate/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (

"github.com/blang/semver"
rspec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/runtime-tools/utils"
"github.com/sirupsen/logrus"
"github.com/syndtr/gocapability/capability"
)
Expand Down Expand Up @@ -633,6 +634,20 @@ func (v *Validator) CheckLinuxResources() (msgs []string) {
logrus.Debugf("check linux resources")

r := v.spec.Linux.Resources

if r.CPU != nil {
if r.CPU.Cpus != "" {
if err := utils.UnitListValid(r.CPU.Cpus); err != nil {
msgs = append(msgs, err.Error())
Copy link
Contributor

@wking wking Sep 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this check (or your following mems check) is grounded in the spec, but if the runtime-tools maintainer consensus is that it is (or if they just want “unlikely to be supported by the kernel” warnings), then the validate/ changes from this PR belong in v0.3.0. I've also made a similar suggestion for #386 here, in case runtime-tools maintainers want to handle both instances the same way.

}
}
if r.CPU.Mems != "" {
if err := utils.UnitListValid(r.CPU.Mems); err != nil {
msgs = append(msgs, err.Error())
}
}
}

if r.Memory != nil {
if r.Memory.Limit != nil && r.Memory.Swap != nil && uint64(*r.Memory.Limit) > uint64(*r.Memory.Swap) {
msgs = append(msgs, fmt.Sprintf("Minimum memoryswap should be larger than memory limit"))
Expand All @@ -641,6 +656,7 @@ func (v *Validator) CheckLinuxResources() (msgs []string) {
msgs = append(msgs, fmt.Sprintf("Minimum memory limit should be larger than memory reservation"))
}
}

if r.Network != nil && v.HostSpecific {
var exist bool
interfaces, err := net.Interfaces()
Expand Down Expand Up @@ -715,7 +731,7 @@ func CapValid(c string, hostSpecific bool) error {
}
for _, cap := range capability.List() {
if c == fmt.Sprintf("CAP_%s", strings.ToUpper(cap.String())) {
if hostSpecific && cap > LastCap() {
if hostSpecific && cap > utils.LastCap() {
return fmt.Errorf("CAP_%s is not supported on the current host", c)
}
isValid = true
Expand All @@ -729,17 +745,6 @@ func CapValid(c string, hostSpecific bool) error {
return nil
}

// LastCap return last cap of system
func LastCap() capability.Cap {
last := capability.CAP_LAST_CAP
// hack for RHEL6 which has no /proc/sys/kernel/cap_last_cap
if last == capability.Cap(63) {
last = capability.CAP_BLOCK_SUSPEND
}

return last
}

func envValid(env string) bool {
items := strings.Split(env, "=")
if len(items) < 2 {
Expand Down