Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix defective revision can lead to pods never being removed #14573

Closed
wants to merge 4 commits into from
Closed

Fix defective revision can lead to pods never being removed #14573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
905e516
Take into account the revisionTimeout field instead of the progressDe…
jsanin-vmw Feb 23, 2023
18761cb
Set revision-timeout as an annotation in the PA
jsanin Jan 31, 2024
1218843
Update unit tests
jsanin Jan 31, 2024
b570ea6
Add documentation to functions and const
jsanin Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,11 @@ func (pa *PodAutoscaler) ProgressDeadline() (time.Duration, bool) {
return pa.annotationDuration(serving.ProgressDeadlineAnnotation)
}

// RevisionTimeout returns the revision timeout annotation value, or false if not present.
func (pa *PodAutoscaler) RevisionTimeout() (time.Duration, bool) {
return pa.annotationDuration(serving.RevisionTimeoutAnnotation)
}

// InitialScale returns the initial scale on the revision if present, or false if not present.
func (pa *PodAutoscaler) InitialScale() (int32, bool) {
// The value is validated in the webhook.
Expand All @@ -187,6 +192,12 @@ func (pa *PodAutoscaler) IsReady() bool {
pas.GetCondition(PodAutoscalerConditionReady).IsTrue()
}

// CanFailActivationOnUnreachableRevision checks whether the pod autoscaler is Unreachable and has been activating
// for at least the specified idle period
func (pa *PodAutoscaler) CanFailActivationOnUnreachableRevision(now time.Time, idlePeriod time.Duration) bool {
return pa.Spec.Reachability == ReachabilityUnreachable && pa.Status.CanFailActivation(now, idlePeriod)
}

// IsActive returns true if the pod autoscaler has finished scaling.
func (pas *PodAutoscalerStatus) IsActive() bool {
return pas.GetCondition(PodAutoscalerConditionActive).IsTrue()
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/serving/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ const (

// ProgressDeadlineAnnotationKey is the label key for the per revision progress deadline to set for the deployment
ProgressDeadlineAnnotationKey = GroupName + "/progress-deadline"

// RevisionTimeoutAnnotationKey is the label key for the revision timeout to set for the pod autoscaler
RevisionTimeoutAnnotationKey = GroupName + "/revision-timeout"
)

var (
Expand Down Expand Up @@ -202,4 +205,7 @@ var (
ProgressDeadlineAnnotation = kmap.KeyPriority{
ProgressDeadlineAnnotationKey,
}
RevisionTimeoutAnnotation = kmap.KeyPriority{
RevisionTimeoutAnnotationKey,
}
)
8 changes: 7 additions & 1 deletion pkg/autoscaler/scaling/autoscaler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,13 @@ func (a *autoscaler) Scale(logger *zap.SugaredLogger, now time.Time) ScaleResult
} else {
logger.Errorw("Failed to obtain metrics", zap.Error(err))
}
return invalidSR
if errors.Is(err, metrics.ErrNoData) && !a.panicTime.IsZero() && a.panicTime.Add(spec.StableWindow).Before(now) {
// We are not receiving data, but we panicked before, and the panic stable window has gone, we want to continue.
// We want to un-panic and scale down to 0 if the rest of the conditions allow it
Comment on lines +173 to +174
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with when and how things panic - can you elaborate on this?

Why do we only do this when the scaler panics - why not do something like this when it doesn't panic?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenario is something like this:

  • we have a failing revision (which is what was reported in the bug)
  • this revision got a burst in requests, entered in panic mode.
  • the stable window is 60secs by default.
  • the revision has a custom revisionTimeout less than 60secs ( by default the revisionTimeout is 300 secs). This means requests in the activator will timeout after revisionTimeout.
  • when requests timed out, metrics will be reported. As there are not requests pending, the autoscaler would want to scale down to 0, but because we are still in panic mode, this will be ignored and the desiredPanicPodCount will be applied instead.
  • no metrics will be reported anymore, this condition will always be true:
if err != nil 
   if errors.Is(err, metrics.ErrNoData)

a return invalidSR will be returned always and no change to the decider.Status.DesiredScale will be made

This causes the failing revision never will be scale down to 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when requests timed out, metrics will be reported. As there are not requests pending, the autoscaler would want to scale down to 0, but because we are still in panic mode, this will be ignored and the desiredPanicPodCount will be applied instead.

what is the value of desiredPanicPodCount ? I'm wondering in this scenario if we should opt of panic'ing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hard to say, it depends on the calculations done in the function. In line 205 you can see this

	// We want to keep desired pod count in the  [maxScaleDown, maxScaleUp] range.
	desiredStablePodCount := int32(math.Min(math.Max(dspc, maxScaleDown), maxScaleUp))
	desiredPanicPodCount := int32(math.Min(math.Max(dppc, maxScaleDown), maxScaleUp))

dppc is calculated like (line 194)

	dspc := math.Ceil(observedStableValue / spec.TargetValue)
	dppc := math.Ceil(observedPanicValue / spec.TargetValue)

and these values comes from the metricClient (line 163)

observedStableValue, observedPanicValue, err = a.metricClient.StableAndPanicConcurrency(metricKey, now)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run a manual test again.
I setup a ksvc revision with containerConcurrency: 2. This is a failing revision, it will not start up.
This revision also has timeoutSeconds: 30, which means that request at the activator will timeout after 30 seconds.

I created 4 concurrent requests, which made the autoscaler to determine that 3 pods were needed.

For some seconds these were the values for

desiredStablePodCount = 3, desiredPanicPodCount = 3

I saw as well the message Operating in panic mode. during this period of time.

This block of code makes sure that a.maxPanicPods is the greater of desiredStablePodCount and desiredPanicPodCount. Which at this point is 3.

At some point the values changed to

desiredStablePodCount = 3, desiredPanicPodCount = 2

and finally to

desiredStablePodCount = 3, desiredPanicPodCount = 0

then it moved to

desiredStablePodCount = 2, desiredPanicPodCount = 0

but this decision is skipped due to
Skipping pod count decrease from 3 to 2. (see condition in the above code)

then, even though the values are:

desiredStablePodCount = 0, desiredPanicPodCount = 0

I still see the skipping Skipping pod count decrease from 3 to 0. Because we are in panic mode and we do not decrease values when we are in panic mode.
We are getting to desiredStablePodCount = 0 because there are no more requests at the activator (they timed out)

Without the changes in this PR. No more metrics will be received from now on for this revision. And this block of code will always be true.

	if err != nil {
		if errors.Is(err, metrics.ErrNoData) {
			logger.Debug("No data to scale on yet")
		} else {
			logger.Errorw("Failed to obtain metrics", zap.Error(err))
		}
		return invalidSR
	}

And the revision will remain in panic mode from now on.

The new condition aims for allowing another execution of the function if the revision panicked and the stable window has gone so the revision will scale down.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this change is needed. Let me know your thoughts about the above explanation.

logger.Debug("No data to scale on yet, and panicked before, but the stable window has gone, we want to continue")
} else {
return invalidSR
}
}

// Make sure we don't get stuck with the same number of pods, if the scale up rate
Expand Down
5 changes: 4 additions & 1 deletion pkg/reconciler/autoscaling/kpa/kpa_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ const (
paStableWindow = 45 * time.Second
progressDeadline = 121 * time.Second
stableWindow = 5 * time.Minute
defaultRevisionTimeout = 45
)

func defaultConfigMapData() map[string]string {
Expand Down Expand Up @@ -1782,7 +1783,9 @@ func newTestRevision(namespace, name string) *v1.Revision {
serving.ConfigurationLabelKey: "test-service",
},
},
Spec: v1.RevisionSpec{},
Spec: v1.RevisionSpec{
TimeoutSeconds: ptr.Int64(defaultRevisionTimeout),
},
}
}

