Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: allow empty delta update #12699

Merged
merged 2 commits into from
Sep 11, 2020
Merged

Conversation

kkHAIKE
Copy link
Contributor

@kkHAIKE kkHAIKE commented Aug 18, 2020

Commit Message:
when delta xds recv empty repsonce, it should be call onConfigUpdate, instead of do nothing, and stop init process 15s (initial_fetch_timeout)

Additional Description:
a first delta xds repsonce can't call wildcard subscription's onConfigUpdate (ex: empty CDS), and stop init process 15s. It don't like SOTW part:

if (this_watch_updates == per_watch_updates.end()) {
// This update included no resources this watch cares about.
// 1) If there is only a single, wildcard watch (i.e. Cluster or Listener), always call
// its onConfigUpdate even if just a no-op, to properly maintain state-of-the-world
// semantics and the update_empty stat.
// 2) If this watch previously had some resources, it means this update is removing all
// of this watch's resources, so the watch must be informed with an onConfigUpdate.
// 3) Otherwise, we can skip onConfigUpdate for this watch.
if (map_is_single_wildcard || !watch->state_of_the_world_empty_) {
watch->state_of_the_world_empty_ = true;
watch->callbacks_.onConfigUpdate({}, version_info);
}

Risk Level: Low
Testing: Manual Pass
Docs Changes: None
Release Notes: None

@kkHAIKE kkHAIKE force-pushed the empty_delta_response branch from f13846e to 95cf8c4 Compare August 18, 2020 03:42
@lizan lizan requested a review from htuch August 18, 2020 11:09
Signed-off-by: ouyangxu <[email protected]>
@kkHAIKE
Copy link
Contributor Author

kkHAIKE commented Aug 29, 2020

ping @htuch 😄

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; if you really want to future proof this you could also add an integration test, but don't feel super strong on that. One tiny nit.
/wait

test/common/config/new_grpc_mux_impl_test.cc Outdated Show resolved Hide resolved
Signed-off-by: ouyangxu <[email protected]>
@stale
Copy link

stale bot commented Sep 11, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 11, 2020
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Sep 11, 2020
@htuch
Copy link
Member

htuch commented Sep 11, 2020

LGTM, thanks!

@htuch htuch merged commit 0185c1c into envoyproxy:master Sep 11, 2020
lhluo pushed a commit to lhluo/envoy that referenced this pull request Sep 11, 2020
…code

* upstream/master:
  lint: add more linters for using absl:: over std:: (envoyproxy#13043)
  udpa: filesystem list collection support for inline entries. (envoyproxy#13028)
  filter: http: jwt: implement matching for HTTP CONNECT (envoyproxy#13064)
  [fuzz] split http filter logic into a fuzzing class (envoyproxy#13016)
  xds: allow empty delta update (envoyproxy#12699)
  CacheFilter: parses the allowed_vary_headers from the cache config. (envoyproxy#12928)
  router: extend HTTP CONNECT route matching criteria (envoyproxy#13056)
  docs: clarify use of Extended CONNECT for h/2 (envoyproxy#13051)
  build: shellcheck tools/ (envoyproxy#13007)
  [fuzz] Refactored Health Checker Impl Tests (envoyproxy#13017)

Signed-off-by: Lihao Luo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants