-
Notifications
You must be signed in to change notification settings - Fork 0
[WIP] HdsDelegate can healthcheck an endpoint #1
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
base: master
Are you sure you want to change the base?
Changes from 20 commits
6521aa0
56a047b
0f631e1
6460533
9025369
5244597
9e7a3ab
4e0b252
4f344cf
e050f5e
299591b
9c27339
18cdf0f
ab86abc
4865c9c
6d58cb8
93d96b5
4b871c0
f69baf8
52de772
b686053
e341004
9365720
31abe6e
090676d
2805b46
c563b39
2c3c3e7
07b13c0
3b05bff
a5da078
fe94528
b79d5e1
0f01a2a
75909a7
7253d5f
37ad8fc
8875eba
866597f
4b4fd6a
24dfa19
fd57604
74928c7
5e1eb02
cd95109
2a45375
00bb039
2705764
80be024
c3d2f37
b9db466
9333f23
03cfb87
eab54b5
55e7456
1c39216
ce83509
810ec2e
df7a291
c46a946
a49f8b7
b1f870a
f22d552
6881200
2ba32f2
6a57863
e0977b1
95c3e13
336bce9
9de80f7
37f66d2
c2e3ada
119c4c8
218d112
e718500
a182041
0a0914e
0cf947f
230228b
c92a301
14af459
01d2e16
7caa887
9a685a0
e435d18
45fe83e
606a22a
cf12ea5
05647d6
5f8b25e
b3c5fe6
d69dd92
445f365
27be177
1645fa5
a0b22ef
4aad3f8
03d02e5
c99fff2
8fa8401
1940195
2a68458
17efc83
8e7b0b5
a7cf6df
f9ce8f7
27c99de
1aa2430
a0b6243
3a56d21
0e71582
0f68948
a8fa0c6
b14ce1d
982ebd2
b202f89
8ed7c15
20296c5
b8e019f
598f5c9
3cc6e3c
a5d9885
783fda9
569d70a
a5478ee
391150a
672083c
3a5d126
c4b6aec
e021e4d
2d7b9df
7c11e92
445e746
0a43ae8
eefd06d
054513e
8b3aae8
b32eabf
e89c9d6
b9fc350
a03c343
1f445bd
0621763
1ef23d4
ec0179a
8459237
f3b1100
c27ed1f
3ee3aa3
9b64f4b
5beff99
72a964c
4868061
42109e5
0e2c795
1dfde38
280baee
bcad173
324e628
8babc5e
f4b0a52
9d62fe8
b6c737f
f28dc53
6b65dbe
8b9fd9a
08fadcc
2563fe1
01268da
6a820f7
ed131cf
dc03a9a
46c63e6
7d61b00
f882e74
028387a
f0ca754
83b9e2d
b27068b
eb5ea98
62441f9
2012c3e
b96da53
d0f10fa
6a8b843
315f561
bb0a9c6
85f0ba7
2a0ab6f
79f1dce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| #include "common/upstream/upstream_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Upstream { | ||
|
|
||
| /** | ||
| * Factory for unit testing Upstream::HdsDelegate | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not just for HDS.. it's just a general |
||
| */ | ||
|
|
||
| class ClusterInfoFactory { | ||
| public: | ||
| virtual ~ClusterInfoFactory() {} | ||
| /** | ||
| * @return ClusterInfoConstSharedPtr | ||
| */ | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spacing, please elaborate in comment on |
||
| virtual ClusterInfoConstSharedPtr | ||
| createHdsClusterInfo(Runtime::Loader& runtime, const envoy::api::v2::Cluster& cluster, | ||
| const envoy::api::v2::core::BindConfig& bind_config, Stats::Store& stats, | ||
| Ssl::ContextManager& ssl_context_manager, | ||
| Secret::SecretManager& secret_manager, bool added_via_api) PURE; | ||
| }; | ||
|
|
||
| } // namespace Upstream | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,19 +6,30 @@ namespace Envoy { | |
| namespace Upstream { | ||
|
|
||
| HdsDelegate::HdsDelegate(const envoy::api::v2::core::Node& node, Stats::Scope& scope, | ||
| Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher) | ||
| Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, | ||
| Runtime::Loader& runtime, Envoy::Stats::Store& stats, | ||
| Ssl::ContextManager& ssl_context_manager, | ||
| Secret::SecretManager& secret_manager, Runtime::RandomGenerator& random, | ||
| ClusterInfoFactory& info_factory) | ||
| : stats_{ALL_HDS_STATS(POOL_COUNTER_PREFIX(scope, "hds_delegate."))}, | ||
| async_client_(std::move(async_client)), | ||
| service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName( | ||
| "envoy.service.discovery.v2.HealthDiscoveryService.StreamHealthCheck")) { | ||
| "envoy.service.discovery.v2.HealthDiscoveryService.StreamHealthCheck")), | ||
| runtime_(runtime), store_stats(stats), ssl_context_manager_(ssl_context_manager), | ||
| secret_manager_(secret_manager), random_(random), dispatcher_(dispatcher), | ||
| info_factory_(info_factory) { | ||
| health_check_request_.mutable_node()->MergeFrom(node); | ||
| retry_timer_ = dispatcher.createTimer([this]() -> void { establishNewStream(); }); | ||
| response_timer_ = dispatcher.createTimer([this]() -> void { sendHealthCheckRequest(); }); | ||
| server_response_timer_ = dispatcher.createTimer([this]() -> void { sendResponse(); }); | ||
| establishNewStream(); | ||
| } | ||
|
|
||
| void HdsDelegate::setRetryTimer() { | ||
| retry_timer_->enableTimer(std::chrono::milliseconds(RETRY_DELAY_MS)); | ||
| retry_timer_->enableTimer(std::chrono::milliseconds(RetryDelayMilliseconds)); | ||
| } | ||
|
|
||
| void HdsDelegate::setServerResponseTimer() { | ||
| server_response_timer_->enableTimer(std::chrono::milliseconds(server_response_ms_)); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: prefer not to add unnecessary extra whitespace.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| void HdsDelegate::establishNewStream() { | ||
|
|
@@ -30,22 +41,48 @@ void HdsDelegate::establishNewStream() { | |
| return; | ||
| } | ||
|
|
||
| sendHealthCheckRequest(); | ||
| } | ||
|
|
||
| void HdsDelegate::sendHealthCheckRequest() { | ||
| ENVOY_LOG(debug, "Sending HealthCheckRequest"); | ||
| // TODO(lilika): Add support for other types of healthchecks | ||
| health_check_request_.mutable_capability()->add_health_check_protocol( | ||
| envoy::service::discovery::v2::Capability::HTTP); | ||
| ENVOY_LOG(debug, "Sending HealthCheckRequest {} ", health_check_request_.DebugString()); | ||
| stream_->sendMessage(health_check_request_, false); | ||
| stats_.responses_.inc(); | ||
| } | ||
|
|
||
| // TODO(lilika) : Use jittered backoff as in https://github.com/envoyproxy/envoy/pull/3791 | ||
| void HdsDelegate::handleFailure() { | ||
| ENVOY_LOG(warn, "Load reporter stats stream/connection failure, will retry in {} ms.", | ||
| RETRY_DELAY_MS); | ||
| ENVOY_LOG(warn, "HdsDelegate stream/connection failure, will retry in {} ms.", | ||
| RetryDelayMilliseconds); | ||
| stats_.errors_.inc(); | ||
| setRetryTimer(); | ||
| } | ||
|
|
||
| // TODO(lilika): Add support for the same endpoint in different clusters/ports | ||
| void HdsDelegate::sendResponse() { | ||
| envoy::service::discovery::v2::HealthCheckRequestOrEndpointHealthResponse response; | ||
| for (uint32_t i = 0; i < hds_clusters_.size(); i++) { | ||
| for (const auto& hosts : hds_clusters_[i]->prioritySet().hostSetsPerPriority()) { | ||
| for (const auto& host : hosts->hosts()) { | ||
| auto* endpoint = response.mutable_endpoint_health_response()->add_endpoints_health(); | ||
| endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_address( | ||
| host->address()->ip()->addressAsString()); | ||
| endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_port_value( | ||
| host->address()->ip()->port()); | ||
| // TODO(lilika): Add support for more granular options of envoy::api::v2::core::HealthStatus | ||
| if (host->healthy()) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a TODO to add support for the more granular notions of health status in |
||
| endpoint->set_health_status(envoy::api::v2::core::HealthStatus::HEALTHY); | ||
| } else { | ||
| endpoint->set_health_status(envoy::api::v2::core::HealthStatus::UNHEALTHY); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ENVOY_LOG(debug, "Sending EndpointHealthResponse to server {}", response.DebugString()); | ||
| stream_->sendMessage(response, false); | ||
| stats_.responses_.inc(); | ||
| setServerResponseTimer(); | ||
| } | ||
|
|
||
| void HdsDelegate::onCreateInitialMetadata(Http::HeaderMap& metadata) { | ||
| UNREFERENCED_PARAMETER(metadata); | ||
| } | ||
|
|
@@ -54,12 +91,71 @@ void HdsDelegate::onReceiveInitialMetadata(Http::HeaderMapPtr&& metadata) { | |
| UNREFERENCED_PARAMETER(metadata); | ||
| } | ||
|
|
||
| void HdsDelegate::processMessage( | ||
| std::unique_ptr<envoy::service::discovery::v2::HealthCheckSpecifier>&& message) { | ||
| ENVOY_LOG(debug, "New health check response message {} ", message->DebugString()); | ||
| ASSERT(message); | ||
| for (int i = 0; i < message->health_check_size(); i++) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this be for(const auto& health_check: message->health_check())? |
||
| // Create HdsCluster config | ||
| ENVOY_LOG(debug, "Creating HdsCluster config"); | ||
| envoy::api::v2::core::BindConfig bind_config; | ||
|
|
||
| clusters_config_.emplace_back(); | ||
| envoy::api::v2::Cluster& cluster_config = clusters_config_.back(); | ||
|
|
||
| cluster_config.set_name(message->health_check(i).cluster_name()); | ||
| cluster_config.mutable_connect_timeout()->set_seconds(ClusterTimeoutSeconds); | ||
| cluster_config.mutable_per_connection_buffer_limit_bytes()->set_value( | ||
| ClusterConnectionBufferLimitBytes); | ||
|
|
||
| // Add endpoints to cluster | ||
| for (int j = 0; j < message->health_check(i).endpoints_size(); j++) { | ||
| const auto& endpoints = message->health_check(i).endpoints(j); | ||
| for (int k = 0; k < endpoints.endpoints_size(); k++) { | ||
| cluster_config.add_hosts()->MergeFrom(endpoints.endpoints(k).address()); | ||
| } | ||
| } | ||
|
|
||
| // TODO(lilika): Add support for optional per-endpoint health checks | ||
|
|
||
| // Add healthcheckers to cluster | ||
| auto health_checkers = message->health_check(i); | ||
| for (int j = 0; j < health_checkers.health_checks_size(); j++) { | ||
| cluster_config.add_health_checks()->MergeFrom(health_checkers.health_checks(j)); | ||
| } | ||
|
|
||
| ENVOY_LOG(debug, "New HdsCluster config {} ", cluster_config.DebugString()); | ||
|
|
||
| // Create HdsCluster | ||
| hds_clusters_.emplace_back(new HdsCluster(runtime_, cluster_config, bind_config, store_stats, | ||
| ssl_context_manager_, secret_manager_, false, | ||
| info_factory_)); | ||
|
|
||
| for (int j = 0; j < message->health_check(i).health_checks_size(); j++) { | ||
| // Creating HealthCheckers | ||
| health_checkers_.push_back(Upstream::HealthCheckerFactory::create( | ||
| cluster_config.health_checks(j), *hds_clusters_[i], runtime_, random_, dispatcher_)); | ||
| } | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method looks right to me now, thanks. |
||
|
|
||
| // TODO(lilika): Add support for subsequent HealthCheckSpecifier messages that | ||
| // might modify the HdsClusters | ||
| void HdsDelegate::onReceiveMessage( | ||
| std::unique_ptr<envoy::service::discovery::v2::HealthCheckSpecifier>&& message) { | ||
| ENVOY_LOG(debug, "New health check response ", message->DebugString()); | ||
| stats_.requests_.inc(); | ||
| stream_->sendMessage(health_check_request_, false); | ||
| stats_.responses_.inc(); | ||
| ENVOY_LOG(debug, "New health check response message {} ", message->DebugString()); | ||
|
|
||
| // Process the HealthCheckSpecifier message | ||
| processMessage(std::move(message)); | ||
|
|
||
| // Initializing HealthCheckers | ||
| for (auto& health_checker : health_checkers_) { | ||
| health_checker->start(); | ||
| } | ||
|
|
||
| server_response_ms_ = PROTOBUF_GET_MS_REQUIRED(*message, interval); | ||
| setServerResponseTimer(); | ||
| } | ||
|
|
||
| void HdsDelegate::onReceiveTrailingMetadata(Http::HeaderMapPtr&& metadata) { | ||
|
|
@@ -68,10 +164,82 @@ void HdsDelegate::onReceiveTrailingMetadata(Http::HeaderMapPtr&& metadata) { | |
|
|
||
| void HdsDelegate::onRemoteClose(Grpc::Status::GrpcStatus status, const std::string& message) { | ||
| ENVOY_LOG(warn, "gRPC config stream closed: {}, {}", status, message); | ||
| response_timer_->disableTimer(); | ||
| server_response_timer_->disableTimer(); | ||
| stream_ = nullptr; | ||
| handleFailure(); | ||
| } | ||
|
|
||
| HdsCluster::HdsCluster(Runtime::Loader& runtime, const envoy::api::v2::Cluster& cluster, | ||
| const envoy::api::v2::core::BindConfig& bind_config, Stats::Store& stats, | ||
| Ssl::ContextManager& ssl_context_manager, | ||
| Secret::SecretManager& secret_manager, bool added_via_api, | ||
| ClusterInfoFactory& info_factory) | ||
| : runtime_(runtime), cluster_(cluster), bind_config_(bind_config), stats_(stats), | ||
| ssl_context_manager_(ssl_context_manager), secret_manager_(secret_manager), | ||
| added_via_api_(added_via_api), initial_hosts_(new HostVector()) { | ||
| ENVOY_LOG(debug, "Creating an HdsCluster"); | ||
| priority_set_.getOrCreateHostSet(0); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need to maintain priority levels or localities, so no need for a priority set. You just want a single |
||
|
|
||
| info_ = info_factory.createHdsClusterInfo(runtime_, cluster_, bind_config_, stats_, | ||
| ssl_context_manager_, secret_manager_, added_via_api_); | ||
|
|
||
| for (const auto& host : cluster.hosts()) { | ||
| initial_hosts_->emplace_back(HostSharedPtr{new HostImpl( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/initial_hosts_/hosts_/ i.e. this is the main data structure you need to maintain in |
||
| info_, "", resolveProtoAddress2(host), envoy::api::v2::core::Metadata::default_instance(), | ||
| 1, envoy::api::v2::core::Locality().default_instance(), | ||
| envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance())}); | ||
| } | ||
| initialize([] {}); | ||
| } | ||
|
|
||
| ClusterSharedPtr HdsCluster::create() { NOT_IMPLEMENTED; } | ||
|
|
||
| HostVectorConstSharedPtr HdsCluster::createHealthyHostList(const HostVector& hosts) { | ||
| HostVectorSharedPtr healthy_list(new HostVector()); | ||
| for (const auto& host : hosts) { | ||
| if (host->healthy()) { | ||
| healthy_list->emplace_back(host); | ||
| } | ||
| } | ||
| return healthy_list; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be an empty nop returning nullptr? |
||
| } | ||
|
|
||
| ClusterInfoConstSharedPtr RealClusterInfoFactory::createHdsClusterInfo( | ||
| Runtime::Loader& runtime, const envoy::api::v2::Cluster& cluster, | ||
| const envoy::api::v2::core::BindConfig& bind_config, Stats::Store& stats, | ||
| Ssl::ContextManager& ssl_context_manager, Secret::SecretManager& secret_manager, | ||
| bool added_via_api) { | ||
|
|
||
| return std::make_unique<ClusterInfoImpl>(cluster, bind_config, runtime, stats, | ||
| ssl_context_manager, secret_manager, added_via_api); | ||
| } | ||
|
|
||
| HostsPerLocalityConstSharedPtr HdsCluster::createHealthyHostLists(const HostsPerLocality& hosts) { | ||
| return hosts.filter([](const Host& host) { return host.healthy(); }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be an empty nop returning nullptr? |
||
| } | ||
|
|
||
| void HdsCluster::initialize(std::function<void()> callback) { | ||
| ASSERT(initialization_complete_callback_ == nullptr); | ||
| initialization_complete_callback_ = callback; | ||
| for (const auto& host : *initial_hosts_) { | ||
| host->healthFlagSet(Host::HealthFlag::FAILED_ACTIVE_HC); | ||
| } | ||
|
|
||
| auto& first_host_set = priority_set_.getOrCreateHostSet(0); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need the second half of this method. |
||
| auto healthy = createHealthyHostList(*initial_hosts_); | ||
| ENVOY_LOG(debug, "About to update the priority set."); | ||
| first_host_set.updateHosts(initial_hosts_, healthy, HostsPerLocalityImpl::empty(), | ||
| HostsPerLocalityImpl::empty(), {}, *initial_hosts_, {}); | ||
| initial_hosts_ = nullptr; | ||
| ENVOY_LOG(debug, "Updated the priority set."); | ||
| } | ||
|
|
||
| const Network::Address::InstanceConstSharedPtr | ||
| HdsCluster::resolveProtoAddress2(const envoy::api::v2::core::Address& address) { | ||
| return Network::Address::resolveProtoAddress(address); | ||
| } | ||
|
|
||
| void HdsCluster::setOutlierDetector(const Outlier::DetectorSharedPtr&) { NOT_IMPLEMENTED; } | ||
|
|
||
| } // namespace Upstream | ||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put this in
cluster_manager.horupstream.h; there should be nothing HDS specific in the interface (renamdcreateHdsClusterInfobelow tocreateClusterInfo.