Adds progressDeadline annotation on per revision basis (#12751)

* adds progressDeadline annotation on per revision basis

Signed-off-by: Paul S. Schweigert <[email protected]>

This PR allows setting the progress-deadline on each revision by passing
an annotation, autoscaling.knative.dev/progressDeadline. It aims to
improve the experience where revisions don't fail until the
progressDeadline is reached, which by default is 10 minutes. Not
everyone necessarily wants to wait that long, but previously the only
resource was for the cluster admin to lower the deadline
universally (i.e. change the default). With this change, we allow users
to "fail fast", if they so desire.

* remove unnecessary conversion

Signed-off-by: Paul S. Schweigert <[email protected]>

* address review pt1

Signed-off-by: Paul S. Schweigert <[email protected]>

* unit test

Signed-off-by: Paul S. Schweigert <[email protected]>

* fixing annotation in tests

Signed-off-by: Paul S. Schweigert <[email protected]>

* update key to serving.knative.dev

Signed-off-by: Paul S. Schweigert <[email protected]>

* move annotation to serving register

Signed-off-by: Paul S. Schweigert <[email protected]>

* validate revisions, not services or routes

Signed-off-by: Paul S. Schweigert <[email protected]>

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -27,6 +27,7 @@ import (
 	"knative.dev/pkg/apis"
 	"knative.dev/pkg/kmap"
 	"knative.dev/serving/pkg/apis/autoscaling"
+	"knative.dev/serving/pkg/apis/serving"
 	"knative.dev/serving/pkg/autoscaler/config/autoscalerconfig"
 )
 
@@ -160,6 +161,12 @@ func (pa *PodAutoscaler) PanicThresholdPercentage() (percentage float64, ok bool
 	return pa.annotationFloat64(autoscaling.PanicThresholdPercentageAnnotation)
 }
 
+// ProgressDeadline returns the progress deadline annotation value, or false if not present.
+func (pa *PodAutoscaler) ProgressDeadline() (time.Duration, bool) {
+	// the value is validated in the webhook
+	return pa.annotationDuration(serving.ProgressDeadlineAnnotation)
+}
+
 // 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.

pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go

@@ -30,6 +30,7 @@ import (
 	apistest "knative.dev/pkg/apis/testing"
 	"knative.dev/pkg/ptr"
 	"knative.dev/serving/pkg/apis/autoscaling"
+	"knative.dev/serving/pkg/apis/serving"
 	"knative.dev/serving/pkg/autoscaler/config/autoscalerconfig"
 )
 
@@ -781,6 +782,53 @@ func TestScaleDownDelayAnnotation(t *testing.T) {
 	}
 }
 
+func TestProgressDelayAnnotation(t *testing.T) {
+	cases := []struct {
+		name      string
+		pa        *PodAutoscaler
+		wantDelay time.Duration
+		wantOK    bool
+	}{{
+		name:      "not present",
+		pa:        pa(map[string]string{}),
+		wantDelay: 0,
+		wantOK:    false,
+	}, {
+		name: "present",
+		pa: pa(map[string]string{
+			serving.ProgressDeadlineAnnotationKey: "120s",
+		}),
+		wantDelay: 120 * time.Second,
+		wantOK:    true,
+	}, {
+		name: "complex",
+		pa: pa(map[string]string{
+			serving.ProgressDeadlineAnnotationKey: "2m33s",
+		}),
+		wantDelay: 153 * time.Second,
+		wantOK:    true,
+	}, {
+		name: "invalid",
+		pa: pa(map[string]string{
+			serving.ProgressDeadlineAnnotationKey: "365d",
+		}),
+		wantDelay: 0,
+		wantOK:    false,
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			gotDelay, gotOK := tc.pa.ProgressDeadline()
+			if gotDelay != tc.wantDelay {
+				t.Errorf("ProgressDeadline = %v, want: %v", gotDelay, tc.wantDelay)
+			}
+			if gotOK != tc.wantOK {
+				t.Errorf("OK = %v, want: %v", gotOK, tc.wantOK)
+			}
+		})
+	}
+}
+
 func TestWindowAnnotation(t *testing.T) {
 	cases := []struct {
 		name       string

pkg/apis/serving/register.go

@@ -122,6 +122,9 @@ const (
 	// that will result to the Route/KService getting a cluster local
 	// domain suffix.
 	VisibilityClusterLocal = "cluster-local"
+
+	// ProgressDeadlineAnnotationKey is the label key for the per revision progress deadline to set for the deployment
+	ProgressDeadlineAnnotationKey = GroupName + "/progress-deadline"
 )
 
 var (
@@ -159,4 +162,7 @@ var (
 		QueueSidecarResourcePercentageAnnotationKey,
 		"queue.sidecar." + GroupName + "/resourcePercentage",
 	}
+	ProgressDeadlineAnnotation = kmap.KeyPriority{
+		ProgressDeadlineAnnotationKey,
+	}
 )

pkg/apis/serving/v1/revision_validation.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
+	"time"
 
 	"k8s.io/apimachinery/pkg/api/validation"
 	"knative.dev/pkg/apis"
@@ -68,6 +69,7 @@ func (rts *RevisionTemplateSpec) Validate(ctx context.Context) *apis.FieldError
 	// it follows the requirements on the name.
 	errs = errs.Also(validateRevisionName(ctx, rts.Name, rts.GenerateName))
 	errs = errs.Also(validateQueueSidecarAnnotation(rts.Annotations).ViaField("metadata.annotations"))
+	errs = errs.Also(validateProgressDeadlineAnnotation(rts.Annotations).ViaField("metadata.annotations"))
 	return errs
 }
 
@@ -195,3 +197,30 @@ func validateQueueSidecarAnnotation(m map[string]string) *apis.FieldError {
 	}
 	return nil
 }
