From b489f30c0f96635a226013b3cef5875136d2a260 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Sun, 2 Oct 2016 09:21:36 -0700 Subject: [PATCH 1/2] outlier detection framework This is the outline of an outlier detection framework. It doesn't actually do anything right now. I want to get this deployed just to make sure that the basic flows look good and then I will start work on actual detection algorithms. --- include/envoy/upstream/host_description.h | 6 ++ include/envoy/upstream/outlier_detection.h | 53 +++++++++++++++++ include/envoy/upstream/upstream.h | 17 ++++-- source/common/CMakeLists.txt | 1 + source/common/http/codes.cc | 51 ++++++++-------- source/common/http/codes.h | 3 +- source/common/router/router.cc | 22 ++++--- .../common/upstream/cluster_manager_impl.cc | 2 + source/common/upstream/logical_dns_cluster.h | 3 + .../common/upstream/outlier_detection_impl.cc | 44 ++++++++++++++ .../common/upstream/outlier_detection_impl.h | 58 +++++++++++++++++++ .../common/upstream/resource_manager_impl.h | 2 +- source/common/upstream/upstream_impl.cc | 14 +++++ source/common/upstream/upstream_impl.h | 21 +++++++ test/CMakeLists.txt | 1 + test/common/http/codes_test.cc | 22 ++++--- test/common/router/router_test.cc | 7 +++ .../upstream/logical_dns_cluster_test.cc | 2 + .../upstream/outlier_detection_impl_test.cc | 58 +++++++++++++++++++ test/common/upstream/upstream_impl_test.cc | 38 ++++++++++++ test/mocks/upstream/host.h | 27 +++++++++ test/mocks/upstream/mocks.cc | 15 ++++- 22 files changed, 415 insertions(+), 52 deletions(-) create mode 100644 include/envoy/upstream/outlier_detection.h create mode 100644 source/common/upstream/outlier_detection_impl.cc create mode 100644 source/common/upstream/outlier_detection_impl.h create mode 100644 test/common/upstream/outlier_detection_impl_test.cc diff --git a/include/envoy/upstream/host_description.h b/include/envoy/upstream/host_description.h index 77b47105f55a..669589886b3d 100644 --- a/include/envoy/upstream/host_description.h +++ b/include/envoy/upstream/host_description.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/stats/stats_macros.h" +#include "envoy/upstream/outlier_detection.h" namespace Upstream { @@ -42,6 +43,11 @@ class HostDescription { */ virtual const Cluster& cluster() const PURE; + /** + * @return the host's outlier detection sink. + */ + virtual OutlierDetectorHostSink& outlierDetector() const PURE; + /** * @return the URL used to connect to the host. */ diff --git a/include/envoy/upstream/outlier_detection.h b/include/envoy/upstream/outlier_detection.h new file mode 100644 index 000000000000..8ebdf5d3ec4a --- /dev/null +++ b/include/envoy/upstream/outlier_detection.h @@ -0,0 +1,53 @@ +#pragma once + +#include "envoy/common/pure.h" + +namespace Upstream { + +class Host; +typedef std::shared_ptr HostPtr; + +/** + * Sink for per host data. Proxy filters should send pertinent data when available. + */ +class OutlierDetectorHostSink { +public: + virtual ~OutlierDetectorHostSink() {} + + /** + * Add an HTTP response code for a host. + */ + virtual void putHttpResponseCode(uint64_t code) PURE; + + /** + * Add a response time for a host (in this case response time is generic and might be used for + * different operations including HTTP, Mongo, Redis, etc.). + */ + virtual void putResponseTime(std::chrono::milliseconds time) PURE; +}; + +typedef std::unique_ptr OutlierDetectorHostSinkPtr; + +/** + * Interface for an outlier detection engine. Uses per host data to determine which hosts in a + * cluster are outliers and should be ejected. + */ +class OutlierDetector { +public: + virtual ~OutlierDetector() {} + + /** + * Outlier detection change state callback. + */ + typedef std::function ChangeStateCb; + + /** + * Add a changed state callback to the detector. The callback will be called whenever any host + * changes state (either ejected or brought back in) due to outlier status. + */ + virtual void addChangedStateCb(ChangeStateCb cb) PURE; +}; + +typedef std::unique_ptr OutlierDetectorPtr; + +} // Upstream diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 24f9ac074aae..2334233cd9c9 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -8,10 +8,6 @@ namespace Upstream { -class Host; -typedef std::shared_ptr HostPtr; -typedef std::shared_ptr ConstHostPtr; - /** * An upstream host. */ @@ -24,7 +20,9 @@ class Host : virtual public HostDescription { enum class HealthFlag { // The host is currently failing active health checks. - FAILED_ACTIVE_HC = 0x1 + FAILED_ACTIVE_HC = 0x1, + // The host is currently considered an outlier and has been ejected. + FAILED_OUTLIER_CHECK = 0x02 }; /** @@ -69,6 +67,13 @@ class Host : virtual public HostDescription { */ virtual bool healthy() const PURE; + /** + * Set the host's outlier detector. Outlier detectors are assumed to be thread safe, however + * a new outlier detector must be installed before the host is used across threads. Thus, + * this routine should only be called on the main thread before the host is used across threads. + */ + virtual void setOutlierDetector(OutlierDetectorHostSinkPtr&& outlier_detector) PURE; + /** * @return the current load balancing weight of the host, in the range 1-100. */ @@ -80,6 +85,8 @@ class Host : virtual public HostDescription { virtual void weight(uint32_t new_weight) PURE; }; +typedef std::shared_ptr ConstHostPtr; + /** * Base host set interface. This is used both for clusters, as well as per thread/worker host sets * used during routing/forwarding. diff --git a/source/common/CMakeLists.txt b/source/common/CMakeLists.txt index 559c17b5b721..f79ab09cb104 100644 --- a/source/common/CMakeLists.txt +++ b/source/common/CMakeLists.txt @@ -100,6 +100,7 @@ add_library( upstream/host_utility.cc upstream/load_balancer_impl.cc upstream/logical_dns_cluster.cc + upstream/outlier_detection_impl.cc upstream/sds.cc upstream/upstream_impl.cc ${gen_git_sha_target}) diff --git a/source/common/http/codes.cc b/source/common/http/codes.cc index 3ef9350921c4..031787b67e61 100644 --- a/source/common/http/codes.cc +++ b/source/common/http/codes.cc @@ -59,33 +59,30 @@ void CodeUtility::chargeResponseStat(const ResponseStatInfo& info) { } void CodeUtility::chargeResponseTiming(const ResponseTimingInfo& info) { - if (DateUtil::timePointValid(info.request_send_time_)) { - std::chrono::milliseconds ms = std::chrono::duration_cast( - std::chrono::system_clock::now() - info.request_send_time_); - - info.store_.deliverTimingToSinks(info.prefix_ + "upstream_rq_time", ms); - if (info.upstream_canary_) { - info.store_.deliverTimingToSinks(info.prefix_ + "canary.upstream_rq_time", ms); - } - - if (info.internal_request_) { - info.store_.deliverTimingToSinks(info.prefix_ + "internal.upstream_rq_time", ms); - } else { - info.store_.deliverTimingToSinks(info.prefix_ + "external.upstream_rq_time", ms); - } - - if (!info.request_vcluster_name_.empty()) { - info.store_.deliverTimingToSinks("vhost." + info.request_vhost_name_ + ".vcluster." + - info.request_vcluster_name_ + ".upstream_rq_time", - ms); - } - - // Handle per zone stats. - if (!info.from_zone_.empty() && !info.to_zone_.empty()) { - info.store_.deliverTimingToSinks(fmt::format("{}zone.{}.{}.upstream_rq_time", info.prefix_, - info.from_zone_, info.to_zone_), - ms); - } + info.store_.deliverTimingToSinks(info.prefix_ + "upstream_rq_time", info.response_time_); + if (info.upstream_canary_) { + info.store_.deliverTimingToSinks(info.prefix_ + "canary.upstream_rq_time", info.response_time_); + } + + if (info.internal_request_) { + info.store_.deliverTimingToSinks(info.prefix_ + "internal.upstream_rq_time", + info.response_time_); + } else { + info.store_.deliverTimingToSinks(info.prefix_ + "external.upstream_rq_time", + info.response_time_); + } + + if (!info.request_vcluster_name_.empty()) { + info.store_.deliverTimingToSinks("vhost." + info.request_vhost_name_ + ".vcluster." + + info.request_vcluster_name_ + ".upstream_rq_time", + info.response_time_); + } + + // Handle per zone stats. + if (!info.from_zone_.empty() && !info.to_zone_.empty()) { + info.store_.deliverTimingToSinks( + fmt::format("{}zone.{}.{}.upstream_rq_time", info.prefix_, info.from_zone_, info.to_zone_), + info.response_time_); } } diff --git a/source/common/http/codes.h b/source/common/http/codes.h index 248e982e9b03..37e72fda8422 100644 --- a/source/common/http/codes.h +++ b/source/common/http/codes.h @@ -1,6 +1,5 @@ #pragma once -#include "envoy/common/time.h" #include "envoy/http/codes.h" #include "envoy/http/header_map.h" #include "envoy/stats/stats.h" @@ -40,7 +39,7 @@ class CodeUtility { struct ResponseTimingInfo { Stats::Store& store_; const std::string& prefix_; - SystemTime request_send_time_; + std::chrono::milliseconds response_time_; bool upstream_canary_; bool internal_request_; const std::string& request_vhost_name_; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 3bfe20043bb2..0d59647760b3 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -98,6 +98,11 @@ void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) { bool is_canary = (response_headers.get(Http::Headers::get().EnvoyUpstreamCanary) == "true") || (upstream_host_ ? upstream_host_->canary() : false); + if (upstream_host_) { + upstream_host_->outlierDetector().putHttpResponseCode( + Http::Utility::getResponseStatus(response_headers)); + } + Http::CodeUtility::ResponseStatInfo info{ config_.stats_store_, stat_prefix_, response_headers, downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true", @@ -430,11 +435,16 @@ void Filter::onUpstreamComplete() { upstream_request_->upstream_encoder_->resetStream(); } - if (config_.emit_dynamic_stats_ && !callbacks_->requestInfo().healthCheck()) { + if (config_.emit_dynamic_stats_ && !callbacks_->requestInfo().healthCheck() && + DateUtil::timePointValid(upstream_request_->upstream_encoder_->requestCompleteTime())) { + std::chrono::milliseconds response_time = std::chrono::duration_cast( + std::chrono::system_clock::now() - + upstream_request_->upstream_encoder_->requestCompleteTime()); + + upstream_host_->outlierDetector().putResponseTime(response_time); + Http::CodeUtility::ResponseTimingInfo info{ - config_.stats_store_, stat_prefix_, - upstream_request_->upstream_encoder_->requestCompleteTime(), - upstream_request_->upstream_canary_, + config_.stats_store_, stat_prefix_, response_time, upstream_request_->upstream_canary_, downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true", route_->virtualHostName(), request_vcluster_ ? request_vcluster_->name() : "", config_.service_zone_, upstreamZone()}; @@ -443,9 +453,7 @@ void Filter::onUpstreamComplete() { for (const std::string& alt_prefix : alt_stat_prefixes_) { Http::CodeUtility::ResponseTimingInfo info{ - config_.stats_store_, alt_prefix, - upstream_request_->upstream_encoder_->requestCompleteTime(), - upstream_request_->upstream_canary_, + config_.stats_store_, alt_prefix, response_time, upstream_request_->upstream_canary_, downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true", "", "", config_.service_zone_, upstreamZone()}; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 1e3452f9a05e..263cc8491046 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -130,6 +130,8 @@ void ClusterManagerImpl::loadCluster(const Json::Object& cluster, Stats::Store& } } + new_cluster->setOutlierDetector(OutlierDetectorImplFactory::createForCluster( + *new_cluster, cluster, dns_resolver.dispatcher())); primary_clusters_.emplace(new_cluster->name(), new_cluster); } diff --git a/source/common/upstream/logical_dns_cluster.h b/source/common/upstream/logical_dns_cluster.h index 5d13ebd9b93a..504199cc610d 100644 --- a/source/common/upstream/logical_dns_cluster.h +++ b/source/common/upstream/logical_dns_cluster.h @@ -51,6 +51,9 @@ class LogicalDnsCluster : public ClusterImplBase { // Upstream:HostDescription bool canary() const override { return false; } const Cluster& cluster() const override { return logical_host_->cluster(); } + OutlierDetectorHostSink& outlierDetector() const override { + return logical_host_->outlierDetector(); + } const HostStats& stats() const override { return logical_host_->stats(); } const std::string& url() const override { return url_; } const std::string& zone() const override { return EMPTY_STRING; } diff --git a/source/common/upstream/outlier_detection_impl.cc b/source/common/upstream/outlier_detection_impl.cc new file mode 100644 index 000000000000..fd68de30a022 --- /dev/null +++ b/source/common/upstream/outlier_detection_impl.cc @@ -0,0 +1,44 @@ +#include "outlier_detection_impl.h" + +#include "common/common/assert.h" + +namespace Upstream { + +OutlierDetectorPtr OutlierDetectorImplFactory::createForCluster(Cluster& cluster, + const Json::Object& cluster_config, + Event::Dispatcher& dispatcher) { + // Right now we don't support any configuration but in order to make the config backwards + // compatible we just look for an empty object. + if (cluster_config.hasObject("outlier_detection")) { + return OutlierDetectorPtr{new OutlierDetectorImpl(cluster, dispatcher)}; + } else { + return nullptr; + } +} + +OutlierDetectorImpl::OutlierDetectorImpl(Cluster& cluster, Event::Dispatcher&) { + for (HostPtr host : cluster.hosts()) { + addHostSink(host); + } + + cluster.addMemberUpdateCb([this](const std::vector& hosts_added, + const std::vector& hosts_removed) -> void { + for (HostPtr host : hosts_added) { + addHostSink(host); + } + + for (HostPtr host : hosts_removed) { + ASSERT(host_sinks_.count(host) == 1); + host_sinks_.erase(host); + } + }); +} + +void OutlierDetectorImpl::addHostSink(HostPtr host) { + ASSERT(host_sinks_.count(host) == 0); + OutlierDetectorHostSinkImpl* sink = new OutlierDetectorHostSinkImpl(); + host_sinks_[host] = sink; + host->setOutlierDetector(OutlierDetectorHostSinkPtr{sink}); +} + +} // Upstream diff --git a/source/common/upstream/outlier_detection_impl.h b/source/common/upstream/outlier_detection_impl.h new file mode 100644 index 000000000000..5219fc0b8975 --- /dev/null +++ b/source/common/upstream/outlier_detection_impl.h @@ -0,0 +1,58 @@ +#pragma once + +#include "envoy/upstream/outlier_detection.h" +#include "envoy/upstream/upstream.h" + +#include "common/json/json_loader.h" + +namespace Upstream { + +/** + * Null host sink implementation. + */ +class OutlierDetectorHostSinkNullImpl : public OutlierDetectorHostSink { +public: + // Upstream::OutlierDetectorHostSink + void putHttpResponseCode(uint64_t) override {} + void putResponseTime(std::chrono::milliseconds) override {} +}; + +/** + * Factory for creating a detector from a JSON configuration. + */ +class OutlierDetectorImplFactory { +public: + static OutlierDetectorPtr createForCluster(Cluster& cluster, const Json::Object& cluster_config, + Event::Dispatcher& dispatcher); +}; + +/** + * Implementation of OutlierDetectorHostSink for the generic detector. + */ +class OutlierDetectorHostSinkImpl : public OutlierDetectorHostSink { +public: + // Upstream::OutlierDetectorHostSink + void putHttpResponseCode(uint64_t) override {} + void putResponseTime(std::chrono::milliseconds) override {} +}; + +/** + * An implementation of an outlier detector. In the future we may support multiple outlier detection + * implementations with different configuration. For now, as we iterate everything is contained + * within this implementation. + */ +class OutlierDetectorImpl : public OutlierDetector { +public: + OutlierDetectorImpl(Cluster& cluster, Event::Dispatcher& dispatcher); + + // Upstream::OutlierDetector + void addChangedStateCb(ChangeStateCb cb) override { callbacks_.push_back(cb); } + +private: + void addHostSink(HostPtr host); + + std::list callbacks_; + std::unordered_map host_sinks_; +}; + +} // Upstream diff --git a/source/common/upstream/resource_manager_impl.h b/source/common/upstream/resource_manager_impl.h index 0fde747c69ff..11dfe45eba35 100644 --- a/source/common/upstream/resource_manager_impl.h +++ b/source/common/upstream/resource_manager_impl.h @@ -50,7 +50,7 @@ class ResourceManagerImpl : public ResourceManager { const uint64_t max_; std::atomic current_{}; Runtime::Loader& runtime_; - std::string runtime_key_; + const std::string runtime_key_; }; ResourceImpl connections_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 5915f4c3502a..f33c841c6057 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -16,6 +16,8 @@ namespace Upstream { +OutlierDetectorHostSinkNullImpl HostDescriptionImpl::null_outlier_detector_; + Host::CreateConnectionData HostImpl::createConnection(Event::Dispatcher& dispatcher) const { return {createConnection(dispatcher, cluster_, url_), shared_from_this()}; } @@ -134,6 +136,18 @@ void ClusterImplBase::setHealthChecker(HealthCheckerPtr&& health_checker) { }); } +void ClusterImplBase::setOutlierDetector(OutlierDetectorPtr&& outlier_detector) { + if (!outlier_detector) { + return; + } + + outlier_detector_ = std::move(outlier_detector); + outlier_detector_->addChangedStateCb([this](HostPtr) -> void { + updateHosts(rawHosts(), createHealthyHostList(*rawHosts()), rawLocalZoneHosts(), + createHealthyHostList(*rawLocalZoneHosts()), {}, {}); + }); +} + ClusterImplBase::ResourceManagers::ResourceManagers(const Json::Object& config, Runtime::Loader& runtime, const std::string& cluster_name) { diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 0b7573af771c..5cccee519bca 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -1,5 +1,6 @@ #pragma once +#include "outlier_detection_impl.h" #include "resource_manager_impl.h" #include "envoy/event/timer.h" @@ -32,6 +33,13 @@ class HostDescriptionImpl : virtual public HostDescription { // Upstream::HostDescription bool canary() const override { return canary_; } const Cluster& cluster() const override { return cluster_; } + OutlierDetectorHostSink& outlierDetector() const override { + if (outlier_detector_) { + return *outlier_detector_; + } else { + return null_outlier_detector_; + } + } const HostStats& stats() const override { return stats_; } const std::string& url() const override { return url_; } const std::string& zone() const override { return zone_; } @@ -43,9 +51,12 @@ class HostDescriptionImpl : virtual public HostDescription { const std::string zone_; Stats::IsolatedStoreImpl stats_store_; HostStats stats_; + OutlierDetectorHostSinkPtr outlier_detector_; private: void checkUrl(); + + static OutlierDetectorHostSinkNullImpl null_outlier_detector_; }; /** @@ -72,6 +83,9 @@ class HostImpl : public HostDescriptionImpl, void healthFlagClear(HealthFlag flag) override { health_flags_ &= ~enumToInt(flag); } bool healthFlagGet(HealthFlag flag) const override { return health_flags_ & enumToInt(flag); } void healthFlagSet(HealthFlag flag) override { health_flags_ |= enumToInt(flag); } + void setOutlierDetector(OutlierDetectorHostSinkPtr&& outlier_detector) override { + outlier_detector_ = std::move(outlier_detector); + } bool healthy() const override { return !health_flags_; } uint32_t weight() const override { return weight_; } void weight(uint32_t new_weight); @@ -150,6 +164,12 @@ class ClusterImplBase : public Cluster, */ void setHealthChecker(HealthCheckerPtr&& health_checker); + /** + * Optionally set the outlier detector for the primary cluster. Done for the same reason as + * documented in setHealthChecker(). + */ + void setOutlierDetector(OutlierDetectorPtr&& outlier_detector); + // Upstream::Cluster const std::string& altStatName() const override { return alt_stat_name_; } std::chrono::milliseconds connectTimeout() const override { return connect_timeout_; } @@ -181,6 +201,7 @@ class ClusterImplBase : public Cluster, HealthCheckerPtr health_checker_; std::string alt_stat_name_; uint64_t features_; + OutlierDetectorPtr outlier_detector_; private: struct ResourceManagers { diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a6d9a00b9a86..b543ed1376a1 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -86,6 +86,7 @@ add_executable(envoy-test common/upstream/load_balancer_impl_test.cc common/upstream/load_balancer_simulation_test.cc common/upstream/logical_dns_cluster_test.cc + common/upstream/outlier_detection_impl_test.cc common/upstream/resource_manager_impl_test.cc common/upstream/sds_test.cc common/upstream/upstream_impl_test.cc diff --git a/test/common/http/codes_test.cc b/test/common/http/codes_test.cc index 4c12ae49efbc..3d46978c4f5b 100644 --- a/test/common/http/codes_test.cc +++ b/test/common/http/codes_test.cc @@ -157,15 +157,19 @@ TEST_F(CodeUtilityTest, PerZoneStats) { TEST(CodeUtilityResponseTimingTest, All) { Stats::MockStore store; - CodeUtility::ResponseTimingInfo info{store, "prefix.", std::chrono::system_clock::now(), true, - true, "vhost_name", "req_vcluster_name", "from_az", "to_az"}; - - EXPECT_CALL(store, deliverTimingToSinks("prefix.upstream_rq_time", _)); - EXPECT_CALL(store, deliverTimingToSinks("prefix.canary.upstream_rq_time", _)); - EXPECT_CALL(store, deliverTimingToSinks("prefix.internal.upstream_rq_time", _)); - EXPECT_CALL(store, deliverTimingToSinks( - "vhost.vhost_name.vcluster.req_vcluster_name.upstream_rq_time", _)); - EXPECT_CALL(store, deliverTimingToSinks("prefix.zone.from_az.to_az.upstream_rq_time", _)); + CodeUtility::ResponseTimingInfo info{store, "prefix.", std::chrono::milliseconds(5), true, true, + "vhost_name", "req_vcluster_name", "from_az", "to_az"}; + + EXPECT_CALL(store, deliverTimingToSinks("prefix.upstream_rq_time", std::chrono::milliseconds(5))); + EXPECT_CALL(store, + deliverTimingToSinks("prefix.canary.upstream_rq_time", std::chrono::milliseconds(5))); + EXPECT_CALL(store, deliverTimingToSinks("prefix.internal.upstream_rq_time", + std::chrono::milliseconds(5))); + EXPECT_CALL(store, + deliverTimingToSinks("vhost.vhost_name.vcluster.req_vcluster_name.upstream_rq_time", + std::chrono::milliseconds(5))); + EXPECT_CALL(store, deliverTimingToSinks("prefix.zone.from_az.to_az.upstream_rq_time", + std::chrono::milliseconds(5))); CodeUtility::chargeResponseTiming(info); } diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 9cfd176d3dee..2e3c571bb1a3 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -472,6 +472,8 @@ TEST_F(RouterTest, RetryTimeoutDuringRetryDelay) { // Fire timeout. EXPECT_CALL(callbacks_.request_info_, onFailedResponse(Http::AccessLog::FailureReason::UpstreamRequestTimeout)); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(504)); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putResponseTime(_)).Times(0); Http::HeaderMapImpl response_headers{ {":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}}; EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(response_headers), false)); @@ -525,6 +527,8 @@ TEST_F(RouterTest, RetryUpstream5xxNotComplete) { // Normal response. EXPECT_CALL(*router_.retry_state_, shouldRetry(_, _, _)).WillOnce(Return(false)); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putResponseTime(_)); Http::HeaderMapPtr response_headers2(new Http::HeaderMapImpl{{":status", "200"}}); response_decoder->decodeHeaders(std::move(response_headers2), true); @@ -597,6 +601,9 @@ TEST_F(RouterTest, AltStatName) { HttpTestUtility::addDefaultHeaders(headers); router_.decodeHeaders(headers, true); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putResponseTime(_)); + Http::HeaderMapPtr response_headers( new Http::HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "true"}, diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index da67a259fea5..bc36c2cf6cd7 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -85,6 +85,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { EXPECT_CALL(dns_resolver_.dispatcher_, createClientConnection_("tcp://127.0.0.1:443")); logical_host->createConnection(dns_resolver_.dispatcher_); + logical_host->outlierDetector().putHttpResponseCode(200); expectResolve(); resolve_timer_->callback_(); @@ -101,6 +102,7 @@ TEST_F(LogicalDnsClusterTest, Basic) { EXPECT_EQ(&cluster_->hosts()[0]->stats(), &data.host_description_->stats()); EXPECT_EQ("tcp://127.0.0.1:443", data.host_description_->url()); EXPECT_EQ("", data.host_description_->zone()); + data.host_description_->outlierDetector().putHttpResponseCode(200); expectResolve(); resolve_timer_->callback_(); diff --git a/test/common/upstream/outlier_detection_impl_test.cc b/test/common/upstream/outlier_detection_impl_test.cc new file mode 100644 index 000000000000..61471e62c44f --- /dev/null +++ b/test/common/upstream/outlier_detection_impl_test.cc @@ -0,0 +1,58 @@ +#include "common/upstream/outlier_detection_impl.h" +#include "common/upstream/upstream_impl.h" + +#include "test/mocks/event/mocks.h" +#include "test/mocks/upstream/mocks.h" + +using testing::_; +using testing::NiceMock; + +namespace Upstream { + +TEST(OutlierDetectorImplFactoryTest, NoDetector) { + std::string json = R"EOF( + { + } + )EOF"; + + Json::StringLoader loader(json); + MockCluster cluster; + Event::MockDispatcher dispatcher; + EXPECT_EQ(nullptr, OutlierDetectorImplFactory::createForCluster(cluster, loader, dispatcher)); +} + +TEST(OutlierDetectorImplFactoryTest, Detector) { + std::string json = R"EOF( + { + "outlier_detection": {} + } + )EOF"; + + Json::StringLoader loader(json); + NiceMock cluster; + NiceMock dispatcher; + EXPECT_NE(nullptr, OutlierDetectorImplFactory::createForCluster(cluster, loader, dispatcher)); +} + +TEST(OutlierDetectorImplTest, Callbacks) { + NiceMock cluster; + Event::MockDispatcher dispatcher; + + EXPECT_CALL(cluster, addMemberUpdateCb(_)); + cluster.hosts_ = {HostPtr{new HostImpl(cluster, "tcp://127.0.0.1:80", false, 1, "")}}; + OutlierDetectorImpl detector(cluster, dispatcher); + + // Set up callback. Will replace later with real test when we have real functionality. + detector.addChangedStateCb([](HostPtr) -> void {}); + + cluster.hosts_.push_back(HostPtr{new HostImpl(cluster, "tcp://127.0.0.1:81", false, 1, "")}); + cluster.runCallbacks({cluster.hosts_[1]}, {}); + + // Trivial call through tests to be replaced later with real functionality. + cluster.hosts_[0]->outlierDetector().putHttpResponseCode(200); + cluster.hosts_[0]->outlierDetector().putResponseTime(std::chrono::milliseconds(5)); + + cluster.runCallbacks({}, cluster.hosts_); +} + +} // Upstream diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index db4ac8f6c46a..bd4dc0637733 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -186,6 +186,44 @@ TEST(HostImplTest, MalformedUrl) { EXPECT_THROW(HostImpl(cluster, "fake\\10.0.0.1:1234", false, 1, ""), EnvoyException); } +TEST(StaticClusterImplTest, OutlierDetector) { + Stats::IsolatedStoreImpl stats; + Ssl::MockContextManager ssl_context_manager; + NiceMock runtime; + std::string json = R"EOF( + { + "name": "addressportconfig", + "connect_timeout_ms": 250, + "type": "static", + "lb_type": "random", + "hosts": [{"url": "tcp://10.0.0.1:11001"}, + {"url": "tcp://10.0.0.2:11002"}] + } + )EOF"; + + Json::StringLoader config(json); + StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager); + + MockOutlierDetector* detector = new MockOutlierDetector(); + EXPECT_CALL(*detector, addChangedStateCb(_)); + cluster.setOutlierDetector(OutlierDetectorPtr{detector}); + + EXPECT_EQ(2UL, cluster.healthyHosts().size()); + + // Set a single host as having failed and fire outlier detector callbacks. This should result + // in only a single healthy host. + cluster.hosts()[0]->outlierDetector().putHttpResponseCode(503); + cluster.hosts()[0]->healthFlagSet(Host::HealthFlag::FAILED_OUTLIER_CHECK); + detector->runCallbacks(cluster.hosts()[0]); + EXPECT_EQ(1UL, cluster.healthyHosts().size()); + EXPECT_NE(cluster.healthyHosts()[0], cluster.hosts()[0]); + + // Bring the host back online. + cluster.hosts()[0]->healthFlagClear(Host::HealthFlag::FAILED_OUTLIER_CHECK); + detector->runCallbacks(cluster.hosts()[0]); + EXPECT_EQ(2UL, cluster.healthyHosts().size()); +} + TEST(StaticClusterImplTest, UrlConfig) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; diff --git a/test/mocks/upstream/host.h b/test/mocks/upstream/host.h index d0096c04d325..2aab0feedcdf 100644 --- a/test/mocks/upstream/host.h +++ b/test/mocks/upstream/host.h @@ -4,6 +4,31 @@ namespace Upstream { +class MockOutlierDetectorHostSink : public OutlierDetectorHostSink { +public: + MockOutlierDetectorHostSink(); + ~MockOutlierDetectorHostSink(); + + MOCK_METHOD1(putHttpResponseCode, void(uint64_t code)); + MOCK_METHOD1(putResponseTime, void(std::chrono::milliseconds time)); +}; + +class MockOutlierDetector : public OutlierDetector { +public: + MockOutlierDetector(); + ~MockOutlierDetector(); + + void runCallbacks(HostPtr host) { + for (ChangeStateCb cb : callbacks_) { + cb(host); + } + } + + MOCK_METHOD1(addChangedStateCb, void(ChangeStateCb cb)); + + std::list callbacks_; +}; + class MockHostDescription : public HostDescription { public: MockHostDescription(); @@ -11,11 +36,13 @@ class MockHostDescription : public HostDescription { MOCK_CONST_METHOD0(canary, bool()); MOCK_CONST_METHOD0(cluster, const Cluster&()); + MOCK_CONST_METHOD0(outlierDetector, OutlierDetectorHostSink&()); MOCK_CONST_METHOD0(url, const std::string&()); MOCK_CONST_METHOD0(stats, HostStats&()); MOCK_CONST_METHOD0(zone, const std::string&()); std::string url_{"tcp://10.0.0.1:443"}; + testing::NiceMock outlier_detector_; }; class MockHost : public Host { diff --git a/test/mocks/upstream/mocks.cc b/test/mocks/upstream/mocks.cc index 5700dc065c97..66f9fcdaea74 100644 --- a/test/mocks/upstream/mocks.cc +++ b/test/mocks/upstream/mocks.cc @@ -12,7 +12,20 @@ using testing::ReturnRef; namespace Upstream { -MockHostDescription::MockHostDescription() { ON_CALL(*this, url()).WillByDefault(ReturnRef(url_)); } +MockOutlierDetectorHostSink::MockOutlierDetectorHostSink() {} +MockOutlierDetectorHostSink::~MockOutlierDetectorHostSink() {} + +MockOutlierDetector::MockOutlierDetector() { + ON_CALL(*this, addChangedStateCb(_)) + .WillByDefault(Invoke([this](ChangeStateCb cb) -> void { callbacks_.push_back(cb); })); +} + +MockOutlierDetector::~MockOutlierDetector() {} + +MockHostDescription::MockHostDescription() { + ON_CALL(*this, url()).WillByDefault(ReturnRef(url_)); + ON_CALL(*this, outlierDetector()).WillByDefault(ReturnRef(outlier_detector_)); +} MockHostDescription::~MockHostDescription() {} From e64f65c7c57b5087ba7f165d6862d052b0709a76 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Wed, 12 Oct 2016 12:18:32 -0700 Subject: [PATCH 2/2] comment --- test/common/upstream/outlier_detection_impl_test.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/test/common/upstream/outlier_detection_impl_test.cc b/test/common/upstream/outlier_detection_impl_test.cc index 61471e62c44f..53614d8b98df 100644 --- a/test/common/upstream/outlier_detection_impl_test.cc +++ b/test/common/upstream/outlier_detection_impl_test.cc @@ -10,12 +10,7 @@ using testing::NiceMock; namespace Upstream { TEST(OutlierDetectorImplFactoryTest, NoDetector) { - std::string json = R"EOF( - { - } - )EOF"; - - Json::StringLoader loader(json); + Json::StringLoader loader("{}"); MockCluster cluster; Event::MockDispatcher dispatcher; EXPECT_EQ(nullptr, OutlierDetectorImplFactory::createForCluster(cluster, loader, dispatcher));