Skip to content

Support mutable metadata for endpoints #3814

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 11 commits into from
Jul 10, 2018
Merged

Support mutable metadata for endpoints #3814

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bf002ff
Support mutable metadata for endpoints
Jul 7, 2018
8c7d6a6
Address review comments.
Jul 9, 2018
f7b108f
Grammar fix.
Jul 9, 2018
15259b9
Ensure everything is thread-safe.
Jul 9, 2018
cabd88a
Fix test
Jul 10, 2018
75294de
Fix logical_dns_cluster
Jul 10, 2018
3b6ec60
Fix header_formatter_test
Jul 10, 2018
0969f6b
Fix duplicate comment & method grouping
Jul 10, 2018
3013e96
Matt's review
Jul 10, 2018
5aed1bb
Forgot to promote the mutex to mutable.
Jul 10, 2018
cd8812e
Merge branch 'master' into mutable-metadata-issue-3810
Jul 10, 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
10 changes: 10 additions & 0 deletions include/envoy/upstream/host_description.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,21 @@ class HostDescription {
*/
virtual bool canary() const PURE;

/**
* Update the canary status of the host.
*/
virtual void canary(bool is_canary) PURE;

/**
* @return the metadata associated with this host
*/
virtual const envoy::api::v2::core::Metadata& metadata() const PURE;

/**
* Set the current metadata.
*/
virtual void metadata(const envoy::api::v2::core::Metadata& new_metadata) PURE;

/**
* @return the cluster the host is a member of.
*/
Expand Down
3 changes: 3 additions & 0 deletions source/common/upstream/logical_dns_cluster.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,12 @@ class LogicalDnsCluster : public ClusterImplBase {

// Upstream:HostDescription
bool canary() const override { return false; }
void canary(bool) override {}
const envoy::api::v2::core::Metadata& metadata() const override {
return envoy::api::v2::core::Metadata::default_instance();
}
void metadata(const envoy::api::v2::core::Metadata&) override {}

const ClusterInfo& cluster() const override { return logical_host_->cluster(); }
HealthCheckHostMonitor& healthChecker() const override {
return logical_host_->healthChecker();
Expand Down
16 changes: 15 additions & 1 deletion source/common/upstream/upstream_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ parseClusterSocketOptions(const envoy::api::v2::Cluster& config,
return cluster_options;
}

bool metadataEq(const envoy::api::v2::core::Metadata& lhs,
const envoy::api::v2::core::Metadata& rhs) {
return Protobuf::util::MessageDifferencer::Equivalent(lhs, rhs);

@zuercher zuercher Jul 9, 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 would just inline the call since it's only used once.

}

} // namespace

Host::CreateConnectionData
Expand Down Expand Up @@ -691,7 +696,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts,
// TODO(htuch): We can be smarter about this potentially, and not force a full
// host set update on health status change. The way this would work is to
// implement a HealthChecker subclass that provides thread local health
// updates to the Cluster objeect. This will probably make sense to do in
// updates to the Cluster object. This will probably make sense to do in
// conjunction with https://github.com/envoyproxy/envoy/issues/2874.
bool health_changed = false;

Expand Down Expand Up @@ -734,6 +739,15 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts,
}
}

// Did metadata change?
if (!metadataEq(host->metadata(), (*i)->metadata())) {
(*i)->metadata(host->metadata());
(*i)->canary(host->canary());

@mattklein123 mattklein123 Jul 9, 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.

Seems odd here to do the canary update inside the metadata check. Should this be an independent check for people that use canary but not metadata?

@rgs1 rgs1 Jul 9, 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.

@mattklein123 my thinking is that canary is encoded under envoy.lb/canary, so for it to change metadata has to change — no?

E.g.: https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/endpoint/endpoint.proto#L64

@mattklein123 mattklein123 Jul 9, 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 see. Hmm. 2 questions:

  1. What does setting canary actually do then? Is that just for debugging output?
  2. If we are setting these inline, this is not thread safe unfortunately. Canary is easy to fix by making it atomic, but metadata is not and would need a lock (probably a reader/writer lock if we go that direction).

Thoughts? Whatever we decide we should add more comments here.

@rgs1 rgs1 Jul 9, 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.

Also: https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/upstream_impl.h#L69

@rgs1 rgs1 Jul 9, 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.

So for:

Question 1:
I was setting canary blindly just in case it had changed, given the metadata as a whole changed. This is not (just) for debugging, but to address the case in which an endpoint (e.g.: 10.0.0.2:80) had the following transitions in EDS:

# eds snapshot v1
10.0.0.2:80 canary:false

# eds snapshot v2
10.0.0.2:80 canary:true

That is, it's just a metadata change as everything else and it needs to be updated so that subsets can be properly recreated. Was this what you were asking? I suspect maybe not...

Question 2:
Ah, I thought this was all computed for each worker and thus thread safe. I'll add locking then.

And yeah, I'll add more comments too. Lets clear up question 1. before we move forward though.

@htuch htuch Jul 10, 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.

Should the metadata shared ptr be TLS and we post updates to it, which will then be applied on individual worker threads outside other processing? I think we would still want to latch where you suggest, but we could avoid having to take mutexes and worry about overlap with other execution by doing this.

@zuercher zuercher Jul 10, 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.

Another alternative might be to treat metadata differences as different hosts -- push a new host with the new metadata into hosts_added and have the old one appear in hosts_removed? I think then we don't have to mess with locking?

@mattklein123 mattklein123 Jul 10, 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.

Another alternative might be to treat metadata differences as different hosts -- push a new host with the new metadata into hosts_added and have the old one appear in hosts_removed? I think then we don't have to mess with locking?

Yeah, this occurred to me also. If this ends up getting super complicated (which it seems to be) I think we might want to consider this approach. Basically just consider this a new host and do a remove/add operation.

@rgs1 rgs1 Jul 10, 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.

Hmm, intuitively I rather not treat this as remove/add... Sounds more heavy-handed than needed.

So do you really think it's too complicated as is now? Shall we explore the remove/add route then?

FWIW, I am testing the current version of this patch now since we really really really need mutable metadata for our setup.

@mattklein123 mattklein123 Jul 10, 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 will review your current iteration in a bit. Let's hold here for now until we can review. Thanks for iterating on this. :)


// If metadata changed, we need to rebuild. See github issue #3810.
health_changed = true;

@zuercher zuercher Jul 9, 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.

Probably should rename this var to reflect that it's tracking changes to more than just health.

}

(*i)->weight(host->weight());
final_hosts.push_back(*i);
i = current_hosts.erase(i);
Expand Down
12 changes: 10 additions & 2 deletions source/common/upstream/upstream_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,15 @@ class HostDescriptionImpl : virtual public HostDescription {

// Upstream::HostDescription
bool canary() const override { return canary_; }

// Upstream::HostDescription
void canary(bool is_canary) override { canary_ = is_canary; }

const envoy::api::v2::core::Metadata& metadata() const override { return metadata_; }
virtual void metadata(const envoy::api::v2::core::Metadata& new_metadata) override {
metadata_ = new_metadata;
}

const ClusterInfo& cluster() const override { return *cluster_; }
HealthCheckHostMonitor& healthChecker() const override {
if (health_checker_) {
Expand Down Expand Up @@ -108,8 +116,8 @@ class HostDescriptionImpl : virtual public HostDescription {
const std::string hostname_;
Network::Address::InstanceConstSharedPtr address_;
Network::Address::InstanceConstSharedPtr health_check_address_;
const bool canary_;
const envoy::api::v2::core::Metadata metadata_;
bool canary_;
envoy::api::v2::core::Metadata metadata_;
const envoy::api::v2::core::Locality locality_;
Stats::IsolatedStoreImpl stats_store_;
HostStats stats_;
Expand Down
19 changes: 19 additions & 0 deletions test/common/upstream/eds_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ TEST_F(EdsTest, EndpointMetadata) {
Config::MetadataFilters::get().ENVOY_LB,
Config::MetadataEnvoyLbKeys::get().CANARY)
.set_bool_value(true);
Config::Metadata::mutableMetadataValue(*canary->mutable_metadata(),
Config::MetadataFilters::get().ENVOY_LB, "version")
.set_string_value("v1");

bool initialized = false;
cluster_->initialize([&initialized] { initialized = true; });
Expand Down Expand Up @@ -197,10 +200,26 @@ TEST_F(EdsTest, EndpointMetadata) {
Config::MetadataEnvoyLbKeys::get().CANARY)
.bool_value());
EXPECT_TRUE(hosts[1]->canary());
EXPECT_EQ(Config::Metadata::metadataValue(hosts[1]->metadata(),
Config::MetadataFilters::get().ENVOY_LB, "version")
.string_value(),
"v1");

// We don't rebuild with the exact same config.
VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, ""));
EXPECT_EQ(1UL, stats_.counter("cluster.name.update_no_rebuild").value());

// New resources with Metadata updated.
Config::Metadata::mutableMetadataValue(*canary->mutable_metadata(),
Config::MetadataFilters::get().ENVOY_LB, "version")
.set_string_value("v2");
VERBOSE_EXPECT_NO_THROW(cluster_->onConfigUpdate(resources, ""));
auto& nhosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts();
EXPECT_EQ(nhosts.size(), 2);
EXPECT_EQ(Config::Metadata::metadataValue(nhosts[1]->metadata(),
Config::MetadataFilters::get().ENVOY_LB, "version")
.string_value(),
"v2");
}

// Validate that onConfigUpdate() updates endpoint health status.
Expand Down
4 changes: 4 additions & 0 deletions test/mocks/upstream/host.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ class MockHostDescription : public HostDescription {
MOCK_CONST_METHOD0(address, Network::Address::InstanceConstSharedPtr());
MOCK_CONST_METHOD0(healthCheckAddress, Network::Address::InstanceConstSharedPtr());
MOCK_CONST_METHOD0(canary, bool());
MOCK_METHOD1(canary, void(bool new_canary));
MOCK_CONST_METHOD0(metadata, const envoy::api::v2::core::Metadata&());
MOCK_METHOD1(metadata, void(const envoy::api::v2::core::Metadata&));
MOCK_CONST_METHOD0(cluster, const ClusterInfo&());
MOCK_CONST_METHOD0(outlierDetector, Outlier::DetectorHostMonitor&());
MOCK_CONST_METHOD0(healthChecker, HealthCheckHostMonitor&());
Expand Down Expand Up @@ -128,7 +130,9 @@ class MockHost : public Host {
MOCK_CONST_METHOD0(address, Network::Address::InstanceConstSharedPtr());
MOCK_CONST_METHOD0(healthCheckAddress, Network::Address::InstanceConstSharedPtr());
MOCK_CONST_METHOD0(canary, bool());
MOCK_METHOD1(canary, void(bool new_canary));
MOCK_CONST_METHOD0(metadata, const envoy::api::v2::core::Metadata&());
MOCK_METHOD1(metadata, void(const envoy::api::v2::core::Metadata&));
MOCK_CONST_METHOD0(cluster, const ClusterInfo&());
MOCK_CONST_METHOD0(counters, std::vector<Stats::CounterSharedPtr>());
MOCK_CONST_METHOD2(
Expand Down