fix(kuma-cp) TrafficRoute in multizone issue #1979

lobkovilya · 2021-05-12T13:37:21Z

Summary

In some specific cases, TrafficRoutes in multizone didn't work correctly. It happened because envoy.Cluster doesn't support hosts with the same ip/port but with different metadata for subsets.

Current PR changes the way we implemented TrafficRoutes, instead of ClusterSubsets we will use a separate cluster for each TrafficRoute.Split.

This change affected metric because previously Service name and Cluster name we the same, but now Cluster has autogenerated name service_name-_num_ where num is an index of Split. In order to support existing grafana dashboards Kuma Data Plane hijacks metrics and merge metrics for clusters that belong to the same service.

Full changelog

update envoy Configurers
merge metrics in kuma-dp
rename Subset to Cluster and Cluster to Service + small refactoring
add e2e tests

Issues resolved

Fix #XXX

Documentation

Link to the website documentation PR

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> # Conflicts: # pkg/xds/cache/cla/cache.go

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

test/e2e/trafficroute/kuma_universal.go

jakubdyszkiewicz · 2021-05-17T08:42:35Z

pkg/xds/cache/cla/cache.go

-func (c *Cache) GetCLA(ctx context.Context, meshName, meshHash, service string, apiVersion envoy_common.APIVersion) (proto.Message, error) {
-	key := fmt.Sprintf("%s:%s:%s:%s", apiVersion, meshName, service, meshHash)
+func (c *Cache) GetCLA(ctx context.Context, meshName, meshHash string, cluster envoy_common.Cluster, apiVersion envoy_common.APIVersion) (proto.Message, error) {
+	key := fmt.Sprintf("%s:%s:%s:%s", apiVersion, meshName, cluster.Name(), meshHash)


how would this work with following case

apiVersion: kuma.io/v1alpha1 kind: TrafficRoute mesh: default metadata: name: route-all-default spec: sources: - match: kuma.io/service: 'web' destinations: - match: kuma.io/service: 'backend' conf: split: - weight: 50 destination: kuma.io/service: backend version: 1 - weight: 50 destination: kuma.io/service: backend version: 2 --- apiVersion: kuma.io/v1alpha1 kind: TrafficRoute mesh: default metadata: name: route-all-default spec: sources: - match: kuma.io/service: 'other' destinations: - match: kuma.io/service: 'backend' conf: split: - weight: 50 destination: kuma.io/service: backend version: 3 - weight: 50 destination: kuma.io/service: backend version: 4

if the cache is maintained for the whole mesh, wouldn't we generate _0_, _1_ and have a mismatch?

yes, you're right, I added the method Hash() to use in MeshHash (instead of cluster.Name())

pkg/xds/cache/cla/cache.go

test/e2e/trafficroute/kuma_universal.go

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> # Conflicts: # pkg/xds/envoy/listeners/v2/http_outbound_route_configurer_test.go # pkg/xds/envoy/listeners/v3/http_outbound_route_configurer_test.go

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya · 2021-05-24T08:27:43Z

@Mergifyio update

mergify · 2021-05-24T08:28:18Z

Command update: success

Branch has been successfully updated

CLAassistant · 2021-05-24T08:28:27Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ lobkovilya
❌ mergify[bot]
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

jakubdyszkiewicz · 2021-05-24T13:17:58Z

test/e2e/trafficroute/universal_multizone/traffic_route.go

+		}
+		return errors.Errorf("resulted split values %v and %v aren't within original values %v and %v",
+			echo1Resp/float64(maxAttempts)*100, echo2Resp/float64(maxAttempts)*100, v1Weight, v2Weight)
+	}


What is the advantage of this algo vs simple send 100 requests, see how they are distributed?

jakubdyszkiewicz · 2021-05-24T13:18:33Z

test/e2e/trafficroute/universal_multizone/traffic_route.go

