From 53cf4ec4026e8298905c07eed3ffaf6e195ea9ee Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Sun, 29 Apr 2018 21:24:16 -0700 Subject: [PATCH 1/5] Also provision empty route rules for inactive revisions. This appears to make Istio happy, and if we switch traffic to them later we won't cause Istio to throw spurious 503s. See https://github.com/istio/istio/issues/5204. --- pkg/controller/route/route.go | 62 +++++++++++++--------- pkg/controller/route/route_test.go | 84 ++++++++++++++++++++++++++++++ test/states.go | 16 +++--- 3 files changed, 127 insertions(+), 35 deletions(-) diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index 3b0b52ec9335..c5db3e94eab5 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -352,7 +352,6 @@ func (c *Controller) updateRouteEvent(key string) error { // Call syncTrafficTargetsAndUpdateRouteStatus, which also updates the Route.Status // to contain the domain we will use for Ingress creation. - if _, err = c.syncTrafficTargetsAndUpdateRouteStatus(route); err != nil { return err } @@ -387,14 +386,12 @@ func (c *Controller) syncTrafficTargetsAndUpdateRouteStatus(route *v1alpha1.Rout if err := c.extendRevisionsWithIndirectTrafficTargets(route, configMap, revMap); err != nil { return nil, err } - if err := c.deleteLabelForOutsideOfGivenConfigurations(route, configMap); err != nil { return nil, err } if err := c.setLabelForGivenConfigurations(route, configMap); err != nil { return nil, err } - if err := c.deleteLabelForOutsideOfGivenRevisions(route, revMap); err != nil { return nil, err } @@ -503,7 +500,6 @@ func (c *Controller) getDirectTrafficTargets(route *v1alpha1.Route) ( revMap[revName] = rev } } - return configMap, revMap, nil } @@ -725,43 +721,59 @@ func (c *Controller) computeRevisionRoutes( glog.Infof("Figuring out routes for Route: %s", route.Name) ns := route.Namespace elaNS := controller.GetElaNamespaceName(ns) - ret := []RevisionRoute{} - + revClient := c.elaclientset.ElafrosV1alpha1().Revisions(ns) + revRoutes := []RevisionRoute{} for _, tt := range route.Spec.Traffic { - var rev *v1alpha1.Revision + var targetRev *v1alpha1.Revision var err error + configName := tt.ConfigurationName revName := tt.RevisionName if tt.ConfigurationName != "" { // Get the configuration's LatestReadyRevisionName - revName = configMap[tt.ConfigurationName].Status.LatestReadyRevisionName - if revName == "" { - glog.Errorf("Configuration %s is not ready. Should skip updating route rules", - tt.ConfigurationName) + latestReadyRevName := configMap[tt.ConfigurationName].Status.LatestReadyRevisionName + if latestReadyRevName == "" { + glog.Errorf("Configuration %s is not ready. Should skip updating route rules", configName) return nil, nil } - // Revision has been already fetched indirectly in extendRevisionsWithIndirectTrafficTargets - rev = revMap[revName] - + revs, err := revClient.List(metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s=%s", ela.ConfigurationLabelKey, configName), + }) + if err != nil { + glog.Errorf("Failed to fetch revisions of Configuration %q: %s", configName, err) + return nil, err + } + for _, rev := range revs.Items { + if rev.Name == latestReadyRevName { + // This is the latest active revision, which we will deal with later. + targetRev = rev.DeepCopy() + } 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. + revRoutes = append(revRoutes, RevisionRoute{ + RevisionName: rev.Name, + Service: fmt.Sprintf("%s.%s", rev.Status.ServiceName, elaNS), + Weight: 0, + }) + } + } } else { // Direct revision has already been fetched - rev = revMap[revName] + targetRev = revMap[revName] } - //TODO(grantr): What should happen if revisionName is empty? - - if rev == nil { - // For safety, which should never happen. + if targetRev == nil { + // If we are provided the correct revMap map, this should not happen. glog.Errorf("Failed to fetch Revision %s: %s", revName, err) return nil, err } - rr := RevisionRoute{ + revRoutes = append(revRoutes, RevisionRoute{ Name: tt.Name, - RevisionName: rev.Name, - Service: fmt.Sprintf("%s.%s", rev.Status.ServiceName, elaNS), + RevisionName: targetRev.Name, + Service: fmt.Sprintf("%s.%s", targetRev.Status.ServiceName, elaNS), Weight: tt.Percent, - } - ret = append(ret, rr) + }) } - return ret, nil + return revRoutes, nil } func (c *Controller) createOrUpdateRouteRules(route *v1alpha1.Route, configMap map[string]*v1alpha1.Configuration, diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index c96c71fb0993..44b9cc5a92ce 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -318,6 +318,90 @@ func TestCreateRouteCreatesStuff(t *testing.T) { } } +func TestCreateRouteFromConfigsWithMultipleRevs(t *testing.T) { + _, elaClient, controller, _, elaInformer := newTestController(t) + + // A configuration and associated revision. Normally the revision would be + // created by the configuration controller. + config := getTestConfiguration() + latestReadyRev := getTestRevisionForConfig(config) + otherRev := &v1alpha1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + SelfLink: "/apis/ela/v1alpha1/namespaces/test/revisions/p-livebeef", + Name: "p-livebeef", + Namespace: testNamespace, + Labels: map[string]string{ + ela.ConfigurationLabelKey: config.Name, + }, + }, + Spec: *config.Spec.RevisionTemplate.Spec.DeepCopy(), + Status: v1alpha1.RevisionStatus{ + ServiceName: "p-livebeef-service", + }, + } + config.Status.LatestReadyRevisionName = latestReadyRev.Name + elaClient.ElafrosV1alpha1().Configurations(testNamespace).Create(config) + // Since updateRouteEvent looks in the lister, we need to add it to the informer + elaInformer.Elafros().V1alpha1().Configurations().Informer().GetIndexer().Add(config) + elaClient.ElafrosV1alpha1().Revisions(testNamespace).Create(latestReadyRev) + elaClient.ElafrosV1alpha1().Revisions(testNamespace).Create(otherRev) + + // A route targeting both the config and standalone revision + route := getTestRouteWithTrafficTargets( + []v1alpha1.TrafficTarget{ + v1alpha1.TrafficTarget{ + ConfigurationName: config.Name, + Percent: 100, + }, + }, + ) + elaClient.ElafrosV1alpha1().Routes(testNamespace).Create(route) + // Since updateRouteEvent looks in the lister, we need to add it to the informer + elaInformer.Elafros().V1alpha1().Routes().Informer().GetIndexer().Add(route) + + controller.updateRouteEvent(KeyOrDie(route)) + + routerule, err := elaClient.ConfigV1alpha2().RouteRules(testNamespace).Get(fmt.Sprintf("%s-istio", route.Name), metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting routerule: %v", err) + } + + expectedRouteSpec := v1alpha2.RouteRuleSpec{ + Destination: v1alpha2.IstioService{ + Name: fmt.Sprintf("%s-service", route.Name), + }, + Match: v1alpha2.Match{ + Request: v1alpha2.MatchRequest{ + Headers: v1alpha2.Headers{ + Authority: v1alpha2.MatchString{ + Regex: regexp.QuoteMeta( + strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, "."), + ), + }, + }, + }, + }, + Route: []v1alpha2.DestinationWeight{ + v1alpha2.DestinationWeight{ + Destination: v1alpha2.IstioService{ + Name: fmt.Sprintf("%s-service.test", otherRev.Name), + }, + Weight: 0, + }, + v1alpha2.DestinationWeight{ + Destination: v1alpha2.IstioService{ + Name: fmt.Sprintf("%s-service.test", latestReadyRev.Name), + }, + Weight: 100, + }, + }, + } + + if diff := cmp.Diff(expectedRouteSpec, routerule.Spec); diff != "" { + t.Errorf("Unexpected rule spec diff (-want +got): %v", diff) + } +} + func TestCreateRouteWithMultipleTargets(t *testing.T) { _, elaClient, controller, _, elaInformer := newTestController(t) // A standalone revision diff --git a/test/states.go b/test/states.go index a9cd281896f2..dac766fbbd4b 100644 --- a/test/states.go +++ b/test/states.go @@ -30,16 +30,12 @@ import ( // traffic to and return true if 100% of the traffic is routing to revisionName. func AllRouteTrafficAtRevision(routeName string, revisionName string) func(r *v1alpha1.Route) (bool, error) { return func(r *v1alpha1.Route) (bool, error) { - if len(r.Status.Traffic) > 0 { - if len(r.Status.Traffic) != 1 { - return true, fmt.Errorf("Expected Route to have only one configurated Traffic but had %d. Route: %v", len(r.Status.Traffic), r) - } - if r.Status.Traffic[0].RevisionName == revisionName { - if r.Status.Traffic[0].Percent != 100 { - return true, fmt.Errorf("Expected 100%% of traffic to go to Revision %s but actually is %d", revisionName, r.Status.Traffic[0].Percent) - } - if r.Status.Traffic[0].Name != routeName { - return true, fmt.Errorf("Expected traffic name to be %s but actually is %s", revisionName, r.Status.Traffic[0].Name) + for _, tt := range r.Status.Traffic { + if tt.RevisionName == revisionName { + if r.Status.Traffic[0].Percent == 100 { + if r.Status.Traffic[0].Name != routeName { + return true, fmt.Errorf("Expected traffic name to be %s but actually is %s", revisionName, r.Status.Traffic[0].Name) + } } return true, nil } From 90fda1d9c94815f7c3f1728e5ca9a26cfe6eb2ac Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Tue, 8 May 2018 09:22:54 -0700 Subject: [PATCH 2/5] Update per PR feedback. --- pkg/controller/route/route.go | 86 +++++++++++++++++++++--------- pkg/controller/route/route_test.go | 8 +-- 2 files changed, 64 insertions(+), 30 deletions(-) diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index c5db3e94eab5..ad23c941c19d 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -352,6 +352,7 @@ func (c *Controller) updateRouteEvent(key string) error { // Call syncTrafficTargetsAndUpdateRouteStatus, which also updates the Route.Status // to contain the domain we will use for Ingress creation. + if _, err = c.syncTrafficTargetsAndUpdateRouteStatus(route); err != nil { return err } @@ -386,12 +387,14 @@ func (c *Controller) syncTrafficTargetsAndUpdateRouteStatus(route *v1alpha1.Rout if err := c.extendRevisionsWithIndirectTrafficTargets(route, configMap, revMap); err != nil { return nil, err } + if err := c.deleteLabelForOutsideOfGivenConfigurations(route, configMap); err != nil { return nil, err } if err := c.setLabelForGivenConfigurations(route, configMap); err != nil { return nil, err } + if err := c.deleteLabelForOutsideOfGivenRevisions(route, revMap); err != nil { return nil, err } @@ -500,6 +503,7 @@ func (c *Controller) getDirectTrafficTargets(route *v1alpha1.Route) ( revMap[revName] = rev } } + return configMap, revMap, nil } @@ -721,20 +725,61 @@ func (c *Controller) computeRevisionRoutes( glog.Infof("Figuring out routes for Route: %s", route.Name) ns := route.Namespace elaNS := controller.GetElaNamespaceName(ns) - revClient := c.elaclientset.ElafrosV1alpha1().Revisions(ns) - revRoutes := []RevisionRoute{} + ret := []RevisionRoute{} + for _, tt := range route.Spec.Traffic { - var targetRev *v1alpha1.Revision + var rev *v1alpha1.Revision var err error - configName := tt.ConfigurationName revName := tt.RevisionName if tt.ConfigurationName != "" { // Get the configuration's LatestReadyRevisionName - latestReadyRevName := configMap[tt.ConfigurationName].Status.LatestReadyRevisionName - if latestReadyRevName == "" { - glog.Errorf("Configuration %s is not ready. Should skip updating route rules", configName) + revName = configMap[tt.ConfigurationName].Status.LatestReadyRevisionName + if revName == "" { + glog.Errorf("Configuration %s is not ready. Should skip updating route rules", + tt.ConfigurationName) return nil, nil } + // Revision has been already fetched indirectly in extendRevisionsWithIndirectTrafficTargets + rev = revMap[revName] + + } else { + // Direct revision has already been fetched + rev = revMap[revName] + } + //TODO(grantr): What should happen if revisionName is empty? + + if rev == nil { + // For safety, which should never happen. + glog.Errorf("Failed to fetch Revision %s: %s", revName, err) + return nil, err + } + rr := RevisionRoute{ + Name: tt.Name, + RevisionName: rev.Name, + Service: fmt.Sprintf("%s.%s", rev.Status.ServiceName, elaNS), + Weight: tt.Percent, + } + ret = append(ret, rr) + } + return ret, nil +} + +// computeEmptyRevisionRoutes is a hack to work around https://github.com/istio/istio/issues/5204. +// Here we add empty/dummy route rules for non-target revisions to prepare to switch traffic to +// them in the future. We are tracking this issue in https://github.com/elafros/elafros/issues/348. +// +// TODO: Remove this hack once https://github.com/istio/istio/issues/5204 is fixed. +func (c *Controller) computeEmptyRevisionRoutes( + route *v1alpha1.Route, configMap map[string]*v1alpha1.Configuration, revMap map[string]*v1alpha1.Revision) ([]RevisionRoute, error) { + ns := route.Namespace + elaNS := controller.GetElaNamespaceName(ns) + revClient := c.elaclientset.ElafrosV1alpha1().Revisions(ns) + revRoutes := []RevisionRoute{} + for _, tt := range route.Spec.Traffic { + configName := tt.ConfigurationName + if tt.ConfigurationName != "" { + // Get the configuration's LatestReadyRevisionName + latestReadyRevName := configMap[tt.ConfigurationName].Status.LatestReadyRevisionName revs, err := revClient.List(metav1.ListOptions{ LabelSelector: fmt.Sprintf("%s=%s", ela.ConfigurationLabelKey, configName), }) @@ -743,10 +788,7 @@ func (c *Controller) computeRevisionRoutes( return nil, err } for _, rev := range revs.Items { - if rev.Name == latestReadyRevName { - // This is the latest active revision, which we will deal with later. - targetRev = rev.DeepCopy() - } else if _, ok := revMap[rev.Name]; !ok { + if _, ok := revMap[rev.Name]; !ok && rev.Name != latestReadyRevName { // 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. @@ -757,21 +799,7 @@ func (c *Controller) computeRevisionRoutes( }) } } - } else { - // Direct revision has already been fetched - targetRev = revMap[revName] - } - if targetRev == nil { - // If we are provided the correct revMap map, this should not happen. - glog.Errorf("Failed to fetch Revision %s: %s", revName, err) - return nil, err } - revRoutes = append(revRoutes, RevisionRoute{ - Name: tt.Name, - RevisionName: targetRev.Name, - Service: fmt.Sprintf("%s.%s", targetRev.Status.ServiceName, elaNS), - Weight: tt.Percent, - }) } return revRoutes, nil } @@ -801,7 +829,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. + emptyRoutes, err := c.computeEmptyRevisionRoutes(route, configMap, revMap) + if err != nil { + glog.Errorf("Failed to get empty routes for %s : %q", route.Name, err) + return nil, err + } + revisionRoutes = append(revisionRoutes, emptyRoutes...) // Create route rule for the route domain routeRuleName := controller.GetRouteRuleName(route, nil) routeRules, err := routeClient.Get(routeRuleName, metav1.GetOptions{}) diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index 44b9cc5a92ce..3c1a2400cb0b 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -384,15 +384,15 @@ func TestCreateRouteFromConfigsWithMultipleRevs(t *testing.T) { Route: []v1alpha2.DestinationWeight{ v1alpha2.DestinationWeight{ Destination: v1alpha2.IstioService{ - Name: fmt.Sprintf("%s-service.test", otherRev.Name), + Name: fmt.Sprintf("%s-service.test", latestReadyRev.Name), }, - Weight: 0, + Weight: 100, }, v1alpha2.DestinationWeight{ Destination: v1alpha2.IstioService{ - Name: fmt.Sprintf("%s-service.test", latestReadyRev.Name), + Name: fmt.Sprintf("%s-service.test", otherRev.Name), }, - Weight: 100, + Weight: 0, }, }, } From 366c0249452478b9cf324fc6461f07cbc0c7485e Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Tue, 8 May 2018 09:24:21 -0700 Subject: [PATCH 3/5] Address PR feedback. --- pkg/controller/route/route.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index ad23c941c19d..7f61bfea1f55 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -777,7 +777,7 @@ func (c *Controller) computeEmptyRevisionRoutes( revRoutes := []RevisionRoute{} for _, tt := range route.Spec.Traffic { configName := tt.ConfigurationName - if tt.ConfigurationName != "" { + if configName != "" { // Get the configuration's LatestReadyRevisionName latestReadyRevName := configMap[tt.ConfigurationName].Status.LatestReadyRevisionName revs, err := revClient.List(metav1.ListOptions{ From 557cf8ccc3bae6afdb2d208135ae7593acdb29e4 Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Tue, 8 May 2018 09:29:37 -0700 Subject: [PATCH 4/5] Update comment per PR feedback. --- pkg/controller/route/route.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index 7f61bfea1f55..ab964d704bf2 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -768,7 +768,9 @@ func (c *Controller) computeRevisionRoutes( // Here we add empty/dummy route rules for non-target revisions to prepare to switch traffic to // them in the future. We are tracking this issue in https://github.com/elafros/elafros/issues/348. // -// TODO: Remove this hack once https://github.com/istio/istio/issues/5204 is fixed. +// TODO: Even though this fixes the 503s when switching revisions, revisions will have empty route +// rules to them for perpetuity, therefore not ideal. We should remove this hack once +// https://github.com/istio/istio/issues/5204 is fixed, probably in 0.8.1. func (c *Controller) computeEmptyRevisionRoutes( route *v1alpha1.Route, configMap map[string]*v1alpha1.Configuration, revMap map[string]*v1alpha1.Revision) ([]RevisionRoute, error) { ns := route.Namespace From ecfde284ead30ef40e67d15238c5b9f171fe414d Mon Sep 17 00:00:00 2001 From: Nghia Tran Date: Tue, 8 May 2018 10:58:29 -0700 Subject: [PATCH 5/5] Address PR feedback. --- pkg/controller/route/route.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index ab964d704bf2..c74dda325d0e 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -781,7 +781,7 @@ func (c *Controller) computeEmptyRevisionRoutes( configName := tt.ConfigurationName if configName != "" { // Get the configuration's LatestReadyRevisionName - latestReadyRevName := configMap[tt.ConfigurationName].Status.LatestReadyRevisionName + latestReadyRevName := configMap[configName].Status.LatestReadyRevisionName revs, err := revClient.List(metav1.ListOptions{ LabelSelector: fmt.Sprintf("%s=%s", ela.ConfigurationLabelKey, configName), }) @@ -831,7 +831,7 @@ 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. + // TODO: remove this once https://github.com/istio/istio/issues/5204 is fixed. emptyRoutes, err := c.computeEmptyRevisionRoutes(route, configMap, revMap) if err != nil { glog.Errorf("Failed to get empty routes for %s : %q", route.Name, err)