diff --git a/docs/advanced-usage/resource-management.md b/docs/advanced-usage/resource-management.md index 804397489..df420ee5e 100644 --- a/docs/advanced-usage/resource-management.md +++ b/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. diff --git a/docs/deployment/schedulers/docker-local.md b/docs/deployment/schedulers/docker-local.md index 089db7113..0c8361298 100644 --- a/docs/deployment/schedulers/docker-local.md +++ b/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 diff --git a/docs/deployment/schedulers/k3s.md b/docs/deployment/schedulers/k3s.md index a27cbbffd..1a9e7ce87 100644 --- a/docs/deployment/schedulers/k3s.md +++ b/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. diff --git a/plugins/common/memory.go b/plugins/common/memory.go new file mode 100644 index 000000000..7a8463209 --- /dev/null +++ b/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 +} diff --git a/plugins/common/memory_test.go b/plugins/common/memory_test.go new file mode 100644 index 000000000..402bec165 --- /dev/null +++ b/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) + } +} diff --git a/plugins/resource/functions.go b/plugins/resource/functions.go index ef382605d..ea3a11f58 100644 --- a/plugins/resource/functions.go +++ b/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" diff --git a/plugins/resource/functions_test.go b/plugins/resource/functions_test.go new file mode 100644 index 000000000..67cd2de69 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/plugins/resource/triggers.go b/plugins/resource/triggers.go index 6994c69d1..7a45dd28f 100644 --- a/plugins/resource/triggers.go +++ b/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 } diff --git a/plugins/scheduler-k3s/functions.go b/plugins/scheduler-k3s/functions.go index dafa96067..efe69fcc0 100644 --- a/plugins/scheduler-k3s/functions.go +++ b/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 { diff --git a/plugins/scheduler-k3s/functions_test.go b/plugins/scheduler-k3s/functions_test.go index 3425d346d..8e343e9a2 100644 --- a/plugins/scheduler-k3s/functions_test.go +++ b/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 diff --git a/tests.mk b/tests.mk index 576f4fbd7..ff23614f5 100644 --- a/tests.mk +++ b/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 diff --git a/tests/unit/resource_1.bats b/tests/unit/resource_1.bats index c36031e89..4d2ea90cb 100644 --- a/tests/unit/resource_1.bats +++ b/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" diff --git a/tests/unit/resource_3.bats b/tests/unit/resource_3.bats index fcf3423e8..91d03a8ff 100644 --- a/tests/unit/resource_3.bats +++ b/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" +}