Implemented possibility of traffic splitting configuration #397

nastassia-dailidava · 2023-08-25T06:49:00Z

No description provided.

…ck using aggregate cluster #292

...-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/utils/ClusterNames.kt

...core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotProperties.kt

...l/allegro/tech/servicemesh/envoycontrol/snapshot/resource/endpoints/EnvoyEndpointsFactory.kt

...l/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactory.kt

.../pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/clusters/EnvoyClustersFactory.kt

...re/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt

…ck using aggregate cluster| Added tests and fixed review issues #292

…ck using aggregate cluster| fixed tests #292

Ferdudas97 · 2023-08-29T08:43:08Z

.../pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/clusters/EnvoyClustersFactory.kt

@@ -393,7 +493,7 @@ class EnvoyClustersFactory(
                                        )
                                )
                    }
-                ).setServiceName(clusterConfiguration.serviceName)


Why is it deleted?

#397 (comment)

Ferdudas97 · 2023-08-29T08:46:54Z

.../pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/clusters/EnvoyClustersFactory.kt

+        clusterLoadAssignment: ClusterLoadAssignment?
+    ): Boolean {
+        val trafficSplitting = properties.loadBalancing.trafficSplitting
+        val trafficSplitEnabled = trafficSplitting.serviceByWeightsProperties.containsKey(serviceName) &&


Why do we have to check if dependencies contains this cluster? IMO it is unnecessary, because we create clusters for each dependency

Ferdudas97 · 2023-08-29T08:51:39Z

...l/allegro/tech/servicemesh/envoycontrol/snapshot/resource/endpoints/EnvoyEndpointsFactory.kt

+            .onEach { logger.debug("Traffic splitting is enabled for cluster: ${it.clusterName}") }
+            .mapNotNull { routeSpec ->
+                clusterLoadAssignments[routeSpec.clusterName]?.let { assignment ->
+                    ClusterLoadAssignment.newBuilder(assignment)


Maybe instead of clearing all endpoints and adding the proper ones after, it can be filtered previously and added only the valid endpoints

In ClusterLoadAssignment.Builder doesn't allow to set list of endpoints. Because of that, we have to clear them, before we add new ones.

Ferdudas97 · 2023-08-29T08:54:29Z

...l/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactory.kt

@@ -337,6 +343,37 @@ class EnvoyEgressRoutesFactory(

        return routeAction
    }
+
+    private fun RouteAction.Builder.setCluster(routeSpec: RouteSpecification): RouteAction.Builder {
+        val hasWeightsConfig = routeSpec.clusterWeights.keys.containsAll(listOf("main", "secondary"))


This main and secondary could be a some static constant or clusterWeights should have properties like a main and secondary instead of being a map

Ferdudas97 · 2023-08-29T08:57:29Z

...l/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactory.kt

+            )
+            this.setWeightedClusters(
+                WeightedCluster.newBuilder()
+                    .withClusterWeight(routeSpec.clusterName, routeSpec.clusterWeights["main"]!!)


If we add only main and secondar, we should not use containsAll during creation of hasWeightsConfig - there should be used an equal operator

Ferdudas97 · 2023-08-29T08:59:42Z

...re/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt

                    )
                }
                allServicesRoutes + definedServicesRoutes
            }
        }
    }

