Skip to content

upstream: make sure all_hosts_ is updated correctly #4575

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.

Sign up for GitHub

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

Merged
mattklein123 merged 13 commits into from
Oct 4, 2018
Merged

upstream: make sure all_hosts_ is updated correctly #4575

Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0ad25a5
Make sure we have no duplicated hosts
dio Oct 1, 2018
232006b
Hack the initialization of updated_hosts
dio Oct 1, 2018
4e8c0b9
Move to after dynamic hosts update
dio Oct 1, 2018
feb0fba
Fix test
dio Oct 1, 2018
f6ac401
Make sure we have all hosts updated
dio Oct 1, 2018
f2146af
Add update_no_rebuild
dio Oct 1, 2018
f3e1e5d
If no change, no cluster rebuild
dio Oct 2, 2018
97cf9d5
Simulate different updates
dio Oct 2, 2018
c32348f
Kick CI
dio Oct 2, 2018
2490830
Use local variable
dio Oct 3, 2018
e3c0de0
Use remove_if
dio Oct 3, 2018
95510b5
Remove dedupe code, add comments
dio Oct 3, 2018
a1d2995
Update comments
dio Oct 4, 2018
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
23 changes: 19 additions & 4 deletions source/common/upstream/upstream_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,10 +1127,17 @@ void StrictDnsClusterImpl::updateAllHosts(const HostVector& hosts_added,
// At this point we know that we are different so make a new host list and notify.
for (const ResolveTargetPtr& target : resolve_targets_) {
priority_state_manager.initializePriorityFor(target->locality_lb_endpoint_);
for (const HostSharedPtr& host : target->hosts_) {
if (target->locality_lb_endpoint_.priority() == current_priority) {
priority_state_manager.registerHostForPriority(host, target->locality_lb_endpoint_,
target->lb_endpoint_, absl::nullopt);
std::unordered_set<std::string> host_addresses;

@mattklein123 mattklein123 Oct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a little confused on what the underlying bug actually is here. Can you add more comments so it's easier to understand what this code is doing now? Also, should whatever this is doing actually be done inside the priority state manager so it applies to all discovery types? Is this related to @snowp's comment in the issue around reconciling DNS with EDS behavior? Maybe the comments will help me understand more. Feel free to leave TODOs on what else needs to be done later. Thank you!

@dio dio Oct 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure will add more comments here. While I think this dedupe here probably is not required anymore since the underlying problem is indeed when we update the internal all_hosts_ data (for EDS (STATIC), the updated hosts consist of all host sets, while for STRICT_DNS it is not, depends on each DNS resolution). This:

envoy/source/common/upstream/upstream_impl.cc

Lines 1208 to 1213 in e3c0de0

for (const auto& set : parent_.prioritySet().hostSetsPerPriority()) {
for (const auto& host : set->hosts()) {
updated_hosts.insert({host->address()->asString(), host});
}
}
parent_.updateHostMap(std::move(updated_hosts));
should remedy the issue.

Will make sure the priority state manager can actually handle this (no duplications).

@dio dio Oct 3, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mattklein123 for following up this issue, I have opened one here: #4590 to track. This is based on @snowp's comment. I'll take a look at that later.

if (target->locality_lb_endpoint_.priority() == current_priority) {
for (auto i = target->hosts_.begin(); i != target->hosts_.end();) {
if (host_addresses.count((*i)->address()->asString()) == 0) {
priority_state_manager.registerHostForPriority(*i, target->locality_lb_endpoint_,
target->lb_endpoint_, absl::nullopt);
host_addresses.insert((*i)->address()->asString());
++i;
} else {
i = target->hosts_.erase(i);

@tonya11en tonya11en Oct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the case of multiple instances of the same host, you could probably save yourself a few calls to erase if you just used std::remove_if(i, hosts_.end(), XXXX). Your predicate function would just check for the existence of each element in host_addresses. std::remove_if also has the same complexity as a vector erase.

@lizan lizan Oct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove_if doesn't work on associative containers so that won't work.

@tonya11en tonya11en Oct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@lizan target->hosts_ is a vector, so it will work. Which container are you referring to?

@lizan lizan Oct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah ignore my comment above, I thought it is about host_addresses (unordered_set)

}
}
}
}
Expand Down Expand Up @@ -1193,6 +1200,14 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() {
parent_.updateAllHosts(hosts_added, hosts_removed, locality_lb_endpoint_.priority());
}

// TODO(dio): Not sure if we should do this if active health checking is disabled.

@snowp snowp Oct 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should, the all_hosts_ mapping is also used to determine if the new hosts match within the same priority, which affects hosts_changed

if (parent_.health_checker_ != nullptr) {
for (const auto& set : parent_.prioritySet().hostSetsPerPriority()) {
for (const auto& host : set->hosts()) {
updated_hosts.insert({host->address()->asString(), host});
}
}
}
parent_.updateHostMap(std::move(updated_hosts));

// If there is an initialize callback, fire it now. Note that if the cluster refers to
Expand Down