Don't delete non-existent resources in non-wildcard mode in delta xds #488

s-matyukevich · 2021-08-27T03:50:19Z

Before this PR we used to always add resource to the RemovedResources field of the DeltaDiscoveryResponse if it doesn't exist on the server. This causes problems in the following scenario:

Control plane instructs envoy to create a cluster "foo" via CDS.
Envoy sends DeltaDiscoveryRequest with type ClusterLoadAssignment and ResourceNamesSubscribe field set to []string{"foo"}
At the moment control plane doesn't have any endpoints for the cluster "foo", so it sends DeltaDiscoveryResponse with RemovedResources name set to []string{"foo"}, which instructs envoy to unsubscribe from this resource.
Envoy sends ACK and control plane creates a delta watch that doesn't watch for changes in "foo".
After some time a few endpoints are registered in the control plane for the cluster "foo". Control plane updates corresponding cache (either simple or linear)
Envoy doesn't receive any updates.

This PR fixes the issue by preventing resources from being deleted for non-wildcard requests. Instead, now we are going to wait for the client to unsubscribe from resources by itself. This works fine if the client always uses wildcard requests to pull initial resources and it also uses non-wildcard requests only to pull dependent resources (This is the case with envoy: it always uses wildcard requests to pull clusters and listeners and then non-wildcard requests to pull all other dependent resources)

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

…into incremental-linear

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

…into incremental-linear

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Co-authored-by: Alec Holmes <alec.holmes@greymatter.io> Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

…into incremental-linear

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

…into incremental-linear

…ent and doesn't exist on the server in delta xds Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

pkg/cache/v3/delta.go

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

…into incremental-linear

s-matyukevich · 2021-08-27T17:59:07Z

I spend some time trying to debug failing delta ADS integration tests. I think they are failing because of broken ADS ordering (in particular, cluster updates are send before listener updates, which is causing the following error unknown cluster 'cluster-v0-0') I thing the ordering was broken before this PR but this change makes it apparent. Looking more into that.

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

s-matyukevich · 2021-08-27T20:02:18Z

I spend some time trying to debug failing delta ADS integration tests. I think they are failing because of broken ADS ordering (in particular, cluster updates are send before listener updates, which is causing the following error unknown cluster 'cluster-v0-0') I thing the ordering was broken before this PR but this change makes it apparent. Looking more into that.

Changing ADS ordering didn't help. No mater how we order resources we will always have some races. Here are 2 cases that I checked:

Update clusters first and then update routes
In this case after you updated clusters the current routes still reference old clusters, which doesn't exist anymore. We are getting 503 errors with unknown cluster 'cluster-v0-0' error message
Update routes first and then update clusters.
In this case after we update routes the reference clusters that doesn't exist yes. Some requests are still failing with 503 unknown cluster 'cluster-v1-0'

The only solution I can think of is to skip deleting non-existent routes (they are requested in non-wildcard mode). They still will be removed after envoy receives listener updates and will figure out that nobody is referencing old routes anymore.

s-matyukevich · 2021-08-27T20:04:03Z

This PR should be ready for review, cc @snowp @alecholmez

alecholmez · 2021-08-30T01:52:00Z

@s-matyukevich I'm not sure if you've seen this PR: #461 but maybe the ordering of the resources issue can be fixed by porting this over to delta

alecholmez · 2021-08-30T02:34:45Z

@snowp your input might be valuable here, I remember us having a conversation on this bit of the logic but I can't remember the decisions as to why we decided to not keep the blank string entry

s-matyukevich · 2021-08-30T13:49:00Z

@s-matyukevich I'm not sure if you've seen this PR: #461 but maybe the ordering of the resources issue can be fixed by porting this over to delta

Yeah, I saw this PR and implemented ordering. It didn't. help me to fix integration tests because I was also trying to implement this part of the spec (sent DeltaDiscoverResponse with empty Resource field for non-existent resources) In this case envoy indeed deleted the resource immediately, which was causing issue mo matter how we order resources.

After I updated the code and stopped sending anything for non-existent resources requested in non-wildcard mode, everything started working as expected. (We just wait for envoy to unsubscribed from resources that are no longer references by any other resource) This also works ok with any resource ordering, so I deleted the ordering code to keep this PR focused.

I am not sure whether and how we are supposed to implement the above mentioned part of the spec.

snowp · 2021-08-30T16:00:03Z

Haven't read the full thread but this doesn't seem right:

At the moment control plane doesn't have any endpoints for the cluster "foo", so it sends DeltaDiscoveryResponse with RemovedResources name set to []string{"foo"}, which instructs envoy to unsubscribe from this resource.

Envoy sends ACK and control plane creates a delta watch that doesn't watch for changes in "foo".

Whether or not a resource is subscribed to should not be up to the server at all, the client knows what resources it wants. removed_resources should be used to instruct the client that a resource that it is subscribed to and thinks exist no longer does, but once it's removed from the client the subscription should remain (unless the client includes it in a unsubscribe call).

snowp · 2021-08-30T16:03:07Z

@snowp your input might be valuable here, I remember us having a conversation on this bit of the logic but I can't remember the decisions as to why we decided to not keep the blank string entry

I don't recall what the semantics of empty string used to mean, but we should definitely be keeping track of resources that are currently being subscribed to but they don't have a current value.

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

s-matyukevich · 2021-08-30T23:09:31Z

Haven't read the full thread but this doesn't seem right:
At the moment control plane doesn't have any endpoints for the cluster "foo", so it sends DeltaDiscoveryResponse with RemovedResources name set to []string{"foo"}, which instructs envoy to unsubscribe from this resource.

Envoy sends ACK and control plane creates a delta watch that doesn't watch for changes in "foo".
Whether or not a resource is subscribed to should not be up to the server at all, the client knows what resources it wants. removed_resources should be used to instruct the client that a resource that it is subscribed to and thinks exist no longer does, but once it's removed from the client the subscription should remain (unless the client includes it in a unsubscribe call).

I updated the code accordingly to this comment. Now we keep the subscription active for the non-existent resources while still populating removed_resources field. I'll test this in our staging env tomorrow.

s-matyukevich · 2021-08-31T18:56:11Z

@snowp @alecholmez I tested the latest version of this PR in our staging environment and verified that it indeed fixes the issue.

alecholmez · 2021-09-02T20:01:45Z

Cool I think this satisfies everything we need. @snowp this look good to you? I have nothing else to add

alecholmez

Just a few comment nitpicks. Everything else looks good though

pkg/cache/v3/delta.go

Co-authored-by: Alec Holmes <alec.holmes@greymatter.io> Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

alecholmez · 2021-09-03T15:10:04Z

Hmm it seems like envoy is seg faulting..

…into incremental-linear

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

s-matyukevich · 2021-09-03T20:50:17Z

Hmm it seems like envoy is seg faulting..

#490 (comment)

I am pretty sure something is wrong with the latest envoy dev build.

alecholmez · 2021-09-07T22:06:45Z

I've just notified the Envoy team about the segfault

s-matyukevich · 2021-09-07T22:13:40Z

I've just notified the Envoy team about the segfault

Thanks! This #493 also could be helpful (Basically, I did a binary search on recent envoy commits and figured out the one after which the build starts failing) I was also trying to use stack_decode.py to decode the stack trace, but I have some issues with building the envoy binary with debug symbols and using it in CI.

alecholmez · 2021-09-07T22:14:52Z

Awesome thank you @s-matyukevich. Would you also mind alerting the security team as well? Here's a doc for how to do so

s-matyukevich · 2021-09-07T22:23:02Z

Awesome thank you @s-matyukevich. Would you also mind alerting the security team as well? Here's a doc for how to do so

Sure, done.

…into incremental-linear

s-matyukevich · 2021-09-13T15:08:25Z

@alecholmez This now passes CI, anything else needs to be done before we can merge?

alecholmez

Last 2 things! I'll approve and merge once these are addressed

alecholmez · 2021-09-14T18:50:14Z

pkg/cache/v3/delta.go

@@ -56,13 +56,19 @@ func createDeltaResponse(ctx context.Context, req *DeltaRequest, state stream.St
 					filtered = append(filtered, r)
 				}
 				nextVersionMap[name] = nextVersion
+			} else {
+				// we track non-existent resources for non-wildcard streams until the client explicitly unsubscribes from them.
+


can you remove this space?

alecholmez · 2021-09-14T18:50:39Z

pkg/cache/v3/delta.go

@@ -56,13 +56,19 @@ func createDeltaResponse(ctx context.Context, req *DeltaRequest, state stream.St
 					filtered = append(filtered, r)
 				}
 				nextVersionMap[name] = nextVersion
+			} else {
+				// we track non-existent resources for non-wildcard streams until the client explicitly unsubscribes from them.


Suggested change

// we track non-existent resources for non-wildcard streams until the client explicitly unsubscribes from them.

// We track non-existent resources for non-wildcard streams until the client explicitly unsubscribes from them.

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

s-matyukevich · 2021-09-14T19:36:46Z

Last 2 things! I'll approve and merge once these are addressed

Sure, done.

alecholmez · 2021-09-15T14:37:47Z

build/integration.sh

@@ -39,6 +39,7 @@ ENVOY_PID=$!
 function cleanup() {
  kill ${ENVOY_PID} ${UPSTREAM_PID}
  wait ${ENVOY_PID} ${UPSTREAM_PID} 2> /dev/null || true
+  cat ${ENVOY_LOG}


Ah can you remove this cat? We don't want to pollute this log here, i think this is just leftover from debugging

oops, I forgot about this, this is fixed now.

Awesome thank you!

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

alecholmez

Looks good to me! Thanks for all the cooperation @s-matyukevich 😄

s-matyukevich and others added 16 commits July 1, 2021 12:05

Add delta xDS support to Linear and Mux caches

ce80713

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Add more tests

e6b4caf

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

merge with main

758c527

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Merge branch 'main' of https://github.com/envoyproxy/go-control-plane …

29e88a0

…into incremental-linear

merge with main

ed17309

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Refactor resourceContainer interface

0f16bca

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

merge with main

c57c226

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Merge branch 'main' of https://github.com/envoyproxy/go-control-plane …

3218e00

…into incremental-linear

refactoring

0d72159

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Update pkg/cache/v3/delta.go

4d5c8f5

Co-authored-by: Alec Holmes <alec.holmes@greymatter.io> Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

make format

66990d5

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Merge branch 'main' of https://github.com/envoyproxy/go-control-plane …

eac6f00

…into incremental-linear

address review comments

01b2649

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

all logging tags

2c9fcfb

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Merge branch 'main' of https://github.com/envoyproxy/go-control-plane …

d1d94f1

…into incremental-linear

Respond with empty resource if it was explicitly requested by the cli…

201c736

…ent and doesn't exist on the server in delta xds Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

valerian-roche reviewed Aug 27, 2021

View reviewed changes

pkg/cache/v3/delta.go Outdated Show resolved Hide resolved

s-matyukevich added 2 commits August 27, 2021 09:51

add non existent resources to the next version map

c9b1f68

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Merge branch 'main' of https://github.com/envoyproxy/go-control-plane …

3597378

…into incremental-linear

fix ads tests

e9f8f18

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

s-matyukevich changed the title ~~Respond with empty resource if it was explicitly requested by the client and doesn't exist on the server in delta xds~~ Don't delete non-existent resources in non-wildcard mode in delta xds Aug 27, 2021

populate removed_resources list by keep the subscription going

d6cb30a

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

fix tests

917b00a

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

alecholmez requested changes Sep 2, 2021

View reviewed changes

pkg/cache/v3/delta.go Outdated Show resolved Hide resolved

pkg/cache/v3/delta.go Outdated Show resolved Hide resolved

pkg/cache/v3/delta.go Outdated Show resolved Hide resolved

Apply suggestions from code review

40f16db

Co-authored-by: Alec Holmes <alec.holmes@greymatter.io> Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

s-matyukevich force-pushed the incremental-linear branch from c1a4415 to 40f16db Compare September 3, 2021 13:32

format

1960372

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

s-matyukevich added 2 commits September 3, 2021 09:47

Merge branch 'main' of https://github.com/envoyproxy/go-control-plane …

b2a7ecb

…into incremental-linear

debug

ac7790b

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

Merge branch 'main' of https://github.com/envoyproxy/go-control-plane …

42a78b9

…into incremental-linear

alecholmez self-requested a review September 14, 2021 18:37

alecholmez requested changes Sep 14, 2021

View reviewed changes

Fix comment

ce5d53d

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

alecholmez requested changes Sep 15, 2021

View reviewed changes

revert changes to build/integration.sh

cdd8c17

Signed-off-by: Sergey Matyukevich <s.matyukevich@gmail.com>

alecholmez approved these changes Sep 15, 2021

View reviewed changes

alecholmez merged commit e483d0d into envoyproxy:main Sep 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't delete non-existent resources in non-wildcard mode in delta xds #488

Don't delete non-existent resources in non-wildcard mode in delta xds #488

s-matyukevich commented Aug 27, 2021 •

edited

Loading

s-matyukevich commented Aug 27, 2021

s-matyukevich commented Aug 27, 2021

s-matyukevich commented Aug 27, 2021

alecholmez commented Aug 30, 2021

alecholmez commented Aug 30, 2021

s-matyukevich commented Aug 30, 2021

snowp commented Aug 30, 2021

snowp commented Aug 30, 2021

s-matyukevich commented Aug 30, 2021

s-matyukevich commented Aug 31, 2021

alecholmez commented Sep 2, 2021

alecholmez left a comment

alecholmez commented Sep 3, 2021 •

edited

Loading

s-matyukevich commented Sep 3, 2021

alecholmez commented Sep 7, 2021

s-matyukevich commented Sep 7, 2021 •

edited

Loading

alecholmez commented Sep 7, 2021

s-matyukevich commented Sep 7, 2021

s-matyukevich commented Sep 13, 2021

alecholmez left a comment

alecholmez Sep 14, 2021

alecholmez Sep 14, 2021

s-matyukevich commented Sep 14, 2021

alecholmez Sep 15, 2021 •

edited

Loading

s-matyukevich Sep 15, 2021

alecholmez Sep 15, 2021

alecholmez left a comment

	// we track non-existent resources for non-wildcard streams until the client explicitly unsubscribes from them.
	// We track non-existent resources for non-wildcard streams until the client explicitly unsubscribes from them.

Don't delete non-existent resources in non-wildcard mode in delta xds #488

Don't delete non-existent resources in non-wildcard mode in delta xds #488

Conversation

s-matyukevich commented Aug 27, 2021 • edited Loading

s-matyukevich commented Aug 27, 2021

s-matyukevich commented Aug 27, 2021

s-matyukevich commented Aug 27, 2021

alecholmez commented Aug 30, 2021

alecholmez commented Aug 30, 2021

s-matyukevich commented Aug 30, 2021

snowp commented Aug 30, 2021

snowp commented Aug 30, 2021

s-matyukevich commented Aug 30, 2021

s-matyukevich commented Aug 31, 2021

alecholmez commented Sep 2, 2021

alecholmez left a comment

Choose a reason for hiding this comment

alecholmez commented Sep 3, 2021 • edited Loading

s-matyukevich commented Sep 3, 2021

alecholmez commented Sep 7, 2021

s-matyukevich commented Sep 7, 2021 • edited Loading

alecholmez commented Sep 7, 2021

s-matyukevich commented Sep 7, 2021

s-matyukevich commented Sep 13, 2021

alecholmez left a comment

Choose a reason for hiding this comment

alecholmez Sep 14, 2021

Choose a reason for hiding this comment

alecholmez Sep 14, 2021

Choose a reason for hiding this comment

s-matyukevich commented Sep 14, 2021

alecholmez Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

s-matyukevich Sep 15, 2021

Choose a reason for hiding this comment

alecholmez Sep 15, 2021

Choose a reason for hiding this comment

alecholmez left a comment

Choose a reason for hiding this comment

s-matyukevich commented Aug 27, 2021 •

edited

Loading

alecholmez commented Sep 3, 2021 •

edited

Loading

s-matyukevich commented Sep 7, 2021 •

edited

Loading

alecholmez Sep 15, 2021 •

edited

Loading