Skip to content

Commit

Permalink
fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
skonto committed Jan 20, 2025
1 parent 8c18f73 commit 39037b8
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 11 deletions.
6 changes: 3 additions & 3 deletions pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}

Expand Down
14 changes: 7 additions & 7 deletions pkg/apis/serving/v1/revision_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/autoscaling/kpa/scaler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
32 changes: 32 additions & 0 deletions pkg/reconciler/revision/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions pkg/testing/functional.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/testing/v1/revision.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 39037b8

Please sign in to comment.