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

config: WatchMap: cleaner management of watches #7108

Merged
merged 40 commits into from
Aug 13, 2019
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
2ff7a0d
initial WatchMap snapshot
fredlas May 29, 2019
502e3b7
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas May 29, 2019
3816de1
cleanup and comments
fredlas May 29, 2019
2d7a247
compiles and a basic test passes
fredlas May 29, 2019
d823320
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas May 29, 2019
d30e229
add two more tests, and fix a bug, thanks unit testing
fredlas May 29, 2019
2ce83f4
test delta too
fredlas May 30, 2019
df14958
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas May 30, 2019
33aff0f
tiny rearrangement
fredlas May 30, 2019
67dcc67
spellcheck
fredlas May 30, 2019
2ac1518
unmock resourceName to enable switching off of NiceMock
fredlas May 30, 2019
fdb0a18
add a test, fix another bug, thanks again unit testing
fredlas May 30, 2019
21f428a
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas May 30, 2019
38f13cc
support watches that want to watch everything by providing no names
fredlas Jun 6, 2019
b8b7b08
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas Jun 6, 2019
eb0f380
fix compile after merge
fredlas Jun 6, 2019
00f7f55
snapshot
fredlas Jun 7, 2019
943949a
change std pair to AddedRemoved
fredlas Jun 7, 2019
c61a4aa
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas Jun 7, 2019
5582614
remove virtual
fredlas Jun 14, 2019
3ac614e
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas Jun 14, 2019
a8d53c2
add PNG diagram of intended usage of WatchMap
fredlas Jun 17, 2019
e571c53
add interface, move WatchMap to WatchMapImpl
fredlas Jun 17, 2019
db4fd7a
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas Jun 17, 2019
3ce07f4
privatize SubscriptionCallbacks
fredlas Jun 17, 2019
34dd5ce
move AddedRemoved, move png
fredlas Jul 1, 2019
885bc2c
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas Jul 1, 2019
ea4a01f
private friend Watch
fredlas Jul 9, 2019
37ffff6
clang tidy
fredlas Jul 9, 2019
4900eb9
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas Jul 9, 2019
c238f1f
change Watch to class and interface
fredlas Jul 15, 2019
46e3fa9
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas Jul 15, 2019
8010950
add another word to the dictionary
fredlas Jul 15, 2019
8df252b
back to earlier design
fredlas Jul 31, 2019
76bd613
merge conflict, realphabetize BUILD
fredlas Jul 31, 2019
4dba42d
clang tidy
fredlas Aug 7, 2019
692e982
Merge remote-tracking branch 'upstream/master' into WAT_watch
fredlas Aug 7, 2019
8a4ae35
merge conflict
fredlas Aug 7, 2019
064fe75
merge conflict
fredlas Aug 7, 2019
3a1d2ba
snapshot
fredlas Aug 7, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions include/envoy/config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "watch_map_interface",
hdrs = ["watch_map.h"],
deps = [
":subscription_interface",
],
)

envoy_cc_library(
name = "xds_grpc_context_interface",
hdrs = ["xds_grpc_context.h"],
Expand Down
1 change: 1 addition & 0 deletions include/envoy/config/grpc_mux.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ struct ControlPlaneStats {
ALL_CONTROL_PLANE_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT)
};

