Allow manual scaling of the Pod Autoscaler

pkg/reconciler/revision/reconcile_resources.go

@@ -19,6 +19,8 @@ package revision
 import (
 	"context"
 	"fmt"
+	"maps"
+	"strings"
 
 	"go.uber.org/zap"
 	"knative.dev/pkg/tracker"
@@ -36,6 +38,7 @@ import (
 	"knative.dev/pkg/kmp"
 	"knative.dev/pkg/logging"
 	"knative.dev/pkg/logging/logkey"
+	"knative.dev/serving/pkg/apis/autoscaling"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/networking"
 	"knative.dev/serving/pkg/reconciler/revision/config"
@@ -176,26 +179,90 @@ func (c *Reconciler) reconcilePA(ctx context.Context, rev *v1.Revision) error {
 		return fmt.Errorf("revision: %q does not own PodAutoscaler: %q", rev.Name, paName)
 	}
 
+	logger.Debugf("Observed PA Status=%#v", pa.Status)
+	rev.Status.PropagateAutoscalerStatus(&pa.Status)
+
 	// Perhaps tha PA spec changed underneath ourselves?
 	// We no longer require immutability, so need to reconcile PA each time.
 	tmpl := resources.MakePA(rev, deployment)
 	logger.Debugf("Desired PASpec: %#v", tmpl.Spec)
-	if !equality.Semantic.DeepEqual(tmpl.Spec, pa.Spec) {
-		diff, _ := kmp.SafeDiff(tmpl.Spec, pa.Spec) // Can't realistically fail on PASpec.
-		logger.Infof("PA %s needs reconciliation, diff(-want,+got):\n%s", pa.Name, diff)
-
+	if !equality.Semantic.DeepEqual(tmpl.Spec, pa.Spec) || !annotationsPresent(pa.Annotations, tmpl.Annotations) {
 		want := pa.DeepCopy()
 		want.Spec = tmpl.Spec
-		if pa, err = c.client.AutoscalingV1alpha1().PodAutoscalers(ns).Update(ctx, want, metav1.UpdateOptions{}); err != nil {
-			return fmt.Errorf("failed to update PA %q: %w", paName, err)
+
+		processAnnotations(want.Annotations, tmpl.Annotations)
+
+		// Can't realistically fail on PASpec.
+		if diff, _ := kmp.SafeDiff(want.Spec, pa.Spec); diff != "" {
+			logger.Infof("PA %q spec needs reconciliation, diff(-want,+got):\n%s", pa.Name, diff)
+		}
+
+		if diff, _ := kmp.SafeDiff(want.Annotations, pa.Annotations); diff != "" {
+			logger.Infof("PA %q annotations needs reconciliation, diff(-want,+got):\n%s", pa.Name, diff)
+		}
+
+		_, err := c.client.AutoscalingV1alpha1().PodAutoscalers(ns).Update(ctx, want, metav1.UpdateOptions{})
+		if err != nil {
+			return fmt.Errorf("failed to update PA %q: %w", want.Name, err)
 		}
 	}
 
-	logger.Debugf("Observed PA Status=%#v", pa.Status)
-	rev.Status.PropagateAutoscalerStatus(&pa.Status)
 	return nil
 }
 
+func processAnnotations(dst, src map[string]string) {
+	// Delete autoscaling annotations from destination map
+	// This ensures that setting these annotation on the Revision is the source of truth
+	for k := range dst {
+		if k == autoscaling.ClassAnnotationKey || k == autoscaling.MetricAnnotationKey {
+			// Exclude defaulted annotation
+			continue
+		}
+		if strings.HasPrefix(k, autoscaling.GroupName) {
+			delete(dst, k)
+		}
+	}
+
+	// copy source annotations over while preserving existing ones
+	maps.Copy(dst, src)
+}
+
+func annotationsPresent(dst, src map[string]string) bool {
+	// Check for extra autoscaling annotations that don't exist src
+	for k := range dst {
+		if !strings.HasPrefix(k, autoscaling.GroupName) {
+			continue
+		}
+		// Exclude defaulted annotation
+		if k == autoscaling.ClassAnnotationKey || k == autoscaling.MetricAnnotationKey {
+			continue
+		}
+
+		if _, ok := src[k]; !ok {
+			// Scaling annotation is in dst but not src
+			// return false to trigger reconciliation
+			return false
+		}
+	}
+
+	for k, want := range src {
+		got, ok := dst[k]
+
+		if !ok {
+			if k == autoscaling.ClassAnnotationKey || k == autoscaling.MetricAnnotationKey {
+				continue
+			}
+
+			return false
+		}
+
+		if got != want {
+			return false
+		}
+	}
+	return true
+}
+
 func hasDeploymentTimedOut(deployment *appsv1.Deployment) bool {
 	// as per https://kubernetes.io/docs/concepts/workloads/controllers/deployment
 	for _, cond := range deployment.Status.Conditions {

pkg/reconciler/revision/resources/deploy.go

@@ -372,8 +372,6 @@ func MakeDeployment(rev *v1.Revision, cfg *config.Config) (*appsv1.Deployment, e
 	}
 
 	labels := makeLabels(rev)
-	anns := makeAnnotations(rev)
-	annsPod := makeAnnotationsForPod(rev, anns)
 
 	// Slowly but steadily roll the deployment out, to have the least possible impact.
 	maxUnavailable := intstr.FromInt(0)
@@ -382,7 +380,7 @@ func MakeDeployment(rev *v1.Revision, cfg *config.Config) (*appsv1.Deployment, e
 			Name:            names.Deployment(rev),
 			Namespace:       rev.Namespace,
 			Labels:          labels,
-			Annotations:     anns,
+			Annotations:     deploymentAnnotations(rev),
 			OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(rev)},
 		},
 		Spec: appsv1.DeploymentSpec{
@@ -399,7 +397,7 @@ func MakeDeployment(rev *v1.Revision, cfg *config.Config) (*appsv1.Deployment, e
 			Template: corev1.PodTemplateSpec{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels:      labels,
-					Annotations: annsPod,
+					Annotations: podAnnotations(rev),
 				},
 				Spec: *podSpec,
 			},

pkg/reconciler/revision/resources/deploy_test.go

@@ -1916,12 +1916,8 @@ func TestMakeDeployment(t *testing.T) {
 		),
 		want: appsv1deployment(func(deploy *appsv1.Deployment) {
 			deploy.Spec.Replicas = ptr.Int32(int32(20))
-			deploy.Annotations = map[string]string{
-				autoscaling.InitialScaleAnnotationKey: "20",
-			}
 			deploy.Spec.Template.Annotations = map[string]string{
-				autoscaling.InitialScaleAnnotationKey: "20",
-				DefaultContainerAnnotationName:        servingContainerName,
+				DefaultContainerAnnotationName: servingContainerName,
 			}
 		}),
 	}}

pkg/reconciler/revision/resources/imagecache.go

@@ -32,7 +32,7 @@ func MakeImageCache(rev *v1.Revision, containerName, image string) *caching.Imag
 			Name:            kmeta.ChildName(names.ImageCache(rev), "-"+containerName),
 			Namespace:       rev.Namespace,
 			Labels:          makeLabels(rev),
-			Annotations:     makeAnnotations(rev),
+			Annotations:     imageCacheAnnotations(rev),
 			OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(rev)},
 		},
 		Spec: caching.ImageSpec{

pkg/reconciler/revision/resources/meta.go

@@ -17,11 +17,12 @@ limitations under the License.
 package resources
 
 import (
-	"maps"
+	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"knative.dev/pkg/kmeta"
+	"knative.dev/pkg/kmap"
+	"knative.dev/serving/pkg/apis/autoscaling"
 	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 )
@@ -46,8 +47,8 @@ const (
 
 // makeLabels constructs the labels we will apply to K8s resources.
 func makeLabels(revision *v1.Revision) map[string]string {
-	labels := kmeta.FilterMap(revision.GetLabels(), excludeLabels.Has)
-	labels = kmeta.UnionMaps(labels, map[string]string{
+	labels := kmap.Filter(revision.GetLabels(), excludeLabels.Has)
+	labels = kmap.Union(labels, map[string]string{
 		serving.RevisionLabelKey: revision.Name,
 		serving.RevisionUID:      string(revision.UID),
 	})
@@ -61,19 +62,30 @@ func makeLabels(revision *v1.Revision) map[string]string {
 	return labels
 }
 
-func makeAnnotations(revision *v1.Revision) map[string]string {
-	return kmeta.FilterMap(revision.GetAnnotations(), excludeAnnotations.Has)
+func filterExcludedAndAutoscalingAnnotations(val string) bool {
+	return excludeAnnotations.Has(val) || strings.HasPrefix(val, autoscaling.GroupName)
 }
 
-func makeAnnotationsForPod(revision *v1.Revision, baseAnnotations map[string]string) map[string]string {
-	podAnnotations := maps.Clone(baseAnnotations)
+func deploymentAnnotations(r *v1.Revision) map[string]string {
+	return kmap.Filter(r.GetAnnotations(), filterExcludedAndAutoscalingAnnotations)
+}
+
+func imageCacheAnnotations(r *v1.Revision) map[string]string {
+	return kmap.Filter(r.GetAnnotations(), filterExcludedAndAutoscalingAnnotations)
+}
+
+func podAutoscalerAnnotations(r *v1.Revision) map[string]string {
+	return kmap.Filter(r.GetAnnotations(), excludeAnnotations.Has)
+}
+
+func podAnnotations(r *v1.Revision) map[string]string {
+	ann := kmap.Filter(r.GetAnnotations(), filterExcludedAndAutoscalingAnnotations)
 
-	// Add default container annotation to the pod meta
-	if userContainer := revision.Spec.GetContainer(); userContainer.Name != "" {
-		podAnnotations[DefaultContainerAnnotationName] = userContainer.Name
+	if userContainer := r.Spec.GetContainer(); userContainer.Name != "" {
+		ann[DefaultContainerAnnotationName] = userContainer.Name
 	}
 
-	return podAnnotations
+	return ann
 }
 
 // makeSelector constructs the Selector we will apply to K8s resources.

pkg/reconciler/revision/resources/meta_test.go

@@ -17,12 +17,17 @@ limitations under the License.
 package resources
 
 import (
+	"fmt"
+	"reflect"
+	"runtime"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	"knative.dev/serving/pkg/apis/autoscaling"
 	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 )
@@ -104,41 +109,87 @@ func TestMakeLabels(t *testing.T) {
 }
 
 func TestMakeAnnotations(t *testing.T) {
+	type buildFuncs []func(*v1.Revision) map[string]string
+
 	tests := []struct {
-		name string
-		rev  *v1.Revision
-		want map[string]string
+		name       string
+		buildFuncs buildFuncs
+		revAnn     map[string]string
+		want       map[string]string
 	}{{
 		name: "no user annotations",
-		rev: &v1.Revision{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "foo",
-				Name:      "bar",
-			},
+		buildFuncs: buildFuncs{
+			deploymentAnnotations,
+			imageCacheAnnotations,
+			podAutoscalerAnnotations,
+			podAnnotations,
 		},
-		want: map[string]string{},
+		revAnn: map[string]string{},
+		want:   map[string]string{},
 	}, {
-		name: "exclude annotation",
-		rev: &v1.Revision{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "foo",
-				Name:      "bar",
-				Annotations: map[string]string{
-					serving.RoutingStateModifiedAnnotationKey: "exclude me",
-					"keep": "keep me",
-				},
-			},
+		name: "excluded annotations",
+		buildFuncs: buildFuncs{
+			deploymentAnnotations,
+			imageCacheAnnotations,
+			podAutoscalerAnnotations,
+			podAnnotations,
+		},
+		revAnn: map[string]string{
+			serving.RoutingStateModifiedAnnotationKey: "exclude me",
+			"keep": "keep me",
 		},
 		want: map[string]string{"keep": "keep me"},
+	}, {
+		name: "exclude autoscaling annotations",
+		buildFuncs: buildFuncs{
+			deploymentAnnotations,
+			imageCacheAnnotations,
+			podAnnotations,
+		},
+		revAnn: map[string]string{
+			autoscaling.MinScaleAnnotationKey: "1",
+			"keep":                            "keep me",
+		},
+		want: map[string]string{"keep": "keep me"},
+	}, {
+		name: "include autoscaling annotations",
+		buildFuncs: buildFuncs{
+			podAutoscalerAnnotations,
+		},
+		revAnn: map[string]string{
+			autoscaling.MinScaleAnnotationKey: "1",
+			"keep":                            "keep me",
+		},
+		want: map[string]string{
+			autoscaling.MinScaleAnnotationKey: "1",
+			"keep":                            "keep me",
+		},
 	}}
 
 	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			got := makeAnnotations(test.rev)
-			if diff := cmp.Diff(test.want, got); diff != "" {
-				t.Error("makeLabels (-want, +got) =", diff)
-			}
-		})
+		for _, buildFunc := range test.buildFuncs {
+			funcName := runtime.FuncForPC(reflect.ValueOf(buildFunc).Pointer()).Name()
+			i := strings.LastIndex(funcName, ".")
+			funcName = funcName[i+1:]
+
+			testName := fmt.Sprint(test.name, " ", funcName)
+
+			t.Run(testName, func(t *testing.T) {
+				rev := &v1.Revision{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace:   "foo",
+						Name:        "bar",
+						Annotations: test.revAnn,
+					},
+				}
+
+				got := buildFunc(rev)
+
+				if diff := cmp.Diff(test.want, got); diff != "" {
+					t.Errorf("%s(-want, +got) = %s", funcName, diff)
+				}
+			})
+		}
 	}
 }
 
@@ -151,6 +202,11 @@ func TestMakeAnnotationsForPod(t *testing.T) {
 	}{{
 		name: "multiple containers single port with base annotation",
 		rev: &v1.Revision{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					"asdf": "fdsa",
+				},
+			},
 			Spec: v1.RevisionSpec{
 				PodSpec: corev1.PodSpec{
 					Containers: []corev1.Container{{
@@ -164,7 +220,6 @@ func TestMakeAnnotationsForPod(t *testing.T) {
 				},
 			},
 		},
-		baseAnnotations: map[string]string{"asdf": "fdsa"},
 		want: map[string]string{
 			"asdf":                         "fdsa",
 			DefaultContainerAnnotationName: "bar",
@@ -173,7 +228,7 @@ func TestMakeAnnotationsForPod(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			got := makeAnnotationsForPod(test.rev, test.baseAnnotations)
+			got := podAnnotations(test.rev)
 			if diff := cmp.Diff(test.want, got); diff != "" {
 				t.Error("getUserContainerName (-want, +got) =", diff)
 			}

pkg/reconciler/revision/resources/pa.go

@@ -35,7 +35,7 @@ func MakePA(rev *v1.Revision, deployment *appsv1.Deployment) *autoscalingv1alpha
 			Name:            names.PA(rev),
 			Namespace:       rev.Namespace,
 			Labels:          makeLabels(rev),
-			Annotations:     makeAnnotations(rev),
+			Annotations:     podAutoscalerAnnotations(rev),
 			OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(rev)},
 		},
 		Spec: autoscalingv1alpha1.PodAutoscalerSpec{