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

upstream: rebuild cluster when health check config is changed #4075

Merged
merged 21 commits into from
Aug 17, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 6 additions & 1 deletion include/envoy/upstream/host_description.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,14 @@ class HostDescription {
* @return the address used to health check the host.
*/
virtual Network::Address::InstanceConstSharedPtr healthCheckAddress() const PURE;

/**
* Sets the address used to health check the host. This noop by default.
*/
virtual void setHealthCheckAddress(Network::Address::InstanceConstSharedPtr){};
};

typedef std::shared_ptr<const HostDescription> HostDescriptionConstSharedPtr;

} // Upstream
} // namespace Upstream
} // namespace Envoy
7 changes: 7 additions & 0 deletions source/common/upstream/upstream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,13 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts,
hosts_changed = true;
}

// Did the health check address change?
if (*(*i)->healthCheckAddress() != *host->healthCheckAddress()) {
// If the health check address is updated, set the current host's health check address
// using the updated one.
(*i)->setHealthCheckAddress(host->healthCheckAddress());
Copy link
Member

Choose a reason for hiding this comment

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

Although this is safe from a locking perspective since HC is only on the main thread, it seems like the HC address should be an immutable property of a host? How often does it really change? Did we consider making this a thing that causes a host to get removed/re-added? (Basically having the host check pivot off of normal address and HC address)

Copy link
Member Author

Choose a reason for hiding this comment

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

should be an immutable property of a host

Yeah, on the second thought this PR seems like a quick hack for me.

How often does it really change? Did we consider making this a thing that causes a host to get removed/re-added?

In practice probably not that often. Will update accordingly to make it as a case of causing a host to get removed/re-added.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense to do a full add/remove, this code is already crazy complicated ☣️

}

(*i)->weight(host->weight());
final_hosts.push_back(*i);
i = current_hosts.erase(i);
Expand Down
4 changes: 4 additions & 0 deletions source/common/upstream/upstream_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ class HostDescriptionImpl : virtual public HostDescription {
Network::Address::InstanceConstSharedPtr healthCheckAddress() const override {
return health_check_address_;
}
void
setHealthCheckAddress(Network::Address::InstanceConstSharedPtr health_check_address) override {
health_check_address_ = health_check_address;
}
const envoy::api::v2::core::Locality& locality() const override { return locality_; }

protected:
Expand Down
32 changes: 32 additions & 0 deletions test/common/upstream/eds_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,7 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) {
EXPECT_TRUE(initialized);
EXPECT_EQ(0UL, stats_.counter("cluster.name.update_no_rebuild").value());

uint32_t host_index = 0;
{
auto& first_hosts_per_locality =
cluster_->prioritySet().hostSetsPerPriority()[0]->hostsPerLocality();
Expand All @@ -1044,13 +1045,44 @@ TEST_F(EdsTest, PriorityAndLocalityWeighted) {
EXPECT_EQ(60, second_locality_weights[0]);
EXPECT_EQ(2, second_hosts_per_locality.get()[1].size());
EXPECT_EQ(40, second_locality_weights[1]);

// Find the index of host that has health check port value equals to 1000 (i.e. the first
// endpoint).
const auto& hosts = first_hosts_per_locality.get()[1];
for (uint32_t i = 0; i < hosts.size(); ++i) {
if (hosts[i]->healthCheckAddress()->ip()->port() == 1000) {
host_index = i;
break;
}
}
}

// Update the health check port value of the first endpoint.
const uint32_t updated_health_check_port = 8000;
cluster_load_assignment->mutable_endpoints(0)
->mutable_lb_endpoints(0)
->mutable_endpoint()
->mutable_health_check_config()
->set_port_value(updated_health_check_port);

// This should noop (regression test for earlier bug where we would still
// rebuild).
VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, ""));
EXPECT_EQ(1UL, stats_.counter("cluster.name.update_no_rebuild").value());

{
auto& first_hosts_per_locality =
cluster_->prioritySet().hostSetsPerPriority()[0]->hostsPerLocality();
EXPECT_EQ(2, first_hosts_per_locality.get().size());
EXPECT_EQ(1, first_hosts_per_locality.get()[0].size());

// Check if the health check port value is updated.
const auto& hosts = first_hosts_per_locality.get()[1];
const auto& current_health_check_port = hosts[host_index]->healthCheckAddress()->ip()->port();
EXPECT_NE(port, current_health_check_port);
EXPECT_EQ(updated_health_check_port, current_health_check_port);
}

// Adjust locality weights, validate that we observe an update.
cluster_load_assignment->mutable_endpoints(0)->mutable_load_balancing_weight()->set_value(60);
cluster_load_assignment->mutable_endpoints(1)->mutable_load_balancing_weight()->set_value(40);
Expand Down