Expand Down
3 changes: 3 additions & 0 deletions pkg/reconciler/autoscaling/kpa/scaler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,9 @@ func (ks *scaler) handleScaleToZero(ctx context.Context, pa *autoscalingv1alpha1
if pa.Status.CanFailActivation(now, activationTimeout) {
logger.Info("Activation has timed out after ", activationTimeout)
return desiredScale, true
} else if revTimeout, ok := pa.RevisionTimeout(); ok && pa.CanFailActivationOnUnreachableRevision(now, revTimeout) {
logger.Info("Activation has timed out after revision-timeout ", revTimeout)
return desiredScale, true
Comment on lines +204 to +206
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After spending time thinking about this I don't think we should be failing the revision based on the desired request timeout. The user should be setting the progressdeadline annotation directly if they want to lower this value.

Also there are valid use cases where spinning up a new 'unreachable' revision is the norm. eg. I have all my traffic pointing to an older revision and I'm applying changes to my service spec.template.spec to rollout new changes by creating new revisions. In my manual testing the changes in this PR will cause the newer unreachable revisions to fail - when normally they should spin up and then scale to zero. The change here short-circuits that.

}
ks.enqueueCB(pa, activationTimeout)
return scaleUnknown, false
Expand Down
61 changes: 45 additions & 16 deletions pkg/reconciler/revision/resources/deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ import (
. "knative.dev/serving/pkg/testing/v1"
)

const (
defaultRevisionTimeout = 45
defaultRevisionTimeoutStr = "45s"
)

var (
servingContainerName = "serving-container"
sidecarContainerName = "sidecar-container-1"
Expand Down Expand Up @@ -195,7 +200,7 @@ var (
}

defaultPodSpec = &corev1.PodSpec{
TerminationGracePeriodSeconds: refInt64(45),
TerminationGracePeriodSeconds: ptr.Int64(45),
EnableServiceLinks: ptr.Bool(false),
}

Expand Down Expand Up @@ -273,15 +278,11 @@ func defaultRevision() *v1.Revision {
UID: "1234",
},
Spec: v1.RevisionSpec{
TimeoutSeconds: ptr.Int64(45),
TimeoutSeconds: ptr.Int64(defaultRevisionTimeout),
},
}
}

func refInt64(num int64) *int64 {
return &num
}

type containerOption func(*corev1.Container)
type podSpecOption func(*corev1.PodSpec)
type deploymentOption func(*appsv1.Deployment)
Expand Down Expand Up @@ -1425,7 +1426,10 @@ func TestMakeDeployment(t *testing.T) {
}, {
ImageDigest: "ubuntu@sha256:deadbffe",
}})),
want: appsv1deployment(),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Annotations = map[string]string{serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
deploy.Spec.Template.Annotations = map[string]string{serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
}),
}, {
name: "with owner",
rev: revision("bar", "foo",
Expand All @@ -1439,7 +1443,10 @@ func TestMakeDeployment(t *testing.T) {
WithContainerStatuses([]v1.ContainerStatus{{
ImageDigest: "busybox@sha256:deadbeef",
}})),
want: appsv1deployment(),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Annotations = map[string]string{serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
deploy.Spec.Template.Annotations = map[string]string{serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
}),
}, {
name: "with sidecar annotation override",
rev: revision("bar", "foo",
Expand All @@ -1458,9 +1465,13 @@ func TestMakeDeployment(t *testing.T) {
}),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Annotations = kmeta.UnionMaps(deploy.Annotations,
map[string]string{sidecarIstioInjectAnnotation: "false"})
map[string]string{
sidecarIstioInjectAnnotation: "false",
serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr})
deploy.Spec.Template.Annotations = kmeta.UnionMaps(deploy.Spec.Template.Annotations,
map[string]string{sidecarIstioInjectAnnotation: "false"})
map[string]string{
sidecarIstioInjectAnnotation: "false",
serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr})
}),
}, {
name: "with progress-deadline override",
Expand All @@ -1478,6 +1489,8 @@ func TestMakeDeployment(t *testing.T) {
}}), withoutLabels),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Spec.ProgressDeadlineSeconds = ptr.Int32(42)
deploy.Annotations = map[string]string{serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
deploy.Spec.Template.Annotations = map[string]string{serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
}),
}, {
name: "with progress-deadline annotation",
Expand All @@ -1493,8 +1506,13 @@ func TestMakeDeployment(t *testing.T) {
}}), withoutLabels),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Spec.ProgressDeadlineSeconds = ptr.Int32(42)
deploy.Annotations = map[string]string{serving.ProgressDeadlineAnnotationKey: "42s"}
deploy.Spec.Template.Annotations = map[string]string{serving.ProgressDeadlineAnnotationKey: "42s"}
deploy.Annotations = map[string]string{
serving.ProgressDeadlineAnnotationKey: "42s",
serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
deploy.Spec.Template.Annotations = map[string]string{
serving.ProgressDeadlineAnnotationKey: "42s",
serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}

}),
}, {
name: "with ProgressDeadline annotation and configmap override",
Expand All @@ -1513,8 +1531,12 @@ func TestMakeDeployment(t *testing.T) {
}}), withoutLabels),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Spec.ProgressDeadlineSeconds = ptr.Int32(42)
deploy.Annotations = map[string]string{serving.ProgressDeadlineAnnotationKey: "42s"}
deploy.Spec.Template.Annotations = map[string]string{serving.ProgressDeadlineAnnotationKey: "42s"}
deploy.Annotations = map[string]string{
serving.ProgressDeadlineAnnotationKey: "42s",
serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
deploy.Spec.Template.Annotations = map[string]string{
serving.ProgressDeadlineAnnotationKey: "42s",
serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
}),
}, {
name: "cluster initial scale",
Expand All @@ -1531,6 +1553,9 @@ func TestMakeDeployment(t *testing.T) {
),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Spec.Replicas = ptr.Int32(int32(10))
deploy.Annotations = map[string]string{serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
deploy.Spec.Template.Annotations = map[string]string{serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}

}),
}, {
name: "cluster initial scale override by revision initial scale",
Expand All @@ -1550,8 +1575,12 @@ func TestMakeDeployment(t *testing.T) {
),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Spec.Replicas = ptr.Int32(int32(20))
deploy.Spec.Template.Annotations = map[string]string{autoscaling.InitialScaleAnnotationKey: "20"}
deploy.Annotations = map[string]string{autoscaling.InitialScaleAnnotationKey: "20"}
deploy.Spec.Template.Annotations = map[string]string{
autoscaling.InitialScaleAnnotationKey: "20",
serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
deploy.Annotations = map[string]string{
autoscaling.InitialScaleAnnotationKey: "20",
serving.RevisionTimeoutAnnotationKey: defaultRevisionTimeoutStr}
}),
}}