+			}
+
+			for _, ne := range notExpected {
+				Expect(stdout).ToNot(ContainSubstring(ne))


is it safe to do expect from eventually? I was under impression this is not a good idea. Will this allow Eventually to do next try or will this fail the test?

jakubdyszkiewicz · 2021-05-24T13:20:38Z

test/e2e/trafficroute/universal_multizone/traffic_route.go

+        kuma.io/service: echo-server_kuma-test_svc_8080
+        version: v4
+`
+		Expect(NewClusterSetup().Install(YamlUniversal(trafficRoute)).Setup(global)).To(Succeed())


nit: YamlUniversal(trafficRoute)(global) is shorter

jakubdyszkiewicz · 2021-05-24T13:32:24Z

test/e2e/trafficroute/universal_multizone/traffic_route.go

+		err := waitRatio("echo-server_kuma-test_svc_8080.mesh", v1Weight, v2Weight, "echo-v1", "echo-v2")
+		Expect(err).ToNot(HaveOccurred())
+	})
+}


I feel like waitRatio and collectResponses are a bit cryptic. Names do not suggest that those are assertion and I think both of them contains a bit complicated logic.

Here's what I would do:

Write a method collectResponses := func(destination string) map[string]int (instance to responses) that only sends 10 (can be 100 if needed) requests and collects responses, don't do any assertions

In should access all instances of the service I would write - eventually (collectResponses and then do assertion in the test that there is 1,2,4 and not 3)

In should route 100 percent of the traffic to the different service I would write - eventually (collectResponses and then do assertion that responses are from one instance)

In should route split traffic between the versions with 20/80 ratio I would write - eventually (collectResponses and count if the traffic is propagated properly)

I think it will be also more flexible to add new test cases. Also I think collectResponses could be reused between both tests.

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

jakubdyszkiewicz · 2021-05-25T14:22:50Z

That's some next-level sorcery with those chained assertions 👀

lobkovilya added 6 commits May 12, 2021 18:39

fix(kuma-cp) new cluster per split, merge metrics

14d2a75

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

fix(kuma-cp) make check

0c494ec

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Merge branch 'master' into fix/tr-in-multizone

6cb710d

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> # Conflicts: # pkg/xds/cache/cla/cache.go

fix(kuma-cp) make check

7e2deed

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

fix(kuma-cp) create method 'GetSplitOrdered'

c4239a3

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

fix(kuma-cp) make check

6db437b

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya requested a review from a team as a code owner May 12, 2021 13:37

fix(kuma-cp) fix test

7da4227

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

tharun208 reviewed May 13, 2021

View reviewed changes

test/e2e/trafficroute/kuma_universal.go Outdated Show resolved Hide resolved

jakubdyszkiewicz reviewed May 17, 2021

View reviewed changes

lobkovilya added 3 commits May 18, 2021 18:12

Merge branch 'master' into fix/tr-in-multizone

51d040a

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> # Conflicts: # pkg/xds/envoy/listeners/v2/http_outbound_route_configurer_test.go # pkg/xds/envoy/listeners/v3/http_outbound_route_configurer_test.go

fix(kuma-cp) review

a8411bb

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

fix(kuma-cp) make check

03f0dcc

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Merge branch 'master' into fix/tr-in-multizone

cfb233c

fix(test) create method Endpoint#ContainsTags

ac6c7f2

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

jakubdyszkiewicz reviewed May 24, 2021

View reviewed changes

lobkovilya added 2 commits May 25, 2021 18:39

fix(kuma-cp) e2e tests review

c2fb655

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

fix(kuma-cp) make check

0af67fc

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

jakubdyszkiewicz approved these changes May 25, 2021

View reviewed changes

lobkovilya merged commit f0211ec into master May 25, 2021

lobkovilya deleted the fix/tr-in-multizone branch May 25, 2021 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(kuma-cp) TrafficRoute in multizone issue #1979

fix(kuma-cp) TrafficRoute in multizone issue #1979

lobkovilya commented May 12, 2021

jakubdyszkiewicz May 17, 2021

lobkovilya May 24, 2021

lobkovilya commented May 24, 2021

mergify bot commented May 24, 2021

CLAassistant commented May 24, 2021

jakubdyszkiewicz May 24, 2021

jakubdyszkiewicz May 24, 2021

jakubdyszkiewicz May 24, 2021

jakubdyszkiewicz May 24, 2021

jakubdyszkiewicz commented May 25, 2021

fix(kuma-cp) TrafficRoute in multizone issue #1979

fix(kuma-cp) TrafficRoute in multizone issue #1979

Conversation

lobkovilya commented May 12, 2021

Summary

Full changelog

Issues resolved

Documentation

jakubdyszkiewicz May 17, 2021

Choose a reason for hiding this comment

lobkovilya May 24, 2021

Choose a reason for hiding this comment

lobkovilya commented May 24, 2021

mergify bot commented May 24, 2021

CLAassistant commented May 24, 2021

jakubdyszkiewicz May 24, 2021

Choose a reason for hiding this comment

jakubdyszkiewicz May 24, 2021

Choose a reason for hiding this comment

jakubdyszkiewicz May 24, 2021

Choose a reason for hiding this comment

jakubdyszkiewicz May 24, 2021

Choose a reason for hiding this comment

jakubdyszkiewicz commented May 25, 2021