Also provision empty route rules for revisions that are not yet Active #785

tcnghia · 2018-04-30T19:24:28Z

This appears to make Istio happy, that if we switch traffic to them
later we won't cause Istio to throw spurious 503s. See
istio/istio#5204.

Fixes #348

Proposed Changes

Also provision empty route rules for revisions that are not yet Active.

tcnghia · 2018-04-30T19:36:39Z

/retest

tcnghia · 2018-04-30T20:13:25Z

/retest

tcnghia · 2018-04-30T21:15:43Z

/retest

tcnghia · 2018-04-30T22:27:17Z

/retest

tcnghia · 2018-04-30T23:17:28Z

/retest

bobcatfish · 2018-05-01T15:45:46Z

test/conformance/route_test.go

@@ -95,12 +96,12 @@ func isRouteReady() func(r *v1alpha1.Route) (bool, error) {

 func allRouteTrafficAtRevision(routeName string, revisionName string) func(r *v1alpha1.Route) (bool, error) {


sorry @tcnghia I refactored this, the file you'll need to move this change over to is https://github.com/elafros/elafros/blob/master/test/states.go

thanks for pointing that out. updated.

mdemirhan · 2018-05-01T18:00:27Z

/lgtm
/approve

adrcunha · 2018-05-01T19:38:16Z

/lgtm
/approve

google-prow-robot · 2018-05-01T19:38:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrcunha, mdemirhan, tcnghia

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/controller/OWNERS~~ [tcnghia]
~~test/OWNERS~~ [adrcunha]

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

tcnghia · 2018-05-02T15:22:56Z

/retest

adrcunha · 2018-05-02T17:19:36Z

/lgtm

evankanderson · 2018-05-04T16:34:24Z

/hold

Waiting on CODEOWNERS review; the hold is to keep this from blocking the Tide merge queue.

mattmoor · 2018-05-07T15:28:28Z

pkg/controller/route/route.go

 		var err error
+		configName := tt.ConfigurationName
 		revName := tt.RevisionName
 		if tt.ConfigurationName != "" {


configName

mattmoor · 2018-05-07T15:28:43Z

pkg/controller/route/route.go

-			if revName == "" {
-				glog.Errorf("Configuration %s is not ready. Should skip updating route rules",
-					tt.ConfigurationName)
+			latestReadyRevName := configMap[tt.ConfigurationName].Status.LatestReadyRevisionName


[configName]

mattmoor · 2018-05-07T15:37:56Z

pkg/controller/route/route.go

+				} else if _, ok := revMap[rev.Name]; !ok {
+					// This is a non-target revision.  Adding a dummy rule to make Istio happy,
+					// so that if we switch traffic to them later we won't cause Istio to
+					// throw spurious 503s. See https://github.com/istio/istio/issues/5204.


I find this loop pretty hard to follow.

Since I believe this is a temporary workaround, I think it would be better to have:

Control loop that materializes the semantics we want (route rule per active revision)

Control loop that materializes route rules to work around the istio issue

Above 2. we should have a clear comment outlining the Istio issue (and a link to an issue tracking it's resolution), we should also have an Ela tracking issue, which shouldn't close until the second control loop is ripped out.

Also, I believe this means that the number of routes will monotonically increase and that revisions will remain routable in perpetuity, which certainly shouldn't be the end state. We should be clear about the trade-offs here while we wait on a proper fix from Istio.

This appears to make Istio happy, and if we switch traffic to them later we won't cause Istio to throw spurious 503s. See istio/istio#5204.

mattmoor

couple nits, thanks!

mattmoor · 2018-05-08T17:38:07Z

pkg/controller/route/route.go

+	revRoutes := []RevisionRoute{}
+	for _, tt := range route.Spec.Traffic {
+		configName := tt.ConfigurationName
+		if configName != "" {


if configName == "" { continue }

mattmoor · 2018-05-08T17:38:18Z

pkg/controller/route/route.go

+		configName := tt.ConfigurationName
+		if configName != "" {
+			// Get the configuration's LatestReadyRevisionName
+			latestReadyRevName := configMap[tt.ConfigurationName].Status.LatestReadyRevisionName


configMap[configName]

mattmoor · 2018-05-08T17:38:55Z

pkg/controller/route/route.go

@@ -789,7 +831,13 @@ func (c *Controller) createOrUpdateRouteRules(route *v1alpha1.Route, configMap m
 		glog.Errorf("Failed to check if should direct traffic to activator: %s", err)
 		return nil, err
 	}
-
+	// TODO: remove this once https://github.com/istio/istio/issues/5204 is fixe.


typo: fixed

mattmoor · 2018-05-08T17:39:32Z

pkg/controller/route/route.go

+		glog.Errorf("Failed to get empty routes for %s : %q", route.Name, err)
+		return nil, err
+	}
+	revisionRoutes = append(revisionRoutes, emptyRoutes...)


Thanks, this will be significantly easier to excise later when Istio fixes things!

tcnghia · 2018-05-08T18:16:02Z

/retest

mattmoor · 2018-05-08T19:33:57Z

/lgtm
/hold remove

tcnghia · 2018-05-08T20:03:49Z

/retest

tcnghia requested a review from mdemirhan April 30, 2018 19:24

tcnghia requested review from grantr and mattmoor as code owners April 30, 2018 19:24

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 30, 2018

tcnghia force-pushed the istio-route-0 branch from d20f25d to 96b6760 Compare April 30, 2018 19:34

tcnghia force-pushed the istio-route-0 branch from 96b6760 to 2f8f787 Compare May 1, 2018 00:29

tcnghia requested review from adrcunha and bobcatfish as code owners May 1, 2018 00:29

google-prow-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2018

bobcatfish reviewed May 1, 2018

View reviewed changes

tcnghia force-pushed the istio-route-0 branch from 2f8f787 to 92b4f75 Compare May 1, 2018 16:48

tcnghia requested review from srinivashegde86 and steuhs as code owners May 1, 2018 16:48

tcnghia force-pushed the istio-route-0 branch from 92b4f75 to 4893b7d Compare May 1, 2018 17:01

mdemirhan approved these changes May 1, 2018

View reviewed changes

google-prow-robot assigned mdemirhan May 1, 2018

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

adrcunha approved these changes May 1, 2018

View reviewed changes

google-prow-robot assigned adrcunha May 1, 2018

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2018

tcnghia force-pushed the istio-route-0 branch from 4893b7d to 5093826 Compare May 2, 2018 16:10

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

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

evankanderson assigned grantr and mattmoor and unassigned mdemirhan and adrcunha May 2, 2018

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

mattmoor suggested changes May 7, 2018

View reviewed changes

Nghia Tran added 2 commits May 8, 2018 08:19

Also provision empty route rules for inactive revisions.

53cf4ec

This appears to make Istio happy, and if we switch traffic to them later we won't cause Istio to throw spurious 503s. See istio/istio#5204.

Update per PR feedback.

90fda1d

tcnghia force-pushed the istio-route-0 branch from 5093826 to 90fda1d Compare May 8, 2018 16:23

tcnghia requested a review from a team May 8, 2018 16:23

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

Nghia Tran added 2 commits May 8, 2018 09:24

Address PR feedback.

366c024

Update comment per PR feedback.

557cf8c

mattmoor approved these changes May 8, 2018

View reviewed changes

Address PR feedback.

ecfde28

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

tcnghia removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 8, 2018

google-prow-robot merged commit 4b5e183 into knative:master May 8, 2018

bsnchan mentioned this pull request Jun 20, 2018

Removes 503 as a retryable status code in tests #1298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Also provision empty route rules for revisions that are not yet Active #785

Also provision empty route rules for revisions that are not yet Active #785

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

bobcatfish May 1, 2018

tcnghia May 1, 2018

mdemirhan commented May 1, 2018

adrcunha commented May 1, 2018

google-prow-robot commented May 1, 2018

tcnghia commented May 2, 2018

adrcunha commented May 2, 2018

evankanderson commented May 4, 2018

mattmoor May 7, 2018

mattmoor May 7, 2018

mattmoor May 7, 2018

mattmoor May 7, 2018

mattmoor left a comment

mattmoor May 8, 2018

mattmoor May 8, 2018

mattmoor May 8, 2018

mattmoor May 8, 2018

tcnghia commented May 8, 2018

mattmoor commented May 8, 2018

tcnghia commented May 8, 2018

		@@ -95,12 +96,12 @@ func isRouteReady() func(r *v1alpha1.Route) (bool, error) {

		func allRouteTrafficAtRevision(routeName string, revisionName string) func(r *v1alpha1.Route) (bool, error) {

Also provision empty route rules for revisions that are not yet Active #785

Also provision empty route rules for revisions that are not yet Active #785

Conversation

tcnghia commented Apr 30, 2018

Proposed Changes

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

tcnghia commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdemirhan commented May 1, 2018

adrcunha commented May 1, 2018

google-prow-robot commented May 1, 2018

tcnghia commented May 2, 2018

adrcunha commented May 2, 2018

evankanderson commented May 4, 2018

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

mattmoor 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

tcnghia commented May 8, 2018

mattmoor commented May 8, 2018

tcnghia commented May 8, 2018