Propagate HasBeenActive to a terminal condition as "ScaleTargetInitialized". by markusthoemmes · Pull Request #8569 · knative/serving

markusthoemmes · 2020-07-07T13:26:46Z

First step of #8540

Proposed Changes

Propagate HasBeenActive to a terminal condition.

Release Note

NONE

/assign @vagababov @julz @taragu

/hold
Until after the release.

It will eventually be superceding "Active" as the PA's readiness gate.

knative-prow-robot

@markusthoemmes: 0 warnings.

Details

In response to this:

First step of #8540

Proposed Changes

Propagate HasBeenActive to a terminal condition.

Release Note
NONE
/assign @vagababov @julz @taragu

/hold
Until after the release.

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.

knative-prow-robot · 2020-07-07T13:27:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/apis/autoscaling/OWNERS~~ [markusthoemmes]
~~pkg/reconciler/OWNERS~~ [markusthoemmes]
~~pkg/testing/OWNERS~~ [markusthoemmes]

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

julz

/lgtm

markusthoemmes · 2020-07-07T14:50:07Z

/retest

Doubt it's related.

knative-test-reporter-robot · 2020-07-07T14:55:39Z

The following jobs failed:

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

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

pkg/reconciler/revision.TestNewRevisionCallsSyncHandler

markusthoemmes · 2020-07-07T15:15:31Z

/test pull-knative-serving-unit-tests

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

vagababov · 2020-07-07T17:53:12Z

/lgtm

dprotaso · 2020-07-07T18:09:31Z

pkg/apis/autoscaling/v1alpha1/pa_types.go

 	PodAutoscalerConditionReady = apis.ConditionReady
+	// PodAutoscalerConditionHasBeenActive is set when the PodAutoscaler's ScaleTargetRef was ready to
+	// serve traffic once.
+	PodAutoscalerConditionHasBeenActive apis.ConditionType = "HasBeenActive"


HasBeenActive sounds a bit odd to me - how about Deployed

ScaleTargetDeployed (maybe just TargetDeployed) might be more specific

I almost feel like we should just set Ready 😂

Not necessarily true. With initialScale=0 — nothing will ever be deployed

Hmm ScaleTargetDeployed might imply new calculated autoscaler scale targets. What about InitialTargetDeployed?

TargetInitialized ?

Or maybe ReachedInitialScale, since that’s what actually gates the condition

ScaleTargetInitialized would be my favorite mix from the above. I would like to keep InitialScale out of this, it doesn't necessarily gate this, for example if minScale is > initialScale.

Hm, in my head when minScale > initialScale, initialScale is implicitly set to minScale, but I see how that could be confusing.

ScaleTargetInitialized seems alright, just a bit odd that it talks about a "Scale Target" concept that isn't really defined anywhere else, and doesn't mention the quite important fact that it's the scale target for the first scale we're talking about. I'm also not totally sure what "Initialized" means in this context.. "-Reached" seems clearer about what has to happen. Not the end of the world, though, if everyone else is happy Im happy.

I think it works because it's part of the PodAutoscaler spec

serving/pkg/apis/autoscaling/v1alpha1/pa_types.go

Lines 98 to 100 in f431439

// ScaleTargetRef defines the /scale-able resource that this PodAutoscaler

// is responsible for quickly right-sizing.

ScaleTargetRef corev1.ObjectReference `json:"scaleTargetRef"`

pkg/testing/functional.go

dprotaso · 2020-07-08T14:28:19Z

pkg/reconciler/autoscaling/kpa/kpa.go

-			pa.Status.MarkHasBeenActive()
+			pa.Status.MarkScaleTargetInitialized()
 			// SKS should already be active.
 			pa.Status.MarkActive()


Could MarkActive also set the MarkScaleTargetInitialized or do you prefer to keep the two methods separate?

I think I prefer them to be separate. I tried that too, but it felt slightly awkward.

dprotaso · 2020-07-08T14:42:19Z

/lgtm

vagababov

/lgtm

vagababov · 2020-07-08T17:23:16Z

/unhold

julz

/lgtm

knative-metrics-robot · 2020-07-08T20:28:21Z

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/hpa/hpa.go	96.7%	96.8%	0.1

Propagate HasBeenActive to a terminal condition.

34e09d9

It will eventually be superceding "Active" as the PA's readiness gate.

knative-prow-robot assigned julz Jul 7, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2020

knative-prow-robot assigned taragu and vagababov Jul 7, 2020

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 7, 2020

knative-prow-robot requested review from coryrc, mattmoor, vagababov and yanweiguo July 7, 2020 13:26

knative-prow-robot reviewed Jul 7, 2020

View reviewed changes

knative-prow-robot added area/API API objects and controllers area/autoscale area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Jul 7, 2020

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

julz reviewed Jul 7, 2020

View reviewed changes

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

Fix HPA.

ad6e4b5

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

taragu reviewed Jul 7, 2020

View reviewed changes

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

Remove unused constant.

f431439

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

markusthoemmes unassigned julz and vagababov Jul 7, 2020

markusthoemmes assigned dprotaso, julz and vagababov and unassigned taragu Jul 7, 2020

dprotaso reviewed Jul 7, 2020

View reviewed changes

markusthoemmes mentioned this pull request Jul 8, 2020

Rework PodAutoscaler conditions to better fit K8s readiness model #8540

Closed

markusthoemmes changed the title ~~Propagate HasBeenActive to a terminal condition.~~ Propagate HasBeenActive to a terminal condition as "ScaleTargetInitialized". Jul 8, 2020

markusthoemmes added 2 commits July 8, 2020 11:51

Rename condition to ScaleTargetInitialized.

241a3e3

Narrow down test.

751a4b1

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 8, 2020

julz reviewed Jul 8, 2020

View reviewed changes

pkg/testing/functional.go Outdated Show resolved Hide resolved

Fix comment.

57e4ea5

dprotaso reviewed Jul 8, 2020

View reviewed changes

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

vagababov reviewed Jul 8, 2020

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2020

julz reviewed Jul 8, 2020

View reviewed changes

markusthoemmes closed this Jul 8, 2020

markusthoemmes reopened this Jul 8, 2020

knative-prow-robot merged commit ddd6bd9 into knative:master Jul 8, 2020

julz mentioned this pull request Jul 23, 2020

Add julz to autoscaling approvers #8771

Merged

3 tasks

	// ScaleTargetRef defines the /scale-able resource that this PodAutoscaler
	// is responsible for quickly right-sizing.
	ScaleTargetRef corev1.ObjectReference `json:"scaleTargetRef"`

Conversation

markusthoemmes commented Jul 7, 2020

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

knative-prow-robot commented Jul 7, 2020

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Jul 7, 2020

Uh oh!

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

Uh oh!

markusthoemmes commented Jul 7, 2020

Uh oh!

Uh oh!

vagababov commented Jul 7, 2020

Uh oh!

dprotaso Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Jul 8, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

vagababov commented Jul 8, 2020

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

dprotaso Jul 7, 2020 •

edited

Loading

dprotaso Jul 7, 2020 •

edited

Loading