+    private fun getTrafficSplittingWeights(


in this PR, i saw two or three simillar functions

Ferdudas97 · 2023-08-29T09:02:40Z

...core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotProperties.kt

@@ -154,6 +155,11 @@ class CanaryProperties {
    var headerValue = "1"
 }

+class TrafficSplittingProperties {
+    var zoneName = ""


What is zoneName? Is it GCP, or gcp/west and gcp/central?. If the second option then LoadBalancingProperties should have a list or map of trafficsplitting properties

It is gcp-central. For now there is no requirement to support more then one zone, and supporting multiple will make code unnecessary complex. So I think it is good, to leave it as it is

Ferdudas97 · 2023-08-29T09:04:51Z

...re/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt

@@ -375,5 +399,6 @@ data class ClusterConfiguration(
 class RouteSpecification(


Maybe instead of adding a clusterWeight Property, we should create WeightedRouteSpecification? It will simplify some code and we coud get rid of some checks if this map contains keys like main and secondary.

Added logs Ignored failing test #292

# Conflicts: # CHANGELOG.md

Ferdudas97 · 2023-09-25T12:36:13Z

...l/allegro/tech/servicemesh/envoycontrol/snapshot/resource/endpoints/EnvoyEndpointsFactory.kt

@@ -91,7 +90,10 @@ class EnvoyEndpointsFactory(
                        .addAllEndpoints(assignment.endpointsList?.filter { e ->
                            e.locality.zone == properties.loadBalancing.trafficSplitting.zoneName
                        })
-                        .setClusterName(getSecondaryClusterName(routeSpec.clusterName))
+                        .setClusterName(
+                            "${routeSpec.clusterName}-" + properties.loadBalancing


it would be better to hide this under the function(like it was before), it is used few times - getSecondaryClusterName this can have second param - config

nastassia-dailidava added 7 commits August 24, 2023 19:42

Implemented possibility for configuring traffic splitting, and fallba…

cdf8d9c

…ck using aggregate cluster #292

Implemented possibility for configuring traffic splitting, and fallba…

2865fb2

…ck using aggregate cluster #292

Implemented possibility for configuring traffic splitting, and fallba…

b65015c

…ck using aggregate cluster #292

Implemented possibility for configuring traffic splitting, and fallba…

1f01680

…ck using aggregate cluster #292

Implemented possibility for configuring traffic splitting, and fallba…

1e06196

…ck using aggregate cluster #292

Implemented possibility for configuring traffic splitting, and fallba…

19714b3

…ck using aggregate cluster #292

Implemented possibility for configuring traffic splitting, and fallba…

fd999eb

…ck using aggregate cluster #292

KSmigielski reviewed Aug 25, 2023

View reviewed changes

nastassia-dailidava added 3 commits August 25, 2023 14:26

Implemented possibility for configuring traffic splitting, and fallba…

ec4bb94

…ck using aggregate cluster| Added tests and fixed review issues #292

Implemented possibility for configuring traffic splitting, and fallba…

a579901

…ck using aggregate cluster| Added tests and fixed review issues #292

Implemented possibility for configuring traffic splitting, and fallba…

919cff0

…ck using aggregate cluster| Added tests and fixed review issues #292

KSmigielski previously approved these changes Aug 25, 2023

View reviewed changes

Implemented possibility for configuring traffic splitting, and fallba…

a3aed17

…ck using aggregate cluster| fixed tests #292

nastassia-dailidava dismissed KSmigielski’s stale review via a3aed17 August 26, 2023 07:17

Ferdudas97 requested changes Aug 29, 2023

View reviewed changes

KSmigielski added 5 commits August 29, 2023 12:27

review fixes

3449ff3

review fixes

9f39576

fix lint

a5ff490

fix tests

46935f6

fix lint

25ce7aa

Ferdudas97 previously approved these changes Sep 4, 2023

View reviewed changes

fix test properties

88c7676

KSmigielski dismissed Ferdudas97’s stale review via 88c7676 September 5, 2023 10:09

fix lint

332e94d

Ferdudas97 previously approved these changes Sep 7, 2023

View reviewed changes

nastassia-dailidava dismissed Ferdudas97’s stale review via 1870014 September 11, 2023 11:44

nastassia-dailidava temporarily deployed to ci September 11, 2023 13:55 — with GitHub Actions Inactive

nastassia-dailidava temporarily deployed to ci September 11, 2023 15:13 — with GitHub Actions Inactive

nastassia-dailidava temporarily deployed to ci September 11, 2023 15:47 — with GitHub Actions Inactive

nastassia-dailidava had a problem deploying to ci September 11, 2023 15:54 — with GitHub Actions Error

nastassia-dailidava temporarily deployed to ci September 11, 2023 15:55 — with GitHub Actions Inactive

nastassia-dailidava temporarily deployed to ci September 11, 2023 16:05 — with GitHub Actions Inactive

nastassia-dailidava temporarily deployed to ci September 11, 2023 16:53 — with GitHub Actions Inactive

nastassia-dailidava force-pushed the flex-roadmap-traffic-splitting branch from 92f3c6e to 637b507 Compare September 12, 2023 15:38

nastassia-dailidava temporarily deployed to ci September 12, 2023 16:02 — with GitHub Actions Inactive

nastassia-dailidava temporarily deployed to ci September 14, 2023 16:18 — with GitHub Actions Inactive

nastassia-dailidava temporarily deployed to ci September 14, 2023 17:24 — with GitHub Actions Inactive

nastassia-dailidava temporarily deployed to ci September 15, 2023 10:02 — with GitHub Actions Inactive

added postfix, added service name

832138e

Added logs Ignored failing test #292

nastassia-dailidava force-pushed the flex-roadmap-traffic-splitting branch from 141574d to 832138e Compare September 25, 2023 10:51

nastassia-dailidava added 2 commits September 25, 2023 13:06

Merge branch 'master' into flex-roadmap-traffic-splitting

a721bd1

# Conflicts: # CHANGELOG.md

added service name to eds config #292

83ebfc5

nastassia-dailidava force-pushed the flex-roadmap-traffic-splitting branch from 74f7afb to 83ebfc5 Compare September 25, 2023 11:58

Ferdudas97 requested changes Sep 25, 2023

View reviewed changes

review fixes #292

f7ccbfc

Ferdudas97 approved these changes Sep 26, 2023

View reviewed changes

KSmigielski approved these changes Sep 28, 2023

View reviewed changes

nastassia-dailidava merged commit 2b63f78 into master Oct 4, 2023
5 checks passed

nastassia-dailidava deleted the flex-roadmap-traffic-splitting branch October 4, 2023 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented possibility of traffic splitting configuration #397

Implemented possibility of traffic splitting configuration #397

nastassia-dailidava commented Aug 25, 2023

Ferdudas97 Aug 29, 2023

KSmigielski Aug 29, 2023

Ferdudas97 Aug 29, 2023

Ferdudas97 Aug 29, 2023

KSmigielski Aug 29, 2023

Ferdudas97 Aug 29, 2023

Ferdudas97 Aug 29, 2023

Ferdudas97 Aug 29, 2023

Ferdudas97 Aug 29, 2023

KSmigielski Aug 29, 2023

Ferdudas97 Aug 29, 2023

Ferdudas97 Sep 25, 2023

nastassia-dailidava Sep 25, 2023

		@@ -375,5 +399,6 @@ data class ClusterConfiguration(
		class RouteSpecification(

Implemented possibility of traffic splitting configuration #397

Implemented possibility of traffic splitting configuration #397

Conversation

nastassia-dailidava commented Aug 25, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment