ads: pause and resume v3 apis #11300

sschepens · 2020-05-22T22:10:02Z

Pause and resune V3 Api Verions as well as V2 when using ads.

Currently only V2 Api is being paused, this causes envoy to send a separate discovery request for every resource on CDS/LDS/SRDS updates.

Risk Level: Medium?
Testing:
Fixes: #11267

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch

Thanks. Patch looks good but I wonder we should probably leave a TODO to make this a bit more robust by allowing pause/resume of the latest message type and then chain backwards inside the pause/resume implementation to cover earlier versions.

On the test front, this behavior does have tests, e.g. see #1991. Most of the ones that matter in that PR were the integration tests, but it should be possible to do unit/mock tests as well if that's simpler.
/wait

sschepens · 2020-05-26T19:33:16Z

@htuch i'd like to make integration tests with v3, i was thinking about adding a parameter the integration tests (as with delta vs sotw) and making the whole suit run with v2 and v3. Does that seem correct?

htuch · 2020-05-27T00:30:21Z

@sschepens the tests are pretty slow, so if we do this for all the ADS integration tests, that might be a bit much. You can parameterize on v2/v3 resource type for just a pause/resume test, subclassing AdsIntegrationTest.

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens · 2020-06-01T20:05:33Z

@htuch sorry for the delay, had a hard time understanding why the test i created was failing, had to duplicate some logic to be able to send v3 messages in integration tests, don't know if that's ok though

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch

Thanks, a few comments. I think it's going to be very desirable to avoid the copy+paste.
/wait

source/common/router/scoped_rds.cc

source/common/upstream/cluster_manager_impl.cc

test/integration/ads_integration_test.cc

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

howardjohn · 2020-06-09T04:11:45Z

fixes #11512

Thanks!

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens · 2020-06-09T22:48:06Z

@htuch clang_tidy is complaining about the case for the static function AdsIntegrationConfig i'll lowercase the first letter to fix this

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch

Yep, the approach taken here looks good. Would be good to figure out if we can get rid of some of the remaining boiler plate in AdsClusterV3 test, but this is on the right track IMHO.

include/envoy/config/grpc_mux.h

source/common/upstream/cluster_manager_impl.cc

test/integration/ads_integration.cc

htuch · 2020-06-10T00:04:55Z

test/integration/ads_integration_test.cc

+    sendDeltaDiscoveryResponse(type_url, added_or_updated, removed, version, xds_stream_);
+  }
+
+  template <class T>


Why are these templated? Aren't they all v3? Do we need these wrappers?

These are templated because they receive any type of proto message to encode in the response.
I needed to create these to enforce V3 DiscoveryResponses

I'm still a bit confused; these belong to AdsClusterV3Test, which is only doing v3 AFAICT. So, why do they need templating?

The templating is not for the api version but for the resource type this methods receive clusters, cluster load assignements, routes, etc.

I'm still wondering if we really need these. There is a transport/resource version distinction in xDS, here we only care about v3 xDS resource API version, there should be no need to have specialized methods for the v3 API transport to validate the pause/resume behavior.

the reason i added these is to override the default resource version downgrading here and here

i could probably make those two methods virtual and override only those, would that be ok?

Sure, or even just parameterize the downgrade in the function args.

test/integration/ads_integration_test.cc

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch

Other than the thread on ADS integration test, this looks awesome and ready to ship! Thanks for iterating.

htuch · 2020-06-15T01:36:34Z

test/integration/ads_integration_test.cc

+    sendDeltaDiscoveryResponse(type_url, added_or_updated, removed, version, xds_stream_);
+  }
+
+  template <class T>


I'm still wondering if we really need these. There is a transport/resource version distinction in xDS, here we only care about v3 xDS resource API version, there should be no need to have specialized methods for the v3 API transport to validate the pause/resume behavior.

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens · 2020-06-16T19:43:24Z

@htuch addressed the last comment and added another batching test, let me know if that's ok

htuch

Awesome! Thanks @sschepens for iterating on this. This will have a real benefit to users of v3.

source/server/lds_api.cc

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

Pause and resune V3 Api Verions as well as V2 when using ads. Currently only V2 Api is being paused, this causes envoy to send a separate discovery request for every resource on CDS/LDS/SRDS updates. Risk Level: Medium? Testing: Fixes: envoyproxy#11267 Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

Pause and resune V3 Api Verions as well as V2 when using ads. Currently only V2 Api is being paused, this causes envoy to send a separate discovery request for every resource on CDS/LDS/SRDS updates. Risk Level: Medium? Testing: Fixes: envoyproxy#11267 Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

Add mocks to MockGrpcMuxWatch. follow-up to #11300. Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

Pause and resune V3 Api Verions as well as V2 when using ads. Currently only V2 Api is being paused, this causes envoy to send a separate discovery request for every resource on CDS/LDS/SRDS updates. Risk Level: Medium? Testing: Fixes: envoyproxy#11267 Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

Add mocks to MockGrpcMuxWatch. follow-up to envoyproxy#11300. Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

ads: pause and resume v3 apis

82235e3

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens mentioned this pull request May 22, 2020

ads: Envoy sending DeltaDiscoveryRequest for each EDS resource #11267

Closed

fix scoped rds lamba parameters

d218f6a

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch self-assigned this May 24, 2020

htuch suggested changes May 24, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label May 24, 2020

sschepens added 2 commits June 1, 2020 16:56

Merge remote-tracking branch 'upstream/master' into ads-pause-unpause-v3

ac528b9

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

add test

395f9f9

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

repokitteh-read-only bot removed the waiting label Jun 1, 2020

fix method case

fe22a4e

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch suggested changes Jun 2, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 2, 2020

add pause/resume vector overrides

da1316e

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

repokitteh-read-only bot removed the waiting label Jun 5, 2020

make resume non virtual

91c6199

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch added the waiting label Jun 8, 2020

mattklein123 mentioned this pull request Jun 9, 2020

xds v3 regression: spammy EDS/RDS requests only on v3 #11512

Closed

make ads integration test parametrizable api version

fea8b65

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

repokitteh-read-only bot removed the waiting label Jun 9, 2020

fix method case

f6ef9f3

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch reviewed Jun 10, 2020

View reviewed changes

sschepens added 4 commits June 12, 2020 12:30

push pause/resume to impls

0d1e885

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

rename test

ccce5cc

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

refactor boosting logic to method

435561f

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

add paused invariant with vec of type urls

047d0c2

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch reviewed Jun 15, 2020

View reviewed changes

sschepens added 2 commits June 16, 2020 16:21

parametrize api downgrading

fcc3672

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

add v3 batching test

26651f3

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

htuch previously approved these changes Jun 16, 2020

View reviewed changes

ramaraochavali reviewed Jun 17, 2020

View reviewed changes

source/server/lds_api.cc Outdated Show resolved Hide resolved

rename variable

5909b0a

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens dismissed htuch’s stale review via 5909b0a June 17, 2020 14:13

htuch approved these changes Jun 17, 2020

View reviewed changes

htuch merged commit 9aa85a3 into envoyproxy:master Jun 17, 2020

sschepens deleted the ads-pause-unpause-v3 branch June 17, 2020 18:35

samflattery mentioned this pull request Jun 17, 2020

utility: refactor utility functions in ads integration test #11588

Merged

yuval-k mentioned this pull request Jun 30, 2020

test: add missing mocks to fix test compile error with gcc 9 #11827

Merged

ggreenway pushed a commit that referenced this pull request Jul 6, 2020

fix building tests with gcc (#11827)

91592cd

Add mocks to MockGrpcMuxWatch. follow-up to #11300. Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ads: pause and resume v3 apis #11300

ads: pause and resume v3 apis #11300

sschepens commented May 22, 2020

htuch left a comment

sschepens commented May 26, 2020

htuch commented May 27, 2020

sschepens commented Jun 1, 2020

htuch left a comment

howardjohn commented Jun 9, 2020

sschepens commented Jun 9, 2020

htuch left a comment

htuch Jun 10, 2020

sschepens Jun 10, 2020

htuch Jun 11, 2020

sschepens Jun 12, 2020

htuch Jun 15, 2020

sschepens Jun 16, 2020

sschepens Jun 16, 2020

htuch Jun 16, 2020

htuch left a comment

htuch Jun 15, 2020

sschepens commented Jun 16, 2020

htuch left a comment

ads: pause and resume v3 apis #11300

ads: pause and resume v3 apis #11300

Conversation

sschepens commented May 22, 2020

htuch left a comment

Choose a reason for hiding this comment

sschepens commented May 26, 2020

htuch commented May 27, 2020

sschepens commented Jun 1, 2020

htuch left a comment

Choose a reason for hiding this comment

howardjohn commented Jun 9, 2020

sschepens commented Jun 9, 2020

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschepens commented Jun 16, 2020

htuch left a comment

Choose a reason for hiding this comment