Do not override condition in buffers limits translator

Limits translator overrides NotReadyForProvisioning condition, which
contains an error message from the previous translators. That hides the
error from the user, making it harder to troubleshoot issues.

Author: norbertcyran

cluster-autoscaler/capacitybuffer/testutil/testutil.go

@@ -96,10 +96,15 @@ func GetBufferStatus(podTempRef *v1.LocalObjectRef, replicas *int32, podTemplate
 
 // GetConditionReady returns a list of conditions with a condition ready and empty message, should be used for testing purposes only
 func GetConditionReady() []metav1.Condition {
+	return GetConditionReadyWithMessage("")
+}
+
+// GetConditionReadyWithMessage returns a list of conditions with a condition ready and the specified message
+func GetConditionReadyWithMessage(message string) []metav1.Condition {
 	readyCondition := metav1.Condition{
 		Type:               capacitybuffer.ReadyForProvisioningCondition,
 		Status:             metav1.ConditionTrue,
-		Message:            "",
+		Message:            message,
 		Reason:             capacitybuffer.AttributesSetSuccessfullyReason,
 		LastTransitionTime: metav1.Time{},
 	}
@@ -108,10 +113,15 @@ func GetConditionReady() []metav1.Condition {
 
 // GetConditionNotReady returns a list of conditions with a condition not ready and empty message, should be used for testing purposes only
 func GetConditionNotReady() []metav1.Condition {
+	return GetConditionNotReadyWithMessage("")
+}
+
+// GetConditionNotReadyWithMessage returns a list of condition with a condition not ready and specified message.
+func GetConditionNotReadyWithMessage(message string) []metav1.Condition {
 	notReadyCondition := metav1.Condition{
 		Type:               capacitybuffer.ReadyForProvisioningCondition,
 		Status:             metav1.ConditionFalse,
-		Message:            "",
+		Message:            message,
 		Reason:             "error",
 		LastTransitionTime: metav1.Time{},
 	}
@@ -155,6 +165,13 @@ func WithReplicas(replicas int32) BufferOption {
 	}
 }
 
+// WithLimits sets the Spec.Limits
+func WithLimits(limits v1.ResourceList) BufferOption {
+	return func(b *v1.CapacityBuffer) {
+		b.Spec.Limits = &limits
+	}
+}
+
 // WithStatusPodTemplateRef sets the Status.PodTemplateRef
 func WithStatusPodTemplateRef(name string) BufferOption {
 	return func(b *v1.CapacityBuffer) {

cluster-autoscaler/capacitybuffer/translators/resource_limits_translator.go

@@ -17,10 +17,13 @@ limitations under the License.
 package translator
 
 import (
+	"errors"
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	api_v1 "k8s.io/autoscaler/cluster-autoscaler/apis/capacitybuffer/autoscaling.x-k8s.io/v1beta1"
+	"k8s.io/autoscaler/cluster-autoscaler/capacitybuffer"
 	cbclient "k8s.io/autoscaler/cluster-autoscaler/capacitybuffer/client"
 	"k8s.io/autoscaler/cluster-autoscaler/capacitybuffer/common"
 )
@@ -39,36 +42,35 @@ func NewResourceLimitsTranslator(client *cbclient.CapacityBufferClient) *resourc
 
 // Translate translates buffers processors into pod capacity.
 func (t *resourceLimitsTranslator) Translate(buffers []*api_v1.CapacityBuffer) []error {
-	errors := []error{}
+	var errs []error
 	for _, buffer := range buffers {
 		if isResourcesLimitsDefinedInBuffer(buffer) {
-			if buffer.Status.PodTemplateRef == nil {
-				err := fmt.Errorf("Can't get pod template, PodTemplateRef is nil")
-				common.SetBufferAsNotReadyForProvisioning(buffer, nil, nil, nil, buffer.Spec.ProvisioningStrategy, err)
-				errors = append(errors, err)
+			if buffer.Status.PodTemplateRef == nil || buffer.Status.PodTemplateGeneration == nil || meta.IsStatusConditionFalse(buffer.Status.Conditions, capacitybuffer.ReadyForProvisioningCondition) {
+				// that means that previous translators failed to resolve the pod template, we don't want to override
+				// the condition here in order to keep the error message from previous translators.
 				continue
 			}
 			podTemplate, err := t.client.GetPodTemplate(buffer.Namespace, buffer.Status.PodTemplateRef.Name)
 			if err != nil {
 				err = fmt.Errorf("Couldn't get pod template, error: %v", err)
 				common.SetBufferAsNotReadyForProvisioning(buffer, nil, nil, nil, buffer.Spec.ProvisioningStrategy, err)
-				errors = append(errors, err)
+				errs = append(errs, err)
 				continue
 			}
 			numberOfPods := limitNumberOfPodsForResource(podTemplate, *buffer.Spec.Limits)
 			if numberOfPods == nil {
-				err := fmt.Errorf("Couldn't calculate number of pods for buffer %v based on provided resource limits %v", buffer.Name, *buffer.Spec.Limits)
+				err := errors.New("couldn't calculate number of pods for buffer based on provided resource limits. Check if the pod template requests at least one limited resource")
 				common.SetBufferAsNotReadyForProvisioning(buffer, nil, nil, nil, buffer.Spec.ProvisioningStrategy, err)
-				errors = append(errors, err)
+				// this error is expected when the buffer is misconfigured, so we don't want it to trigger requeue
 				continue
 			}
 			if buffer.Status.Replicas != nil {
 				numberOfPods = pointerToInt32(min(*buffer.Status.Replicas, *numberOfPods))
 			}
-			common.SetBufferAsReadyForProvisioning(buffer, buffer.Status.PodTemplateRef, &podTemplate.Generation, numberOfPods, buffer.Spec.ProvisioningStrategy)
+			common.SetBufferAsReadyForProvisioning(buffer, buffer.Status.PodTemplateRef, buffer.Status.PodTemplateGeneration, numberOfPods, buffer.Spec.ProvisioningStrategy)
 		}
 	}
-	return errors
+	return errs
 }
 
 func limitNumberOfPodsForResource(podTemplate *corev1.PodTemplate, limits api_v1.ResourceList) *int32 {

cluster-autoscaler/capacitybuffer/translators/resource_limits_translator_test.go

@@ -19,15 +19,17 @@ package translator
 import (
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	fakeClient "k8s.io/client-go/kubernetes/fake"
-
-	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/autoscaler/cluster-autoscaler/apis/capacitybuffer/autoscaling.x-k8s.io/v1beta1"
 	cbclient "k8s.io/autoscaler/cluster-autoscaler/capacitybuffer/client"
 	"k8s.io/autoscaler/cluster-autoscaler/capacitybuffer/testutil"
+	fakeClient "k8s.io/client-go/kubernetes/fake"
+	"k8s.io/utils/ptr"
 )
 
 func TestResourceLimitsTranslator(t *testing.T) {
@@ -46,6 +48,7 @@ func TestResourceLimitsTranslator(t *testing.T) {
 	})
 	fakeClient := fakeClient.NewSimpleClientset(podTemp4mem100cpu, podTemp8mem200cpu, podTemp4gpu)
 	fakeCapacityBuffersClient, _ := cbclient.NewCapacityBufferClient(nil, fakeClient, nil, nil, nil, nil, nil, nil, nil, nil, nil)
+	noResourcesSetMessage := "couldn't calculate number of pods for buffer based on provided resource limits. Check if the pod template requests at least one limited resource"
 
 	tests := []struct {
 		name                   string
@@ -71,9 +74,9 @@ func TestResourceLimitsTranslator(t *testing.T) {
 				}),
 			},
 			expectedStatus: []*v1.CapacityBufferStatus{
-				testutil.GetBufferStatus(nil, nil, nil, &testutil.ProvisioningStrategy, testutil.GetConditionNotReady()),
+				testutil.GetBufferStatus(nil, nil, nil, &testutil.ProvisioningStrategy, nil),
 			},
-			expectedNumberOfErrors: 1,
+			expectedNumberOfErrors: 0,
 		},
 		{
 			name: "Limits exist and no replicas, buffer filtered",
@@ -185,27 +188,56 @@ func TestResourceLimitsTranslator(t *testing.T) {
 				}),
 			},
 			expectedStatus: []*v1.CapacityBufferStatus{
-				testutil.GetBufferStatus(nil, nil, nil, &testutil.ProvisioningStrategy, testutil.GetConditionNotReady()),
+				testutil.GetBufferStatus(
+					nil, nil, nil, &testutil.ProvisioningStrategy, testutil.GetConditionNotReadyWithMessage(noResourcesSetMessage)),
 			},
-			expectedNumberOfErrors: 1,
+			expectedNumberOfErrors: 0,
+		},
+		{
+			name: "conditions are not overridden if buffer is not ready",
+			buffers: []*v1.CapacityBuffer{
+				testutil.NewBuffer(
+					testutil.WithPodTemplateRef(podTemp4mem100cpu.Name),
+					testutil.WithLimits(v1.ResourceList{
+						"nvidia.com/gpu": resource.MustParse("100m"),
+					}),
+					testutil.WithActiveProvisioningStrategy(),
+					func(buffer *v1.CapacityBuffer) {
+						buffer.Status.Conditions = testutil.GetConditionNotReadyWithMessage("test error")
+					},
+				),
+			},
+			expectedStatus: []*v1.CapacityBufferStatus{
+				testutil.GetBufferStatus(nil, nil, nil, nil, testutil.GetConditionNotReadyWithMessage("test error")),
+			},
+			expectedNumberOfErrors: 0,
 		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			translator := NewResourceLimitsTranslator(fakeCapacityBuffersClient)
 			errors := translator.Translate(test.buffers)
 			assert.Equal(t, len(errors), test.expectedNumberOfErrors)
-			assert.ElementsMatch(t, test.expectedStatus, testutil.SanitizeBuffersStatus(test.buffers))
+			for i, buffer := range test.buffers {
+				wantStatus := test.expectedStatus[i]
+				if diff := cmp.Diff(wantStatus, &buffer.Status, cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime")); diff != "" {
+					t.Errorf("buffer status mismatch (-want +got):\n%s", diff)
+				}
+			}
 		})
 	}
 }
 
 func getTestBufferWithLimits(podTemplateRef *v1.LocalObjectRef, replicas *int32, limits *v1.ResourceList) *v1.CapacityBuffer {
-	return testutil.GetBuffer(&testutil.ProvisioningStrategy, nil, nil, podTemplateRef, replicas, nil, nil, limits)
+	var podTemplateGeneration *int64
+	if podTemplateRef != nil {
+		podTemplateGeneration = ptr.To[int64](1)
+	}
+	return testutil.GetBuffer(&testutil.ProvisioningStrategy, nil, nil, podTemplateRef, replicas, podTemplateGeneration, nil, limits)
 }
 
 func getTestBufferStatusWithReplicas(podTemplateRef *v1.LocalObjectRef, replicas int32) *v1.CapacityBufferStatus {
-	return testutil.GetBufferStatus(podTemplateRef, &replicas, pointerToInt64(1), &testutil.ProvisioningStrategy, testutil.GetConditionReady())
+	return testutil.GetBufferStatus(podTemplateRef, &replicas, ptr.To[int64](1), &testutil.ProvisioningStrategy, testutil.GetConditionReadyWithMessage("ready"))
 }
 
 func getPodTemplateWithResources(name string, resources corev1.ResourceList) *corev1.PodTemplate {