Add PA condition InitiallyActive #8354

taragu · 2020-06-17T15:07:45Z

/lint

Part 5 of #7682

Proposed Changes

This PR adds (and sets) a new condition on PA: InitiallyActive. This condition is set to true when a PA becomes active, and never set back to false. We need this condition to be able to tell if a PA has reached its initial scale before in its existence because InitialScale only applies during the first activation, so we need this condition to know when this activation is the first one.

Release Note

NONE

/cc @julz @vagababov @markusthoemmes @dprotaso @mattmoor

knative-prow-robot

@taragu: 0 warnings.

In response to this:

/lint

Part 5 of #7682

Proposed Changes

This PR adds (and sets) a new condition on PA: InitiallyActive. This condition is set to true when a PA becomes active, and never set back to false. We need this condition to be able to tell if a PA has reached its initial scale before in its existence.

Release Note
NONE
/cc @julz @vagababov @markusthoemmes @dprotaso @mattmoor

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

julz · 2020-06-17T15:13:57Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -170,6 +170,16 @@ func (pas *PodAutoscalerStatus) IsInactive() bool {
 	return pas.GetCondition(PodAutoscalerConditionActive).IsFalse()
 }

+// IsInitiallyActive returns true if the pod autoscaler has reached its initial scale.
+func (pas *PodAutoscalerStatus) IsInitiallyActive() bool {


could be worth bike-shedding on this a little. Maybe HasBeenActive or HasActivatedOnce? IsInitiallyActive seems to imply it's currently in its first activation, which isn't quite right

julz

looks good to me apart from wanting to bike-shed a bit on the name, @vagababov @markusthoemmes wdyt?

vagababov · 2020-06-17T18:50:08Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go

@@ -1143,3 +1143,14 @@ func TestScaleToZeroPodRetention(t *testing.T) {
 		})
 	}
 }
+
+func TestInitiallyActive(t *testing.T) {
+	p := &PodAutoscaler{}


Suggested change

p := &PodAutoscaler{}

p := PodAutoscaler{}

will probably work :)

vagababov · 2020-06-17T18:51:15Z

pkg/apis/autoscaling/v1alpha1/pa_types.go

+	// PodAutoscalerConditionInitiallyActive is set when PodAutoscaler has first reached its initial scale.
+	PodAutoscalerConditionInitiallyActive apis.ConditionType = "InitiallyActive"


Is this an informational status? If so we should comment to that. If not — it'd be interesting to know why.
Also should it be a status, or perhaps this is a good thing to put in the status.Annotations?

My 2c is this is informational. As a user the behaviour will be different before/after first activation, so knowing if the revision successfully passed its first activation is a reasonable thing for a user to want to see in conditions.

I don't think this is informational for the users. We can put this into status.Annotations.

(not that I’m super bothered personally, an annotations works too)

I guess we can always expose it as a condition later if we want to 🤷‍♂️

That's a good question, I think I was mostly rumbling as these changes are kinda out of scope. Maybe it makes sense to think through them before adding new conditions though.

SO what's the consensus? Informational status for now and long overdue refactoring later?

I think on slack we landed on a true/false status for now, so long as you agree too :)

Sure, but true/false can be mandatory (i.e. included in happy computation) or informational :)

Eh, status.Annotation :) Sure.

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

vagababov · 2020-06-17T21:56:56Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -170,6 +172,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[initiallyActiveAnnotation]; !ok || val != "true" {


would you rather care for the time when it happened? Might be useful in future and it'll be harder to change later.

Hmm if we want to know how long it’s been “initially active”, making this a condition is probably better. Also if we write a time stamp here for the annotation, table tests will be difficult.

🤷 we can keep true. May be we don't need when.
I am fine with status as well. But I think it should be informational, rather than gating.

julz

/lgtm

vagababov · 2020-06-18T19:26:37Z

needs merge

vagababov · 2020-06-18T19:28:25Z

/approve

knative-prow-robot · 2020-06-18T19:28:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: taragu, 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~~ [vagababov]
~~pkg/reconciler/autoscaling/OWNERS~~ [vagababov]

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

knative-metrics-robot · 2020-06-18T19:43:43Z

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	93.5%	93.6%	0.1

julz · 2020-06-18T19:52:18Z

/lgtm

knative-prow-robot requested review from dprotaso, julz, markusthoemmes, mattmoor and vagababov June 17, 2020 15:07

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

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/API API objects and controllers area/autoscale labels Jun 17, 2020

knative-prow-robot reviewed Jun 17, 2020

View reviewed changes

julz reviewed Jun 17, 2020

View reviewed changes

vagababov reviewed Jun 17, 2020

View reviewed changes

julz reviewed Jun 17, 2020

View reviewed changes

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

vagababov reviewed Jun 17, 2020

View reviewed changes

taragu force-pushed the tara-init-scale-0-PA-condition branch from 921b749 to 55a031d Compare June 18, 2020 16:41

julz reviewed Jun 18, 2020

View reviewed changes

knative-prow-robot assigned julz Jun 18, 2020

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

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2020

Tara Gu added 4 commits June 18, 2020 15:34

Add PodAutoscaler condition for initial scale

438a25e

Set initial active condition

610e94a

Use PA status annotation instead of condition

a817249

Rename MarkInitiallyActive -> MarkHasBeenActive

66e8a3a

taragu force-pushed the tara-init-scale-0-PA-condition branch from 55a031d to 66e8a3a Compare June 18, 2020 19:37

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2020

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2020

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

knative-prow-robot merged commit 7be0f43 into knative:master Jun 18, 2020

taragu deleted the tara-init-scale-0-PA-condition branch July 29, 2020 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PA condition InitiallyActive #8354

Add PA condition InitiallyActive #8354

taragu commented Jun 17, 2020 •

edited

Loading

knative-prow-robot left a comment

julz Jun 17, 2020

julz left a comment

vagababov Jun 17, 2020

vagababov Jun 17, 2020

julz Jun 17, 2020

taragu Jun 17, 2020

julz Jun 17, 2020

julz Jun 17, 2020

markusthoemmes Jun 18, 2020

vagababov Jun 18, 2020

julz Jun 18, 2020

vagababov Jun 18, 2020

vagababov Jun 18, 2020

vagababov Jun 17, 2020

taragu Jun 17, 2020

vagababov Jun 18, 2020

julz left a comment

vagababov commented Jun 18, 2020

vagababov commented Jun 18, 2020

knative-prow-robot commented Jun 18, 2020

knative-metrics-robot commented Jun 18, 2020

julz commented Jun 18, 2020

		// PodAutoscalerConditionInitiallyActive is set when PodAutoscaler has first reached its initial scale.
		PodAutoscalerConditionInitiallyActive apis.ConditionType = "InitiallyActive"

Add PA condition InitiallyActive #8354

Add PA condition InitiallyActive #8354

Conversation

taragu commented Jun 17, 2020 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Choose a reason for hiding this comment

julz left a 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

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

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

julz left a comment

Choose a reason for hiding this comment

vagababov commented Jun 18, 2020

vagababov commented Jun 18, 2020

knative-prow-robot commented Jun 18, 2020

knative-metrics-robot commented Jun 18, 2020

julz commented Jun 18, 2020

taragu commented Jun 17, 2020 •

edited

Loading