From 34e09d9bc862b74a5e5f6c90343dd2d327912229 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Tue, 7 Jul 2020 15:20:45 +0200 Subject: [PATCH 1/6] Propagate HasBeenActive to a terminal condition. It will eventually be superceding "Active" as the PA's readiness gate. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 11 +++-------- pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go | 1 + pkg/apis/autoscaling/v1alpha1/pa_types.go | 3 +++ pkg/reconciler/autoscaling/hpa/hpa_test.go | 1 + pkg/reconciler/autoscaling/kpa/kpa_test.go | 1 + pkg/reconciler/revision/revision_test.go | 1 + pkg/reconciler/revision/table_test.go | 8 ++++---- pkg/testing/functional.go | 5 +++++ 8 files changed, 19 insertions(+), 12 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index c4d7ce1e2e97..13d3d506695b 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -32,6 +32,7 @@ const hasBeenActiveAnnotation = "HasBeenActive" var podCondSet = apis.NewLivingConditionSet( PodAutoscalerConditionActive, + PodAutoscalerConditionHasBeenActive, ) // GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface. @@ -176,18 +177,12 @@ func (pas *PodAutoscalerStatus) IsInactive() bool { // 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 + return pas.GetCondition(PodAutoscalerConditionHasBeenActive).IsTrue() } // 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" + podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionHasBeenActive) } // GetCondition gets the condition `t`. diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index 87b78db8273f..ece3b6bc6115 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -980,6 +980,7 @@ func TestTypicalFlow(t *testing.T) { // When we see traffic, mark ourselves active. r.MarkActive() + r.MarkHasBeenActive() apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t) apistest.CheckConditionSucceeded(r, PodAutoscalerConditionReady, t) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_types.go b/pkg/apis/autoscaling/v1alpha1/pa_types.go index 02fc64084cb2..57cdcaad2713 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_types.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_types.go @@ -112,6 +112,9 @@ const ( // PodAutoscalerConditionReady is set when the revision is starting to materialize // runtime resources, and becomes true when those resources are ready. PodAutoscalerConditionReady = apis.ConditionReady + // PodAutoscalerConditionHasBeenActive is set when the PodAutoscaler's ScaleTargetRef was ready to + // serve traffic once. + PodAutoscalerConditionHasBeenActive apis.ConditionType = "HasBeenActive" // PodAutoscalerConditionActive is set when the PodAutoscaler's ScaleTargetRef is receiving traffic. PodAutoscalerConditionActive apis.ConditionType = "Active" ) diff --git a/pkg/reconciler/autoscaling/hpa/hpa_test.go b/pkg/reconciler/autoscaling/hpa/hpa_test.go index dfd4fd26285c..8dbdb979f18b 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa_test.go +++ b/pkg/reconciler/autoscaling/hpa/hpa_test.go @@ -442,6 +442,7 @@ func pa(namespace, name string, options ...PodAutoscalerOption) *asv1a1.PodAutos ProtocolType: networking.ProtocolHTTP1, }, } + pa.Status.InitializeConditions() for _, opt := range options { opt(pa) } diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index 6b9deb7e7306..b4e287290bd7 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -202,6 +202,7 @@ func kpa(ns, n string, opts ...PodAutoscalerOption) *asv1a1.PodAutoscaler { kpa.Generation = 1 kpa.Annotations["autoscaling.knative.dev/class"] = "kpa.autoscaling.knative.dev" kpa.Annotations["autoscaling.knative.dev/metric"] = "concurrency" + kpa.Status.InitializeConditions() for _, opt := range opts { opt(kpa) } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 71d6bbb53ac6..ef34b6bcb225 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -108,6 +108,7 @@ func testReadyPA(rev *v1.Revision) *av1alpha1.PodAutoscaler { pa := resources.MakePA(rev) pa.Status.InitializeConditions() pa.Status.MarkActive() + pa.Status.MarkHasBeenActive() pa.Status.ServiceName = serviceName(rev.Name) return pa } diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 75153ddefa91..9b4bb82acfcb 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -247,7 +247,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "pa-ready", withK8sServiceName("old-stuff"), WithLogURL, AllUnknownConditions), - pa("foo", "pa-ready", WithTraffic, WithPAStatusService("new-stuff"), WithReachabilityUnknown), + pa("foo", "pa-ready", WithTraffic, WithHasBeenActive, WithPAStatusService("new-stuff"), WithReachabilityUnknown), deploy(t, "foo", "pa-ready"), image("foo", "pa-ready"), }, @@ -343,7 +343,7 @@ func TestReconcile(t *testing.T) { withK8sServiceName("ill-follow-the-sun"), WithLogURL, MarkRevisionReady, WithRevisionLabel(serving.RouteLabelKey, "foo")), pa("foo", "fix-mutated-pa", WithProtocolType(networking.ProtocolH2C), - WithTraffic, WithPAStatusService("fix-mutated-pa")), + WithTraffic, WithHasBeenActive, WithPAStatusService("fix-mutated-pa")), deploy(t, "foo", "fix-mutated-pa"), image("foo", "fix-mutated-pa"), }, @@ -357,7 +357,7 @@ func TestReconcile(t *testing.T) { withDefaultContainerStatuses(), withObservedGeneration(1)), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ - Object: pa("foo", "fix-mutated-pa", WithTraffic, + Object: pa("foo", "fix-mutated-pa", WithTraffic, WithHasBeenActive, WithPAStatusService("fix-mutated-pa"), WithReachabilityReachable), }}, WantEvents: []string{ @@ -511,7 +511,7 @@ func TestReconcile(t *testing.T) { // This signal should make our Reconcile mark the Revision as Ready. Objects: []runtime.Object{ rev("foo", "steady-ready", withK8sServiceName("very-steady"), WithLogURL), - pa("foo", "steady-ready", WithTraffic, WithPAStatusService("steadier-even")), + pa("foo", "steady-ready", WithTraffic, WithHasBeenActive, WithPAStatusService("steadier-even")), deploy(t, "foo", "steady-ready"), image("foo", "steady-ready"), }, diff --git a/pkg/testing/functional.go b/pkg/testing/functional.go index 4a9103f6acc0..620c28077be1 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -84,6 +84,11 @@ func WithTraffic(pa *asv1a1.PodAutoscaler) { pa.Status.MarkActive() } +// WithHasBeenActive updates the PA to reflect it having been active. +func WithHasBeenActive(pa *asv1a1.PodAutoscaler) { + pa.Status.MarkHasBeenActive() +} + // WithPAStatusService annotates PA Status with the provided service name. func WithPAStatusService(svc string) PodAutoscalerOption { return func(pa *asv1a1.PodAutoscaler) { From ad6e4b560e014537afe378d80b188ddcdcb0501f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Tue, 7 Jul 2020 16:24:57 +0200 Subject: [PATCH 2/6] Fix HPA. --- pkg/reconciler/autoscaling/hpa/hpa.go | 1 + pkg/reconciler/autoscaling/hpa/hpa_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/autoscaling/hpa/hpa.go b/pkg/reconciler/autoscaling/hpa/hpa.go index 98e2a92be18d..af280e205933 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa.go +++ b/pkg/reconciler/autoscaling/hpa/hpa.go @@ -91,6 +91,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *pav1alpha1.PodAutosc if !sks.IsReady() { pa.Status.MarkInactive("ServicesNotReady", "SKS Services are not ready yet") } else { + pa.Status.MarkHasBeenActive() pa.Status.MarkActive() } diff --git a/pkg/reconciler/autoscaling/hpa/hpa_test.go b/pkg/reconciler/autoscaling/hpa/hpa_test.go index 8dbdb979f18b..822edcd425bb 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa_test.go +++ b/pkg/reconciler/autoscaling/hpa/hpa_test.go @@ -134,8 +134,8 @@ func TestReconcile(t *testing.T) { Name: "no op", Objects: []runtime.Object{ hpa(pa(testNamespace, testRevision, WithHPAClass, WithMetricAnnotation("cpu"))), - pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithPAStatusService(testRevision), - WithPAMetricsService(privateSvc), withScales(0, 0)), + pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithHasBeenActive, + WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), withScales(0, 0)), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), }, @@ -193,7 +193,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testNamespace, testRevision, WithHPAClass, withScales(0, 0), - WithTraffic, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), + WithTraffic, WithHasBeenActive, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), }}, Key: key(testNamespace, testRevision), }, { @@ -205,7 +205,7 @@ func TestReconcile(t *testing.T) { sks(testNamespace, testRevision, WithDeployRef("bar"), WithSKSReady), }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testNamespace, testRevision, WithHPAClass, WithTraffic, withScales(5, 3), + Object: pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithHasBeenActive, withScales(5, 3), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), }}, Key: key(testNamespace, testRevision), @@ -319,7 +319,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testNamespace, testRevision, WithHPAClass, withScales(19, 18), - WithTraffic, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), + WithTraffic, WithHasBeenActive, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), }}, Key: key(testNamespace, testRevision), WantErr: true, @@ -352,7 +352,7 @@ func TestReconcile(t *testing.T) { }, { Name: "update hpa with target usage", Objects: []runtime.Object{ - pa(testNamespace, testRevision, WithHPAClass, WithTraffic, withScales(0, 0), + pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithHasBeenActive, withScales(0, 0), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithTargetAnnotation("1")), hpa(pa(testNamespace, testRevision, WithHPAClass, WithMetricAnnotation("cpu"))), deploy(testNamespace, testRevision), From f43143931a69a23ba9b2ddddff4b190494e72ca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Tue, 7 Jul 2020 17:27:42 +0200 Subject: [PATCH 3/6] Remove unused constant. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index 13d3d506695b..dcd313f331bf 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -28,8 +28,6 @@ import ( "knative.dev/serving/pkg/apis/autoscaling" ) -const hasBeenActiveAnnotation = "HasBeenActive" - var podCondSet = apis.NewLivingConditionSet( PodAutoscalerConditionActive, PodAutoscalerConditionHasBeenActive, From 241a3e329da914e1bb3532f773ac0818f748ce5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Wed, 8 Jul 2020 11:51:21 +0200 Subject: [PATCH 4/6] Rename condition to ScaleTargetInitialized. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 16 +++++---- .../autoscaling/v1alpha1/pa_lifecycle_test.go | 10 +++--- pkg/apis/autoscaling/v1alpha1/pa_types.go | 6 ++-- pkg/reconciler/autoscaling/hpa/hpa.go | 2 +- pkg/reconciler/autoscaling/hpa/hpa_test.go | 10 +++--- pkg/reconciler/autoscaling/kpa/kpa.go | 2 +- pkg/reconciler/autoscaling/kpa/kpa_test.go | 34 +++++++++---------- pkg/reconciler/revision/revision_test.go | 2 +- pkg/reconciler/revision/table_test.go | 8 ++--- pkg/testing/functional.go | 6 ++-- 10 files changed, 49 insertions(+), 47 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index dcd313f331bf..2d424874f27e 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go @@ -30,7 +30,7 @@ import ( var podCondSet = apis.NewLivingConditionSet( PodAutoscalerConditionActive, - PodAutoscalerConditionHasBeenActive, + PodAutoscalerConditionScaleTargetInitialized, ) // GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface. @@ -173,14 +173,16 @@ 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 { - return pas.GetCondition(PodAutoscalerConditionHasBeenActive).IsTrue() +// IsScaleTargetInitialized returns true if the PodAutoscaler's scale target has been +// initialized successfully. +func (pas *PodAutoscalerStatus) IsScaleTargetInitialized() bool { + return pas.GetCondition(PodAutoscalerConditionScaleTargetInitialized).IsTrue() } -// MarkHasBeenActive marks the PA's PodAutoscalerConditionInitiallyActive condition true. -func (pas *PodAutoscalerStatus) MarkHasBeenActive() { - podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionHasBeenActive) +// MarkScaleTargetInitialized marks the PA's PodAutoscalerConditionScaleTargetInitialized +// condition true. +func (pas *PodAutoscalerStatus) MarkScaleTargetInitialized() { + podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionScaleTargetInitialized) } // GetCondition gets the condition `t`. diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index ece3b6bc6115..664e678fe3d9 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -980,7 +980,7 @@ func TestTypicalFlow(t *testing.T) { // When we see traffic, mark ourselves active. r.MarkActive() - r.MarkHasBeenActive() + r.MarkScaleTargetInitialized() apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t) apistest.CheckConditionSucceeded(r, PodAutoscalerConditionReady, t) @@ -1173,13 +1173,13 @@ func TestInitialScale(t *testing.T) { } } -func TestHasBeenActive(t *testing.T) { +func TestIsScaleTargetInitialized(t *testing.T) { p := PodAutoscaler{} - if got, want := p.Status.HasBeenActive(), false; got != want { + if got, want := p.Status.IsScaleTargetInitialized(), 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 { + p.Status.MarkScaleTargetInitialized() + if got, want := p.Status.IsScaleTargetInitialized(), true; got != want { t.Errorf("after marking initially active: got: %v, want: %v", got, want) } } diff --git a/pkg/apis/autoscaling/v1alpha1/pa_types.go b/pkg/apis/autoscaling/v1alpha1/pa_types.go index 57cdcaad2713..163189ecaf7d 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_types.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_types.go @@ -112,9 +112,9 @@ const ( // PodAutoscalerConditionReady is set when the revision is starting to materialize // runtime resources, and becomes true when those resources are ready. PodAutoscalerConditionReady = apis.ConditionReady - // PodAutoscalerConditionHasBeenActive is set when the PodAutoscaler's ScaleTargetRef was ready to - // serve traffic once. - PodAutoscalerConditionHasBeenActive apis.ConditionType = "HasBeenActive" + // PodAutoscalerConditionScaleTargetInitialized is set when the PodAutoscaler's + // ScaleTargetRef was ready to serve traffic once. + PodAutoscalerConditionScaleTargetInitialized apis.ConditionType = "ScaleTargetInitialized" // PodAutoscalerConditionActive is set when the PodAutoscaler's ScaleTargetRef is receiving traffic. PodAutoscalerConditionActive apis.ConditionType = "Active" ) diff --git a/pkg/reconciler/autoscaling/hpa/hpa.go b/pkg/reconciler/autoscaling/hpa/hpa.go index af280e205933..fb48c93eb03d 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa.go +++ b/pkg/reconciler/autoscaling/hpa/hpa.go @@ -91,7 +91,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *pav1alpha1.PodAutosc if !sks.IsReady() { pa.Status.MarkInactive("ServicesNotReady", "SKS Services are not ready yet") } else { - pa.Status.MarkHasBeenActive() + pa.Status.MarkScaleTargetInitialized() pa.Status.MarkActive() } diff --git a/pkg/reconciler/autoscaling/hpa/hpa_test.go b/pkg/reconciler/autoscaling/hpa/hpa_test.go index 822edcd425bb..a931750ddc1d 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa_test.go +++ b/pkg/reconciler/autoscaling/hpa/hpa_test.go @@ -134,7 +134,7 @@ func TestReconcile(t *testing.T) { Name: "no op", Objects: []runtime.Object{ hpa(pa(testNamespace, testRevision, WithHPAClass, WithMetricAnnotation("cpu"))), - pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithHasBeenActive, + pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithScaleTargetInitialized, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), withScales(0, 0)), deploy(testNamespace, testRevision), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), @@ -193,7 +193,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testNamespace, testRevision, WithHPAClass, withScales(0, 0), - WithTraffic, WithHasBeenActive, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), + WithTraffic, WithScaleTargetInitialized, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), }}, Key: key(testNamespace, testRevision), }, { @@ -205,7 +205,7 @@ func TestReconcile(t *testing.T) { sks(testNamespace, testRevision, WithDeployRef("bar"), WithSKSReady), }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ - Object: pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithHasBeenActive, withScales(5, 3), + Object: pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithScaleTargetInitialized, withScales(5, 3), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), }}, Key: key(testNamespace, testRevision), @@ -319,7 +319,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []ktesting.UpdateActionImpl{{ Object: pa(testNamespace, testRevision, WithHPAClass, withScales(19, 18), - WithTraffic, WithHasBeenActive, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), + WithTraffic, WithScaleTargetInitialized, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), }}, Key: key(testNamespace, testRevision), WantErr: true, @@ -352,7 +352,7 @@ func TestReconcile(t *testing.T) { }, { Name: "update hpa with target usage", Objects: []runtime.Object{ - pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithHasBeenActive, withScales(0, 0), + pa(testNamespace, testRevision, WithHPAClass, WithTraffic, WithScaleTargetInitialized, withScales(0, 0), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithTargetAnnotation("1")), hpa(pa(testNamespace, testRevision, WithHPAClass, WithMetricAnnotation("cpu"))), deploy(testNamespace, testRevision), diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index f9fc38d696b4..aaa08106c546 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -259,7 +259,7 @@ func computeActiveCondition(pa *pav1alpha1.PodAutoscaler, pc podCounts) { case pc.ready >= minReady: if pc.want > 0 || !pa.Status.IsInactive() { - pa.Status.MarkHasBeenActive() + pa.Status.MarkScaleTargetInitialized() // 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 b4e287290bd7..766c410b65ca 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -192,8 +192,8 @@ func markInactive(pa *asv1a1.PodAutoscaler) { pa.Status.MarkInactive("NoTraffic", "The target is not receiving traffic.") } -func markHasBeenActive(pa *asv1a1.PodAutoscaler) { - pa.Status.MarkHasBeenActive() +func markScaleTargetInitialized(pa *asv1a1.PodAutoscaler) { + pa.Status.MarkScaleTargetInitialized() } func kpa(ns, n string, opts ...PodAutoscalerOption) *asv1a1.PodAutoscaler { @@ -265,7 +265,7 @@ func TestReconcile(t *testing.T) { } activeKPAMinScale := func(g, w int32) *asv1a1.PodAutoscaler { return kpa( - testNamespace, testRevision, markActive, markHasBeenActive, withScales(g, w), WithReachabilityReachable, + testNamespace, testRevision, markActive, markScaleTargetInitialized, withScales(g, w), WithReachabilityReachable, withMinScale(defaultScale), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1), ) @@ -301,7 +301,7 @@ func TestReconcile(t *testing.T) { Name: "steady state", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metric(testNamespace, testRevision), @@ -383,14 +383,14 @@ func TestReconcile(t *testing.T) { Name: "create metric", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markHasBeenActive, + kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withScales(1, defaultScale), WithPAStatusService(testRevision)), defaultSKS, defaultDeployment, defaultEndpoints, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, withScales(1, defaultScale), + Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withScales(1, defaultScale), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), }}, WantCreates: []runtime.Object{ @@ -400,7 +400,7 @@ func TestReconcile(t *testing.T) { Name: "scale up deployment", Key: key, Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultSKS, metric(testNamespace, testRevision), @@ -501,7 +501,7 @@ func TestReconcile(t *testing.T) { defaultDeployment, defaultEndpoints, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAStatusService(testRevision), + Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithObservedGeneration(1)), }}, }, { @@ -545,7 +545,7 @@ func TestReconcile(t *testing.T) { defaultDeployment, defaultEndpoints, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, withMinScale(2), WithPAMetricsService(privateSvc), + Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withMinScale(2), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnreachable, WithObservedGeneration(1)), }}, @@ -561,7 +561,7 @@ func TestReconcile(t *testing.T) { makeSKSPrivateEndpoints(2, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, withMinScale(2), WithPAMetricsService(privateSvc), + Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withMinScale(2), WithPAMetricsService(privateSvc), withScales(2, defaultScale), WithPAStatusService(testRevision), WithReachabilityReachable, WithObservedGeneration(1)), }}, @@ -577,7 +577,7 @@ func TestReconcile(t *testing.T) { makeSKSPrivateEndpoints(2, testNamespace, testRevision), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markActive, markHasBeenActive, withMinScale(2), WithPAMetricsService(privateSvc), + Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withMinScale(2), WithPAMetricsService(privateSvc), withScales(2, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnknown, WithObservedGeneration(1)), }}, @@ -756,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, markHasBeenActive, withScales(1, 1), + kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withScales(1, 1), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), defaultSKS, metric(testNamespace, testRevision), @@ -853,7 +853,7 @@ func TestReconcile(t *testing.T) { minScalePatch, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: activatingKPAMinScale(underscale, markHasBeenActive), + Object: activatingKPAMinScale(underscale, markScaleTargetInitialized), }}, }, { // Scale to `minScale` and mark PA "active" @@ -943,7 +943,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -42 /* ebc */, scaling.MinActivators)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), metric(testNamespace, testRevision), @@ -956,7 +956,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -42 /* ebc */, 1982 /*numActivators*/)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady, WithNumActivators(4)), @@ -974,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, markHasBeenActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithNumActivators(2)), @@ -992,7 +992,7 @@ func TestReconcile(t *testing.T) { decider(testNamespace, testRevision, defaultScale, /* desiredScale */ 1 /* ebc */, scaling.MinActivators)), Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, markHasBeenActive, WithPAMetricsService(privateSvc), + kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithProxyMode, WithNumActivators(3)), diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index ef34b6bcb225..7cfe27f1d912 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -108,7 +108,7 @@ func testReadyPA(rev *v1.Revision) *av1alpha1.PodAutoscaler { pa := resources.MakePA(rev) pa.Status.InitializeConditions() pa.Status.MarkActive() - pa.Status.MarkHasBeenActive() + pa.Status.MarkScaleTargetInitialized() pa.Status.ServiceName = serviceName(rev.Name) return pa } diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 9b4bb82acfcb..8761b318a027 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -247,7 +247,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "pa-ready", withK8sServiceName("old-stuff"), WithLogURL, AllUnknownConditions), - pa("foo", "pa-ready", WithTraffic, WithHasBeenActive, WithPAStatusService("new-stuff"), WithReachabilityUnknown), + pa("foo", "pa-ready", WithTraffic, WithScaleTargetInitialized, WithPAStatusService("new-stuff"), WithReachabilityUnknown), deploy(t, "foo", "pa-ready"), image("foo", "pa-ready"), }, @@ -343,7 +343,7 @@ func TestReconcile(t *testing.T) { withK8sServiceName("ill-follow-the-sun"), WithLogURL, MarkRevisionReady, WithRevisionLabel(serving.RouteLabelKey, "foo")), pa("foo", "fix-mutated-pa", WithProtocolType(networking.ProtocolH2C), - WithTraffic, WithHasBeenActive, WithPAStatusService("fix-mutated-pa")), + WithTraffic, WithScaleTargetInitialized, WithPAStatusService("fix-mutated-pa")), deploy(t, "foo", "fix-mutated-pa"), image("foo", "fix-mutated-pa"), }, @@ -357,7 +357,7 @@ func TestReconcile(t *testing.T) { withDefaultContainerStatuses(), withObservedGeneration(1)), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ - Object: pa("foo", "fix-mutated-pa", WithTraffic, WithHasBeenActive, + Object: pa("foo", "fix-mutated-pa", WithTraffic, WithScaleTargetInitialized, WithPAStatusService("fix-mutated-pa"), WithReachabilityReachable), }}, WantEvents: []string{ @@ -511,7 +511,7 @@ func TestReconcile(t *testing.T) { // This signal should make our Reconcile mark the Revision as Ready. Objects: []runtime.Object{ rev("foo", "steady-ready", withK8sServiceName("very-steady"), WithLogURL), - pa("foo", "steady-ready", WithTraffic, WithHasBeenActive, WithPAStatusService("steadier-even")), + pa("foo", "steady-ready", WithTraffic, WithScaleTargetInitialized, WithPAStatusService("steadier-even")), deploy(t, "foo", "steady-ready"), image("foo", "steady-ready"), }, diff --git a/pkg/testing/functional.go b/pkg/testing/functional.go index 620c28077be1..cb66b45020e4 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -84,9 +84,9 @@ func WithTraffic(pa *asv1a1.PodAutoscaler) { pa.Status.MarkActive() } -// WithHasBeenActive updates the PA to reflect it having been active. -func WithHasBeenActive(pa *asv1a1.PodAutoscaler) { - pa.Status.MarkHasBeenActive() +// WithScaleTargetInitialized updates the PA to reflect it having been active. +func WithScaleTargetInitialized(pa *asv1a1.PodAutoscaler) { + pa.Status.MarkScaleTargetInitialized() } // WithPAStatusService annotates PA Status with the provided service name. From 751a4b1927b0e9b8c9ded02d4a717e3b152fdaf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Wed, 8 Jul 2020 11:52:48 +0200 Subject: [PATCH 5/6] Narrow down test. --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go index 664e678fe3d9..f7bd77060b68 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go +++ b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go @@ -981,16 +981,20 @@ func TestTypicalFlow(t *testing.T) { // When we see traffic, mark ourselves active. r.MarkActive() r.MarkScaleTargetInitialized() + apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t) apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t) apistest.CheckConditionSucceeded(r, PodAutoscalerConditionReady, t) // Check idempotency. r.MarkActive() + r.MarkScaleTargetInitialized() + apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t) apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t) apistest.CheckConditionSucceeded(r, PodAutoscalerConditionReady, t) // When we stop seeing traffic, mark ourselves inactive. r.MarkInactive("TheReason", "the message") + apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t) apistest.CheckConditionFailed(r, PodAutoscalerConditionActive, t) if !r.IsInactive() { t.Error("IsInactive was not set.") @@ -1000,6 +1004,7 @@ func TestTypicalFlow(t *testing.T) { // When traffic hits the activator and we scale up the deployment we mark // ourselves as activating. r.MarkActivating("Activating", "Red team, GO!") + apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t) apistest.CheckConditionOngoing(r, PodAutoscalerConditionActive, t) apistest.CheckConditionOngoing(r, PodAutoscalerConditionReady, t) @@ -1009,6 +1014,8 @@ func TestTypicalFlow(t *testing.T) { if !r.IsActive() { t.Error("Active was not set.") } + apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t) + apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t) apistest.CheckConditionSucceeded(r, PodAutoscalerConditionReady, t) } From 57e4ea5c9943d07facc83727189a257fd3abfb09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Wed, 8 Jul 2020 14:38:02 +0200 Subject: [PATCH 6/6] Fix comment. --- pkg/testing/functional.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/testing/functional.go b/pkg/testing/functional.go index cb66b45020e4..2b8ef3fa4998 100644 --- a/pkg/testing/functional.go +++ b/pkg/testing/functional.go @@ -84,7 +84,8 @@ func WithTraffic(pa *asv1a1.PodAutoscaler) { pa.Status.MarkActive() } -// WithScaleTargetInitialized updates the PA to reflect it having been active. +// WithScaleTargetInitialized updates the PA to reflect it having initialized its +// ScaleTarget. func WithScaleTargetInitialized(pa *asv1a1.PodAutoscaler) { pa.Status.MarkScaleTargetInitialized() }