fixup

Signed-off-by: Anatolii Bazko <[email protected]>

Author: tolusha

apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go

@@ -183,10 +183,11 @@ type WorkspaceConfig struct {
 	// the value "0" should be used. By default, the memory limit is 128Mi and the memory request is 64Mi.
 	// No CPU limit or request is added by default.
 	DefaultContainerResources *corev1.ResourceRequirements `json:"defaultContainerResources,omitempty"`
-	// ContainerResourceCaps defines the maximum resource requirements enforced for all
-	// workspace containers. If a container specifies higher limits or requests, they
-	// will be capped at these maximum values. This is not applied to initContainers or
-	// the projectClone container.
+	// ContainerResourceCaps defines the maximum resource requirements enforced for workspace
+	// containers. If a container specifies limits or requests that exceed these values, they
+	// will be capped at the maximum. Note: Caps only apply when resources are already specified
+	// on a container. For containers without resource specifications, use DefaultContainerResources
+	// instead. These resource caps do not apply to initContainers or the projectClone container.
 	ContainerResourceCaps *corev1.ResourceRequirements `json:"containerResourceCaps,omitempty"`
 	// PodAnnotations defines the metadata.annotations for DevWorkspace pods created by the DevWorkspace Operator.
 	PodAnnotations map[string]string `json:"podAnnotations,omitempty"`

deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml

@@ -231,37 +231,21 @@ func ApplyCaps(resources, caps *corev1.ResourceRequirements) *corev1.ResourceReq
 		if capLimit.IsZero() {
 			continue
 		}
-		if result.Limits == nil {
-			result.Limits = corev1.ResourceList{}
-		}
 		existingLimit, hasExisting := result.Limits[resourceName]
-		if !hasExisting || existingLimit.IsZero() {
-			// No existing limit, use caps as maximum
-			result.Limits[resourceName] = capLimit
-		} else if existingLimit.Cmp(capLimit) > 0 {
-			// Existing limit is higher than caps, apply caps maximum
+		if hasExisting && existingLimit.Cmp(capLimit) > 0 {
 			result.Limits[resourceName] = capLimit
 		}
-		// Otherwise, keep existing limit (it's already lower than or equal to caps)
 	}
 
 	// Apply caps requests as maximum values (use the smaller of existing and caps)
 	for resourceName, capRequest := range caps.Requests {
 		if capRequest.IsZero() {
 			continue
 		}
-		if result.Requests == nil {
-			result.Requests = corev1.ResourceList{}
-		}
 		existingRequest, hasExisting := result.Requests[resourceName]
-		if !hasExisting || existingRequest.IsZero() {
-			// No existing request, use caps as maximum
-			result.Requests[resourceName] = capRequest
-		} else if existingRequest.Cmp(capRequest) > 0 {
-			// Existing request is higher than caps, apply caps maximum
+		if hasExisting && existingRequest.Cmp(capRequest) > 0 {
 			result.Requests[resourceName] = capRequest
 		}
-		// Otherwise, keep existing request (it's already lower than or equal to caps)
 	}
 
 	result = handleCapsEdgeCase(result, caps, corev1.ResourceMemory)
@@ -291,7 +275,8 @@ func handleCapsEdgeCase(resources, caps *corev1.ResourceRequirements, resourceNa
 			}
 		case capResLimit.IsZero() && !capResRequest.IsZero():
 			// Only a resource request cap was set, and it's higher than the existing limit.
-			// Adjust the limit up to match the capped request.
+			// It means, that invalid state (limit < request) existed in the original resources before caps were applied.
+			// Since the request is adjusted by the caps, we also adjust the limit up to match the capped request.
 			if resRequest.Equal(capResRequest) {
 				result.Limits[resourceName] = capResRequest
 			} else {

deploy/deployment/kubernetes/combined.yaml

@@ -312,22 +312,16 @@ func TestApplyCaps(t *testing.T) {
 		expected *corev1.ResourceRequirements
 	}{
 		{
-			name:     "Applies all caps values to empty resources",
+			name:     "Doesn't apply all caps values to empty resources",
 			base:     &corev1.ResourceRequirements{},
 			caps:     getResourceRequirements("2000Mi", "200Mi", "2000m", "200m"),
-			expected: getResourceRequirements("2000Mi", "200Mi", "2000m", "200m"),
-		},
-		{
-			name:     "Applies all caps values",
-			base:     getResourceRequirements("3000Mi", "500Mi", "3000m", "500m"),
-			caps:     getResourceRequirements("2000Mi", "200Mi", "2000m", "200m"),
-			expected: getResourceRequirements("2000Mi", "200Mi", "2000m", "200m"),
+			expected: &corev1.ResourceRequirements{},
 		},
 		{
-			name:     "Keeps existing values when within caps bounds",
-			base:     getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
+			name:     "Doesn't apply all caps values to '0' resources",
+			base:     getResourceRequirements("0", "0", "0", "0"),
 			caps:     getResourceRequirements("2000Mi", "200Mi", "2000m", "200m"),
-			expected: getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
+			expected: getResourceRequirements("0", "0", "0", "0"),
 		},
 		{
 			name:     "Does not apply empty caps fields",
@@ -336,28 +330,40 @@ func TestApplyCaps(t *testing.T) {
 			expected: getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
 		},
 		{
-			name:     "Handles nil caps",
+			name:     "Does not apply nil caps",
 			base:     getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
 			caps:     nil,
 			expected: getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
 		},
 		{
-			name:     "Ignores '0' fields in caps",
+			name:     "Does not apply '0' caps",
 			base:     getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
 			caps:     getResourceRequirements("0", "0", "0", "0"),
 			expected: getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
 		},
+		{
+			name:     "Applies all caps values",
+			base:     getResourceRequirements("3000Mi", "500Mi", "3000m", "500m"),
+			caps:     getResourceRequirements("2000Mi", "200Mi", "2000m", "200m"),
+			expected: getResourceRequirements("2000Mi", "200Mi", "2000m", "200m"),
+		},
+		{
+			name:     "Keeps existing values when within caps bounds",
+			base:     getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
+			caps:     getResourceRequirements("2000Mi", "200Mi", "2000m", "200m"),
+			expected: getResourceRequirements("1000Mi", "100Mi", "1000m", "100m"),
+		},
 		{
 			name:     "Adjusts request when caps limit creates conflict",
-			base:     getResourceRequirements("2000Mi", "1000Mi", "", "1000m"),
+			base:     getResourceRequirements("2000Mi", "1000Mi", "2000m", "1000m"),
 			caps:     getResourceRequirements("500Mi", "", "500m", ""),
 			expected: getResourceRequirements("500Mi", "500Mi", "500m", "500m"),
 		},
 		{
 			name:     "Adjusts limit when caps request creates conflict",
-			base:     getResourceRequirements("500Mi", "", "500m", ""),
-			caps:     getResourceRequirements("", "1000Mi", "", "1000m"),
-			expected: getResourceRequirements("1000Mi", "1000Mi", "1000m", "1000m"),
+			base:     getResourceRequirements("1000Mi", "2000Mi", "1000m", "2000m"),
+			caps:     getResourceRequirements("", "1500Mi", "", "1500m"),
+			expected: getResourceRequirements("1500Mi", "1500Mi", "1500m", "1500m"),
 		},
 	}
 	for _, tt := range tests {