Avoid blocking child type updates on parent ack #15083

freddygv · 2022-10-20T21:06:08Z

Description

When providing updates to Envoy it is safest to follow the update order of: Clusters, Endpoints, Listeners, Routes. This order is encoded in our delta xDS response generation code.

Another noteworthy attribute of these resources is that there are parent->child relationships between: Cluster->Endpoints and Listener->Routes. Envoy couples the storage of a parent and a child type such that whenever a control plane sends an update for a parent type, it MUST also send fresh data for the child type, even if Envoy previously had the latest data for the child type.

A wrinkle that isn't accounted for currently is that Envoy does not actually ACK parent types until after attempting to receive data for the child types. This behavior also actually differs between Clusters and Listeners:

When a cluster is sent, Envoy will wait up to a 15s timeout for endpoints to arrive before sending the cluster ACK.
When a listener is sent, and it specifies RDS routes, Envoy will wait until those routes arrive before ACKing the listener. Though it's not clear to me what the timeout is for this, if any, since it exceeds a minute from my tests.

However, a behavior encoded in our xDS update order is that we avoid sending ANY data if we are waiting on ACKs for ANY resource. Meaning that when we first send a cluster to Envoy and it requests endpoints for that cluster, the endpoints do not actually get sent for at least 15s. This is because the endpoint update is paused by Consul until the cluster ACK, which Envoy pauses until it gets endpoints or times out.

This commit changes the xDS order gating such that we only block listener/route updates if there are cluster/endpoin updates pending. This is to avoid routing to an invalid destination.

Testing & Reproduction steps

Ran manual tests for various scenarios against a live Envoy to validate assumptions (Envoy v1.21 and v1.23)
Updated the unit tests to more accurately reflect Envoy's behavior
Anecdotally the integration tests seem to be running ~1min faster than usual with this patch

Links

Likely related to: envoyproxy/envoy#5887

PR Checklist

updated test coverage
external facing docs updated
not a security concern

freddygv · 2022-10-20T21:16:49Z

@erichaberkorn Something that would be good to look into is having a couple Delta xDS protocol tests like in delta_test.go but against live Envoys, to catch these sorts of expectations mismatches

freddygv · 2022-10-20T21:20:27Z

agent/xds/delta.go

