-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
Delta xds #255
Delta xds #255
Conversation
…pe checking fails
import io.envoyproxy.controlplane.cache.StatusInfo; | ||
import io.envoyproxy.controlplane.cache.Watch; | ||
import io.envoyproxy.controlplane.cache.XdsRequest; | ||
import io.envoyproxy.controlplane.cache.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wildcard imports, here and in few other files
|
||
override fun onV3StreamDeltaRequest( | ||
streamId: Long, | ||
request: io.envoyproxy.envoy.service.discovery.v3.DeltaDiscoveryRequest? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make alias for import import io.envoyproxy.envoy.service.discovery.v3.DeltaDiscoveryRequest? as DeltaDiscoveryRequestV3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change all imports to DiscoveryRV2/3.
|
||
override fun onV3StreamDeltaRequest( | ||
streamId: Long, | ||
request: io.envoyproxy.envoy.service.discovery.v3.DeltaDiscoveryRequest? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do also alias import
…ol into delta-xds-hash-bytes
|
||
override fun onV3StreamDeltaRequest( | ||
streamId: Long, | ||
request: io.envoyproxy.envoy.service.discovery.v3.DeltaDiscoveryRequest? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too
@@ -38,6 +38,13 @@ val AdsCustomHealthCheck = EnvoyConfig("envoy/config_ads_custom_health_check.yam | |||
val AdsDynamicForwardProxy = EnvoyConfig("envoy/config_ads_dynamic_forward_proxy.yaml") | |||
val FaultyConfig = EnvoyConfig("envoy/bad_config.yaml") | |||
val Ads = EnvoyConfig("envoy/config_ads.yaml") | |||
val DeltaAds = Ads.copy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any tests checking in egress works/propagation changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can switch to delta-xds for RandomConfigFile or just everywhere since it's going to be the default. WDYT?
EDIT: Also those things are tested in jcp.
@@ -217,8 +217,9 @@ class EnvoySnapshotFactory( | |||
globalSnapshot: GlobalSnapshot, | |||
egressRouteSpecifications: Collection<RouteSpecification> | |||
): List<ClusterLoadAssignment> { | |||
val resources = globalSnapshot.endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why just endpoints are called resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old refactoring, I can call it endpoints.
@@ -59,11 +60,24 @@ class MetricsDiscoveryServerCallbacks(private val meterRegistry: MeterRegistry) | |||
.increment() | |||
} | |||
|
|||
override fun onV3StreamDeltaRequest( | |||
streamId: Long, | |||
request: io.envoyproxy.envoy.service.discovery.v3.DeltaDiscoveryRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too
822ac21
to
3c91147
Compare
3c91147
to
3d099e8
Compare
# Conflicts: # envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotProperties.kt
This PR is 1 year old 🎂 🥳 |
Nice @slonka, and congratulations on such a big and long PR since it is still your pull request :) We are unable to merge this PR before we merge the JCP pull request. Which we are unable to merge because JCP community is almost dead, we are unable to publish JCP package, and because of that we cannot test this changes on any of our environments:( |
# Conflicts: # .github/workflows/changelog.yaml # CHANGELOG.md # build.gradle # docs/changelog_symlink.md # docs/configuration.md # envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/NodeMetadataValidator.kt # envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/server/CachedProtoResourcesSerializer.kt # envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/server/callbacks/CompositeDiscoveryServerCallbacks.kt # envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/server/callbacks/LoggingDiscoveryServerCallbacks.kt # envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/server/callbacks/MetricsDiscoveryServerCallbacks.kt # envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/GlobalSnapshot.kt # envoy-control-core/src/test/java/pl/allegro/tech/servicemesh/envoycontrol/v2/SimpleCacheTest.java # envoy-control-core/src/test/java/pl/allegro/tech/servicemesh/envoycontrol/v2/SimpleCacheWithMissingEndpointsTest.java
# Conflicts: # envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/EnvoySnapshotFactoryTest.kt
c26b71b
to
dbfefc9
Compare
This PR enables support for delta-xds (also known as incremental xds) in Envoy.
We're trying to optimize CPU usage in services that use '*' in outgoing dependencies. Based on our tests this should decrease Envoy's CPU usage between 50% and 75%.
Will change JCP snapshot to release when it's merged.