// TODO(fredlas) redundant to SubscriptionCallbacks; remove this one.
class GrpcMuxCallbacks {
public:
virtual ~GrpcMuxCallbacks() = default;
Expand Down
102 changes: 57 additions & 45 deletions source/common/config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,20 @@ envoy_cc_library(
)

envoy_cc_library(
name = "filesystem_subscription_lib",
srcs = ["filesystem_subscription_impl.cc"],
hdrs = ["filesystem_subscription_impl.h"],
name = "config_provider_lib",
srcs = ["config_provider_impl.cc"],
hdrs = ["config_provider_impl.h"],
deps = [
"//include/envoy/config:subscription_interface",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/filesystem:filesystem_interface",
"//source/common/common:minimal_logger_lib",
"//source/common/config:utility_lib",
":utility_lib",
"//include/envoy/config:config_provider_interface",
"//include/envoy/config:config_provider_manager_interface",
"//include/envoy/init:manager_interface",
"//include/envoy/server:admin_interface",
"//include/envoy/server:config_tracker_interface",
"//include/envoy/singleton:instance_interface",
"//include/envoy/thread_local:thread_local_interface",
"//source/common/init:target_lib",
"//source/common/protobuf",
"//source/common/protobuf:message_validator_lib",
"//source/common/protobuf:utility_lib",
],
)

Expand Down Expand Up @@ -100,6 +102,22 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "filesystem_subscription_lib",
srcs = ["filesystem_subscription_impl.cc"],
hdrs = ["filesystem_subscription_impl.h"],
deps = [
"//include/envoy/config:subscription_interface",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/filesystem:filesystem_interface",
"//source/common/common:minimal_logger_lib",
"//source/common/config:utility_lib",
"//source/common/protobuf",
"//source/common/protobuf:message_validator_lib",
"//source/common/protobuf:utility_lib",
],
)

envoy_cc_library(
name = "grpc_stream_lib",
hdrs = ["grpc_stream.h"],
Expand Down Expand Up @@ -265,12 +283,6 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "resources_lib",
hdrs = ["resources.h"],
deps = ["//source/common/singleton:const_singleton"],
)

envoy_cc_library(
name = "rds_json_lib",
srcs = ["rds_json.cc"],
Expand All @@ -288,6 +300,25 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "resources_lib",
hdrs = ["resources.h"],
deps = ["//source/common/singleton:const_singleton"],
)

envoy_cc_library(
name = "remote_data_fetcher_lib",
srcs = ["remote_data_fetcher.cc"],
hdrs = ["remote_data_fetcher.h"],
deps = [
"//include/envoy/upstream:cluster_manager_interface",
"//source/common/common:hex_lib",
"//source/common/crypto:utility_lib",
"//source/common/http:utility_lib",
"@envoy_api//envoy/api/v2/core:http_uri_cc",
],
)

envoy_cc_library(
name = "runtime_utility_lib",
srcs = ["runtime_utility.cc"],
Expand Down Expand Up @@ -361,42 +392,23 @@ envoy_cc_library(
)

envoy_cc_library(
name = "well_known_names",
srcs = ["well_known_names.cc"],
hdrs = ["well_known_names.h"],
name = "watch_map_lib",
srcs = ["watch_map.cc"],
hdrs = ["watch_map.h"],
deps = [
"//include/envoy/config:subscription_interface",
"//source/common/common:assert_lib",
"//source/common/singleton:const_singleton",
],
)

envoy_cc_library(
name = "config_provider_lib",
srcs = ["config_provider_impl.cc"],
hdrs = ["config_provider_impl.h"],
deps = [
":utility_lib",
"//include/envoy/config:config_provider_interface",
"//include/envoy/config:config_provider_manager_interface",
"//include/envoy/init:manager_interface",
"//include/envoy/server:admin_interface",
"//include/envoy/server:config_tracker_interface",
"//include/envoy/singleton:instance_interface",
"//include/envoy/thread_local:thread_local_interface",
"//source/common/init:target_lib",
"//source/common/common:minimal_logger_lib",
"//source/common/protobuf",
],
)

envoy_cc_library(
name = "remote_data_fetcher_lib",
srcs = ["remote_data_fetcher.cc"],
hdrs = ["remote_data_fetcher.h"],
name = "well_known_names",
srcs = ["well_known_names.cc"],
hdrs = ["well_known_names.h"],
deps = [
"//include/envoy/upstream:cluster_manager_interface",
"//source/common/common:hex_lib",
"//source/common/crypto:utility_lib",
"//source/common/http:utility_lib",
"@envoy_api//envoy/api/v2/core:http_uri_cc",
"//source/common/common:assert_lib",
"//source/common/singleton:const_singleton",
],
)
171 changes: 171 additions & 0 deletions source/common/config/watch_map.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
#include "common/config/watch_map.h"

namespace Envoy {
namespace Config {

Watch* WatchMap::addWatch(SubscriptionCallbacks& callbacks) {
auto watch = std::make_unique<Watch>(callbacks);
Watch* watch_ptr = watch.get();
wildcard_watches_.insert(watch_ptr);
watches_.insert(std::move(watch));
return watch_ptr;
}

void WatchMap::removeWatch(Watch* watch) {
wildcard_watches_.erase(watch); // may or may not be in there, but we want it gone.
watches_.erase(watch);
Copy link
Member

Choose a reason for hiding this comment

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

Should we assert it's in at least one of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say no; removeWatch is purely for cleanup. Weird as it may be for that to happen, it doesn't mean anything is wrong with the WatchMap.

}

AddedRemoved WatchMap::updateWatchInterest(Watch* watch,
const std::set<std::string>& update_to_these_names) {
if (update_to_these_names.empty()) {
wildcard_watches_.insert(watch);
} else {
wildcard_watches_.erase(watch);
}

std::vector<std::string> newly_added_to_watch;
std::set_difference(update_to_these_names.begin(), update_to_these_names.end(),
watch->resource_names_.begin(), watch->resource_names_.end(),
std::inserter(newly_added_to_watch, newly_added_to_watch.begin()));

std::vector<std::string> newly_removed_from_watch;
std::set_difference(watch->resource_names_.begin(), watch->resource_names_.end(),
update_to_these_names.begin(), update_to_these_names.end(),
std::inserter(newly_removed_from_watch, newly_removed_from_watch.begin()));

watch->resource_names_ = update_to_these_names;

return AddedRemoved(findAdditions(newly_added_to_watch, watch),
findRemovals(newly_removed_from_watch, watch));
}

absl::flat_hash_set<Watch*> WatchMap::watchesInterestedIn(const std::string& resource_name) {
fredlas marked this conversation as resolved.
Show resolved Hide resolved
// Note that std::set_union needs sorted sets. Better to do it ourselves with insert().
Copy link
Member

Choose a reason for hiding this comment

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

Could we just use an ordered set? std::less gives a total ordering of pointers in C++14.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but then updates would be more expensive; watch_interest_ would need to hold ordered sets. Removed that comment; it actually took me a minute to figure out what it was even saying!

absl::flat_hash_set<Watch*> ret = wildcard_watches_;
auto watches_interested = watch_interest_.find(resource_name);
if (watches_interested != watch_interest_.end()) {
for (const auto& watch : watches_interested->second) {
ret.insert(watch);
}
}
return ret;
}

void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::Any>& resources,
const std::string& version_info) {
if (watches_.empty()) {
ENVOY_LOG(warn, "WatchMap::onConfigUpdate: there are no watches!");
return;
}
SubscriptionCallbacks& name_getter = (*watches_.begin())->callbacks_;

// Build a map from watches, to the set of updated resources that each watch cares about. Each
// entry in the map is then a nice little bundle that can be fed directly into the individual
// onConfigUpdate()s.
absl::flat_hash_map<Watch*, Protobuf::RepeatedPtrField<ProtobufWkt::Any>> per_watch_updates;
for (const auto& r : resources) {
const absl::flat_hash_set<Watch*>& interested_in_r =
watchesInterestedIn(name_getter.resourceName(r));
for (const auto& interested_watch : interested_in_r) {
per_watch_updates[interested_watch].Add()->CopyFrom(r);
}
}

// We just bundled up the updates into nice per-watch packages. Now, deliver them.
for (auto& watch : watches_) {
auto this_watch_updates = per_watch_updates.find(watch);
if (this_watch_updates == per_watch_updates.end()) {
// This update included no resources this watch cares about - so we do an empty
// onConfigUpdate(), to notify the watch that its resources - if they existed before this -
// were dropped.
watch->callbacks_.onConfigUpdate({}, version_info);
} else {
watch->callbacks_.onConfigUpdate(this_watch_updates->second, version_info);
}
}
}

void WatchMap::onConfigUpdate(
const Protobuf::RepeatedPtrField<envoy::api::v2::Resource>& added_resources,
const Protobuf::RepeatedPtrField<std::string>& removed_resources,
const std::string& system_version_info) {
// Build a pair of maps: from watches, to the set of resources {added,removed} that each watch
// cares about. Each entry in the map-pair is then a nice little bundle that can be fed directly
// into the individual onConfigUpdate()s.
absl::flat_hash_map<Watch*, Protobuf::RepeatedPtrField<envoy::api::v2::Resource>> per_watch_added;
for (const auto& r : added_resources) {
const absl::flat_hash_set<Watch*>& interested_in_r = watchesInterestedIn(r.name());
for (const auto& interested_watch : interested_in_r) {
per_watch_added[interested_watch].Add()->CopyFrom(r);
}
}
absl::flat_hash_map<Watch*, Protobuf::RepeatedPtrField<std::string>> per_watch_removed;
for (const auto& r : removed_resources) {
const absl::flat_hash_set<Watch*>& interested_in_r = watchesInterestedIn(r);
for (const auto& interested_watch : interested_in_r) {
*per_watch_removed[interested_watch].Add() = r;
}
}

// We just bundled up the updates into nice per-watch packages. Now, deliver them.
for (const auto& added : per_watch_added) {
const Watch* cur_watch = added.first;
auto removed = per_watch_removed.find(cur_watch);
if (removed == per_watch_removed.end()) {
// additions only, no removals
cur_watch->callbacks_.onConfigUpdate(added.second, {}, system_version_info);
} else {
// both additions and removals
cur_watch->callbacks_.onConfigUpdate(added.second, removed->second, system_version_info);
// Drop the removals now, so the final removals-only pass won't use them.
per_watch_removed.erase(removed);
}
}
// Any removals-only updates will not have been picked up in the per_watch_added loop.
for (auto& removed : per_watch_removed) {
removed.first->callbacks_.onConfigUpdate({}, removed.second, system_version_info);
fredlas marked this conversation as resolved.
Show resolved Hide resolved
}
}

void WatchMap::onConfigUpdateFailed(const EnvoyException* e) {
for (auto& watch : watches_) {
watch->callbacks_.onConfigUpdateFailed(e);
}
}

std::set<std::string> WatchMap::findAdditions(const std::vector<std::string>& newly_added_to_watch,
Watch* watch) {
std::set<std::string> newly_added_to_subscription;
for (const auto& name : newly_added_to_watch) {
auto entry = watch_interest_.find(name);
if (entry == watch_interest_.end()) {
newly_added_to_subscription.insert(name);
watch_interest_[name] = {watch};
fredlas marked this conversation as resolved.
Show resolved Hide resolved
} else {
entry->second.insert(watch);
}
}
return newly_added_to_subscription;
}

std::set<std::string>
WatchMap::findRemovals(const std::vector<std::string>& newly_removed_from_watch, Watch* watch) {
std::set<std::string> newly_removed_from_subscription;
for (const auto& name : newly_removed_from_watch) {
auto entry = watch_interest_.find(name);
RELEASE_ASSERT(
entry != watch_interest_.end(),
fmt::format("WatchMap: tried to remove a watch from untracked resource {}", name));

entry->second.erase(watch);
if (entry->second.empty()) {
watch_interest_.erase(entry);
newly_removed_from_subscription.insert(name);
}
}
return newly_removed_from_subscription;
}

} // namespace Config
} // namespace Envoy
Loading