+
+// ValidateProgressDeadlineAnnotation validates the revision progress deadline annotation.
+func validateProgressDeadlineAnnotation(annos map[string]string) *apis.FieldError {
+	if k, v, _ := serving.ProgressDeadlineAnnotation.Get(annos); v != "" {
+		// Parse as duration.
+		d, err := time.ParseDuration(v)
+		if err != nil {
+			return apis.ErrInvalidValue(v, k)
+		}
+		// Validate that it has second precision.
+		if d.Round(time.Second) != d {
+			return &apis.FieldError{
+				// Even if tempting %v won't work here, since it might output the value spelled differently.
+				Message: fmt.Sprintf("progress-deadline=%s is not at second precision", v),
+				Paths:   []string{k},
+			}
+		}
+		// And positive.
+		if d < 0 {
+			return &apis.FieldError{
+				Message: fmt.Sprintf("progress-deadline=%s must be positive", v),
+				Paths:   []string{k},
+			}
+		}
+	}
+	return nil
+}

pkg/apis/serving/v1/revision_validation_test.go

@@ -966,6 +966,87 @@ func TestRevisionTemplateSpecValidation(t *testing.T) {
 			},
 		},
 		want: nil,
+	}, {
+		name: "Valid progress-deadline",
+		ctx:  autoscalerConfigCtx(true, 1),
+		rts: &RevisionTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					serving.ProgressDeadlineAnnotationKey: "1m3s",
+				},
+			},
+			Spec: RevisionSpec{
+				PodSpec: corev1.PodSpec{
+					Containers: []corev1.Container{{
+						Image: "helloworld",
+					}},
+				},
+			},
+		},
+		want: nil,
+	}, {
+		name: "progress-deadline too precise",
+		ctx:  autoscalerConfigCtx(true, 1),
+		rts: &RevisionTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					serving.ProgressDeadlineAnnotationKey: "1m3s34ms",
+				},
+			},
+			Spec: RevisionSpec{
+				PodSpec: corev1.PodSpec{
+					Containers: []corev1.Container{{
+						Image: "helloworld",
+					}},
+				},
+			},
+		},
+		want: (&apis.FieldError{
+			Message: "progress-deadline=1m3s34ms is not at second precision",
+			Paths:   []string{serving.ProgressDeadlineAnnotationKey},
+		}).ViaField("metadata.annotations"),
+	}, {
+		name: "invalid progress-deadline duration",
+		ctx:  autoscalerConfigCtx(true, 1),
+		rts: &RevisionTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					serving.ProgressDeadlineAnnotationKey: "not-a-duration",
+				},
+			},
+			Spec: RevisionSpec{
+				PodSpec: corev1.PodSpec{
+					Containers: []corev1.Container{{
+						Image: "helloworld",
+					}},
+				},
+			},
+		},
+		want: (&apis.FieldError{
+			Message: "invalid value: not-a-duration",
+			Paths:   []string{serving.ProgressDeadlineAnnotationKey},
+		}).ViaField("metadata.annotations"),
+	}, {
+		name: "negative progress-deadline",
+		ctx:  autoscalerConfigCtx(true, 1),
+		rts: &RevisionTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					serving.ProgressDeadlineAnnotationKey: "-1m3s",
+				},
+			},
+			Spec: RevisionSpec{
+				PodSpec: corev1.PodSpec{
+					Containers: []corev1.Container{{
+						Image: "helloworld",
+					}},
+				},
+			},
+		},
+		want: (&apis.FieldError{
+			Message: "progress-deadline=-1m3s must be positive",
+			Paths:   []string{serving.ProgressDeadlineAnnotationKey},
+		}).ViaField("metadata.annotations"),
 	}}
 
 	for _, test := range tests {

pkg/apis/serving/v1/route_validation.go

@@ -29,8 +29,7 @@ import (
 func (r *Route) Validate(ctx context.Context) *apis.FieldError {
 	errs := serving.ValidateObjectMetadata(ctx, r.GetObjectMeta(), false).Also(
 		r.validateLabels().ViaField("labels"))
-	errs = errs.Also(serving.ValidateRolloutDurationAnnotation(
-		r.GetAnnotations()).ViaField("annotations"))
+	errs = errs.Also(serving.ValidateRolloutDurationAnnotation(r.GetAnnotations()).ViaField("annotations"))
 	errs = errs.ViaField("metadata")
 	errs = errs.Also(r.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
 

pkg/apis/serving/v1/service_validation.go

@@ -31,8 +31,7 @@ func (s *Service) Validate(ctx context.Context) (errs *apis.FieldError) {
 	// spec validation.
 	if !apis.IsInStatusUpdate(ctx) {
 		errs = errs.Also(serving.ValidateObjectMetadata(ctx, s.GetObjectMeta(), false))
-		errs = errs.Also(serving.ValidateRolloutDurationAnnotation(
-			s.GetAnnotations()).ViaField("annotations"))
+		errs = errs.Also(serving.ValidateRolloutDurationAnnotation(s.GetAnnotations()).ViaField("annotations"))
 		errs = errs.ViaField("metadata")
 
 		ctx = apis.WithinParent(ctx, s.ObjectMeta)

pkg/reconciler/autoscaling/kpa/scaler.go

@@ -185,7 +185,12 @@ func (ks *scaler) handleScaleToZero(ctx context.Context, pa *autoscalingv1alpha1
 		return 1, true
 	}
 	cfgD := cfgs.Deployment
-	activationTimeout := cfgD.ProgressDeadline + activationTimeoutBuffer
+	var activationTimeout time.Duration
+	if progressDeadline, ok := pa.ProgressDeadline(); ok {
+		activationTimeout = progressDeadline + activationTimeoutBuffer
+	} else {
+		activationTimeout = cfgD.ProgressDeadline + activationTimeoutBuffer
+	}
 
 	now := time.Now()
 	logger := logging.FromContext(ctx)

pkg/reconciler/autoscaling/kpa/scaler_test.go

@@ -323,6 +323,18 @@ func TestScaler(t *testing.T) {
 		paMutation: func(k *autoscalingv1alpha1.PodAutoscaler) {
 			paMarkActivating(k, time.Now().Add(-(activationTimeout + time.Second)))
 		},
+	}, {
+		label:         "scale to zero while activating after revision deadline exceeded",
+		startReplicas: 1,
+		scaleTo:       0,
+		wantReplicas:  0,
+		wantScaling:   true,
+		paMutation: func(k *autoscalingv1alpha1.PodAutoscaler) {
+			progressDeadline := "5s"
+			k.Annotations[serving.ProgressDeadlineAnnotationKey] = progressDeadline
+			customActivationTimeout, _ := time.ParseDuration(progressDeadline)
+			paMarkActivating(k, time.Now().Add(-(customActivationTimeout + +activationTimeoutBuffer + time.Second)))
+		},
 	}, {
 		label:         "scale down to minScale before grace period",
 		startReplicas: 10,

pkg/reconciler/revision/resources/deploy.go

@@ -19,11 +19,13 @@ package resources
 import (
 	"fmt"
 	"strconv"
+	"time"
 
 	network "knative.dev/networking/pkg"
 	"knative.dev/pkg/kmeta"
 	"knative.dev/pkg/ptr"
 	"knative.dev/serving/pkg/apis/autoscaling"
+	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/networking"
 	"knative.dev/serving/pkg/queue"
@@ -262,13 +264,21 @@ func MakeDeployment(rev *v1.Revision, cfg *config.Config) (*appsv1.Deployment, e
 	}
 
 	replicaCount := cfg.Autoscaler.InitialScale
-	ann, found := rev.Annotations[autoscaling.InitialScaleAnnotationKey]
+	_, ann, found := autoscaling.InitialScaleAnnotation.Get(rev.Annotations)
 	if found {
 		// Ignore errors and no error checking because already validated in webhook.
 		rc, _ := strconv.ParseInt(ann, 10, 32)
 		replicaCount = int32(rc)
 	}
 
+	progressDeadline := int32(cfg.Deployment.ProgressDeadline.Seconds())
+	_, pdAnn, pdFound := serving.ProgressDeadlineAnnotation.Get(rev.Annotations)
+	if pdFound {
+		// Ignore errors and no error checking because already validated in webhook.
+		pd, _ := time.ParseDuration(pdAnn)
+		progressDeadline = int32(pd.Seconds())
+	}
+
 	labels := makeLabels(rev)
 	anns := makeAnnotations(rev)
 
@@ -285,7 +295,7 @@ func MakeDeployment(rev *v1.Revision, cfg *config.Config) (*appsv1.Deployment, e
 		Spec: appsv1.DeploymentSpec{
 			Replicas:                ptr.Int32(replicaCount),
 			Selector:                makeSelector(rev),
-			ProgressDeadlineSeconds: ptr.Int32(int32(cfg.Deployment.ProgressDeadline.Seconds())),
+			ProgressDeadlineSeconds: ptr.Int32(progressDeadline),
 			Strategy: appsv1.DeploymentStrategy{
 				Type: appsv1.RollingUpdateDeploymentStrategyType,
 				RollingUpdate: &appsv1.RollingUpdateDeployment{

pkg/reconciler/revision/resources/deploy_test.go

@@ -1236,7 +1236,7 @@ func TestMakeDeployment(t *testing.T) {
 				map[string]string{sidecarIstioInjectAnnotation: "false"})
 		}),
 	}, {
-		name: "with ProgressDeadline override",
+		name: "with progress-deadline override",
 		dc: deployment.Config{
 			ProgressDeadline: 42 * time.Second,
 		},
@@ -1252,6 +1252,43 @@ func TestMakeDeployment(t *testing.T) {
 		want: appsv1deployment(func(deploy *appsv1.Deployment) {
 			deploy.Spec.ProgressDeadlineSeconds = ptr.Int32(42)
 		}),
+	}, {
+		name: "with progress-deadline annotation",
+		rev: revision("bar", "foo",
+			WithRevisionAnn("serving.knative.dev/progress-deadline", "42s"),
+			withContainers([]corev1.Container{{
+				Name:           servingContainerName,
+				Image:          "ubuntu",
+				ReadinessProbe: withTCPReadinessProbe(12345),
+			}}),
+			WithContainerStatuses([]v1.ContainerStatus{{
+				ImageDigest: "busybox@sha256:deadbeef",
+			}}), 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"}
+		}),
+	}, {
+		name: "with ProgressDeadline annotation and configmap override",
+		dc: deployment.Config{
+			ProgressDeadline: 503 * time.Second,
+		},
+		rev: revision("bar", "foo",
+			WithRevisionAnn("serving.knative.dev/progress-deadline", "42s"),
+			withContainers([]corev1.Container{{
+				Name:           servingContainerName,
+				Image:          "ubuntu",
+				ReadinessProbe: withTCPReadinessProbe(12345),
+			}}),
+			WithContainerStatuses([]v1.ContainerStatus{{
+				ImageDigest: "busybox@sha256:deadbeef",
+			}}), 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"}
+		}),
 	}, {
 		name: "cluster initial scale",
 		acMutator: func(ac *autoscalerconfig.Config) {