From b9d79594a3dce84f51f433062a500698c171ec33 Mon Sep 17 00:00:00 2001 From: Lukasz Dziedziak Date: Wed, 26 Aug 2020 15:36:38 +0200 Subject: [PATCH 1/3] Fix the order of services by name to do not generate CDS when service order change --- .../snapshot/EnvoySnapshotFactory.kt | 1 + .../snapshot/SnapshotUpdaterTest.kt | 57 ++++++++++++++++++- 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt index 8f262a623..295d70c5b 100644 --- a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt +++ b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt @@ -89,6 +89,7 @@ class EnvoySnapshotFactory( } return addRemovedClusters(previousClusters, currentClusters) + .toSortedMap(Comparator.naturalOrder()) } private fun addRemovedClusters( diff --git a/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt b/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt index a5bdb9c79..d6c65ead9 100644 --- a/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt +++ b/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt @@ -26,19 +26,19 @@ import pl.allegro.tech.servicemesh.envoycontrol.groups.ProxySettings import pl.allegro.tech.servicemesh.envoycontrol.groups.ServiceDependency import pl.allegro.tech.servicemesh.envoycontrol.groups.ServicesGroup import pl.allegro.tech.servicemesh.envoycontrol.groups.with -import pl.allegro.tech.servicemesh.envoycontrol.services.Locality import pl.allegro.tech.servicemesh.envoycontrol.services.ClusterState +import pl.allegro.tech.servicemesh.envoycontrol.services.Locality import pl.allegro.tech.servicemesh.envoycontrol.services.MultiClusterState import pl.allegro.tech.servicemesh.envoycontrol.services.MultiClusterState.Companion.toMultiClusterState import pl.allegro.tech.servicemesh.envoycontrol.services.ServiceInstance import pl.allegro.tech.servicemesh.envoycontrol.services.ServiceInstances import pl.allegro.tech.servicemesh.envoycontrol.services.ServicesState import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.clusters.EnvoyClustersFactory -import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.routes.EnvoyEgressRoutesFactory import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.endpoints.EnvoyEndpointsFactory -import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.routes.EnvoyIngressRoutesFactory import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.listeners.EnvoyListenersFactory import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.listeners.filters.EnvoyHttpFilters +import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.routes.EnvoyEgressRoutesFactory +import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.routes.EnvoyIngressRoutesFactory import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.routes.ServiceTagMetadataGenerator import pl.allegro.tech.servicemesh.envoycontrol.utils.DirectScheduler import pl.allegro.tech.servicemesh.envoycontrol.utils.ParallelScheduler @@ -51,6 +51,7 @@ import java.util.concurrent.Executors import java.util.concurrent.TimeUnit import java.util.function.Consumer +@Suppress("LargeClass") class SnapshotUpdaterTest { companion object { @@ -262,6 +263,56 @@ class SnapshotUpdaterTest { results[1].xdsSnapshot!!.hasHttp2Cluster("service") } + @Test + fun `should not change CDS version when service order in remote cluster is different`() { + // given + val cache = MockCache() + val allServicesGroup = AllServicesGroup(communicationMode = ADS) + val groups = listOf(allServicesGroup) + val updater = snapshotUpdater( + cache = cache, + properties = SnapshotProperties().apply { + stateSampleDuration = Duration.ZERO + }, + groups = groups + ) + + val clusterWithOrder = ClusterState( + ServicesState( + serviceNameToInstances = mapOf( + "service" to ServiceInstances("service", setOf()), + "service2" to ServiceInstances("service2", setOf()) + ) + ), + Locality.LOCAL, "cluster" + ).toMultiClusterState() + val clusterWithoutOrder = ClusterState( + ServicesState( + serviceNameToInstances = mapOf( + "service2" to ServiceInstances("service2", setOf()), + "service" to ServiceInstances("service", setOf()) + ) + ), + Locality.REMOTE, "cluster2" + ).toMultiClusterState() + + // when + updater.start( + Flux.just(clusterWithoutOrder, clusterWithOrder) + ).collectList().block() + val previousClusters = cache.getSnapshot(allServicesGroup)!!.clusters() + + // when + updater.start( + Flux.just(clusterWithOrder, clusterWithoutOrder) + ).collectList().block() + val currentClusters = cache.getSnapshot(allServicesGroup)!!.clusters() + + // then + assertThat(previousClusters.version()).isEqualTo(currentClusters.version()) + assertThat(previousClusters.resources()).isEqualTo(currentClusters.resources()) + } + @Test fun `should not remove clusters`() { // given From 368e5e7c577cea24278c1e22d6732c0724a16542 Mon Sep 17 00:00:00 2001 From: Lukasz Dziedziak Date: Mon, 31 Aug 2020 14:44:56 +0200 Subject: [PATCH 2/3] Removed default and added comment --- .../envoycontrol/snapshot/EnvoySnapshotFactory.kt | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt index 295d70c5b..376e59bc8 100644 --- a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt +++ b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt @@ -23,6 +23,7 @@ import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.routes.EnvoyEg import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.endpoints.EnvoyEndpointsFactory import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.routes.EnvoyIngressRoutesFactory import pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.listeners.EnvoyListenersFactory +import java.util.SortedMap class EnvoySnapshotFactory( private val ingressRoutesFactory: EnvoyIngressRoutesFactory, @@ -72,7 +73,7 @@ class EnvoySnapshotFactory( fun clusterConfigurations( servicesStates: MultiClusterState, previousClusters: Map - ): Map { + ): SortedMap { val currentClusters = if (properties.egress.http2.enabled) { servicesStates.flatMap { it.servicesState.serviceNameToInstances.values @@ -88,8 +89,10 @@ class EnvoySnapshotFactory( .associateWith { ClusterConfiguration(serviceName = it, http2Enabled = false) } } + // Clusters need to be sorted because if clusters are in different order to previous snapshot then CDS version + // is changed and that causes unnecessary CDS responses. return addRemovedClusters(previousClusters, currentClusters) - .toSortedMap(Comparator.naturalOrder()) + .toSortedMap() } private fun addRemovedClusters( From c551c897828e13a87c4694f15cd6b525756054dd Mon Sep 17 00:00:00 2001 From: Lukasz Dziedziak Date: Tue, 15 Sep 2020 14:03:44 +0200 Subject: [PATCH 3/3] Fixed ktlint --- .../servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt b/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt index 7b0eb9c1b..4e112802e 100644 --- a/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt +++ b/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt @@ -312,7 +312,7 @@ class SnapshotUpdaterTest { assertThat(previousClusters.version()).isEqualTo(currentClusters.version()) assertThat(previousClusters.resources()).isEqualTo(currentClusters.resources()) } - + @Test fun `should not change EDS when remote doesn't have state of service`() { // given