test: adding an integration test framework for file-based LDS by alyssawilk · Pull Request #6933 · envoyproxy/envoy

alyssawilk · 2019-05-14T14:26:13Z

Risk Level: low (test only)
Testing: new LDS test
Docs Changes: n/a
Release Notes: n/a
#5624

alyssawilk · 2019-05-14T14:27:02Z

@htuch this is what I had in mind for reload/rollback tests. File-based just because IMO it's easier to manage than an extra upstream, but we could do the work to move it over to gRPC instead.

@danzh2010 mind taking a look for how readable this is to someone not steeped in xDS? :-)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

Cool; I actually like that this is file-system based, way easier to debug and no network ick. I'd be keen to see this decoupled from xds_integation_test and demoed in a more standalone way, e.g. with the regular HTTP integration tests. The idea here I gather is that we're not creating better xDS integration tests, but instead making it easy for any integration test to benefit from dynamic config changes.

htuch · 2019-05-14T21:32:43Z

test/config/utility.h

  const envoy::config::bootstrap::v2::Bootstrap& bootstrap() { return bootstrap_; }

+  // Allow a finalized configuration to be edited for generating xDS responses
+  void allowFurtherEdits() { finalized_ = false; }


Can we do something less stateful here? I'm thinking specifically that it's easier to reason about an immutable bootstrap and then immutable updates to this. Why does ConfigHelper need to be edited, could we have a second ConfigHelper instantiated independently? Can we decompose ConfigHelper into parts that deal with downstream (LDS) and upstream (CDS)?

htuch · 2019-05-14T21:41:24Z

test/integration/xds_integration_test.cc

+  initialize();
+  fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
+  const std::string lds_filename =
+      config_helper_.bootstrap().dynamic_resources().lds_config().path();


Can we factor this kind of boiler plate outside of the individual tests. Ideally there are more declarative and have the file naming and renaming handled by some method which is just applyUpdateConfig() or something in the test fixture.

htuch · 2019-05-14T21:42:11Z

test/integration/xds_integration_test.cc

+                             {Http::CodecClient::Type::HTTP1}, {FakeHttpConnection::Type::HTTP1})),
+                         HttpProtocolIntegrationTest::protocolTestParamsToString);
+
+// Sample test making sure our config framework correctly reloads listeners.


Yeah, this is already somewhat covered by https://github.com/envoyproxy/envoy/blob/master/test/integration/ads_integration_test.cc#L658 and other xDS integration tests, but if the idea is to show off the new config helper features, this works.

danzh2010 · 2019-05-14T19:46:00Z

test/integration/integration.cc

+    for (auto& listener : config_helper_.bootstrap().static_resources().listeners()) {
+      ProtobufWkt::Any* resource = lds.add_resources();
+      resource->PackFrom(listener);
+    }


use createLdsResponse() instead?

danzh2010 · 2019-05-14T21:45:36Z

test/integration/xds_integration_test.cc

+      "new_lds_file", MessageUtil::getJsonStringFromMessage(lds));
+  TestUtility::renameFile(file, lds_filename);
+
+  test_server_->waitForCounterGe("listener_manager.lds.update_success", 2);


Is listener_manager.lds.update_success == 1 after initialize()? If so, can we add an EXPECT there?

danzh2010 · 2019-05-14T21:51:35Z

test/integration/integration.cc

+                                              true);
+
+    // Now that the listeners have been written to the lds file, remove them from static resources
+    // or they will not be reloadable.


I'm curious how does static_resources() get reloaded?

Never. If you put something in static resources it's loaded into config with a "non-modifiable" tag and all updates to that listener are disregarded. Which I only discovered when debugging this test :-P

Sorry, what I mean is after clearing static resources, how does a modified listener config in file get reloaded?

Ah, gotcha.
So by sticking the filename in the dynamic config, we say "config lives here"
There's an initial read on start-up, and then any time a new file is moved there, Envoy will reread and (generally) reload.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-05-15T15:17:46Z

Yep, the goal is to make it really easy to test reloadable config in any of the integration tests.
I stuck the actual new sample test in xds_integration test as you say as an example (and to debug my code as the first draft had some issues)
I'd like us to have reloadable config by default for new DSes and I'll try to extend our framework to the other existing DSes as we have time.
I think as we add more DSes we may evolve the utilities but this is at least a start

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-05-15T17:19:29Z