+							"typeUrl", op.TypeUrl, "dependent", xdscommon.ClusterType)
+						break
+					}
+					if endpointHandler := handlers[xdscommon.EndpointType]; endpointHandler.registered && len(endpointHandler.pendingUpdates) > 0 {


This endpoints check might be redundant given that Envoy waits for endpoints before ACKing clusters

freddygv · 2022-10-20T22:40:44Z

agent/xds/endpoints.go

@@ -650,10 +650,6 @@ func (s *ResourceGenerator) endpointsFromDiscoveryChain(
 		primaryTargetID := node.Resolver.Target
 		failover := node.Resolver.Failover

-		type targetLoadAssignmentOption struct {


agent/xds/delta.go

erichaberkorn · 2022-10-21T14:33:29Z

agent/xds/delta.go

+	// we MUST send new data for all its children. Envoy will NOT re-subscribe to the child data upon
+	// receiving updates for the parent, so we need to handle this ourselves.
+	//
+	// Note that we do not check whether the deltaChild.childType is registered here, since we


Could you complete this sentence?

erichaberkorn · 2022-10-21T14:42:25Z

agent/xds/delta.go

+				if op.TypeUrl == xdscommon.ListenerType || op.TypeUrl == xdscommon.RouteType {
+					if clusterHandler := handlers[xdscommon.ClusterType]; clusterHandler.registered && len(clusterHandler.pendingUpdates) > 0 {
+						generator.Logger.Trace("Skipping delta computation for resource because there are dependent updates pending",
+							"typeUrl", op.TypeUrl, "dependent", xdscommon.ClusterType)
+						break
+					}
+					if endpointHandler := handlers[xdscommon.EndpointType]; endpointHandler.registered && len(endpointHandler.pendingUpdates) > 0 {
+						generator.Logger.Trace("Skipping delta computation for resource because there are dependent updates pending",
+							"typeUrl", op.TypeUrl, "dependent", xdscommon.EndpointType)
+						break
+					}
+				}


Is this to enforce the Clusters, Endpoints, Listeners, Routes order?

How are the xDS updates being retriggered when we skip them here?

Is this to enforce the Clusters, Endpoints, Listeners, Routes order?

Yes.

How are the xDS updates being retriggered when we skip them here?

The updates get unblocked by the select statement at the beginning of this big loop. Pushing xDS updates is generally triggered by Envoy sending us a message (receiving on reqCh) or a new proxycfg snapshot due to internal updates (receiving on stateCh). The message from Envoy could be subscribe/unsubscribe/ack/nack.

So if we block updating listeners because of pending updates for endpoints, once those endpoints are ACKd then we're unblocked again. In the NACK case we actually re-block for changes because we assume that we got bad data from proxycfg and want to avoid a hot loop of errors (has happened before).

Re-blocking on NACK should be OK since if clusters/endpoints are NACKed we still don't want to send listeners because they depend on the clusters/endpoints.

agent/xds/delta.go

When providing updates to Envoy it is safest to follow the update order of: Clusters, Endpoints, Listeners, Routes. This order is encoded in our delta xDS response generation code. Another noteworthy attribute of these resources is that there are parent->child relationships between: Cluster->Endpoints and Listener->Routes. Envoy couples the storage of a parent and a child type such that whenever a control plane sends an update for a parent type, it MUST also send fresh data for the child type, even if Envoy previously had the latest data for the child type. A wrinkle that isn't accounted for currently is that Envoy does not actually ACK parent types until after attempting to receive data for the child types. This behavior also actually differs between Clusters and Listeners: - When a cluster is sent, Envoy will wait up to a 15s timeout for endpoints to arrive before sending the cluster ACK. - When a listener is sent, and it specifies RDS routes, Envoy will wait until those routes arrive before ACKing the listener. Though it's not clear to me what the timeout is for this, it exceeds a minute from my tests. However, a behavior encoded in our xDS update order is that we avoid sending ANY data if we are waiting on ACKs for ANY resource. Meaning that when we first send a cluster to Envoy and it requests endpoints for that cluster, the endpoints do not actually get sent for at least 15. This is because the endpoint update is paused by Consul until the cluster ACK, which Envoy pauses until it gets endpoints or times out. This commit changes the xDS order gating such that we only block listener/route updates if there are cluster/endpoin updates pending. This is to avoid routing to an invalid destination.

freddygv requested a review from rboyer October 20, 2022 21:06

github-actions bot added the theme/envoy/xds Related to Envoy support label Oct 20, 2022

freddygv requested review from erichaberkorn and ndhanushkodi October 20, 2022 21:11

freddygv commented Oct 20, 2022

View reviewed changes

freddygv marked this pull request as ready for review October 20, 2022 22:38

freddygv commented Oct 20, 2022

View reviewed changes

erichaberkorn reviewed Oct 21, 2022

View reviewed changes

erichaberkorn approved these changes Oct 21, 2022

View reviewed changes

rboyer reviewed Oct 24, 2022

View reviewed changes

agent/xds/delta.go Show resolved Hide resolved

rboyer reviewed Oct 24, 2022

View reviewed changes

agent/xds/delta.go Show resolved Hide resolved

rboyer approved these changes Nov 7, 2022

View reviewed changes

freddygv added 4 commits November 7, 2022 12:35

Clean up dead code and comment

3d0b984

Update comments

c47a0c3

add changelog entry

5581010

freddygv force-pushed the xds/cds-ack branch from 4ab769a to 5581010 Compare November 7, 2022 19:38

Fixup flaky test

dea2bd9

freddygv merged commit 7f5f7e9 into main Nov 8, 2022

freddygv deleted the xds/cds-ack branch November 8, 2022 01:10

freddygv added the backport/1.14 label Nov 9, 2022

hc-github-team-consul-core mentioned this pull request Nov 9, 2022

Backport of Avoid blocking child type updates on parent ack into release/1.14.x #15321

Merged

3 tasks

analogue added the backport/1.12 label Nov 21, 2022

hc-github-team-consul-core mentioned this pull request Nov 21, 2022

Backport of Avoid blocking child type updates on parent ack into release/1.12.x #15501

Merged

3 tasks

analogue restored the xds/cds-ack branch November 21, 2022 15:54

analogue added the backport/1.13 label Nov 21, 2022

hc-github-team-consul-core mentioned this pull request Nov 21, 2022

Backport of Avoid blocking child type updates on parent ack into release/1.13.x #15517

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid blocking child type updates on parent ack #15083

Avoid blocking child type updates on parent ack #15083

freddygv commented Oct 20, 2022 •

edited

Loading

freddygv commented Oct 20, 2022

freddygv Oct 20, 2022

freddygv Oct 20, 2022

erichaberkorn Oct 21, 2022

freddygv Oct 21, 2022

erichaberkorn Oct 21, 2022

erichaberkorn Oct 21, 2022

freddygv Oct 21, 2022 •

edited

Loading

erichaberkorn Oct 21, 2022

Avoid blocking child type updates on parent ack #15083

Avoid blocking child type updates on parent ack #15083

Conversation

freddygv commented Oct 20, 2022 • edited Loading

Description

Testing & Reproduction steps

Links

PR Checklist

freddygv commented Oct 20, 2022

freddygv Oct 20, 2022

Choose a reason for hiding this comment

freddygv Oct 20, 2022

Choose a reason for hiding this comment

erichaberkorn Oct 21, 2022

Choose a reason for hiding this comment

freddygv Oct 21, 2022

Choose a reason for hiding this comment

erichaberkorn Oct 21, 2022

Choose a reason for hiding this comment

erichaberkorn Oct 21, 2022

Choose a reason for hiding this comment

freddygv Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

erichaberkorn Oct 21, 2022

Choose a reason for hiding this comment

freddygv commented Oct 20, 2022 •

edited

Loading

freddygv Oct 21, 2022 •

edited

Loading