Merge pull request #9175 from dokku/9171-clearing-resource-limits-without-a-process-type-also-removes-reservations

Only clear the requested resource type

docs/advanced-usage/resource-management.md

@@ -5,10 +5,10 @@
 
 ```
 resource:limit [--process-type <process-type>] [RESOURCE_OPTS...] <app>    # Limit resources for a given app/process-type combination
-resource:limit-clear [--process-type <process-type>] <app>                 # Limit resources for a given app/process-type combination
+resource:limit-clear [--process-type <process-type>] <app>                 # Clear resource limits for a given app/process-type combination
 resource:report [<app>] [<flag>]                                           # Displays a resource report for one or more apps
 resource:reserve [--process-type <process-type>] [RESOURCE_OPTS...] <app>  # Reserve resources for a given app/process-type combination
-resource:reserve-clear [--process-type <process-type>] <app>               # Reserve resources for a given app/process-type combination
+resource:reserve-clear [--process-type <process-type>] <app>               # Clear resource reservations for a given app/process-type combination
 ```
 
 The resource plugin is meant to allow users to limit or reserve resources for a given app/process-type combination.
@@ -158,24 +158,24 @@ Resource keys outside the supported set for a builder are silently ignored. Rese
 
 #### Clearing Resource Limits
 
-In cases where the values are incorrect - or there is no desire to limit resources - resource limits may be cleared using the `resource:limit-clear` command.
+In cases where the values are incorrect - or there is no desire to limit resources - resource limits may be cleared using the `resource:limit-clear` command. When no process type is specified, resource limits are cleared for all process types. Resource reservations are left untouched.
 
 ```shell
 dokku resource:limit-clear node-js-app
 ```
 
 ```
------> Clearing resource limit for node-js-app
+=====> clearing node-js-app limits
 ```
 
-Defaults can also be cleared by leaving the app unspecified.
+Resource limits for a single process type can be cleared by specifying the `--process-type` flag.
 
 ```shell
-dokku resource:limit-clear
+dokku resource:limit-clear --process-type worker node-js-app
 ```
 
 ```
------> Clearing default resource limits
+=====> clearing node-js-app limits (worker)
 ```
 
 ### Resource Reservations
@@ -266,24 +266,24 @@ dokku resource:reserve --process-type web node-js-app
 
 #### Clearing Resource Reservations
 
-In cases where the values are incorrect - or there is no desire to reserve resources - resource reservations may be cleared using the `resource:reserve-clear` command.
+In cases where the values are incorrect - or there is no desire to reserve resources - resource reservations may be cleared using the `resource:reserve-clear` command. When no process type is specified, resource reservations are cleared for all process types. Resource limits are left untouched.
 
 ```shell
 dokku resource:reserve-clear node-js-app
 ```
 
 ```
------> Clearing resource reservation for node-js-app
+=====> clearing node-js-app reservation
 ```
 
-Defaults can also be cleared by leaving the app unspecified.
+Resource reservations for a single process type can be cleared by specifying the `--process-type` flag.
 
 ```shell
-dokku resource:reserve-clear
+dokku resource:reserve-clear --process-type worker node-js-app
 ```
 
 ```
------> Clearing default resource reservation
+=====> clearing node-js-app reservation (worker)
 ```
 
 ### Displaying resource reports for an app

plugins/resource/functions.go

@@ -2,6 +2,7 @@ package resource
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/dokku/dokku/plugins/common"
 )
@@ -36,6 +37,17 @@ func formatResourceValueForDocker(key string, value string) (string, error) {
 	return value, nil
 }
 
+// isResourceTypeKey returns true if the property key is a `<process>.<resource-type>.<resource>` key
+// for the given resource type. The process type may contain dots, so the key is matched from the right
+func isResourceTypeKey(key string, resourceType string) bool {
+	parts := strings.Split(key, ".")
+	if len(parts) < 3 {
+		return false
+	}
+
+	return parts[len(parts)-2] == resourceType
+}
+
 func clearByResourceType(appName string, processType string, resourceType string) {
 	noun := "limits"
 	if resourceType == "reserve" {
@@ -54,6 +66,9 @@ func clearByResourceType(appName string, processType string, resourceType string
 			return
 		}
 		for key := range resources {
+			if !isResourceTypeKey(key, resourceType) {
+				continue
+			}
 			common.PropertyDelete("resource", appName, key)
 		}
 	} else {

plugins/resource/functions_test.go

@@ -53,3 +53,30 @@ func TestFormatResourceValueForDocker(t *testing.T) {
 		})
 	}
 }
+
+func TestIsResourceTypeKey(t *testing.T) {
+	testCases := []struct {
+		name         string
+		key          string
+		resourceType string
+		expected     bool
+	}{
+		{name: "default limit key", key: "_default_.limit.memory", resourceType: "limit", expected: true},
+		{name: "process limit key", key: "web.limit.cpu", resourceType: "limit", expected: true},
+		{name: "process reserve key", key: "web.reserve.memory", resourceType: "reserve", expected: true},
+		{name: "dotted process type", key: "foo.bar.limit.memory", resourceType: "limit", expected: true},
+		{name: "reserve key does not match limit", key: "_default_.reserve.memory", resourceType: "limit", expected: false},
+		{name: "limit key does not match reserve", key: "web.limit.memory", resourceType: "reserve", expected: false},
+		{name: "key without process type", key: "limit.memory", resourceType: "limit", expected: false},
+		{name: "key with a single part", key: "limit", resourceType: "limit", expected: false},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			value := isResourceTypeKey(testCase.key, testCase.resourceType)
+			if value != testCase.expected {
+				t.Fatalf("expected %v for %s/%s, got %v", testCase.expected, testCase.key, testCase.resourceType, value)
+			}
+		})
+	}
+}

plugins/resource/src/commands/commands.go

@@ -19,10 +19,10 @@ Additional commands:`
 
 	helpContent = `
     resource:limit [--process-type <process-type>] [RESOURCE_OPTS...] <app>, Limit resources for a given app/process-type combination
-    resource:limit-clear [--process-type <process-type>] <app>, Limit resources for a given app/process-type combination
+    resource:limit-clear [--process-type <process-type>] <app>, Clear resource limits for a given app/process-type combination
     resource:report [<app>] [<flag>], Displays a resource report for one or more apps
     resource:reserve [--process-type <process-type>] [RESOURCE_OPTS...] <app>, Reserve resources for a given app/process-type combination
-    resource:reserve-clear [--process-type <process-type>] <app>, Reserve resources for a given app/process-type combination`
+    resource:reserve-clear [--process-type <process-type>] <app>, Clear resource reservations for a given app/process-type combination`
 )
 
 func main() {

tests/unit/resource_1.bats

@@ -190,3 +190,52 @@ teardown() {
   echo "status: $status"
   assert_failure
 }
+
+@test "(resource) resource:limit-clear preserves reservations" {
+  run /bin/bash -c "dokku resource:limit --memory 512m $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:limit --memory 384m --process-type web $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:reserve --memory 256m $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:reserve --memory 128m --process-type web $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:limit-clear $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  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-web.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_success
+  assert_output "256m"
+
+  run /bin/bash -c "dokku resource:report --resource-web.reserve.memory $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "128m"
+}

tests/unit/resource_2.bats

@@ -149,3 +149,52 @@ teardown() {
   echo "status: $status"
   assert_failure
 }
+
+@test "(resource) resource:reserve-clear preserves limits" {
+  run /bin/bash -c "dokku resource:limit --memory 512m $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:limit --memory 384m --process-type web $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:reserve --memory 256m $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:reserve --memory 128m --process-type web $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku resource:reserve-clear $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  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:report --resource-web.reserve.memory $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_success
+  assert_output "512m"
+
+  run /bin/bash -c "dokku resource:report --resource-web.limit.memory $TEST_APP"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "384m"
+}