test/extensions/filters/http/fault/fault_filter_integration_test.cc

 public:
  void initializeFilter(const std::string& filter_config) {
+    // There's some deadlock issue with simtime where the alarm may be scheduled
+    // and will not fire. Waiting for the injected delay appears to help.


This is fixed by #6956 - I'll re-enable it when either this PR or that one lands.

If in one event loop, two things tried to post to a dispatcher, the dispatcher would (multiple times) set a 0 ms timer. For both instances, simtime would see the duration was 0, call activateLockHeld, increment pending alarms and set the base alarm to fire. The base alarm would only fire once though, so the pending alarms would decrement once, pending would be true for ever and the whole system would "deadlock" waiting for pending to decrease to 0. The symptom was the fault filter alarm being set to 200ms in #6933 but because of the extra dispatcher post triggering this deadlock the test would spin forever. Risk Level: low (test only) Testing: new unit test, verified fault filter test now works in the LDS PR Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

LGTM modulo one comment.
/wait

test/integration/xds_integration_test.cc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

LGTM

* master: (65 commits) proto: Add PATCH method to RequestMethod enum (envoyproxy#6737) exe: drop unused deps on zlib compressor code (envoyproxy#7022) coverage: fix some misc coverage (envoyproxy#7033) Enable proto schema for router_check_tool (envoyproxy#6992) stats: rework stat sink flushing to centralize counter latching (envoyproxy#6996) [test] convert lds api test config stubs to v2 (envoyproxy#7021) router: scoped rds (2c): implement scoped rds API (envoyproxy#6932) build: Add option for size-optimized binary (envoyproxy#6960) test: adding an integration test framework for file-based LDS (envoyproxy#6933) doc: update obsolete ref to api/XDS_PROTOCOL.md (envoyproxy#7002) dispatcher: faster runOnAllThreads (envoyproxy#7011) example: add csrf sandbox (envoyproxy#6805) fix syntax of gcov exclusion zone. (envoyproxy#7023) /runtime_modify: add support for query params in body (envoyproxy#6977) stats: Create stats for http codes with the symbol table. (envoyproxy#6733) health check: fix more fallout from inline deletion change (envoyproxy#6988) Max heap fix (envoyproxy#7016) Add support to unregister from lifecycle notifications (envoyproxy#6984) build spdy_core_alt_svc_wire_format (envoyproxy#7010) ext_authz: Make sure initiateCall only called once (envoyproxy#6949) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

alyssawilk assigned htuch and danzh2010 May 14, 2019

test: adding an integration test framework for file-based LDS

b39681a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the xDS branch from 66edbdd to b39681a Compare May 14, 2019 14:31

alyssawilk added 2 commits May 14, 2019 10:51

on by default

a7b7262

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'master' into xDS

e9a1f7e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch suggested changes May 14, 2019

View reviewed changes

htuch added the waiting label May 14, 2019

danzh2010 reviewed May 14, 2019

View reviewed changes

deflaking integration test

2fa11bd

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

reviwer comments

2acc386

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label May 15, 2019

alyssawilk mentioned this pull request May 15, 2019

test: fixing a deadlock in simtime #6956

Merged

alyssawilk commented May 15, 2019

View reviewed changes

alyssawilk added 2 commits May 15, 2019 16:56

Merge branch 'master' into xDS

3bcafb2

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

enabling test now envoyproxy#6956 landed

5860969

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch suggested changes May 16, 2019

View reviewed changes

test/integration/xds_integration_test.cc Show resolved Hide resolved

repokitteh-read-only bot added the waiting label May 16, 2019

Merge branch 'master' into xDS

abc6bb7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

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

alyssawilk added 2 commits May 21, 2019 11:46

Merge branch 'master' into xDS

c0fb0af

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Fix merge flake

827737e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch approved these changes May 22, 2019

View reviewed changes

htuch merged commit 6cfeef7 into envoyproxy:master May 22, 2019

alyssawilk mentioned this pull request May 30, 2019

config: logging LDS file on config failure #7113

Merged

alyssawilk deleted the xDS branch December 9, 2019 21:11

Conversation

alyssawilk commented May 14, 2019

Uh oh!

alyssawilk commented May 14, 2019

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!

alyssawilk commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

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

alyssawilk commented May 15, 2019 •

edited

Loading