Merge pull request #9269 from norbertcyran/buffers-limits-hijack-condition

Do not override condition in buffers limits translator

Author: k8s-ci-robot

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 {