Fix Memory Swappiness passing in Container Clone

`DefineCreateFlags` was excluding clone from using the memory-swappiness flag leading the value to be zero when our deafult is -1. Rearrange the if/else to give clone these memory related options resolves containers#13856 Signed-off-by: cdoern <[email protected]>
cdoern · Apr 14, 2022 · f38b03d · f38b03d
1 parent 9822c46
commit f38b03d
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 48 deletions.
diff --git a/cmd/podman/common/create.go b/cmd/podman/common/create.go
@@ -312,30 +312,6 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 		)
 		_ = cmd.RegisterFlagCompletionFunc(logOptFlagName, AutocompleteLogOpt)
 
-		memoryReservationFlagName := "memory-reservation"
-		createFlags.StringVar(
-			&cf.MemoryReservation,
-			memoryReservationFlagName, "",
-			"Memory soft limit "+sizeWithUnitFormat,
-		)
-		_ = cmd.RegisterFlagCompletionFunc(memoryReservationFlagName, completion.AutocompleteNone)
-
-		memorySwapFlagName := "memory-swap"
-		createFlags.StringVar(
-			&cf.MemorySwap,
-			memorySwapFlagName, "",
-			"Swap limit equal to memory plus swap: '-1' to enable unlimited swap",
-		)
-		_ = cmd.RegisterFlagCompletionFunc(memorySwapFlagName, completion.AutocompleteNone)
-
-		memorySwappinessFlagName := "memory-swappiness"
-		createFlags.Int64Var(
-			&cf.MemorySwappiness,
-			memorySwappinessFlagName, -1,
-			"Tune container memory swappiness (0 to 100, or -1 for system default)",
-		)
-		_ = cmd.RegisterFlagCompletionFunc(memorySwappinessFlagName, completion.AutocompleteNone)
-
 		createFlags.BoolVar(
 			&cf.NoHealthCheck,
 			"no-healthcheck", false,
@@ -891,6 +867,30 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 			"Memory limit "+sizeWithUnitFormat,
 		)
 		_ = cmd.RegisterFlagCompletionFunc(memoryFlagName, completion.AutocompleteNone)
+
+		memoryReservationFlagName := "memory-reservation"
+		createFlags.StringVar(
+			&cf.MemoryReservation,
+			memoryReservationFlagName, "",
+			"Memory soft limit "+sizeWithUnitFormat,
+		)
+		_ = cmd.RegisterFlagCompletionFunc(memoryReservationFlagName, completion.AutocompleteNone)
+
+		memorySwapFlagName := "memory-swap"
+		createFlags.StringVar(
+			&cf.MemorySwap,
+			memorySwapFlagName, "",
+			"Swap limit equal to memory plus swap: '-1' to enable unlimited swap",
+		)
+		_ = cmd.RegisterFlagCompletionFunc(memorySwapFlagName, completion.AutocompleteNone)
+
+		memorySwappinessFlagName := "memory-swappiness"
+		createFlags.Int64Var(
+			&cf.MemorySwappiness,
+			memorySwappinessFlagName, -1,
+			"Tune container memory swappiness (0 to 100, or -1 for system default)",
+		)
+		_ = cmd.RegisterFlagCompletionFunc(memorySwappinessFlagName, completion.AutocompleteNone)
 	}
 	//anyone can use these
 	cpusFlagName := "cpus"

diff --git a/docs/source/markdown/podman-container-clone.1.md b/docs/source/markdown/podman-container-clone.1.md
@@ -137,6 +137,33 @@ system's page size (the value would be very large, that's millions of trillions)
 
 If no memory limits are specified, the original container's will be used.
 
+#### **--memory-reservation**=*limit*
+
+Memory soft limit (format: `<number>[<unit>]`, where unit = b (bytes), k (kilobytes), m (megabytes), or g (gigabytes))
+
+After setting memory reservation, when the system detects memory contention
+or low memory, containers are forced to restrict their consumption to their
+reservation. So you should always set the value below **--memory**, otherwise the
+hard limit will take precedence. By default, memory reservation will be the same
+as memory limit from the container being cloned.
+
+#### **--memory-swap**=*limit*
+
+A limit value equal to memory plus swap. Must be used with the  **-m**
+(**--memory**) flag. The swap `LIMIT` should always be larger than **-m**
+(**--memory**) value. By default, the swap `LIMIT` will be set to double
+the value of --memory if specified. Otherwise, the container being cloned will be used to derive the swap value.
+
+The format of `LIMIT` is `<number>[<unit>]`. Unit can be `b` (bytes),
+`k` (kilobytes), `m` (megabytes), or `g` (gigabytes). If you don't specify a
+unit, `b` is used. Set LIMIT to `-1` to enable unlimited swap.
+
+#### **--memory-swappiness**=*number*
+
+Tune a container's memory swappiness behavior. Accepts an integer between 0 and 100.
+
+This flag is not supported on cgroups V2 systems.
+
 #### **--name**
 
 Set a custom name for the cloned container. The default if not specified is of the syntax: **<ORIGINAL_NAME>-clone**

diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go
@@ -8,7 +8,6 @@ import (
 
 	cdi "github.com/container-orchestrated-devices/container-device-interface/pkg/cdi"
 	"github.com/containers/common/libimage"
-	"github.com/containers/common/pkg/cgroups"
 	"github.com/containers/podman/v4/libpod"
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/pkg/namespaces"
@@ -184,32 +183,19 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener
 		if err != nil {
 			return nil, nil, nil, err
 		}
-
-		switch {
-		case s.ResourceLimits.CPU != nil:
-			runtimeSpec.Linux.Resources.CPU = s.ResourceLimits.CPU
-		case s.ResourceLimits.Memory != nil:
-			runtimeSpec.Linux.Resources.Memory = s.ResourceLimits.Memory
-		case s.ResourceLimits.BlockIO != nil:
-			runtimeSpec.Linux.Resources.BlockIO = s.ResourceLimits.BlockIO
-		case s.ResourceLimits.Devices != nil:
-			runtimeSpec.Linux.Resources.Devices = s.ResourceLimits.Devices
-		}
-
-		cgroup2, err := cgroups.IsCgroup2UnifiedMode()
-		if err != nil {
-			return nil, nil, nil, err
-		}
-		if cgroup2 && s.ResourceLimits.Memory != nil && s.ResourceLimits.Memory.Swappiness != nil { // conf.Spec.Linux contains memory swappiness established after the spec process we need to remove that
-			s.ResourceLimits.Memory.Swappiness = nil
-			if runtimeSpec.Linux.Resources.Memory != nil {
-				runtimeSpec.Linux.Resources.Memory.Swappiness = nil
+		if s.ResourceLimits != nil {
+			switch {
+			case s.ResourceLimits.CPU != nil:
+				runtimeSpec.Linux.Resources.CPU = s.ResourceLimits.CPU
+			case s.ResourceLimits.Memory != nil:
+				runtimeSpec.Linux.Resources.Memory = s.ResourceLimits.Memory
+			case s.ResourceLimits.BlockIO != nil:
+				runtimeSpec.Linux.Resources.BlockIO = s.ResourceLimits.BlockIO
+			case s.ResourceLimits.Devices != nil:
+				runtimeSpec.Linux.Resources.Devices = s.ResourceLimits.Devices
 			}
 		}
 	}
-	if err != nil {
-		return nil, nil, nil, err
-	}
 	if len(s.HostDeviceList) > 0 {
 		options = append(options, libpod.WithHostDevice(s.HostDeviceList))
 	}

diff --git a/test/e2e/container_clone_test.go b/test/e2e/container_clone_test.go
@@ -146,6 +146,20 @@ var _ = Describe("Podman container clone", func() {
 		cloneData = cloneInspect.InspectContainerToJSON()
 		Expect(createData[0].HostConfig.NanoCpus).ToNot(Equal(cloneData[0].HostConfig.NanoCpus))
 		Expect(cloneData[0].HostConfig.NanoCpus).To(Equal(nanoCPUs))
+
+		create = podmanTest.Podman([]string{"create", ALPINE})
+		create.WaitWithDefaultTimeout()
+		Expect(create).To(Exit(0))
+		clone = podmanTest.Podman([]string{"container", "clone", "--cpus=4", create.OutputToString()})
+		clone.WaitWithDefaultTimeout()
+		Expect(clone).To(Exit(0))
+
+		cloneInspect = podmanTest.Podman([]string{"inspect", clone.OutputToString()})
+		cloneInspect.WaitWithDefaultTimeout()
+		Expect(cloneInspect).To(Exit(0))
+		cloneData = cloneInspect.InspectContainerToJSON()
+		Expect(cloneData[0].HostConfig.MemorySwappiness).To(Equal(int64(0)))
+
 	})
 
 	It("podman container clone in a pod", func() {