config: fix update empty stat for eds #4276

ramaraochavali · 2018-08-28T08:40:24Z

Description:
Fixes incorrect increment of update_empty stat for EDS when resource list contain does contain a particular cluster
Risk Level: Low
Testing: Automated tests
Docs Changes: N/A
Release Notes: N/A
Fixes #4241

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-08-28T13:07:35Z

source/common/config/grpc_mux_impl.cc

@@ -206,6 +206,9 @@ void GrpcMuxImpl::onReceiveMessage(std::unique_ptr<envoy::api::v2::DiscoveryResp
      resources.emplace(resource_name, resource);
    }
    for (auto watch : api_state_[type_url].watches_) {
+      // onConfigUpdate should be called in all cases for single watch xDS (Cluster and Listener)
+      // even if the message does not have resources so that update_empty stat is properly
+      // incremented.
      if (watch->resources_.empty()) {


@htuch single watch xDS case is already handled here. So just added a comment to be clear. Also added a test which validates that.

htuch · 2018-08-28T14:37:22Z

source/common/config/grpc_mux_impl.cc

@@ -217,7 +220,12 @@ void GrpcMuxImpl::onReceiveMessage(std::unique_ptr<envoy::api::v2::DiscoveryResp
          found_resources.Add()->MergeFrom(it->second);
        }
      }
-      watch->callbacks_.onConfigUpdate(found_resources, message->version_info());
+      // onConfigUpdate should be called for all watches only if message has no resources for multi
+      // watch xDS (ClusterLoadAssignment and RouteConfiguration). Otherwise only call on


I think this behavior is a bit inconsistent; you don't see empty updates unless all the other EDS updates are empty, this makes per-cluster stats non-local, since they depend on other cluster EDS updates. I would prefer we just have the cluster stats empty be always zero for RDS/EDS.

Yeah. Was thinking the same when I put that condition. Will change it.

Signed-off-by: Rama <[email protected]>

htuch · 2018-08-28T15:30:57Z

test/common/config/grpc_mux_impl_test.cc

@@ -198,8 +198,6 @@ TEST_F(GrpcMuxImplTest, TypeUrlMismatch) {
 // Validate behavior when watches has an unknown resource name.
 TEST_F(GrpcMuxImplTest, WildcardWatch) {
  setup();
-
-  InSequence s;


Why remove this?

Sorry. This was not intentional. Some merge issue master looks like

htuch · 2018-08-28T15:31:23Z

test/common/config/grpc_mux_impl_test.cc

@@ -251,9 +248,6 @@ TEST_F(GrpcMuxImplTest, WatchDemux) {
    envoy::api::v2::ClusterLoadAssignment load_assignment;
    load_assignment.set_cluster_name("x");
    response->add_resources()->PackFrom(load_assignment);
-    EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "1"))


Could make this a .Times(0) to make clear it's not expected.

htuch · 2018-08-28T15:32:40Z

test/common/config/grpc_mux_impl_test.cc

@@ -311,6 +305,31 @@ TEST_F(GrpcMuxImplTest, WatchDemux) {
  expectSendMessage(type_url, {}, "2");
 }

+// Validate behavior when Cluster sends empty updates.


I would phrase it a bit differently; we want one test when there is a single watcher with empty updates and another test when we have multiple watchers with an empty update.

htuch · 2018-08-28T15:33:25Z

source/common/config/grpc_mux_impl.cc

@@ -206,6 +206,9 @@ void GrpcMuxImpl::onReceiveMessage(std::unique_ptr<envoy::api::v2::DiscoveryResp
      resources.emplace(resource_name, resource);
    }
    for (auto watch : api_state_[type_url].watches_) {
+      // onConfigUpdate should be called in all cases for single watch xDS (Cluster and Listener)
+      // even if the message does not have resources so that update_empty stat is properly
+      // incremented.


properly incremented and state-of-the-world semantics are maintained.

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-08-28T17:11:53Z

@htuch added the additional test. PTAL.

htuch

Looks good, thanks, just once thing.

htuch · 2018-08-28T21:11:40Z

test/common/config/grpc_mux_impl_test.cc

+  expectSendMessage(type_url, {"x", "y"}, "");
+  grpc_mux_->start();
+
+  {


What's the reason for these block constructs? They are used elsewhere in the file for forcing subscription objects to be destroyed, do we need them in these new tests?

There is no specific reason other than follow the same pattern of having subscription in a block construct. Will remove them

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-08-29T03:38:41Z

@htuch removed the block constructs. PTAL

htuch

Thanks!

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <[email protected]>

…d because of envoyproxy#4485 via envoyproxy#4490 Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali added 3 commits August 28, 2018 13:53

update empty stat fix

4a29d7a

Signed-off-by: Rama <[email protected]>

resolve conflicts

ecc6188

Signed-off-by: Rama <[email protected]>

fix ads integration test

981914f

Signed-off-by: Rama <[email protected]>

ramaraochavali commented Aug 28, 2018

View reviewed changes

dnoe assigned dio Aug 28, 2018

htuch reviewed Aug 28, 2018

View reviewed changes

htuch self-assigned this Aug 28, 2018

review feedback

e2c49d9

Signed-off-by: Rama <[email protected]>

htuch reviewed Aug 28, 2018

View reviewed changes

add one more test

d8c8196

Signed-off-by: Rama <[email protected]>

htuch suggested changes Aug 28, 2018

View reviewed changes

remove block constructs

f51d592

Signed-off-by: Rama <[email protected]>

htuch approved these changes Aug 29, 2018

View reviewed changes

htuch merged commit f952033 into envoyproxy:master Aug 29, 2018

htuch mentioned this pull request Sep 20, 2018

Empty EDS prevents LDS and RDS requests #4485

Closed

ramaraochavali deleted the fix/eds_update_empty branch September 21, 2018 02:26

ramaraochavali mentioned this pull request Feb 26, 2019

cluster: copy active cluster hosts during warming #6021

Closed

ramaraochavali added a commit to ramaraochavali/envoy that referenced this pull request Mar 4, 2019

brings back the behaviour added in envoyproxy#4276, which was reverte…

7ba54b9

…d because of envoyproxy#4485 via envoyproxy#4490 Signed-off-by: Rama Chavali <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: fix update empty stat for eds #4276

config: fix update empty stat for eds #4276

ramaraochavali commented Aug 28, 2018

ramaraochavali Aug 28, 2018

htuch Aug 28, 2018

ramaraochavali Aug 28, 2018

htuch Aug 28, 2018

ramaraochavali Aug 28, 2018

htuch Aug 28, 2018

htuch Aug 28, 2018

htuch Aug 28, 2018

ramaraochavali commented Aug 28, 2018

htuch left a comment

htuch Aug 28, 2018

ramaraochavali Aug 29, 2018

ramaraochavali commented Aug 29, 2018

htuch left a comment

config: fix update empty stat for eds #4276

config: fix update empty stat for eds #4276

Conversation

ramaraochavali commented Aug 28, 2018

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

ramaraochavali commented Aug 28, 2018

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

ramaraochavali commented Aug 29, 2018

htuch left a comment

Choose a reason for hiding this comment