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

Show file tree
Hide file tree
Changes from 11 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
25 changes: 21 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,20 @@ 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();) {
const auto& address = (*i)->address()->asString();
if (host_addresses.count(address) == 0) {
priority_state_manager.registerHostForPriority(*i, target->locality_lb_endpoint_,
target->lb_endpoint_, absl::nullopt);
host_addresses.insert(address);
i++;
} else {
i = std::remove_if(i, target->hosts_.end(), [&address](const auto& host) {
return host->address()->asString() == address;
});
}
}
}
}
Expand Down Expand Up @@ -1191,8 +1201,15 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() {
return host->priority() == locality_lb_endpoint_.priority();
}));
parent_.updateAllHosts(hosts_added, hosts_removed, locality_lb_endpoint_.priority());
} else {
parent_.info_->stats().update_no_rebuild_.inc();
}

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
27 changes: 25 additions & 2 deletions test/common/upstream/upstream_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,9 @@ TEST(StrictDnsClusterImplTest, LoadAssignmentBasic) {
EXPECT_EQ("localhost1", cluster.prioritySet().hostSetsPerPriority()[0]->hosts()[0]->hostname());
EXPECT_EQ("localhost1", cluster.prioritySet().hostSetsPerPriority()[0]->hosts()[1]->hostname());

// This is the first time we receveived an update for localhost1, we expect to rebuild.
EXPECT_EQ(0UL, stats.counter("cluster.name.update_no_rebuild").value());

resolver1.expectResolve(*dns_resolver);
resolver1.timer_->callback_();
EXPECT_CALL(*resolver1.timer_, enableTimer(std::chrono::milliseconds(4000)));
Expand All @@ -529,6 +532,9 @@ TEST(StrictDnsClusterImplTest, LoadAssignmentBasic) {
std::list<std::string>({"127.0.0.1:11001", "127.0.0.2:11001"}),
ContainerEq(hostListToAddresses(cluster.prioritySet().hostSetsPerPriority()[0]->hosts())));

// Since no change for localhost1, we expect no rebuild.
EXPECT_EQ(1UL, stats.counter("cluster.name.update_no_rebuild").value());

resolver1.expectResolve(*dns_resolver);
resolver1.timer_->callback_();
EXPECT_CALL(*resolver1.timer_, enableTimer(std::chrono::milliseconds(4000)));
Expand All @@ -537,17 +543,34 @@ TEST(StrictDnsClusterImplTest, LoadAssignmentBasic) {
std::list<std::string>({"127.0.0.1:11001", "127.0.0.2:11001"}),
ContainerEq(hostListToAddresses(cluster.prioritySet().hostSetsPerPriority()[0]->hosts())));

// Since no change for localhost1, we expect no rebuild.
EXPECT_EQ(2UL, stats.counter("cluster.name.update_no_rebuild").value());

EXPECT_CALL(*resolver2.timer_, enableTimer(std::chrono::milliseconds(4000)));
EXPECT_CALL(membership_updated, ready());
resolver2.dns_callback_(TestUtility::makeDnsResponse({"10.0.0.1", "10.0.0.1"}));

// We received a new set of hosts for localhost2. Should rebuild the cluster.
EXPECT_EQ(2UL, stats.counter("cluster.name.update_no_rebuild").value());

resolver1.expectResolve(*dns_resolver);
resolver1.timer_->callback_();
EXPECT_CALL(*resolver1.timer_, enableTimer(std::chrono::milliseconds(4000)));
resolver1.dns_callback_(TestUtility::makeDnsResponse({"127.0.0.2", "127.0.0.1"}));

// We again received the same set as before for localhost1. No rebuild this time.
EXPECT_EQ(3UL, stats.counter("cluster.name.update_no_rebuild").value());

resolver1.timer_->callback_();
EXPECT_CALL(*resolver1.timer_, enableTimer(std::chrono::milliseconds(4000)));
EXPECT_CALL(membership_updated, ready());
resolver1.dns_callback_(TestUtility::makeDnsResponse({"127.0.0.3"}));
EXPECT_THAT(
std::list<std::string>({"127.0.0.3:11001"}),
std::list<std::string>({"127.0.0.3:11001", "10.0.0.1:11002"}),
ContainerEq(hostListToAddresses(cluster.prioritySet().hostSetsPerPriority()[0]->hosts())));

// Make sure we de-dup the same address.
EXPECT_CALL(*resolver2.timer_, enableTimer(std::chrono::milliseconds(4000)));
EXPECT_CALL(membership_updated, ready());
resolver2.dns_callback_(TestUtility::makeDnsResponse({"10.0.0.1", "10.0.0.1"}));
EXPECT_THAT(
std::list<std::string>({"127.0.0.3:11001", "10.0.0.1:11002"}),
Expand Down