Also provision empty route rules for revisions that are not yet Active by tcnghia · Pull Request #785 · knative/serving

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

@@ -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

Details

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

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


configName

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

-			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

+				} 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

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


if configName == "" { continue }

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

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


configMap[configName]

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

 		return nil, err
 	}
-
+	// TODO: remove this once https://github.com/istio/istio/issues/5204 is fixe.


typo: fixed

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

+		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

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

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

Conversation

tcnghia commented Apr 30, 2018

Proposed Changes

Uh oh!

tcnghia commented Apr 30, 2018

Uh oh!

tcnghia commented Apr 30, 2018

Uh oh!

tcnghia commented Apr 30, 2018

Uh oh!

tcnghia commented Apr 30, 2018

Uh oh!

tcnghia commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdemirhan commented May 1, 2018

Uh oh!

adrcunha commented May 1, 2018

Uh oh!

google-prow-robot commented May 1, 2018

Uh oh!

tcnghia commented May 2, 2018

Uh oh!

adrcunha commented May 2, 2018

Uh oh!

evankanderson commented May 4, 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!

mattmoor 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!

tcnghia commented May 8, 2018

Uh oh!

mattmoor commented May 8, 2018

Uh oh!

tcnghia commented May 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants