From 39037b8799ebbf3c9c3b8f3322516333b3df40cb Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Mon, 7 Oct 2024 13:24:44 +0300 Subject: [PATCH] fixes --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 6 ++-- pkg/apis/serving/v1/revision_lifecycle.go | 14 ++++---- pkg/reconciler/autoscaling/kpa/scaler.go | 2 +- pkg/reconciler/revision/table_test.go | 32 +++++++++++++++++++ pkg/testing/functional.go | 7 ++++ pkg/testing/v1/revision.go | 6 ++++ 6 files changed, 56 insertions(+), 11 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index b4c35c188b7f..fa433a7cb2a2 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -216,9 +216,9 @@ func (pas *PodAutoscalerStatus) MarkScaleTargetInitialized() { podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionScaleTargetInitialized) } -// ScaleTargetNotScaledAfterFailure returns true if the PodAutoscaler's scale target has been -// scaled successfully. -func (pas *PodAutoscalerStatus) ScaleTargetNotScaledAfterFailure() bool { +// ScaleTargetNotScaled returns true if the PodAutoscaler's scale target has failed +// to scaled. +func (pas *PodAutoscalerStatus) ScaleTargetNotScaled() bool { return pas.GetCondition(PodAutoscalerConditionScaleTargetScaled).IsFalse() } diff --git a/pkg/apis/serving/v1/revision_lifecycle.go b/pkg/apis/serving/v1/revision_lifecycle.go index a5fb242e5f5b..6924635cc328 100644 --- a/pkg/apis/serving/v1/revision_lifecycle.go +++ b/pkg/apis/serving/v1/revision_lifecycle.go @@ -200,6 +200,13 @@ func (rs *RevisionStatus) PropagateAutoscalerStatus(ps *autoscalingv1alpha1.PodA // that implies that |service.endpoints| > 0. rs.MarkResourcesAvailableTrue() rs.MarkContainerHealthyTrue() + + // Mark resource unavailable if we are scaling back to zero, but we never achieved the required scale + // and deployment status was not updated properly by K8s. For example due to an image pull error. + if ps.ScaleTargetNotScaled() { + condScaled := ps.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionScaleTargetScaled) + rs.MarkResourcesAvailableFalse(condScaled.Reason, condScaled.Message) + } } // Mark resource unavailable if we don't have a Service Name and the deployment is ready @@ -234,13 +241,6 @@ func (rs *RevisionStatus) PropagateAutoscalerStatus(ps *autoscalingv1alpha1.PodA case corev1.ConditionTrue: rs.MarkActiveTrue() } - - // Mark resource unavailable if we scaled back to zero, but we never achieved the required scale - // and deployment status was not updated properly byt K8s. For example due to a temporary image pull error. - if ps.IsScaleTargetInitialized() && !resUnavailable && ps.ScaleTargetNotScaledAfterFailure() { - condScaled := ps.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionScaleTargetScaled) - rs.MarkResourcesAvailableFalse(condScaled.Reason, condScaled.Message) - } } // ResourceNotOwnedMessage constructs the status message if ownership on the diff --git a/pkg/reconciler/autoscaling/kpa/scaler.go b/pkg/reconciler/autoscaling/kpa/scaler.go index 4db7c7d4db63..ac36a045118b 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler.go +++ b/pkg/reconciler/autoscaling/kpa/scaler.go @@ -388,7 +388,7 @@ func (ks *scaler) scale(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscal if err != nil { return desiredScale, fmt.Errorf("error getting pod counts: %w", err) } - if ready == 0 { + if ready == 0 && currentScale > 0 { var pod *corev1.Pod pod, err = podCounter.GetAnyPod() if err != nil { diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index c1c1b810216c..4a7fdc7559d6 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -41,6 +41,7 @@ import ( tracingconfig "knative.dev/pkg/tracing/config" autoscalingv1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1" defaultconfig "knative.dev/serving/pkg/apis/config" + "knative.dev/serving/pkg/apis/serving" v1 "knative.dev/serving/pkg/apis/serving/v1" "knative.dev/serving/pkg/autoscaler/config/autoscalerconfig" servingclient "knative.dev/serving/pkg/client/injection/client" @@ -744,6 +745,37 @@ func TestReconcile(t *testing.T) { PodSpecPersistentVolumeClaim: defaultconfig.Enabled, PodSpecPersistentVolumeWrite: defaultconfig.Enabled, }}), + }, { + Name: "revision becomes not ready when PA reports a failed container", + Objects: []runtime.Object{ + Revision("foo", "container-failed", + WithLogURL, + MarkRevisionReady, + withDefaultContainerStatuses(), + WithRevisionLabel(serving.RoutingStateLabelKey, "active"), + ), + pa("foo", "container-failed", + WithPAStatusScaleTargetScaleFailures("ImagePullBackOff", "Back-off pulling image..."), + WithScaleTargetInitialized, + WithTraffic, + WithReachabilityReachable, + WithPAStatusService("something"), + ), + readyDeploy(deploy(t, "foo", "container-failed")), + image("foo", "container-failed"), + }, + Key: "foo/container-failed", + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: Revision("foo", "container-failed", + WithLogURL, + MarkInactive("ImagePullBackOff", "Back-off pulling image..."), + MarkResourcesUnavailable("ImagePullBackOff", "Back-off pulling image..."), + MarkContainerHealthy(), + withDefaultContainerStatuses(), + WithRevisionObservedGeneration(1), + WithRevisionLabel(serving.RoutingStateLabelKey, "active"), + ), + }}, }} table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, _ configmap.Watcher) controller.Reconciler { diff --git a/pkg/testing/functional.go b/pkg/testing/functional.go index 904dde5939f5..3b2a15a109ce 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -113,6 +113,13 @@ func WithScaleTargetScaleFailures(pa *autoscalingv1alpha1.PodAutoscaler, reason, pa.Status.MarkWithScaleFailures(reason, message) } +// WithPAStatusScaleTargetScaleFailures updates the PA to reflect that there were scale failures +func WithPAStatusScaleTargetScaleFailures(reason, message string) PodAutoscalerOption { + return func(pa *autoscalingv1alpha1.PodAutoscaler) { + pa.Status.MarkWithScaleFailures(reason, message) + } +} + // WithPAStatusService annotates PA Status with the provided service name. func WithPAStatusService(svc string) PodAutoscalerOption { return func(pa *autoscalingv1alpha1.PodAutoscaler) { diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go index a475bad0aa40..491ab69422e5 100644 --- a/pkg/testing/v1/revision.go +++ b/pkg/testing/v1/revision.go @@ -158,6 +158,12 @@ func MarkContainerHealthyUnknown(reason string) RevisionOption { } } +func MarkContainerHealthy() RevisionOption { + return func(r *v1.Revision) { + r.Status.MarkContainerHealthyTrue() + } +} + // MarkProgressDeadlineExceeded calls the method of the same name on the Revision // with the message we expect the Revision Reconciler to pass. func MarkProgressDeadlineExceeded(message string) RevisionOption {