Merge pull request #6636 from mheon/add_warnings

Re-add resource limit warnings to Specgen

6 files changed, 270 insertions, 77 deletions

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 19512ddf1..8e0ffc075 100644
--- a/pkg/domain/infra/abi/containers.go
+++ b/pkg/domain/infra/abi/containers.go
@@ -542,9 +542,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
@@ -804,9 +809,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 5f9e2bdd2..45fbc64f8 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -347,6 +347,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
 }
 
@@ -504,6 +507,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
+}