Add new KPA status for dependencies #8809

vagababov · 2020-07-25T02:53:51Z

This initial version only deals with SKS and sets deps
not ready if SKS is not ready and ready otherwise.

I did not remove yet the defaulting to 0 of got, since it would require too many test changes, which I did not want to put in this PR.

This is for #8792.

/assign @yanweiguo mattmoor @markusthoemmes @julz

This initial version only deals with SKS and sets deps not ready if SKS is not ready and ready otherwise. Lots of test churn... :-)

vagababov · 2020-07-25T03:46:13Z

Alright, should be all golden to go :)

pkg/reconciler/autoscaling/kpa/kpa.go

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

pkg/apis/autoscaling/v1alpha1/pa_types.go

pkg/reconciler/autoscaling/kpa/kpa_test.go

vagababov

Updated. Thanks.

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

pkg/apis/autoscaling/v1alpha1/pa_types.go

pkg/reconciler/autoscaling/kpa/kpa.go

knative-test-reporter-robot · 2020-07-25T07:14:19Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/e2e.TestMultipleNamespace
test/e2e.TestRollbackBYOName
test/e2e.TestCallToPublicService

vagababov · 2020-07-25T17:32:49Z

/retest

mattmoor · 2020-07-26T13:37:15Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -185,6 +186,18 @@ func (pas *PodAutoscalerStatus) MarkScaleTargetInitialized() {
 	podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionScaleTargetInitialized)
 }

+// MarkDependenciesReadymarks the PA condition denoting that all its


Suggested change

// MarkDependenciesReadymarks the PA condition denoting that all its

// MarkDependenciesReady marks the PA condition denoting that all its

mattmoor · 2020-07-26T13:38:34Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

