config: fix delta xDS's use of (un)subscribe fields, more explicit protocol spec by fredlas · Pull Request #6545 · envoyproxy/envoy

fredlas · 2019-04-10T19:36:08Z

I realized that, with the unreliable queue implementation copied from SotW xDS, delta xDS could get into a state where Envoy thinks it has subscribed, but the server hasn't heard the subscription, with no way for either to realize the mistake. I fixed that by converting the queue setup to a cleaner "do I currently want to send a request?" with the request's (un)subscriptions only populated immediately before the request is actually sent into gRPC.

While doing that, I further realized there was a problem when a given resource was subscribed then unsubscribed (or reversed), all in between request sends. I made sure Envoy handles that sensibly, and added explicit requirements to the xDS protocol spec to ensure servers will also handle it sensibly.

Added unit tests for those fixes.

Risk Level: low
Testing: added unit tests for bugs uncovered

#4991

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-04-10T19:40:47Z

source/common/config/delta_subscription_impl.h


    grpc_stream_.establishNewStream();
-    subscribe(resources);
-    // The attempt stat here is maintained for the purposes of having consistency between ADS and


It looks like this is referring to the same thing as the TODOs I added. Just how meaningless is update_attempt for ADS? Can we just do nothing with it and let it sit at 0?

Signed-off-by: Fred Douglas <fredlas@google.com>

source/common/config/delta_subscription_impl.h

snowp · 2019-04-11T15:22:45Z

source/common/config/delta_subscription_impl.h

-
-    for (const auto& added : diff.added_) {
-      setResourceWaitingForServer(added);
+  void updateResources(std::vector<std::string> resource_names) override {


I'm not sure what the calling code looks like, but would it be possible to have this parameter be a set instead? That way you'd avoid having to pass by value and sorting

Definitely makes sense, and it doesn't look like any of the implementors would run into problems with this change. However, it is a pretty far reaching change, and there appears to be some tests I'll need to fix. Working on it.

Done. This uncovered an issue with how the old non-delta gRPC xDS implementation uses its "watches". Previously, when updating your interest from resources 0 and 1 to just 2, you would first send a request for 0,1,2, and then send one for just 2. With my change, you'll now send a request for nothing, and then just 2. Well, I don't know, maybe this is too much churn; maybe the existing logic makes sense. Fortunately, this will all be replaced in the near future with logic that will have the correct behavior (just send one request, for 2).

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch

Thanks for thinking about these corner cases. It got me thinking about whether we can try make delta discovery requests/responses a bit easier to reason about.

htuch · 2019-04-12T18:28:47Z

source/common/common/utility.h

+   * @param delimiter supplies the delimiter to join them together.
+   * @return string combining elements of `source` with `delimiter` in between each element.
+   */
+  static std::string join(const std::set<std::string>& source, const std::string& delimiter);


Can we maybe just have a templatized join that works on iterators? That could replace the one for vector as well.

Yeah, I was wondering if there might be something like that, but.... seemed like a lot of trouble! This duplicated join() broke some other tests I didn't notice, so I have already addressed that by just inlining the single use of join(). Does that sound ok?

It's OK, but it's probably only a couple of lines to make it a template with iterators :)

htuch · 2019-04-12T18:33:38Z

source/common/config/delta_subscription_impl.h

+    stats_.update_attempt_.inc();
+    // Tell the server about our new interests (but only if there are any).
+    if (!names_added_.empty() || !names_removed_.empty()) {
+      kickOffDiscoveryRequest();


How does this affect ACKs for server response nonces?

This had me thinking; for incremental xDS, do we want to consider splitting out ACK/NACK from the DeltaDiscoveryRequest? It made sense in state-of-world, because it was much simpler, but now we have different concerns, so a top-level oneof object might make more sense. You could end up sending two messages, an ack and then the next request.

Do you think ^^ would be easier to reason about?

By "this" I'm guessing you mean the if condition here? There are a few paths that kickOffDiscoveryRequest(); this is not the one that would ever be doing it in the ACK scenario.

I think that's a very good point. IIRC there's some language like "the nonce field should only be set when ACKing a response", meaning there is somewhat of a de facto oneof already going on. If we do this, there is another bit of perceptual cleanup that would make sense, and that is dumping the Request/Response nomenclature. IMO it is usually not appropriate for bidi streaming, and this is definitely one such case.

One thing: I think the clarity benefits would be almost entirely at the "reading the .proto and docs / understanding the protocol" level (maybe the server implementations would also benefit; no idea). I think Envoy's implementation wouldn't change structure much; you would have one .cc file dealing with both oneof members in relatively close proximity. (Actually, maybe this would be more useful than I can understand, since I already fully "get" when the code is using a DiscoveryRequest as an ACK vs a request).

Sorry, I wasn't precise; I meant by "this" the idea of coalescing multiple Envoy DeltaDiscoveryRequests, each of which would have been sent in reply to some unique DeltaDiscoveryResponse with its own nonce. We will lose ACK/NACK as a result potentially, or maybe there is some detail that avoids this that I missed.

By splitting out the ACK/NACK as a distinct message and avoiding collapsing it into the request, when we unpause we could issue a series of ACK/NACKs with the relevant nonces, and then the new delta request.

IIIIII see. Yes, that's definitely right; I will fix this PR accordingly. When you put it that way, I realize I'm pretty sure that xDS, including SotW, has been broken all along! (Due to how happy it is to drop things from the queue, and the fact that the nonce is just directly copied into request_, rather than queued up).

I think this would ideally be cleanest as a repeated field, rather than a series of messages. It would be very easy to implement here in Envoy; not sure how much work it would be in the server. Because it's so easy, there's no need to do it now "while we're already here" or anything like that, though.

I'm not sure if SotW is broken, since the last DiscoveryResponse is always authoritative, replacing all earlier ones, so it's safe to just have the latest DiscoveryRequest in that situation.

Oh, because when you're always sending everything, ACKs are automatically "cumulative". Thanks, makes sense!

Did you decide to punt on making ACK/NACKs a distinct message?

Hmm, I think yes, punt. My big aggregated delta implementation is already working without that, and I'd rather not go back and add it in. Plus, I think it will be most clear whether we want it / how best to do it once that aggregated delta stuff is submitted, and we're looking at applying the prospective changes on top of it.

Oh, but, with this in place, what I said about "I think I could easily prevent those [no-ops] from being sent, but I was worried that that would be needlessly cute/complex" would probably be much cleaner to implement. So I bet we will want it.

Ugh, I wrote a whole big response and then github threw it away. Basically, yes. It will be easier to see how we want to implement it once we can start freshly on top of the upcoming aggregated delta implementation. I think it will help make it cleaner to avoid sending the no-op messages, which is nice.

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-04-16T22:09:30Z

Ready, PTAL. Note the "no-op" requests that now have to be expected in the unit tests. I think I could easily prevent those from being sent, but I was worried that that would be needlessly cute/complex.

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch

Thanks, looks good now modulo a few comments. This will also be much nicer on the wire for SoW.
/wait

api/XDS_PROTOCOL.md

source/common/config/delta_subscription_impl.h

htuch · 2019-04-17T22:24:30Z

test/common/config/delta_subscription_impl_test.cc

+// add-then-remove case *also* causes the resource to be referred to in the request (as an
+// unsubscribe).
+// Unlike the remove-then-add case, this one really is unnecessary, and ideally we would have
+// the request simply not include any mention of the resource. Oh well.


Should this one be a TODO?

I don't think the code complexity would be worth it, like even if the labor to make the change was free. It would only be avoiding an extraneous-but-not-harmful message in a corner case.

I think it's not worth the code complexity, like even if the labor was free. The benefit would just be a small efficiency gain in a pretty corner-y corner case.

test/common/config/delta_subscription_impl_test.cc

htuch · 2019-04-17T22:26:04Z

source/common/config/delta_subscription_impl.h

+    stats_.update_attempt_.inc();
+    // Tell the server about our new interests (but only if there are any).
+    if (!names_added_.empty() || !names_removed_.empty()) {
+      kickOffDiscoveryRequest();


Did you decide to punt on making ACK/NACKs a distinct message?

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-04-18T17:48:04Z

PTAL. BTW it is on my todo list to split this and the others out into .cc's now that they're detemplatized... although maybe not this one, since it's going to be outright replaced by the aggregated PR.

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch

Awesome, LGTM.

* master: thread: remove ThreadFactorySingleton (envoyproxy#6658) router: support offseting downstream provided grpc timeout (envoyproxy#6628) router: defer per try timeout until downstream request is done (envoyproxy#6643) update bazel readme for clang-format-8 on mac (envoyproxy#6660) Implement some TODOs in quic_endian_impl.h (envoyproxy#6644) docs: add aspell to mac dependencies to fix check format script (envoyproxy#6661) config: fix delta xDS's use of (un)subscribe fields, more explicit protocol spec (envoyproxy#6545) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

fredlas added 4 commits April 10, 2019 14:41

remove queue from delta xDS, fix sub/unsub logic, add tests

e9391d8

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into DEQ_dequeify_xds

9d8ff09

Signed-off-by: Fred Douglas <fredlas@google.com>

more explicit specification in XDS_PROTOCOL.md

13d224d

Signed-off-by: Fred Douglas <fredlas@google.com>

another unit test

d6afbcb

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas commented Apr 10, 2019

View reviewed changes

fredlas added 5 commits April 10, 2019 15:43

add else. might matter if checkRateLimitAllowsDrain has an effect.

c1f533f

Signed-off-by: Fred Douglas <fredlas@google.com>

double negative

8a82b87

Signed-off-by: Fred Douglas <fredlas@google.com>

typo

278344c

Signed-off-by: Fred Douglas <fredlas@google.com>

clang tidy, clarify test comments

e2c1370

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into DEQ_dequeify_xds

ece66ff

Signed-off-by: Fred Douglas <fredlas@google.com>

snowp assigned htuch Apr 11, 2019

snowp suggested changes Apr 11, 2019

View reviewed changes

fredlas added 3 commits April 11, 2019 13:21

better name for resource_names

d2d9dc6

Signed-off-by: Fred Douglas <fredlas@google.com>

std set everywhere, fix watch issue

823123b

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into DEQ_dequeify_xds

2177517

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch reviewed Apr 12, 2019

View reviewed changes

htuch added the waiting:any label Apr 12, 2019

repokitteh-read-only bot removed the waiting:any label Apr 12, 2019

htuch added the waiting:any label Apr 15, 2019

fredlas added 2 commits April 16, 2019 18:07

add queuing back

ea842dd

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into DEQ_dequeify_xds

8326c82

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting:any label Apr 16, 2019

fredlas added 2 commits April 17, 2019 13:56

add unwatched to dictionary

9702684

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into DEQ_dequeify_xds

402135b

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch suggested changes Apr 17, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 17, 2019

fredlas added 3 commits April 18, 2019 13:42

comments and formatting

83fae55

Signed-off-by: Fred Douglas <fredlas@google.com>

better grouping of functions

5714c48

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into DEQ_dequeify_xds

57eafa3

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting label Apr 18, 2019

typo

1d1e024

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch approved these changes Apr 19, 2019

View reviewed changes

htuch merged commit 3abb7d0 into envoyproxy:master Apr 19, 2019

fredlas deleted the DEQ_dequeify_xds branch May 1, 2019 20:53

Conversation

fredlas commented Apr 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredlas commented Apr 16, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredlas commented Apr 18, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants