config: improve temporary delta xDS choosing mechanism by fredlas · Pull Request #8587 · envoyproxy/envoy

fredlas · 2019-10-11T15:09:22Z

The is_delta temporary param breaks things when ADS is configured as state-of-the-world, and then a subscription is called with is_delta true. This PR replaces is_delta with an (also temporary) isDelta() function on GrpcMux, which is the real source of truth on whether it's safe to use a DeltaSubscriptionImpl for a new ADS subscription. All of this will be removed by #8478.

Risk Level: low

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

fredlas · 2019-10-11T15:47:17Z

/assign stevenzzzz

stevenzzzz · 2019-10-11T16:03:31Z

could you add some tests? Also since the is_delta is not used, maybe we can do a cleanup to remove that (temp) parameter from the function definition.

stevenzzzz · 2019-10-11T16:03:59Z

loop in htuch@ for Senior maintainer approval.

fredlas · 2019-10-11T16:44:03Z

Removal of is_delta in progress; this reduces the file changed count in #8478, so that's nice.

I understand the desire to add tests for something that was broken, but I think this one is an awkward situation. The brokenness is because #7293 got directly merged, rather than getting a sort of "checkpoint" approval and then merging in what is now #8478. It's brokenness due to multiple versions of the same code existing at once. With #8478 there will only be one version left. So, it won't just be "impossible" for this problem to recur, the problem will actually no longer be a meaningfully existing concept.

stevenzzzz · 2019-10-11T17:52:26Z

Removal of is_delta in progress; this reduces the file changed count in #8478, so that's nice.

I understand the desire to add tests for something that was broken, but I think this one is an awkward situation. The brokenness is because #7293 got directly merged, rather than getting a sort of "checkpoint" approval and then merging in what is now #8478. It's brokenness due to multiple versions of the same code existing at once. With #8478 there will only be one version left. So, it won't just be "impossible" for this problem to recur, the problem will actually no longer be a meaningfully existing concept.

can we cleanup the is_delta parameter? it's not used, so probably just some quick deletion work.

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

fredlas · 2019-10-11T18:34:46Z

is_delta removed.

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

stevenzzzz · 2019-10-14T12:59:41Z

LGTM.

stevenzzzz · 2019-10-14T14:31:20Z

/retest

repokitteh-read-only · 2019-10-14T14:31:22Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8587 (comment) was created by @stevenzzzz.

see: more, trace.

fredlas · 2019-10-14T14:31:34Z

/retest

repokitteh-read-only · 2019-10-14T14:31:37Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8587 (comment) was created by @fredlas.

see: more, trace.

fredlas · 2019-10-14T14:32:12Z

The envoy-macos one appears to be a flake.

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

fredlas · 2019-10-15T14:54:55Z

Now envoy-macos passed, but envoy-linux failed, with a different test failing. So, I think it's safe to conclude those are flakes.

stevenzzzz · 2019-10-17T14:32:15Z

ping on this PR.
@htuch could you please take a look?

lambdai

For ADS: the singleton ads config determine the delta or not, not per ClusterConfig/ListenerConfig.
For non-ADS: config source determine if it's delta or not since it has its own mux

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

…equirement is really on a knife edge... Signed-off-by: Fred Douglas <fredlas@google.com>

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

fredlas · 2019-10-18T18:52:38Z

I'm not sure how the source/common/config/api_type_db.generated.pb_text changes got into this PR. Should I remove them?

stevenzzzz · 2019-10-18T18:55:12Z

/assign htuch
PTAL.

htuch

Thanks for the fix. Please revert the API type database stuff, this is a bug that I need to fix, it shouldn't be doing that if there are no API changes. Just one comment; I hadn't really though much about mixed delta/SOTW until seeing this PR.
/wait

htuch · 2019-10-18T21:38:44Z

include/envoy/config/grpc_mux.h


+  // TODO(fredlas) PR #8478 will remove this.
+  /**
+   * Whether this GrpcMux is delta.


What's the end goal for supporting a combination of SOTW and delta? I would imagine that some operators will want both, with let's say CDS delta but LDS SOTW. Can that be done over a single gRPC stream?

Yes and no and maybe eventually yes, haha. Non-aggregated xDS can already mix and match delta and SotW no problem. When using ADS, because delta and SotW have different request/response protos, and different gRPC services corresponding to those protos, there can only be one type.

However, the change we briefly discussed a while back, where there would be a single top level carrier proto for request+response, and the various types of messages (delta vs SotW, non-ACK request vs ACK) were just different oneof options, would enable mixing within ADS. In practice that would take a bit of plumbing in this code (probably pretty straightforward), and would also need the bootstrap config format to be reworked a bit: rather than just configuring your CDS as "CDS, provided by ADS [implicitly referring to the ADS config]", you would also need to choose delta/SotW in the CDS config thing. And... actually, the choice would be removed from the ADS config thing.

Ack. OK, let's go ahead with this as is, thanks for the reminder of our earlier conversation on this.

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

htuch

Thanks!

fredlas · 2019-10-22T14:12:23Z

Great, thanks very much, this is much cleaner, not to mention fixed. (And thanks to @stevenzzzz for bringing it to my attention)

The is_delta temporary param breaks things when ADS is configured as state-of-the-world, and then a subscription is called with is_delta true. This PR replaces is_delta with an (also temporary) isDelta() function on GrpcMux, which is the real source of truth on whether it's safe to use a DeltaSubscriptionImpl for a new ADS subscription. All of this will be removed by envoyproxy#8478. Risk Level: low Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas added 2 commits October 11, 2019 10:40

add temporary isDelta getter

20239c3

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

replace is_delta with isDelta

f65051d

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

repokitteh-read-only bot assigned stevenzzzz Oct 11, 2019

fredlas added 2 commits October 11, 2019 14:26

remove is_delta

6c408b0

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

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

051e3f9

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

fredlas requested review from alyssawilk and mattklein123 as code owners October 11, 2019 18:26

ahhhh shoulda known i would need to run a test/...

1fb26a6

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

fredlas mentioned this pull request Oct 11, 2019

config: unification of delta and SotW gRPC xDS #8478

Merged

fredlas changed the title ~~config: improve delta xDS choosing temporary mechanism~~ config: improve temporary delta xDS choosing mechanism Oct 14, 2019

Kick CI

3fee2f0

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

lambdai previously approved these changes Oct 17, 2019

View reviewed changes

fredlas added 3 commits October 18, 2019 09:37

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

ef593d6

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

test coverage for two return const value functions lol the coverage r…

4286c1c

…equirement is really on a knife edge... Signed-off-by: Fred Douglas <fredlas@google.com>

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

f4ac92a

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

fredlas dismissed lambdai’s stale review via f4ac92a October 18, 2019 16:24

repokitteh-read-only bot assigned htuch Oct 18, 2019

htuch suggested changes Oct 18, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 18, 2019

merge conflict

98da219

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

repokitteh-read-only bot removed the waiting label Oct 21, 2019

restore exactly the api_type_db copy currently living in master

174fb4f

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

htuch approved these changes Oct 22, 2019

View reviewed changes

htuch merged commit b261cd5 into envoyproxy:master Oct 22, 2019

fredlas mentioned this pull request Nov 7, 2019

DELTA_GRPC used for GRPC api type #8927

Closed

Conversation

fredlas commented Oct 11, 2019

Uh oh!

fredlas commented Oct 11, 2019

Uh oh!

stevenzzzz commented Oct 11, 2019

Uh oh!

stevenzzzz commented Oct 11, 2019

Uh oh!

fredlas commented Oct 11, 2019

Uh oh!

stevenzzzz commented Oct 11, 2019

Uh oh!

fredlas commented Oct 11, 2019

Uh oh!

stevenzzzz commented Oct 14, 2019

Uh oh!

stevenzzzz commented Oct 14, 2019

Uh oh!

repokitteh-read-only bot commented Oct 14, 2019

Uh oh!

fredlas commented Oct 14, 2019

Uh oh!

repokitteh-read-only bot commented Oct 14, 2019

Uh oh!

fredlas commented Oct 14, 2019

Uh oh!

fredlas commented Oct 15, 2019

Uh oh!

stevenzzzz commented Oct 17, 2019

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

fredlas commented Oct 18, 2019

Uh oh!

stevenzzzz commented Oct 18, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

fredlas Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

fredlas commented Oct 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants