Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Order services by name to do not generate CDS when service state in remote has chanfed #159

Merged
merged 7 commits into from
Sep 15, 2020
Merged

Order services by name to do not generate CDS when service state in remote has chanfed #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b9d7959
Fix the order of services by name to do not generate CDS when service…
Aug 26, 2020
ad72210
Merge branch 'master' into fix_service_order
lukidzi Aug 31, 2020
368e5e7
Removed default and added comment
Aug 31, 2020
4909a34
Merge branch 'fix_service_order' of https://github.com/allegro/envoy-…
Aug 31, 2020
6b20225
Merge branch 'master' into fix_service_order
slonka Sep 10, 2020
b9c5768
Merge branch 'master' into fix_service_order
lukidzi Sep 15, 2020
c551c89
Fixed ktlint
Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/EnvoySnapshotFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -72,7 +73,7 @@ class EnvoySnapshotFactory(
fun clusterConfigurations(
servicesStates: MultiClusterState,
previousClusters: Map<String, ClusterConfiguration>
): Map<String, ClusterConfiguration> {
): SortedMap<String, ClusterConfiguration> {
val currentClusters = if (properties.egress.http2.enabled) {
servicesStates.flatMap {
it.servicesState.serviceNameToInstances.values
Expand All @@ -88,7 +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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like something that should belong to the contract in java-control-plane's Snapshot, right?

// is changed and that causes unnecessary CDS responses.
return addRemovedClusters(previousClusters, currentClusters)
.toSortedMap()
}

private fun addRemovedClusters(
Expand Down
56 changes: 53 additions & 3 deletions .../src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotUpdaterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -263,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 change EDS when remote doesn't have state of service`() {
// given
Expand Down