[WIP] Stabilize 1->0 case by akyyy · Pull Request #1251 · knative/serving

akyyy · 2018-06-18T16:58:49Z

Fixes #
#1250

Proposed Changes

Add deactivating condition for revision so there is no race between revision controller and route controller. Tearing down k8s resources need to happen after we route traffic to activator service.
Keep the revision service and deployment while we tearing down k8s resources for 1->0. So we don't let the terminating pod block creating new pod. This is useful to serve traffic while tearing down.
Don't include route label in revisions. So when we update k8s deployment, there is only one replicaSet.

knative-metrics-robot · 2018-06-18T21:49:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.3%	96.6%	-1.7
pkg/controller/route/route.go	78.5%	78.1%	-0.4
pkg/controller/route/route_test.go	78.5%	78.1%	-0.4
pkg/controller/revision/revision.go	80.0%	76.6%	-3.4
pkg/controller/revision/revision_test.go	80.0%	76.6%	-3.4

*TestCoverage feature is being tested, do not rely on any info here yet

akyyy · 2018-06-18T22:21:31Z

/retest

josephburnett

Just noticed a few nits.

/lgtm

josephburnett · 2018-06-18T23:00:08Z

+	timeout := 500 * time.Millisecond
+
+	i := 0
 	for ; i < maxRetry; i++ {


@isdal had a good point. We should probe for iptable programming by hitting the health check endpoint. That way POST requests are actually retried. But that's fine to do as a separate pull request.

josephburnett · 2018-06-18T23:01:40Z

+		logger.Info("Deleted service")
+	} else {
+		// Serving state is RevisionServingStateReserve. Delete the revision service and update
+		// the dpeloyment replicas to be 0.


Nit: spelling: d[e]ployment

josephburnett · 2018-06-18T23:02:03Z

+			}
+		}
+
+		logger.Info("Scale the deployment to 0")


Nit: scal[ing] the deployment to 0

josephburnett · 2018-06-18T23:03:19Z

+		// Serving state is RevisionServingStateReserve. Delete the revision service and update
+		// the dpeloyment replicas to be 0.
+		if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil {
+			if cond.Reason != "Inactive" {


I think you can combine lines 497 and 498 into a single if statement.

josephburnett · 2018-06-18T23:05:08Z

+		// the dpeloyment replicas to be 0.
+		if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil {
+			if cond.Reason != "Inactive" {
+				if cond.Reason != "Deactivating" {


To reduce the amount of indentation here, how about writing this as a short-circuiting check.

if cond.Reason == "Deactivating" { return nil } ...

josephburnett · 2018-06-18T23:06:06Z

+		return err
+	}
+
+	logger.Infof("Deactvaing Deployment %q", deploymentName)


Nit: spelling: deact[i]va[t]ing

knative-metrics-robot · 2018-06-19T07:50:06Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.3%	96.6%	-1.7
pkg/controller/route/route.go	78.7%	78.1%	-0.6
pkg/controller/route/route_test.go	78.7%	78.1%	-0.6
pkg/controller/revision/revision.go	79.7%	76.6%	-3.1
pkg/controller/revision/revision_test.go	79.7%	76.6%	-3.1

*TestCoverage feature is being tested, do not rely on any info here yet

tcnghia · 2018-06-19T15:36:50Z

+		}
+		logger.Info("Deleted service")
+	} else {
+		// Serving state is RevisionServingStateReserve. Delete the revision service and update


I don't see a deletion below, is this comment correct?

tcnghia · 2018-06-19T15:47:53Z

+	} else {
+		// Serving state is RevisionServingStateReserve. Delete the revision service and update
+		// the deployment replicas to be 0.
+		if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil && cond.Reason != "Inactive" {


I believe there are now constants for reasons in revision_types.go ?

+1. There is also a "Deactivating" reason string above that could be a constant.

The constants are still strings. I don't see them defined in revision_types.go? The issue is still open. #880
We may address that later.

tcnghia · 2018-06-19T15:49:15Z

+	*deployment.Spec.Replicas = int32(0)
+	_, err = dc.Update(deployment)
+	if err != nil {
+		logger.Errorf("Error deactivating deployment %q: %s", deploymentName, err)


return err ?

(or if that isn't desirable we should still return to avoid seeing the log.Infof success here?

mdemirhan · 2018-06-19T16:10:35Z

@@ -48,28 +47,28 @@ type activationHandler struct {
 type retryRoundTripper struct{}

 func (rrt retryRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {


Creating a new transport for every transaction might be problematic. Should we not cache this?

Fixed. Thanks!

knative-metrics-robot · 2018-06-19T16:57:28Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.3%	96.6%	-1.7
pkg/controller/route/route.go	78.3%	78.1%	-0.2
pkg/controller/route/route_test.go	78.3%	78.1%	-0.2
pkg/controller/revision/revision.go	79.7%	75.5%	-4.2
pkg/controller/revision/revision_test.go	79.7%	75.5%	-4.2

*TestCoverage feature is being tested, do not rely on any info here yet

tcnghia

/lgtm
/approve

josephburnett · 2018-06-19T17:05:19Z

/lgtm
/approve

josephburnett · 2018-06-19T19:07:59Z

/lgtm
/approve

google-prow-robot · 2018-06-19T19:08:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akyyy, josephburnett, tcnghia
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vaikas-google

Assign the PR to them by writing /assign @vaikas-google 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:

~~cmd/activator/OWNERS~~ [josephburnett]
pkg/apis/OWNERS
~~pkg/controller/OWNERS~~ [tcnghia]

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

mattmoor · 2018-06-19T21:13:55Z

 	})
 }

+func (rs *RevisionStatus) MarkDeactivating() {


This needs unit test coverage.

google-prow-robot · 2018-06-19T21:20:28Z

New changes are detected. LGTM label has been removed.

mattmoor · 2018-06-19T21:14:48Z


 	for k, v := range revision.ObjectMeta.Labels {
-		labels[k] = v
+		if k != serving.RouteLabelKey {


Why isn't the comment below up here?

mattmoor · 2018-06-19T21:16:28Z

-				Name:      fmt.Sprintf("%s-service", cfgrev.Name),
-				Namespace: testNamespace,
+		Route: []v1alpha2.DestinationWeight{
+			{


collapse as we had on the LHS everywhere please.

mattmoor · 2018-06-19T21:20:39Z

+		// Serving state is RevisionServingStateReserve. Keep the revision service and update
+		// the deployment replicas to be 0.
+		if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil && cond.Reason != "Inactive" {
+			if cond.Reason == "Deactivating" {


We should not be predicating logic on cond.Reason, to me this is an indicator that our model for the lifecycle of inactive revisions is incomplete. This is clearly an extension of the logic we currently have in here, but we need to prioritize fixing this

Yes, we want to do this (#645 (comment)) but before we pivot to the new model, I want what we have at head to not throw 500's. This fixes the last of the 500's that happens just during deactivation.

Yea, that's an issue out of the scope of this pr.

Ack. Does that mean that cleaning this up is the top priority after this goes in?

I can take it after this pr and oncall.

+1. After we fix this 503 issue, the top priority is migrating us to the new model you described and (hopefully) getting rid of serving state.

knative-metrics-robot · 2018-06-19T21:23:13Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.3%	98.3%	0.0
pkg/apis/serving/v1alpha1/revision_types_test.go	98.3%	98.3%	0.0
pkg/controller/route/route.go	78.5%	78.1%	-0.4
pkg/controller/route/route_test.go	78.5%	78.1%	-0.4
pkg/controller/revision/revision.go	79.7%	76.3%	-3.4
pkg/controller/revision/revision_test.go	79.7%	76.3%	-3.4

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-19T21:37:36Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.3%	98.3%	0.0
pkg/apis/serving/v1alpha1/revision_types_test.go	98.3%	98.3%	0.0
pkg/controller/route/route.go	78.5%	77.9%	-0.6
pkg/controller/route/route_test.go	78.5%	77.9%	-0.6
pkg/controller/revision/revision.go	79.7%	76.3%	-3.4
pkg/controller/revision/revision_test.go	79.7%	76.3%	-3.4

*TestCoverage feature is being tested, do not rely on any info here yet

vaikas

Just a couple of suggestions, in addition to what Matt is requesting.

vaikas · 2018-06-19T23:00:30Z

+		logger.Errorf("Failed to get deployment %q", deploymentName)
+		return err
+	}
+


Nit, you could check the deployment here and see if Replicas is already 0 and then shortcircuit without updating?

vaikas · 2018-06-19T23:02:43Z

+	_, err = kubeClient.AppsV1().Deployments(testNamespace).Get(expectedDeploymentName, metav1.GetOptions{})
+
+	if err != nil {
+		t.Fatalf("Expected k8s deployment to be there but it was gone: %s/%s", testNamespace, expectedDeploymentName)


Seems like this could fail for another reason as well and the deployment could be there? Perhaps check isNotFound and then fail with this message?

vaikas · 2018-06-19T23:05:48Z

 			// A revision is considered inactive (yet) if it's in
-			// "Inactive" condition or "Activating" condition.
+			// "Inactive" condition, "Activating" or "Deactivating" condition.
+			logger.Infof("cond: %v", cond)


seems like it's not only the cond, but status that makes this condition happen. I am little confused about this 'double-checking'.

This could be in part of issue #645
#645 (comment)
I'll fix it in that issue.

mattmoor · 2018-06-20T14:43:33Z

+		// end up with multiple replica sets.
+		if k != serving.RouteLabelKey {
+			labels[k] = v
+		}


(thinking about this some more) I'm not thrilled about the Revision controller needing to know things about the Route controller (these are essentially the only references to route in the directory).

This also feels like a band-aid for the one case we know about now, but doesn't solve the more general problem that label mutations could trigger, right?

Yes, this is just a temporary solution. I opened issue #1293 and will add a link of this piece of code to the issue.

mattmoor · 2018-06-20T14:49:41Z

-	err = c.deleteService(ctx, rev)
-	if err != nil {
-		logger.Error("Failed to delete k8s service", zap.Error(err))
+	if rev.Spec.ServingState == v1alpha1.RevisionServingStateRetired {


@josephburnett I thought we just added a comment indicating this state was unused? Should we just delete the code?

I agree it's cleaner to delete Retired state. I added this comment to issue #645

mattmoor · 2018-06-20T14:59:29Z

 	// Then create the actual route rules.
 	logger.Info("Creating Istio route rules")
-	revisionRoutes, err := c.createOrUpdateRouteRules(ctx, route, configMap, revMap)
+	revisionRoutes, inactiveRev, err := c.createOrUpdateRouteRules(ctx, route, configMap, revMap)


It took some digging for me to determine what inactiveRev was, whereas before the method and return value were fairly self-describing. What I don't get is why there is only one inactive revision (name?) returned from this method. The traffic block could have N revisions, all reserve.

For now, activator only forward traffic to one inactive revision with the largest traffic weight. More details are here. #882

mattmoor · 2018-06-20T15:14:53Z

+			return nil, err
+		}
+		rev.Status.MarkInactive()
+		if _, err = revisionClient.Update(rev); err != nil {


I didn't notice this before, but we're updating Revision.Status from the Route controller. That makes me even more uncomfortable that predicating on cond.Reason.

@josephburnett I understand wanting to root out 5XXs within our current model, but cleaning this up needs to be a top priority. We've known about this problem for 2+ months now, I'd really like to see a proper fix.

So I find this aspect of the PR troubling (chatted a bit with @vaikas-google too), and I think we should reconsider our choice of band-aid to fix this.

I'd initially proposed this to remediate this, and it got a bit lost in broader thinking about how we make autoscaling more extensible.

If we are thinking about a nearer term fix, I think we should go with this proposal instead, eliding the aspects around Conditions and Retired for now.

mattmoor · 2018-06-20T15:16:27Z

 		},
-		Route:         []v1alpha2.DestinationWeight{getActivatorDestinationWeight(100)},
+		Route: []v1alpha2.DestinationWeight{
+			{


you can collapse this as well.

mattmoor · 2018-06-20T15:22:53Z

+		// Serving state is RevisionServingStateReserve. Keep the revision service and update
+		// the deployment replicas to be 0.
+		if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil && cond.Reason != "Inactive" {
+			if cond.Reason == "Deactivating" {


Ack. Does that mean that cleaning this up is the top priority after this goes in?

knative-metrics-robot · 2018-06-20T18:01:38Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.3%	98.3%	0.0
pkg/controller/route/route.go	78.5%	77.7%	-0.8
pkg/controller/revision/revision.go	79.7%	75.7%	-4.0

*TestCoverage feature is being tested, do not rely on any info here yet

akyyy · 2018-06-20T18:02:50Z

I assigned #645 to me and will work on it right after finishing my oncall and one half done item.

knative-metrics-robot · 2018-06-20T18:10:16Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.3%	98.3%	0.0
pkg/controller/route/route.go	78.5%	77.9%	-0.6
pkg/controller/revision/revision.go	79.7%	75.7%	-4.0

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-20T20:53:39Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.5%	98.5%	0.0
pkg/controller/route/route.go	78.7%	77.7%	-1.0
pkg/controller/revision/revision.go	81.8%	77.0%	-4.8

*TestCoverage feature is being tested, do not rely on any info here yet

akyyy · 2018-06-21T18:22:36Z

Based on discussion, I'll add a serving state ActiveReserve (this name pending change) which will be used by autoscaler to initiate scale to zero. After the route controller set routerules correspondingly to activator, the route controller will set revision serving state to Reserve. Then the revision controller will tear down k8s resources. This was proposed here.
#645 (comment)

I'll split this pr into two prs.
The first pr is to keep k8s service and deployment.
The second pr is to resolve the race condition by adding a new serving state.

steuhs · 2018-06-23T00:14:36Z

/test pull-knative-serving-go-coverage

google-prow-robot · 2018-06-23T00:15:23Z

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

Test name	Commit	Details	Rerun command
pull-knative-serving-go-coverage	`014f36e`	link	`/test pull-knative-serving-go-coverage`

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.

Signed-off-by: red-hat-konflux-kflux-prd-rh02 <190377777+red-hat-konflux-kflux-prd-rh02[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux-kflux-prd-rh02[bot] <190377777+red-hat-konflux-kflux-prd-rh02[bot]@users.noreply.github.com>

stabilize 1->0 case

c9ca9e2

google-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 18, 2018

akyyy added 3 commits June 18, 2018 13:55

update tests

36bb439

merge changes

adf6823

minor clean up

189d5ce

akyyy assigned josephburnett, mdemirhan, vaikas, grantr, tcnghia and nikkithurmond Jun 18, 2018

delete route label from revisions

440a361

akyyy removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2018

akyyy changed the title ~~[WIP] stabilize 1->0 case~~ Stabilize 1->0 case Jun 18, 2018

josephburnett approved these changes Jun 18, 2018

View reviewed changes

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

merge

ef10a44

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2018

akyyy mentioned this pull request Jun 19, 2018

Autoscaling: 1->0 deadzone #1250

Closed

tcnghia reviewed Jun 19, 2018

View reviewed changes

mdemirhan reviewed Jun 19, 2018

View reviewed changes

address cr

eadba78

tcnghia approved these changes Jun 19, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2018

mdemirhan approved these changes Jun 19, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2018

mattmoor reviewed Jun 19, 2018

View reviewed changes

add unit test for MarkDeactivating

2b7daca

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2018

mattmoor reviewed Jun 19, 2018

View reviewed changes

add comment and format test as cr requested

a345fa3

akyyy mentioned this pull request Jun 19, 2018

Detailed documentation for ServingState semantics #645

Closed

vaikas reviewed Jun 19, 2018

View reviewed changes

mattmoor reviewed Jun 20, 2018

View reviewed changes

address cr comments

81e4a06

Delete a comment

c23bb92

merge

014f36e

akyyy changed the title ~~Stabilize 1->0 case~~ [WIP] Stabilize 1->0 case Jun 21, 2018

google-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 21, 2018

akyyy mentioned this pull request Jun 21, 2018

Keep the deployment when scale to zero. #1320

Closed

mattmoor closed this Jul 12, 2018

		@@ -48,28 +47,28 @@ type activationHandler struct {
		type retryRoundTripper struct{}

		func (rrt retryRoundTripper) RoundTrip(r http.Request) (http.Response, error) {

Conversation

akyyy commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-metrics-robot commented Jun 18, 2018

Uh oh!

akyyy commented Jun 18, 2018

Uh oh!

josephburnett left a comment

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!

knative-metrics-robot commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephburnett Jun 19, 2018 • 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!

knative-metrics-robot commented Jun 19, 2018

Uh oh!

tcnghia left a comment

Choose a reason for hiding this comment

Uh oh!

josephburnett commented Jun 19, 2018

Uh oh!

josephburnett commented Jun 19, 2018

Uh oh!

google-prow-robot commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-prow-robot commented Jun 19, 2018

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!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jun 19, 2018

akyyy commented Jun 18, 2018 •

edited

Loading

josephburnett Jun 19, 2018 •

edited

Loading