From c51c593ff630719d9d897319a7afabf412580d68 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 16 Jun 2020 17:32:01 -0400 Subject: Re-add resource limit warnings to Specgen These were part of Podman v1.9, but were lost in the transition to using Specgen to create containers. Most resource limits are checked via the sysinfo package to ensure they are safe to use (the cgroup is mounted, kernel support is present, etc) and removed if not safe. Further, bounds checks are performed to ensure that values are valid. Ensure these warnings are printed client-side when they occur. This part is a little bit gross, as it happens in pkg/infra and not cmd/podman, which is largely down to how we implemented `podman run` - all the work is done in pkg/infra and it returns only once the container has exited, and we need warnings to print *before* the container runs. The solution here, while inelegant, avoid the need to extensively refactor our handling of run. Should fix blkio-limit warnings that were identified by the FCOS test suite. Signed-off-by: Matthew Heon --- cmd/podman/common/specgen.go | 2 - pkg/api/handlers/libpod/containers_create.go | 5 +- pkg/domain/infra/abi/containers.go | 14 ++- pkg/domain/infra/tunnel/containers.go | 6 + pkg/specgen/generate/container.go | 159 +++++++++++++++------------ pkg/specgen/generate/container_create.go | 4 +- pkg/specgen/generate/validate.go | 159 +++++++++++++++++++++++++++ 7 files changed, 270 insertions(+), 79 deletions(-) create mode 100644 pkg/specgen/generate/validate.go diff --git a/cmd/podman/common/specgen.go b/cmd/podman/common/specgen.go index 0b44ef544..2a070710a 100644 --- a/cmd/podman/common/specgen.go +++ b/cmd/podman/common/specgen.go @@ -635,8 +635,6 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *ContainerCLIOpts, args []string s.Remove = c.Rm s.StopTimeout = &c.StopTimeout - // TODO where should we do this? - // func verifyContainerResources(config *cc.CreateConfig, update bool) ([]string, error) { return nil } diff --git a/pkg/api/handlers/libpod/containers_create.go b/pkg/api/handlers/libpod/containers_create.go index 71f440bce..8fbff9be7 100644 --- a/pkg/api/handlers/libpod/containers_create.go +++ b/pkg/api/handlers/libpod/containers_create.go @@ -22,7 +22,8 @@ func CreateContainer(w http.ResponseWriter, r *http.Request) { utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "Decode()")) return } - if err := generate.CompleteSpec(r.Context(), runtime, &sg); err != nil { + warn, err := generate.CompleteSpec(r.Context(), runtime, &sg) + if err != nil { utils.InternalServerError(w, err) return } @@ -31,6 +32,6 @@ func CreateContainer(w http.ResponseWriter, r *http.Request) { utils.InternalServerError(w, err) return } - response := entities.ContainerCreateResponse{ID: ctr.ID()} + response := entities.ContainerCreateResponse{ID: ctr.ID(), Warnings: warn} utils.WriteJSON(w, http.StatusCreated, response) } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index d2c8aefdc..da9be0946 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -511,9 +511,14 @@ func (ic *ContainerEngine) ContainerRestore(ctx context.Context, namesOrIds []st } func (ic *ContainerEngine) ContainerCreate(ctx context.Context, s *specgen.SpecGenerator) (*entities.ContainerCreateReport, error) { - if err := generate.CompleteSpec(ctx, ic.Libpod, s); err != nil { + warn, err := generate.CompleteSpec(ctx, ic.Libpod, s) + if err != nil { return nil, err } + // Print warnings + for _, w := range warn { + fmt.Fprintf(os.Stderr, "%s\n", w) + } ctr, err := generate.MakeContainer(ctx, ic.Libpod, s) if err != nil { return nil, err @@ -773,9 +778,14 @@ func (ic *ContainerEngine) ContainerDiff(ctx context.Context, nameOrID string, o } func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.ContainerRunOptions) (*entities.ContainerRunReport, error) { - if err := generate.CompleteSpec(ctx, ic.Libpod, opts.Spec); err != nil { + warn, err := generate.CompleteSpec(ctx, ic.Libpod, opts.Spec) + if err != nil { return nil, err } + // Print warnings + for _, w := range warn { + fmt.Fprintf(os.Stderr, "%s\n", w) + } ctr, err := generate.MakeContainer(ctx, ic.Libpod, opts.Spec) if err != nil { return nil, err diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index 955149fcf..a69c6548c 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/pkg/domain/infra/tunnel/containers.go @@ -340,6 +340,9 @@ func (ic *ContainerEngine) ContainerCreate(ctx context.Context, s *specgen.SpecG if err != nil { return nil, err } + for _, w := range response.Warnings { + fmt.Fprintf(os.Stderr, "%s\n", w) + } return &entities.ContainerCreateReport{Id: response.ID}, nil } @@ -497,6 +500,9 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta if err != nil { return nil, err } + for _, w := range con.Warnings { + fmt.Fprintf(os.Stderr, "%s\n", w) + } report := entities.ContainerRunReport{Id: con.ID} // Attach if !opts.Detach { diff --git a/pkg/specgen/generate/container.go b/pkg/specgen/generate/container.go index 3d70571d5..df27f225b 100644 --- a/pkg/specgen/generate/container.go +++ b/pkg/specgen/generate/container.go @@ -5,6 +5,7 @@ import ( "github.com/containers/image/v5/manifest" "github.com/containers/libpod/libpod" + "github.com/containers/libpod/libpod/image" ann "github.com/containers/libpod/pkg/annotations" envLib "github.com/containers/libpod/pkg/env" "github.com/containers/libpod/pkg/signal" @@ -13,91 +14,103 @@ import ( "golang.org/x/sys/unix" ) -func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerator) error { - // If a rootfs is used, then there is no image data - if s.ContainerStorageConfig.Rootfs != "" { - return nil - } - - newImage, err := r.ImageRuntime().NewFromLocal(s.Image) - if err != nil { - return err - } - - _, mediaType, err := newImage.Manifest(ctx) - if err != nil { - return err - } - - if s.HealthConfig == nil && mediaType == manifest.DockerV2Schema2MediaType { - s.HealthConfig, err = newImage.GetHealthCheck(ctx) +// Fill any missing parts of the spec generator (e.g. from the image). +// Returns a set of warnings or any fatal error that occurred. +func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerator) ([]string, error) { + var ( + newImage *image.Image + err error + ) + + // Only add image configuration if we have an image + if s.Image != "" { + newImage, err = r.ImageRuntime().NewFromLocal(s.Image) if err != nil { - return err + return nil, err } - } - // Image stop signal - if s.StopSignal == nil { - stopSignal, err := newImage.StopSignal(ctx) + _, mediaType, err := newImage.Manifest(ctx) if err != nil { - return err + return nil, err } - if stopSignal != "" { - sig, err := signal.ParseSignalNameOrNumber(stopSignal) + + if s.HealthConfig == nil && mediaType == manifest.DockerV2Schema2MediaType { + s.HealthConfig, err = newImage.GetHealthCheck(ctx) if err != nil { - return err + return nil, err + } + } + + // Image stop signal + if s.StopSignal == nil { + stopSignal, err := newImage.StopSignal(ctx) + if err != nil { + return nil, err + } + if stopSignal != "" { + sig, err := signal.ParseSignalNameOrNumber(stopSignal) + if err != nil { + return nil, err + } + s.StopSignal = &sig } - s.StopSignal = &sig } } rtc, err := r.GetConfig() if err != nil { - return err + return nil, err } // Get Default Environment defaultEnvs, err := envLib.ParseSlice(rtc.Containers.Env) if err != nil { - return errors.Wrap(err, "Env fields in containers.conf failed to parse") + return nil, errors.Wrap(err, "Env fields in containers.conf failed to parse") } - // Image envs from the image if they don't exist - // already, overriding the default environments - imageEnvs, err := newImage.Env(ctx) - if err != nil { - return err - } + var envs map[string]string - envs, err := envLib.ParseSlice(imageEnvs) - if err != nil { - return errors.Wrap(err, "Env fields from image failed to parse") - } - s.Env = envLib.Join(envLib.Join(defaultEnvs, envs), s.Env) + if newImage != nil { + // Image envs from the image if they don't exist + // already, overriding the default environments + imageEnvs, err := newImage.Env(ctx) + if err != nil { + return nil, err + } - labels, err := newImage.Labels(ctx) - if err != nil { - return err + envs, err = envLib.ParseSlice(imageEnvs) + if err != nil { + return nil, errors.Wrap(err, "Env fields from image failed to parse") + } } - // labels from the image that dont exist already - if len(labels) > 0 && s.Labels == nil { - s.Labels = make(map[string]string) - } - for k, v := range labels { - if _, exists := s.Labels[k]; !exists { - s.Labels[k] = v + s.Env = envLib.Join(envLib.Join(defaultEnvs, envs), s.Env) + + // Labels and Annotations + annotations := make(map[string]string) + if newImage != nil { + labels, err := newImage.Labels(ctx) + if err != nil { + return nil, err } - } - // annotations + // labels from the image that dont exist already + if len(labels) > 0 && s.Labels == nil { + s.Labels = make(map[string]string) + } + for k, v := range labels { + if _, exists := s.Labels[k]; !exists { + s.Labels[k] = v + } + } - // Add annotations from the image - annotations, err := newImage.Annotations(ctx) - if err != nil { - return err - } - for k, v := range annotations { - annotations[k] = v + // Add annotations from the image + imgAnnotations, err := newImage.Annotations(ctx) + if err != nil { + return nil, err + } + for k, v := range imgAnnotations { + annotations[k] = v + } } // in the event this container is in a pod, and the pod has an infra container @@ -121,40 +134,42 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat s.Annotations = annotations // workdir - workingDir, err := newImage.WorkingDir(ctx) - if err != nil { - return err - } - if len(s.WorkDir) < 1 && len(workingDir) > 1 { - s.WorkDir = workingDir + if newImage != nil { + workingDir, err := newImage.WorkingDir(ctx) + if err != nil { + return nil, err + } + if len(s.WorkDir) < 1 && len(workingDir) > 1 { + s.WorkDir = workingDir + } } if len(s.SeccompProfilePath) < 1 { p, err := libpod.DefaultSeccompPath() if err != nil { - return err + return nil, err } s.SeccompProfilePath = p } - if len(s.User) == 0 { + if len(s.User) == 0 && newImage != nil { s.User, err = newImage.User(ctx) if err != nil { - return err + return nil, err } } if err := finishThrottleDevices(s); err != nil { - return err + return nil, err } // Unless already set via the CLI, check if we need to disable process // labels or set the defaults. if len(s.SelinuxOpts) == 0 { if err := setLabelOpts(s, r, s.PidNS, s.IpcNS); err != nil { - return err + return nil, err } } - return nil + return verifyContainerResources(s) } // finishThrottleDevices takes the temporary representation of the throttle diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 33075b543..869601e93 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -16,7 +16,9 @@ import ( "github.com/sirupsen/logrus" ) -// MakeContainer creates a container based on the SpecGenerator +// MakeContainer creates a container based on the SpecGenerator. +// Returns the created, container and any warnings resulting from creating the +// container, or an error. func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGenerator) (*libpod.Container, error) { rtc, err := rt.GetConfig() if err != nil { diff --git a/pkg/specgen/generate/validate.go b/pkg/specgen/generate/validate.go new file mode 100644 index 000000000..bb3ca9907 --- /dev/null +++ b/pkg/specgen/generate/validate.go @@ -0,0 +1,159 @@ +package generate + +import ( + "github.com/containers/common/pkg/sysinfo" + "github.com/containers/libpod/pkg/cgroups" + "github.com/containers/libpod/pkg/specgen" + "github.com/pkg/errors" +) + +// Verify resource limits are sanely set, removing any limits that are not +// possible with the current cgroups config. +func verifyContainerResources(s *specgen.SpecGenerator) ([]string, error) { + warnings := []string{} + + cgroup2, err := cgroups.IsCgroup2UnifiedMode() + if err != nil || cgroup2 { + return warnings, err + } + + sysInfo := sysinfo.New(true) + + if s.ResourceLimits == nil { + return warnings, nil + } + + // Memory checks + if s.ResourceLimits.Memory != nil { + memory := s.ResourceLimits.Memory + if memory.Limit != nil && !sysInfo.MemoryLimit { + warnings = append(warnings, "Your kernel does not support memory limit capabilities or the cgroup is not mounted. Limitation discarded.") + memory.Limit = nil + memory.Swap = nil + } + if memory.Limit != nil && memory.Swap != nil && !sysInfo.SwapLimit { + warnings = append(warnings, "Your kernel does not support swap limit capabilities,or the cgroup is not mounted. Memory limited without swap.") + memory.Swap = nil + } + if memory.Limit != nil && memory.Swap != nil && *memory.Swap < *memory.Limit { + return warnings, errors.New("minimum memoryswap limit should be larger than memory limit, see usage") + } + if memory.Limit == nil && memory.Swap != nil { + return warnings, errors.New("you should always set a memory limit when using a memoryswap limit, see usage") + } + if memory.Swappiness != nil { + if !sysInfo.MemorySwappiness { + warnings = append(warnings, "Your kernel does not support memory swappiness capabilities, or the cgroup is not mounted. Memory swappiness discarded.") + memory.Swappiness = nil + } else { + if *memory.Swappiness < 0 || *memory.Swappiness > 100 { + return warnings, errors.Errorf("invalid value: %v, valid memory swappiness range is 0-100", *memory.Swappiness) + } + } + } + if memory.Reservation != nil && !sysInfo.MemoryReservation { + warnings = append(warnings, "Your kernel does not support memory soft limit capabilities or the cgroup is not mounted. Limitation discarded.") + memory.Reservation = nil + } + if memory.Limit != nil && memory.Reservation != nil && *memory.Limit < *memory.Reservation { + return warnings, errors.New("minimum memory limit cannot be less than memory reservation limit, see usage") + } + if memory.Kernel != nil && !sysInfo.KernelMemory { + warnings = append(warnings, "Your kernel does not support kernel memory limit capabilities or the cgroup is not mounted. Limitation discarded.") + memory.Kernel = nil + } + if memory.DisableOOMKiller != nil && *memory.DisableOOMKiller && !sysInfo.OomKillDisable { + warnings = append(warnings, "Your kernel does not support OomKillDisable. OomKillDisable discarded.") + memory.DisableOOMKiller = nil + } + } + + // Pids checks + if s.ResourceLimits.Pids != nil { + pids := s.ResourceLimits.Pids + // TODO: Should this be 0, or checking that ResourceLimits.Pids + // is set at all? + if pids.Limit > 0 && !sysInfo.PidsLimit { + warnings = append(warnings, "Your kernel does not support pids limit capabilities or the cgroup is not mounted. PIDs limit discarded.") + s.ResourceLimits.Pids = nil + } + } + + // CPU Checks + if s.ResourceLimits.CPU != nil { + cpu := s.ResourceLimits.CPU + if cpu.Shares != nil && !sysInfo.CPUShares { + warnings = append(warnings, "Your kernel does not support CPU shares or the cgroup is not mounted. Shares discarded.") + cpu.Shares = nil + } + if cpu.Period != nil && !sysInfo.CPUCfsPeriod { + warnings = append(warnings, "Your kernel does not support CPU cfs period or the cgroup is not mounted. Period discarded.") + cpu.Period = nil + } + if cpu.Period != nil && (*cpu.Period < 1000 || *cpu.Period > 1000000) { + return warnings, errors.New("CPU cfs period cannot be less than 1ms (i.e. 1000) or larger than 1s (i.e. 1000000)") + } + if cpu.Quota != nil && !sysInfo.CPUCfsQuota { + warnings = append(warnings, "Your kernel does not support CPU cfs quota or the cgroup is not mounted. Quota discarded.") + cpu.Quota = nil + } + if cpu.Quota != nil && *cpu.Quota < 1000 { + return warnings, errors.New("CPU cfs quota cannot be less than 1ms (i.e. 1000)") + } + if (cpu.Cpus != "" || cpu.Mems != "") && !sysInfo.Cpuset { + warnings = append(warnings, "Your kernel does not support cpuset or the cgroup is not mounted. CPUset discarded.") + cpu.Cpus = "" + cpu.Mems = "" + } + + cpusAvailable, err := sysInfo.IsCpusetCpusAvailable(cpu.Cpus) + if err != nil { + return warnings, errors.Errorf("invalid value %s for cpuset cpus", cpu.Cpus) + } + if !cpusAvailable { + return warnings, errors.Errorf("requested CPUs are not available - requested %s, available: %s", cpu.Cpus, sysInfo.Cpus) + } + + memsAvailable, err := sysInfo.IsCpusetMemsAvailable(cpu.Mems) + if err != nil { + return warnings, errors.Errorf("invalid value %s for cpuset mems", cpu.Mems) + } + if !memsAvailable { + return warnings, errors.Errorf("requested memory nodes are not available - requested %s, available: %s", cpu.Mems, sysInfo.Mems) + } + } + + // Blkio checks + if s.ResourceLimits.BlockIO != nil { + blkio := s.ResourceLimits.BlockIO + if blkio.Weight != nil && !sysInfo.BlkioWeight { + warnings = append(warnings, "Your kernel does not support Block I/O weight or the cgroup is not mounted. Weight discarded.") + blkio.Weight = nil + } + if blkio.Weight != nil && (*blkio.Weight > 1000 || *blkio.Weight < 10) { + return warnings, errors.New("range of blkio weight is from 10 to 1000") + } + if len(blkio.WeightDevice) > 0 && !sysInfo.BlkioWeightDevice { + warnings = append(warnings, "Your kernel does not support Block I/O weight_device or the cgroup is not mounted. Weight-device discarded.") + blkio.WeightDevice = nil + } + if len(blkio.ThrottleReadBpsDevice) > 0 && !sysInfo.BlkioReadBpsDevice { + warnings = append(warnings, "Your kernel does not support BPS Block I/O read limit or the cgroup is not mounted. Block I/O BPS read limit discarded") + blkio.ThrottleReadBpsDevice = nil + } + if len(blkio.ThrottleWriteBpsDevice) > 0 && !sysInfo.BlkioWriteBpsDevice { + warnings = append(warnings, "Your kernel does not support BPS Block I/O write limit or the cgroup is not mounted. Block I/O BPS write limit discarded.") + blkio.ThrottleWriteBpsDevice = nil + } + if len(blkio.ThrottleReadIOPSDevice) > 0 && !sysInfo.BlkioReadIOpsDevice { + warnings = append(warnings, "Your kernel does not support IOPS Block read limit or the cgroup is not mounted. Block I/O IOPS read limit discarded.") + blkio.ThrottleReadIOPSDevice = nil + } + if len(blkio.ThrottleWriteIOPSDevice) > 0 && !sysInfo.BlkioWriteIOpsDevice { + warnings = append(warnings, "Your kernel does not support IOPS Block I/O write limit or the cgroup is not mounted. Block I/O IOPS write limit discarded.") + blkio.ThrottleWriteIOPSDevice = nil + } + } + + return warnings, nil +} -- cgit v1.2.3-54-g00ecf