Merge pull request #9169 from dokku/9153-resource-limit-and-resource-reserve-break-uppercase-memory-units-such-as-1g

Accept all docker memory units in resource values

docs/advanced-usage/resource-management.md

@@ -30,6 +30,8 @@ Valid resource options include:
 - `--network-egress`
 - `--nvidia-gpu`
 
+Values for `--memory` and `--memory-swap` are validated when set, and invalid values are rejected.
+
 See the [Supported Resource Management Properties](/docs/deployment/schedulers/docker-local.md#supported-resource-management-properties) section of the docker local scheduler documentation for more information on how each resource limit maps to Docker.
 
 Resource reservations are applied only during the `run` and `deploy` phases of an application. Resource limits also apply during the `build` phase when explicitly configured against the `build` process type - see [Build-time Resource Limits](#build-time-resource-limits) below.

docs/deployment/schedulers/docker-local.md

@@ -205,16 +205,16 @@ The `docker-local` scheduler supports a minimal list of resource _limits_ and _r
 
 - cpu: (docker option: `--cpus`), is specified in number of CPUs a process can access.
     - See the ["CPU" section](https://docs.docker.com/config/containers/resource_constraints/#cpu) of the Docker Runtime Options documentation for more information.
-- memory: (docker option: `--memory`) should be specified with a suffix of `b` (bytes), `k` (kilobytes), `m` (megabytes), `g` (gigabytes). Default unit is `m` (megabytes).
+- memory: (docker option: `--memory`) should be specified with a suffix of `b` (bytes), `k` (kilobytes), `m` (megabytes), `g` (gigabytes), `t` (terabytes) or `p` (petabytes). Suffixes are case-insensitive and may be followed by `i` and/or `b`, such as `512M`, `1G`, or `1GiB`. Default unit is `m` (megabytes).
     - See the ["Memory" section](https://docs.docker.com/config/containers/resource_constraints/#memory) of the Docker Runtime Options documentation for more information.
-- memory-swap: (docker option: `--memory-swap`) should be specified with a suffix of `b` (bytes), `k` (kilobytes), `m` (megabytes), `g` (gigabytes)
+- memory-swap: (docker option: `--memory-swap`) should be specified with the same suffixes as `memory`, or `-1` for unlimited swap. Values without a suffix are in bytes.
     - See the ["Memory" section](https://docs.docker.com/config/containers/resource_constraints/#memory) of the Docker Runtime Options documentation for more information.
 - nvidia-gpus: (docker option: `--gpus`), is specified in number of Nvidia GPUs a process can access.
     - See the ["GPU" section](https://docs.docker.com/config/containers/resource_constraints/#gpu) of the Docker Runtime Options documentation for more information.
 
 #### Resource Reservations
 
-- memory: (docker option: `--memory-reservation`) should be specified with a suffix of `b` (bytes), `k` (kilobytes), `m` (megabytes), `g` (gigabytes). Default unit is `m` (megabytes).
+- memory: (docker option: `--memory-reservation`) should be specified with the same suffixes as the `memory` limit. Default unit is `m` (megabytes).
     - See the ["Memory" section](https://docs.docker.com/config/containers/resource_constraints/#memory) of the Docker Runtime Options documentation for more information.
 
 ## Properties

docs/deployment/schedulers/k3s.md

@@ -1293,7 +1293,7 @@ Two differences are worth noting:
 The `k3s` scheduler supports a minimal list of resource _limits_ and _reservations_:
 
 - cpu: is specified in number of CPUs a process can access.
-- memory: should be specified with a suffix of `b` (bytes), `Ki` (kilobytes), `Mi` (megabytes), `Gi` (gigabytes). Default unit is `Mi` (megabytes).
+- memory: should be specified with a suffix of `b` (bytes), `k` (kilobytes), `m` (megabytes), `g` (gigabytes), `t` (terabytes) or `p` (petabytes). Suffixes are case-insensitive and may be followed by `i` and/or `b`, such as `512Mi`, `1G`, or `1GiB`. Units are binary, so `1G` and `1Gi` are equivalent. Default unit is `Mi` (megabytes).
 
 If unspecified for any task, the default reservation will be `.1` CPU and `128Mi` RAM, with no limit set for either CPU or RAM. This is to avoid issues with overscheduling pods on a cluster. To avoid issues, set more specific values for at least resource reservations. If unbounded utilization is desired, set CPU and Memory to `0m` and `0Mi`, respectively.
 

plugins/common/memory.go

@@ -0,0 +1,61 @@
+package common
+
+import (
+	"fmt"
+	"regexp"
+	"strconv"
+	"strings"
+)
+
+// memoryValueRegex matches a docker-compatible memory value: a number with an
+// optional unit (b, k, m, g, t, p in either case) optionally followed by i and/or b
+var memoryValueRegex = regexp.MustCompile(`^(\d+(?:\.\d+)?)(?:([kKmMgGtTpP])[iI]?)?([bB])?$`)
+
+// IsValidMemoryValue returns true if the value is a docker-compatible memory value
+func IsValidMemoryValue(value string) bool {
+	return memoryValueRegex.MatchString(value)
+}
+
+// IsUnitlessMemoryValue returns true if the value is a valid memory value without any unit suffix
+func IsUnitlessMemoryValue(value string) bool {
+	matches := memoryValueRegex.FindStringSubmatch(value)
+	if matches == nil {
+		return false
+	}
+
+	return matches[2] == "" && matches[3] == ""
+}
+
+// ParseMemoryBytes converts a docker-compatible memory value into bytes.
+// Units are binary, and a value without a unit is treated as megabytes
+func ParseMemoryBytes(value string) (int64, error) {
+	matches := memoryValueRegex.FindStringSubmatch(value)
+	if matches == nil {
+		return 0, fmt.Errorf("Invalid memory value '%s'", value)
+	}
+
+	size, err := strconv.ParseFloat(matches[1], 64)
+	if err != nil {
+		return 0, fmt.Errorf("Invalid memory value '%s': %w", value, err)
+	}
+
+	multipliers := map[string]float64{
+		"k": 1 << 10,
+		"m": 1 << 20,
+		"g": 1 << 30,
+		"t": 1 << 40,
+		"p": 1 << 50,
+	}
+
+	unit := strings.ToLower(matches[2])
+	if unit == "" && matches[3] == "" {
+		unit = "m"
+	}
+
+	multiplier := float64(1)
+	if unit != "" {
+		multiplier = multipliers[unit]
+	}
+
+	return int64(size * multiplier), nil
+}

plugins/common/memory_test.go

@@ -0,0 +1,66 @@
+package common
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestCommonIsValidMemoryValue(t *testing.T) {
+	RegisterTestingT(t)
+
+	valid := []string{"0", "100", "1.5", "100b", "100B", "512k", "512K", "512kb", "512m", "512M", "512MB", "512Mi", "512MiB", "1g", "1G", "1GB", "1Gi", "1gib", "1t", "1T", "1p", "1P"}
+	for _, value := range valid {
+		Expect(IsValidMemoryValue(value)).To(BeTrue(), value)
+	}
+
+	invalid := []string{"", "-1", "abc", "1x", "1Gm", "1i", "1ib", "1 G", "1.2.3", ".5g", "100G --foo", "1E", "1e9"}
+	for _, value := range invalid {
+		Expect(IsValidMemoryValue(value)).To(BeFalse(), value)
+	}
+}
+
+func TestCommonIsUnitlessMemoryValue(t *testing.T) {
+	RegisterTestingT(t)
+
+	Expect(IsUnitlessMemoryValue("100")).To(BeTrue())
+	Expect(IsUnitlessMemoryValue("1.5")).To(BeTrue())
+	Expect(IsUnitlessMemoryValue("100b")).To(BeFalse())
+	Expect(IsUnitlessMemoryValue("1G")).To(BeFalse())
+	Expect(IsUnitlessMemoryValue("1Gi")).To(BeFalse())
+	Expect(IsUnitlessMemoryValue("abc")).To(BeFalse())
+}
+
+func TestCommonParseMemoryBytes(t *testing.T) {
+	RegisterTestingT(t)
+
+	expected := map[string]int64{
+		"0":      0,
+		"512":    536870912,
+		"512m":   536870912,
+		"512M":   536870912,
+		"512MB":  536870912,
+		"512Mi":  536870912,
+		"512MiB": 536870912,
+		"1g":     1073741824,
+		"1G":     1073741824,
+		"1GB":    1073741824,
+		"1Gi":    1073741824,
+		"1.5g":   1610612736,
+		"1024K":  1048576,
+		"100b":   100,
+		"100B":   100,
+		"1t":     1099511627776,
+		"1p":     1125899906842624,
+	}
+	for value, bytes := range expected {
+		parsed, err := ParseMemoryBytes(value)
+		Expect(err).NotTo(HaveOccurred(), value)
+		Expect(parsed).To(Equal(bytes), value)
+	}
+
+	for _, value := range []string{"", "-1", "1x", "1Gm", "100G --foo"} {
+		_, err := ParseMemoryBytes(value)
+		Expect(err).To(HaveOccurred(), value)
+	}
+}

plugins/resource/functions.go

@@ -2,25 +2,38 @@ package resource
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/dokku/dokku/plugins/common"
 )
 
-func addMemorySuffixForDocker(key string, value string) string {
-	if key == "memory" {
-		hasSuffix := false
-		suffixes := []string{"b", "k", "m", "g", "KB", "MB", "GB", "B"}
-		for _, suffix := range suffixes {
-			if strings.HasSuffix(value, suffix) {
-				hasSuffix = true
-			}
+// validateResourceValue returns an error if the value is not valid for the given resource key
+func validateResourceValue(key string, value string) error {
+	switch key {
+	case "memory":
+		if !common.IsValidMemoryValue(value) {
+			return fmt.Errorf("Invalid memory value '%s': must be a number with an optional unit suffix of b, k, m, g, t or p", value)
 		}
-		if !hasSuffix {
-			value = value + "m"
+	case "memory-swap":
+		if value != "-1" && !common.IsValidMemoryValue(value) {
+			return fmt.Errorf("Invalid memory-swap value '%s': must be -1 or a number with an optional unit suffix of b, k, m, g, t or p", value)
 		}
 	}
-	return value
+
+	return nil
+}
+
+// formatResourceValueForDocker validates a resource value and formats it for use as a docker flag value.
+// Memory values without a unit default to megabytes
+func formatResourceValueForDocker(key string, value string) (string, error) {
+	if err := validateResourceValue(key, value); err != nil {
+		return "", err
+	}
+
+	if key == "memory" && common.IsUnitlessMemoryValue(value) {
+		value = value + "m"
+	}
+
+	return value, nil
 }
 
 func clearByResourceType(appName string, processType string, resourceType string) {
@@ -88,6 +101,16 @@ func setResourceType(appName string, processType string, r Resource, resourceTyp
 		return nil
 	}
 
+	for key, value := range resources {
+		if value == "" || value == "clear" {
+			continue
+		}
+
+		if err := validateResourceValue(key, value); err != nil {
+			return err
+		}
+	}
+
 	noun := "limits"
 	if resourceType == "reserve" {
 		noun = "reservation"

plugins/resource/functions_test.go

@@ -0,0 +1,55 @@
+package resource
+
+import (
+	"testing"
+)
+
+func TestFormatResourceValueForDocker(t *testing.T) {
+	testCases := []struct {
+		name     string
+		key      string
+		value    string
+		expected string
+		wantErr  bool
+	}{
+		{name: "unitless memory defaults to megabytes", key: "memory", value: "100", expected: "100m"},
+		{name: "unitless decimal memory defaults to megabytes", key: "memory", value: "1.5", expected: "1.5m"},
+		{name: "lowercase memory unit", key: "memory", value: "512m", expected: "512m"},
+		{name: "uppercase megabyte unit", key: "memory", value: "512M", expected: "512M"},
+		{name: "uppercase gigabyte unit", key: "memory", value: "1G", expected: "1G"},
+		{name: "uppercase kilobyte unit", key: "memory", value: "1024K", expected: "1024K"},
+		{name: "terabyte unit", key: "memory", value: "1t", expected: "1t"},
+		{name: "two letter unit", key: "memory", value: "512MB", expected: "512MB"},
+		{name: "binary unit", key: "memory", value: "1GiB", expected: "1GiB"},
+		{name: "byte unit", key: "memory", value: "100b", expected: "100b"},
+		{name: "memory rejects negative values", key: "memory", value: "-1", wantErr: true},
+		{name: "memory rejects extra arguments", key: "memory", value: "100G --foo", wantErr: true},
+		{name: "memory rejects invalid units", key: "memory", value: "1Gm", wantErr: true},
+		{name: "unlimited memory-swap", key: "memory-swap", value: "-1", expected: "-1"},
+		{name: "unitless memory-swap stays in bytes", key: "memory-swap", value: "512", expected: "512"},
+		{name: "uppercase memory-swap unit", key: "memory-swap", value: "2G", expected: "2G"},
+		{name: "memory-swap rejects invalid units", key: "memory-swap", value: "1x", wantErr: true},
+		{name: "memory-swap rejects other negative values", key: "memory-swap", value: "-2", wantErr: true},
+		{name: "other keys are passed through", key: "cpus", value: "1.5", expected: "1.5"},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			value, err := formatResourceValueForDocker(testCase.key, testCase.value)
+			if testCase.wantErr {
+				if err == nil {
+					t.Fatalf("expected an error for %s=%s, got %s", testCase.key, testCase.value, value)
+				}
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("unexpected error for %s=%s: %v", testCase.key, testCase.value, err)
+			}
+
+			if value != testCase.expected {
+				t.Fatalf("expected %s, got %s", testCase.expected, value)
+			}
+		})
+	}
+}

plugins/resource/triggers.go

@@ -71,22 +71,30 @@ func TriggerDockerArgsProcessDeploy(appName string, processType string) error {
 		}
 	}
 
+	var args strings.Builder
 	for key, value := range limits {
 		if value == "" {
 			continue
 		}
-		value = addMemorySuffixForDocker(key, value)
-		fmt.Printf(" --%s=%s ", key, value)
+		value, err := formatResourceValueForDocker(key, value)
+		if err != nil {
+			return fmt.Errorf("Unable to apply resource limits for %s: %w", appName, err)
+		}
+		fmt.Fprintf(&args, " --%s=%s ", key, value)
 	}
 
 	for key, value := range reservations {
 		if value == "" {
 			continue
 		}
-		value = addMemorySuffixForDocker(key, value)
-		fmt.Printf(" --%s-reservation=%s ", key, value)
+		value, err := formatResourceValueForDocker(key, value)
+		if err != nil {
+			return fmt.Errorf("Unable to apply resource reservations for %s: %w", appName, err)
+		}
+		fmt.Fprintf(&args, " --%s-reservation=%s ", key, value)
 	}
 
+	fmt.Print(args.String())
 	fmt.Print(string(stdin))
 	return nil
 }
@@ -136,14 +144,19 @@ func TriggerDockerArgsProcessBuild(appName string, builderType string) error {
 		limits[flagName] = value
 	}
 
+	var args strings.Builder
 	for key, value := range limits {
 		if value == "" {
 			continue
 		}
-		value = addMemorySuffixForDocker(key, value)
-		fmt.Printf(" --%s=%s ", key, value)
+		value, err := formatResourceValueForDocker(key, value)
+		if err != nil {
+			return fmt.Errorf("Unable to apply build resource limits for %s: %w", appName, err)
+		}
+		fmt.Fprintf(&args, " --%s=%s ", key, value)
 	}
 
+	fmt.Print(args.String())
 	fmt.Print(string(stdin))
 	return nil
 }

plugins/scheduler-k3s/functions.go

@@ -1946,7 +1946,7 @@ func getProcessResources(appName string, processType string) (ProcessResourcesMa
 		if err != nil {
 			return ProcessResourcesMap{}, fmt.Errorf("Error parsing memory limit: %w", err)
 		}
-		if quantity != "0Mi" {
+		if quantity != "0" {
 			processResources.Limits.Memory = quantity
 		} else {
 			processResources.Limits.Memory = ""
@@ -1977,7 +1977,7 @@ func getProcessResources(appName string, processType string) (ProcessResourcesMa
 		if err != nil {
 			return ProcessResourcesMap{}, fmt.Errorf("Error parsing memory request: %w", err)
 		}
-		if quantity != "0Mi" {
+		if quantity != "0" {
 			processResources.Requests.Memory = quantity
 		} else {
 			processResources.Requests.Memory = ""
@@ -2720,17 +2720,14 @@ func cronIDLabelValue(cronID string) string {
 	return hex.EncodeToString(sum[:])
 }
 
-// parseMemoryQuantity parses a string into a valid memory quantity
+// parseMemoryQuantity parses a docker-compatible memory value into a binary memory quantity
 func parseMemoryQuantity(input string) (string, error) {
-	if _, err := strconv.ParseInt(input, 10, 64); err == nil {
-		input = fmt.Sprintf("%sMi", input)
-	}
-	quantity, err := resource.ParseQuantity(input)
+	bytes, err := common.ParseMemoryBytes(input)
 	if err != nil {
 		return "", err
 	}
 
-	return quantity.String(), nil
+	return resource.NewQuantity(bytes, resource.BinarySI).String(), nil
 }
 
 func uninstallHelperCommands(ctx context.Context) error {

plugins/scheduler-k3s/functions_test.go

@@ -487,6 +487,47 @@ func TestDomainSlug(t *testing.T) {
 	}
 }
 
+func TestParseMemoryQuantity(t *testing.T) {
+	cases := []struct {
+		name    string
+		input   string
+		want    string
+		wantErr bool
+	}{
+		{name: "unitless value defaults to megabytes", input: "512", want: "512Mi"},
+		{name: "lowercase megabytes are not millibytes", input: "256m", want: "256Mi"},
+		{name: "uppercase gigabytes are binary", input: "1G", want: "1Gi"},
+		{name: "lowercase gigabytes", input: "1g", want: "1Gi"},
+		{name: "kubernetes binary unit", input: "1Gi", want: "1Gi"},
+		{name: "two letter unit", input: "512MB", want: "512Mi"},
+		{name: "decimal value", input: "1.5g", want: "1536Mi"},
+		{name: "bytes", input: "100b", want: "100"},
+		{name: "zero", input: "0Mi", want: "0"},
+		{name: "invalid unit", input: "1x", wantErr: true},
+		{name: "kubernetes exponent", input: "1e9", wantErr: true},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			got, err := parseMemoryQuantity(tc.input)
+			if tc.wantErr {
+				if err == nil {
+					t.Fatalf("parseMemoryQuantity(%q) = %q, want error", tc.input, got)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("parseMemoryQuantity(%q) unexpected error: %v", tc.input, err)
+			}
+			if got != tc.want {
+				t.Errorf("parseMemoryQuantity(%q) = %q, want %q", tc.input, got, tc.want)
+			}
+		})
+	}
+}
+
 func TestDomainSlugDistinguishesWildcardFromApex(t *testing.T) {
 	// A kubernetes wildcard host matches exactly one label, so *.example.com does not
 	// cover example.com and both are commonly added to the same app. The generated

tests.mk

@@ -188,6 +188,7 @@ go-tests:
 	@$(MAKE) go-test-plugin PLUGIN_NAME=nginx-vhosts
 	@$(MAKE) go-test-plugin PLUGIN_NAME=ps
 	@$(MAKE) go-test-plugin PLUGIN_NAME=registry
+	@$(MAKE) go-test-plugin PLUGIN_NAME=resource
 	@$(MAKE) go-test-plugin PLUGIN_NAME=buildpacks
 	@$(MAKE) go-test-plugin PLUGIN_NAME=scheduler-k3s
 	@$(MAKE) go-test-plugin PLUGIN_NAME=storage

tests/unit/resource_1.bats

@@ -59,6 +59,22 @@ teardown() {
   echo "status: $status"
   assert_output "536870912"
 
+  run /bin/bash -c "dokku resource:limit --memory 1G $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku ps:rebuild $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  CID=$(<$DOKKU_ROOT/$TEST_APP/CONTAINER.web.1)
+  run /bin/bash -c "docker inspect --format '{{.HostConfig.Memory}}' $CID"
+  echo "output: $output"
+  echo "status: $status"
+  assert_output "1073741824"
+
   run /bin/bash -c "dokku resource:limit --memory 512 $TEST_APP"
   echo "output: $output"
   echo "status: $status"

tests/unit/resource_3.bats

@@ -131,3 +131,80 @@ teardown() {
   echo "status: $status"
   assert_failure
 }
+
+@test "(resource) resource:limit uppercase memory units" {
+  run /bin/bash -c "dokku resource:limit --memory 1G --memory-swap 2G $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:reserve --memory 512M $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run_plugin_script resource docker-args-process-deploy "$TEST_APP" herokuish "dokku/$TEST_APP:latest" web "< /dev/null"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output_contains "--memory=1G "
+  assert_output_contains "--memory-swap=2G "
+  assert_output_contains "--memory-reservation=512M "
+  assert_output_not_contains "1Gm"
+  assert_output_not_contains "512Mm"
+
+  run /bin/bash -c "dokku resource:limit --memory 1G --process-type build $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run_plugn_trigger docker-args-process-build "$TEST_APP" herokuish
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output_contains "--memory=1G "
+  assert_output_not_contains "1Gm"
+}
+
+@test "(resource) resource:limit rejects invalid memory values" {
+  run /bin/bash -c "dokku resource:limit --memory '100G --foo' $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku resource:limit --memory 1x $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku resource:limit --memory-swap 1x $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku resource:reserve --memory 1Gm $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku resource:report --resource-_default_.limit.memory $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku resource:report --resource-_default_.reserve.memory $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku resource:limit --memory-swap -1 $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:report --resource-_default_.limit.memory-swap $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "-1"
+}