Scale zero activating timeout by lvjing2 · Pull Request #4232 · knative/serving

lvjing2 · 2019-06-04T01:37:58Z

Fixes #
problematic pods can't be scale to zero forever.

Proposed Changes

check whether activating is timeout or not, if timeout, then scale to zero if min==0, and EnableScaleToZero=true

Release Note

enable to scale to zero when activating timeout when the pod can't be ready like crashlooping or some other status

knative-prow-robot · 2019-06-04T01:38:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lvjing2
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: greghaynes

If they are not already assigned, you can assign the PR to them by writing /assign @greghaynes in a comment when ready.

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:

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

knative-prow-robot · 2019-06-04T01:38:13Z

Hi @lvjing2. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

vagababov

Is there a related issue?

vagababov · 2019-06-04T01:46:42Z

/ok-to-test

I would like to see an integration test for this as well.

knative-metrics-robot · 2019-06-04T01:51:00Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go	98.2%	96.5%	-1.7
pkg/reconciler/autoscaling/kpa/kpa.go	91.3%	91.4%	0.2
pkg/reconciler/autoscaling/kpa/scaler.go	83.7%	80.4%	-3.3

vagababov · 2019-06-04T02:20:02Z

Also, please run unit tests locally before posting PRs...

knative-metrics-robot · 2019-06-04T05:01:47Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go	98.2%	96.5%	-1.7
pkg/reconciler/autoscaling/kpa/kpa.go	91.3%	91.4%	0.2
pkg/reconciler/autoscaling/kpa/scaler.go	83.7%	83.2%	-0.5

lvjing2 · 2019-06-04T05:03:14Z

Is there a related issue?
I found #3456 is

vagababov · 2019-06-04T05:25:55Z

Then it should be attached to the issue, so it can get closed and tracked :) You can edit the first message.

knative-prow-robot · 2019-06-04T05:26:24Z

@lvjing2: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-serving-integration-tests	`faa3dd5`	link	`/test pull-knative-serving-integration-tests`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

vagababov · 2019-06-04T05:26:57Z

-		logger.Debug("Metrics are not yet being collected.")
-		return desiredScale, nil
+		// check is activating timeout
+		if pa.Status.IsActivatingTimeout(autoscalerConfig.ActivatingTimeout) {


I don't think that this is correct. It should not really matter in the negative case at all.

vagababov · 2019-06-04T05:30:00Z

 	if pa.Status.IsActivating() { // Active=Unknown
 		// Don't scale-to-zero during activation
 		if min, _ := pa.ScaleBounds(); min == 0 {
-			if pa.Status.IsActivatingTimeout(config.ActivatingTimeout) {


This code was the main code that did scale to zero, so it should've remained somewhere, right?

vagababov · 2019-06-04T05:31:04Z


+// IsActivatingTimeout checks whether the pod autoscaler has been in activating
+// for at least the specified timeout period.
+func (pas *PodAutoscalerStatus) IsActivatingTimeout(activatingTimeout time.Duration) bool {


Probably here and in the config-map, etc "Activation Timeout" is a more correct name.

vagababov · 2019-06-04T05:31:21Z


+	if pa.Status.IsActivating() {
+		timeout := config.FromContext(ctx).Autoscaler.ActivatingTimeout
+		logger.Infof("enqueue delay for check activating timeout after %v.", timeout)


Logging statement.

Suggested change

logger.Infof("enqueue delay for check activating timeout after %v.", timeout)

logger.Infof("Enqueue with delay to check activation timeout after %v.", timeout)

markusthoemmes

Sorry to add to the pile but there is #4094 trying to fix this as well.

There have been discussions on how to best attack this and the kind of agreed upon solution is to surface the revision's status in the PodAutoscaler so it can act on that. We already have deployment timeouts and handling of defunct pods in the revision handling so that makes sure we're not duplicating things. The ActivatingTimeout you're proposing essentially duplicates the timeout logic that already lives on the revision level.

/hold

To make sure this gets discussed.

lvjing2 · 2019-06-05T02:33:28Z

@markusthoemmes Hi, I didn't recognized that there is already timeout for revision. if so, I think it would be better if we can unify the timeout status of revision and kpa by #4094 .

markusthoemmes · 2019-07-01T07:59:41Z

/close

Per last comment.

knative-prow-robot · 2019-07-01T07:59:42Z

@markusthoemmes: Closed this PR.

Details

In response to this:

/close

Per last comment.

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.

lvjing2 added 2 commits June 4, 2019 09:30

check activating timeout to scale zeor

595a552

not mark activating timeout

a7f2118

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 4, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 4, 2019

knative-prow-robot added area/API API objects and controllers area/autoscale labels Jun 4, 2019

knative-prow-robot requested review from markusthoemmes, mdemirhan and vagababov June 4, 2019 01:38

vagababov reviewed Jun 4, 2019

View reviewed changes

Comment thread pkg/reconciler/autoscaling/kpa/scaler.go Outdated

Comment thread pkg/reconciler/autoscaling/kpa/scaler.go Outdated

Comment thread pkg/reconciler/autoscaling/kpa/scaler.go Outdated

Comment thread pkg/reconciler/autoscaling/kpa/scaler.go Outdated

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 4, 2019

check activating timeout for all

faa3dd5

vagababov reviewed Jun 4, 2019

View reviewed changes

markusthoemmes suggested changes Jun 4, 2019

View reviewed changes

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

knative-prow-robot closed this Jul 1, 2019

	logger.Infof("enqueue delay for check activating timeout after %v.", timeout)
	logger.Infof("Enqueue with delay to check activation timeout after %v.", timeout)

Conversation

lvjing2 commented Jun 4, 2019

Proposed Changes

Uh oh!

knative-prow-robot commented Jun 4, 2019

Uh oh!

knative-prow-robot commented Jun 4, 2019

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vagababov commented Jun 4, 2019

Uh oh!

knative-metrics-robot commented Jun 4, 2019

Uh oh!

vagababov commented Jun 4, 2019

Uh oh!

knative-metrics-robot commented Jun 4, 2019

Uh oh!

lvjing2 commented Jun 4, 2019

Uh oh!

vagababov commented Jun 4, 2019

Uh oh!

knative-prow-robot commented Jun 4, 2019

Uh oh!

vagababov Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

vagababov Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

vagababov Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

vagababov Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

lvjing2 commented Jun 5, 2019

Uh oh!

markusthoemmes commented Jul 1, 2019

Uh oh!

knative-prow-robot commented Jul 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants