Allow manual scaling of the Pod Autoscaler

pkg/reconciler/revision/reconcile_resources.go

@@ -19,6 +19,9 @@ package revision
 import (
 	"context"
 	"fmt"
+	"maps"
+	"slices"
+	"strings"
 
 	"go.uber.org/zap"
 	"knative.dev/pkg/tracker"
@@ -29,20 +32,29 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 
 	networkingApi "knative.dev/networking/pkg/apis/networking"
 	"knative.dev/networking/pkg/certificates"
 	"knative.dev/pkg/kmeta"
 	"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"
 	"knative.dev/serving/pkg/reconciler/revision/resources"
 	resourcenames "knative.dev/serving/pkg/reconciler/revision/resources/names"
 )
 
+var defaultKPAAnnotations = sets.NewString(
+	slices.Concat[[]string](
+		autoscaling.ClassAnnotation,
+		autoscaling.MetricAnnotation,
+	)...,
+)
+
 func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1.Revision) error {
 	ns := rev.Namespace
 	deploymentName := resourcenames.Deployment(rev)
@@ -176,26 +188,98 @@ 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) || annotationsNeedReconcilingForKPA(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)
+
+		syncAnnotationsForKPA(want.Annotations, tmpl.Annotations)
+
+		// Can't realistically fail on PASpec.
+		if diff, _ := kmp.SafeDiff(want.Spec, pa.Spec); diff != "" {
+			logger.Infof("PA %q spec need reconciliation, diff(-want,+got):\n%s", pa.Name, diff)
+		}
+
+		if diff, _ := kmp.SafeDiff(want.Annotations, pa.Annotations); diff != "" {
+			logger.Infof("PA %q annotations need 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
 }
 
+// syncAnnotationsForKPA will properly handle
+// autoscaling annotations semantics
+//
+// dst should be non-nil
+func syncAnnotationsForKPA(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 defaultKPAAnnotations.Has(k) {
+			// Exclude defaulted annotation
+			continue
+		}
+		if strings.HasPrefix(k, autoscaling.GroupName) {
+			delete(dst, k)
+		}
+	}
+
+	// copy src annotationst to dst
+	maps.Copy(dst, src)
+}
+
+// this function changes to ensure all annotations in
+// src are present in dst.
+//
+// It will detect deletions for kpa annotations
+func annotationsNeedReconcilingForKPA(dst, src map[string]string) bool {
+	// Check for extra autoscaling annotations that don't exist in src
+	for k := range dst {
+		if !strings.HasPrefix(k, autoscaling.GroupName) {
+			continue
+		}
+		// Exclude defaulted annotation
+		if defaultKPAAnnotations.Has(k) {
+			continue
+		}
+
+		if _, ok := src[k]; !ok {
+			// Scaling annotation is in dst but not src
+			// return false to trigger reconciliation
+			return true
+		}
+	}
+
+	for k, want := range src {
+		got, ok := dst[k]
+
+		if !ok {
+			if defaultKPAAnnotations.Has(k) {
+				continue
+			}
+
+			return true
+		}
+
+		if got != want {
+			return true
+		}
+	}
+	return false
+}
+
 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.