Expand Down
8 changes: 7 additions & 1 deletion pkg/reconciler/revision/resources/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check failure on line 1 in pkg/reconciler/revision/resources/meta.go

View workflow job for this annotation

GitHub Actions / style / Golang / Auto-format and Check 

Please run goimports.
diff --git a/pkg/reconciler/revision/resources/meta.go b/pkg/reconciler/revision/resources/meta.go
index e357cfd..ce0918f 100644
--- a/pkg/reconciler/revision/resources/meta.go
+++ b/pkg/reconciler/revision/resources/meta.go
@@ -17,13 +17,14 @@ limitations under the License.
 package resources
 
 import (
+	"strconv"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"knative.dev/pkg/kmap"
 	"knative.dev/pkg/kmeta"
 	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
-	"strconv"
 )
 
 var (
Copyright 2018 The Knative Authors

Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -19,9 +19,11 @@
import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"knative.dev/pkg/kmap"
"knative.dev/pkg/kmeta"
"knative.dev/serving/pkg/apis/serving"
v1 "knative.dev/serving/pkg/apis/serving/v1"
"strconv"
)

var (
Expand Down Expand Up @@ -56,7 +58,11 @@
}

func makeAnnotations(revision *v1.Revision) map[string]string {
return kmeta.FilterMap(revision.GetAnnotations(), excludeAnnotations.Has)
annotations := kmap.Filter(revision.GetAnnotations(), excludeAnnotations.Has)
if revision.Spec.TimeoutSeconds != nil {
annotations[serving.RevisionTimeoutAnnotation.Key()] = strconv.FormatInt(*revision.Spec.TimeoutSeconds, 10) + "s"
}
return annotations
}

// makeSelector constructs the Selector we will apply to K8s resources.
Expand Down
Loading