-
Notifications
You must be signed in to change notification settings - Fork 333
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
feat(kuma-cp): implement delta xDS for envoy config exchange #11296
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
Is it possible with this case? We just allow hight version CP wth low version DP right? |
pkg/xds/bootstrap/generator.go
Outdated
switch request.XdsConfigMode { | ||
case types.DELTA: | ||
params.UseDelta = true |
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.
If the CP disabled UseDeltaXds
and the DP request DeltaXDSMode, should we return the DP an error?
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.
No, we should allow individual DPPs to run with Delta. This approach enables a progressive migration. UseDeltaXds enforces Delta mode for all DPPs but still allows individual DPPs to revert to the previous mode if needed.
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.
OK, I understand it right now. With the file pkg/xds/server/v3/components.go
we enable the deltaServer by default. I think maybe we shold rename the variable Experimental.UseDeltaXds
because we may thought it was a swicth to enable or disable the whole delta XDS
feature.
So we got these configurations:
- run the delta server by default
- CP
Experimental.UseDeltaXds
force enable deltaXDS for all DPPs - DP
ENV KUMA_EXPERIMENTAL_USE_DELTA_XDS
to choose use stow or delta
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 think the DPP environment variable is clear, and it shouldn't be a boolean. If it were, we wouldn’t be able to distinguish between using SOTW and "Not Defined." For the control plane configuration, you're right—I need to find a better name for it
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.
The docs talk about XDSTransportProtocolVariant: https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#variants-of-the-xds-transport-protocol
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.
Or Just APIType:
// APIs may be fetched via either REST or gRPC.
type ApiConfigSource_ApiType int32
const (
// Ideally this would be 'reserved 0' but one can't reserve the default
// value. Instead we throw an exception if this is ever used.
//
// Deprecated: Marked as deprecated in envoy/config/core/v3/config_source.proto.
ApiConfigSource_DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE ApiConfigSource_ApiType = 0
// REST-JSON v2 API. The `canonical JSON encoding
// <https://developers.google.com/protocol-buffers/docs/proto3#json>`_ for
// the v2 protos is used.
ApiConfigSource_REST ApiConfigSource_ApiType = 1
// SotW gRPC service.
ApiConfigSource_GRPC ApiConfigSource_ApiType = 2
// Using the delta xDS gRPC service, i.e. DeltaDiscovery{Request,Response}
// rather than Discovery{Request,Response}. Rather than sending Envoy the entire state
// with every update, the xDS server only sends what has changed since the last update.
ApiConfigSource_DELTA_GRPC ApiConfigSource_ApiType = 3
// SotW xDS gRPC with ADS. All resources which resolve to this configuration source will be
// multiplexed on a single connection to an ADS endpoint.
// [#not-implemented-hide:]
ApiConfigSource_AGGREGATED_GRPC ApiConfigSource_ApiType = 5
// Delta xDS gRPC with ADS. All resources which resolve to this configuration source will be
// multiplexed on a single connection to an ADS endpoint.
// [#not-implemented-hide:]
ApiConfigSource_AGGREGATED_DELTA_GRPC ApiConfigSource_ApiType = 6
)
In which I'd use the values: DELTA_GRPC
and GRPC
btw how come we're not using AGGREGATED_GRPC
?
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.
It's because we set it here : https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/config_source.proto#envoy-v3-api-msg-config-core-v3-apiconfigsource
AdsConfig: &envoy_core_v3.ApiConfigSource{
ApiType: configType,
TransportApiVersion: envoy_core_v3.ApiVersion_V3,
SetNodeOnFirstMessageOnly: true,
GrpcServices: []*envoy_core_v3.GrpcService{
buildGrpcService(parameters, enableReloadableTokens),
},
},
And ApiType
can be API type (gRPC, REST, delta gRPC)
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.
This is looking interesting! One thing that isn't clear to me is how you do the opt-in on Kubernetes. How do I set the dataplane env var to just opt-in 1 deployment?
It was my mistake not to validate a single Kubernetes resource with Delta. I had to introduce a new struct to the Dataplane object, which allows setting this configuration. I fixed it, and now it’s possible to enable Delta using an annotation on the Kubernetes resource.
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.
Didn't look at implem only the API.
This is looking interesting! One thing that isn't clear to me is how you do the opt-in on Kubernetes. How do I set the dataplane env var to just opt-in 1 deployment?
pkg/xds/bootstrap/generator.go
Outdated
switch request.XdsConfigMode { | ||
case types.DELTA: | ||
params.UseDelta = true |
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.
The docs talk about XDSTransportProtocolVariant: https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#variants-of-the-xds-transport-protocol
pkg/xds/bootstrap/generator.go
Outdated
switch request.XdsConfigMode { | ||
case types.DELTA: | ||
params.UseDelta = true |
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.
Or Just APIType:
// APIs may be fetched via either REST or gRPC.
type ApiConfigSource_ApiType int32
const (
// Ideally this would be 'reserved 0' but one can't reserve the default
// value. Instead we throw an exception if this is ever used.
//
// Deprecated: Marked as deprecated in envoy/config/core/v3/config_source.proto.
ApiConfigSource_DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE ApiConfigSource_ApiType = 0
// REST-JSON v2 API. The `canonical JSON encoding
// <https://developers.google.com/protocol-buffers/docs/proto3#json>`_ for
// the v2 protos is used.
ApiConfigSource_REST ApiConfigSource_ApiType = 1
// SotW gRPC service.
ApiConfigSource_GRPC ApiConfigSource_ApiType = 2
// Using the delta xDS gRPC service, i.e. DeltaDiscovery{Request,Response}
// rather than Discovery{Request,Response}. Rather than sending Envoy the entire state
// with every update, the xDS server only sends what has changed since the last update.
ApiConfigSource_DELTA_GRPC ApiConfigSource_ApiType = 3
// SotW xDS gRPC with ADS. All resources which resolve to this configuration source will be
// multiplexed on a single connection to an ADS endpoint.
// [#not-implemented-hide:]
ApiConfigSource_AGGREGATED_GRPC ApiConfigSource_ApiType = 5
// Delta xDS gRPC with ADS. All resources which resolve to this configuration source will be
// multiplexed on a single connection to an ADS endpoint.
// [#not-implemented-hide:]
ApiConfigSource_AGGREGATED_DELTA_GRPC ApiConfigSource_ApiType = 6
)
In which I'd use the values: DELTA_GRPC
and GRPC
btw how come we're not using AGGREGATED_GRPC
?
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
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.
A couple of Qs but really happy that we're going to be shipping this 🚢 . Also does go-control-plane handle the stuff like hashing individual resources instead of whole arrays?
@@ -68,4 +69,7 @@ message ZoneIngress { | |||
// AvailableService contains tags that represent unique subset of | |||
// endpoints | |||
repeated AvailableService availableServices = 3; | |||
|
|||
// EnvoyConfiguration provides additional configuration for the Envoy sidecar. | |||
EnvoyConfiguration envoy = 4; |
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 do we need that in zone ingress? (I might answer that question myself later)
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.
It's because Egress and Ingress also can use delta for configuration exchange
@@ -38,6 +38,7 @@ func NewDefaultBootstrapGenerator( | |||
enableReloadableTokens bool, | |||
hdsEnabled bool, | |||
defaultAdminPort uint32, | |||
deltaXdsEnabled bool, |
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 need a builder for this? it's getting quite packed with parameters
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 could change it, but probably in a separate PR
if isDelta { | ||
d.dpDeltaStreams[streamID] = dpStream | ||
} else { | ||
d.dpStreams[streamID] = dpStream | ||
} |
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.
maybe instead of this constant if/else you could have a generic function that takes either dpStreams
or dpDeltaStreams
? this applies to all functions with isDelta
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've changed to a function as a parameter, You can take a look
Yes, it should verify individual resources so only one endpoint will be sent. I don't know if that is the question. |
Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
I think there is still some edge case causing flake, need to investigate |
Motivation
Envoy supports incremental xDS, which sends only changes rather than the entire state.
Implementation information
Supporting documentation
https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#incremental-xds
Fix #XX