+
+// MarkDependenciesNotReady marks the PA condation that at least one of the
+// dependendcies is not ready.
+func (pas *PodAutoscalerStatus) MarkDependenciesNotReady(reason, message string) {


I expected NotReady to be False, but perhaps it is sort of ambiguous. 🤷

mattmoor · 2020-07-26T13:39:01Z

pkg/apis/autoscaling/v1alpha1/pa_types.go

@@ -117,6 +117,9 @@ const (
 	PodAutoscalerConditionScaleTargetInitialized apis.ConditionType = "ScaleTargetInitialized"
 	// PodAutoscalerConditionActive is set when the PodAutoscaler's ScaleTargetRef is receiving traffic.
 	PodAutoscalerConditionActive apis.ConditionType = "Active"
+	// PodAutoscalerCondtionDependenciesReady is set when all the dependencies are ready and
+	// the revision is ready to scale.
+	PodAutoscalerConditionDependenciesReady = "DependeciesReady"


Suggested change

PodAutoscalerConditionDependenciesReady = "DependeciesReady"

PodAutoscalerConditionDependenciesReady = "DependenciesReady"

mattmoor · 2020-07-26T13:40:13Z

pkg/reconciler/autoscaling/kpa/kpa.go

 		// Before we can reconcile decider and get real number of activators
 		// we start with default of 2.
 		if _, err = c.ReconcileSKS(ctx, pa, nv1alpha1.SKSOperationModeServe, 0 /*numActivators == all*/); err != nil {
 			return fmt.Errorf("error reconciling SKS: %w", err)
 		}
+		pa.Status.MarkDependenciesNotReady("DependenciesNotReady", "SKS is provisioning")


"DependenciesNotReady" is redundant with the new Type. I'd use this to identify the dependency itself, e.g. SKSNotReady or something

mattmoor · 2020-07-26T13:40:35Z

pkg/reconciler/autoscaling/kpa/kpa.go

 		ready = 0
+		pa.Status.MarkDependenciesNotReady("DependenciesNotReady", "SKS is provisioning")


mattmoor · 2020-07-26T13:43:18Z

pkg/reconciler/autoscaling/kpa/kpa.go

+		pa.Status.MarkDependenciesNotReady("DependenciesNotReady", "SKS is provisioning")
+	} else {
+		logger.Debug("SKS is ready, marking dependencies ready")
+		pa.Status.MarkDependenciesReady()


Thinking about this at a meta level for a moment... Are you sure you want a single condition for all dependencies?
In general, I'd probably advocate for a condition per dependency.

I comment on that here because one of the problems you have with an N-ary condition is: Who should mark it ready?

The logic we have for managing the actual "Ready" condition sort of solves this already, assuming you have a sub-condition per dependency.

vagababov · 2020-07-26T17:58:39Z

Sure, rewrote with SKS specific condition.

mattmoor · 2020-07-26T18:15:14Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

+func (pas *PodAutoscalerStatus) MarkSKSNotReady() {
+	podCondSet.Manage(pas).MarkUnknown(PodAutoscalerSKSReady, "SKSNotReady", "SKS is not ready yet")


SKSNotReady is redundant with the Type itself. Generally this context is passed through from the caller.

mattmoor · 2020-07-26T18:16:21Z

pkg/reconciler/autoscaling/kpa/kpa.go

 		// Before we can reconcile decider and get real number of activators
 		// we start with default of 2.
 		if _, err = c.ReconcileSKS(ctx, pa, nv1alpha1.SKSOperationModeServe, 0 /*numActivators == all*/); err != nil {
 			return fmt.Errorf("error reconciling SKS: %w", err)
 		}
+		pa.Status.MarkSKSNotReady()


Suggested change

pa.Status.MarkSKSNotReady()

pa.Status.MarkSKSNotReady("NoPrivateService", "blah")

mattmoor · 2020-07-26T18:16:40Z

pkg/reconciler/autoscaling/kpa/kpa.go

 		ready = 0
+		pa.Status.MarkSKSNotReady()


You could bubble up SKS's Ready condition's Reason/Message here.

vagababov · 2020-07-26T18:50:15Z

used the message, reason just kept generic

knative-metrics-robot · 2020-07-26T19:36:14Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/autoscaling/kpa/kpa.go	94.5%	94.8%	0.3

mattmoor · 2020-07-26T20:39:23Z

pkg/reconciler/autoscaling/kpa/kpa.go

 		ready = 0
-		pa.Status.MarkSKSNotReady()
+		pa.Status.MarkSKSNotReady(sks.Status.GetCondition(nv1alpha1.ServerlessServiceConditionReady).Message)


This can return nil, so I'd guess this will make our controllers crash loop 😬

This one cannot: we get here only if PrivateServiceName is set, which means SKS reconciled this at least once, which means InitializeConditions has been invoked and sks updated the status.

i see the sks.Status.PrivateServiceName guard, which ensures things have been initialized to reach this point.

mattmoor

/lgtm

mattmoor · 2020-07-26T20:48:15Z

pkg/reconciler/autoscaling/kpa/kpa.go

 		ready = 0
-		pa.Status.MarkSKSNotReady()
+		pa.Status.MarkSKSNotReady(sks.Status.GetCondition(nv1alpha1.ServerlessServiceConditionReady).Message)


i see the sks.Status.PrivateServiceName guard, which ensures things have been initialized to reach this point.

knative-prow-robot · 2020-07-26T20:48:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/autoscaling/OWNERS~~ [mattmoor,vagababov]
~~pkg/reconciler/OWNERS~~ [mattmoor,vagababov]
~~pkg/testing/OWNERS~~ [mattmoor,vagababov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vagababov added 2 commits July 24, 2020 19:51

Add new Kpa status for dependencies

2692df5

This initial version only deals with SKS and sets deps not ready if SKS is not ready and ready otherwise. Lots of test churn... :-)

t Merge branch 'master' into 20200724-kpa-status

ad7fba5

knative-prow-robot assigned julz, markusthoemmes, mattmoor and yanweiguo Jul 25, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 25, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2020

knative-prow-robot requested review from julz and yanweiguo July 25, 2020 02:53

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale labels Jul 25, 2020

more files to fix

7b79765

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Jul 25, 2020

yanweiguo reviewed Jul 25, 2020

View reviewed changes

review

a8ba223

vagababov commented Jul 25, 2020

View reviewed changes

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go Outdated Show resolved Hide resolved

pkg/apis/autoscaling/v1alpha1/pa_types.go Outdated Show resolved Hide resolved

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved

restore tests

22d40be

mattmoor reviewed Jul 26, 2020

View reviewed changes

Merge branch 'master' into 20200724-kpa-status

983c811

vagababov added 2 commits July 26, 2020 10:58

rewrite as sks only cond

9a2048c

holdout

3dbfdd2

mattmoor reviewed Jul 26, 2020

View reviewed changes

vagababov changed the title ~~Add new Kpa status for dependencies~~ Add new KPA status for dependencies Jul 26, 2020

more fixes

4b1ca7d

mattmoor reviewed Jul 26, 2020

View reviewed changes

mattmoor approved these changes Jul 26, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2020

knative-prow-robot merged commit 1642dd1 into knative:master Jul 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new KPA status for dependencies #8809

Add new KPA status for dependencies #8809

vagababov commented Jul 25, 2020

vagababov commented Jul 25, 2020

vagababov left a comment

knative-test-reporter-robot commented Jul 25, 2020

vagababov commented Jul 25, 2020

mattmoor Jul 26, 2020

mattmoor Jul 26, 2020

mattmoor Jul 26, 2020

mattmoor Jul 26, 2020

mattmoor Jul 26, 2020

mattmoor Jul 26, 2020

vagababov commented Jul 26, 2020

mattmoor Jul 26, 2020

mattmoor Jul 26, 2020

mattmoor Jul 26, 2020

vagababov commented Jul 26, 2020

knative-metrics-robot commented Jul 26, 2020

mattmoor Jul 26, 2020

vagababov Jul 26, 2020

mattmoor Jul 26, 2020

mattmoor left a comment

mattmoor Jul 26, 2020

knative-prow-robot commented Jul 26, 2020

	// MarkDependenciesReadymarks the PA condition denoting that all its
	// MarkDependenciesReady marks the PA condition denoting that all its

	PodAutoscalerConditionDependenciesReady = "DependeciesReady"
	PodAutoscalerConditionDependenciesReady = "DependenciesReady"

		ready = 0
		pa.Status.MarkDependenciesNotReady("DependenciesNotReady", "SKS is provisioning")

		func (pas *PodAutoscalerStatus) MarkSKSNotReady() {
		podCondSet.Manage(pas).MarkUnknown(PodAutoscalerSKSReady, "SKSNotReady", "SKS is not ready yet")

	pa.Status.MarkSKSNotReady()
	pa.Status.MarkSKSNotReady("NoPrivateService", "blah")

Add new KPA status for dependencies #8809

Add new KPA status for dependencies #8809

Conversation

vagababov commented Jul 25, 2020

vagababov commented Jul 25, 2020

vagababov left a comment

Choose a reason for hiding this comment

knative-test-reporter-robot commented Jul 25, 2020

vagababov commented Jul 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vagababov commented Jul 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vagababov commented Jul 26, 2020

knative-metrics-robot commented Jul 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Jul 26, 2020