From 7be0f439df35157783cfed959c68d918060fdc2c Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Thu, 18 Jun 2020 16:17:25 -0400 Subject: [PATCH] Add PA condition InitiallyActive (#8354) * Add PodAutoscaler condition for initial scale * Set initial active condition * Use PA status annotation instead of condition * Rename MarkInitiallyActive -> MarkHasBeenActive --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 18 ++++++++ .../autoscaling/v1alpha1/pa_lifecycle_test.go | 11 +++++ pkg/reconciler/autoscaling/kpa/kpa.go | 1 + pkg/reconciler/autoscaling/kpa/kpa_test.go | 42 +++++++++++-------- 4 files changed, 55 insertions(+), 17 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index 9cec371cddc8..c4d7ce1e2e97 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -28,6 +28,8 @@ import ( "knative.dev/serving/pkg/apis/autoscaling" ) +const hasBeenActiveAnnotation = "HasBeenActive" + var podCondSet = apis.NewLivingConditionSet( PodAutoscalerConditionActive, ) @@ -172,6 +174,22 @@ func (pas *PodAutoscalerStatus) IsInactive() bool { return pas.GetCondition(PodAutoscalerConditionActive).IsFalse() } +// HasBeenActive returns true if the pod autoscaler has reached its initial scale. +func (pas *PodAutoscalerStatus) HasBeenActive() bool { + if val, ok := pas.Annotations[hasBeenActiveAnnotation]; !ok || val != "true" { + return false + } + return true +} + +// MarkHasBeenActive marks the PA's PodAutoscalerConditionInitiallyActive condition true. +func (pas *PodAutoscalerStatus) MarkHasBeenActive() { + if pas.Annotations == nil { + pas.Annotations = map[string]string{} + } + pas.Annotations[hasBeenActiveAnnotation] = "true" +} + // GetCondition gets the condition `t`. func (pas *PodAutoscalerStatus) GetCondition(t apis.ConditionType) *apis.Condition { return podCondSet.Manage(pas).GetCondition(t) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index 5ab08c9046e9..87b78db8273f 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -1171,3 +1171,14 @@ func TestInitialScale(t *testing.T) { }) } } + +func TestHasBeenActive(t *testing.T) { + p := PodAutoscaler{} + if got, want := p.Status.HasBeenActive(), false; got != want { + t.Errorf("before marking initially active: got: %v, want: %v", got, want) + } + p.Status.MarkHasBeenActive() + if got, want := p.Status.HasBeenActive(), true; got != want { + t.Errorf("after marking initially active: got: %v, want: %v", got, want) + } +} diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index ee372a351670..d481b654d645 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -259,6 +259,7 @@ func computeActiveCondition(pa *pav1alpha1.PodAutoscaler, pc podCounts) { case pc.ready >= minReady: if pc.want > 0 || !pa.Status.IsInactive() { + pa.Status.MarkHasBeenActive() // SKS should already be active. pa.Status.MarkActive() } diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index dbeda6a4dcc0..a206a47e140f 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -193,6 +193,10 @@ func markInactive(pa *asv1a1.PodAutoscaler) { pa.Status.MarkInactive("NoTraffic", "The target is not receiving traffic.") } +func markHasBeenActive(pa *asv1a1.PodAutoscaler) { + pa.Status.MarkHasBeenActive() +} + func kpa(ns, n string, opts ...PodAutoscalerOption) *asv1a1.PodAutoscaler { rev := newTestRevision(ns, n) kpa := revisionresources.MakePA(rev) @@ -248,16 +252,20 @@ func TestReconcile(t *testing.T) { WithObservedGeneration(1), ) } - activatingKPAMinScale := func(g int32) *asv1a1.PodAutoscaler { - return kpa( + activatingKPAMinScale := func(g int32, opts ...PodAutoscalerOption) *asv1a1.PodAutoscaler { + kpa := kpa( testNamespace, testRevision, markActivating, withScales(g, defaultScale), WithReachabilityReachable, withMinScale(defaultScale), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1), ) + for _, opt := range opts { + opt(kpa) + } + return kpa } activeKPAMinScale := func(g, w int32) *asv1a1.PodAutoscaler { return kpa( - testNamespace, testRevision, markActive, withScales(g, w), WithReachabilityReachable, + testNamespace, testRevision, markActive, markHasBeenActive, withScales(g, w), WithReachabilityReachable, withMinScale(defaultScale), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1), ) @@ -293,7 +301,7 @@ func TestReconcile(t *testing.T) { Name: "steady state", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metric(testNamespace, testRevision), @@ -375,14 +383,14 @@ func TestReconcile(t *testing.T) { Name: "create metric", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, + kpa(testNamespace, testRevision, markActive, markHasBeenActive, withScales(1, defaultScale), WithPAStatusService(testRevision)), defaultSKS, defaultDeployment, defaultEndpoints, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, withScales(1, defaultScale), + Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, withScales(1, defaultScale), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), }}, WantCreates: []runtime.Object{ @@ -392,7 +400,7 @@ func TestReconcile(t *testing.T) { Name: "scale up deployment", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultSKS, metric(testNamespace, testRevision), @@ -493,7 +501,7 @@ func TestReconcile(t *testing.T) { defaultDeployment, defaultEndpoints, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, WithPAStatusService(testRevision), + Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithObservedGeneration(1)), }}, }, { @@ -537,7 +545,7 @@ func TestReconcile(t *testing.T) { defaultDeployment, defaultEndpoints, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, withMinScale(2), WithPAMetricsService(privateSvc), + Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, withMinScale(2), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnreachable, WithObservedGeneration(1)), }}, @@ -553,7 +561,7 @@ func TestReconcile(t *testing.T) { makeSKSPrivateEndpoints(2, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, withMinScale(2), WithPAMetricsService(privateSvc), + Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, withMinScale(2), WithPAMetricsService(privateSvc), withScales(2, defaultScale), WithPAStatusService(testRevision), WithReachabilityReachable, WithObservedGeneration(1)), }}, @@ -569,7 +577,7 @@ func TestReconcile(t *testing.T) { makeSKSPrivateEndpoints(2, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, withMinScale(2), WithPAMetricsService(privateSvc), + Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, withMinScale(2), WithPAMetricsService(privateSvc), withScales(2, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnknown, WithObservedGeneration(1)), }}, @@ -748,7 +756,7 @@ func TestReconcile(t *testing.T) { Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 0 /* desiredScale */, 0 /* ebc */, scaling.MinActivators)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, withScales(1, 1), + kpa(testNamespace, testRevision, markActive, markHasBeenActive, withScales(1, 1), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), defaultSKS, metric(testNamespace, testRevision), @@ -845,7 +853,7 @@ func TestReconcile(t *testing.T) { minScalePatch, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: activatingKPAMinScale(underscale), + Object: activatingKPAMinScale(underscale, markHasBeenActive), }}, }, { // Scale to `minScale` and mark PA "active" @@ -935,7 +943,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -42 /* ebc */, scaling.MinActivators)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), metric(testNamespace, testRevision), @@ -948,7 +956,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -42 /* ebc */, 1982 /*numActivators*/)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady, WithNumActivators(4)), @@ -966,7 +974,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -18 /* ebc */, scaling.MinActivators+1)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithNumActivators(2)), @@ -984,7 +992,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ 1 /* ebc */, scaling.MinActivators)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithProxyMode, WithNumActivators(3)),