From fdbc217f46109b852f8297171a85cb3a54de0681 Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Thu, 16 Apr 2026 11:46:31 +0900 Subject: [PATCH 1/2] Clarify HostDBInfo state --- .../core-architecture/hostdb.en.rst | 14 +- include/iocore/hostdb/HostDBProcessor.h | 235 ++++++------------ src/iocore/hostdb/CMakeLists.txt | 1 + src/iocore/hostdb/HostDB.cc | 8 +- src/iocore/hostdb/HostDBInfo.cc | 153 ++++++++++++ src/iocore/hostdb/unit_tests/CMakeLists.txt | 22 ++ .../hostdb/unit_tests/test_HostDBInfo.cc | 214 ++++++++++++++++ src/mgmt/rpc/handlers/hostdb/HostDB.cc | 4 +- src/proxy/http/HttpSM.cc | 7 +- 9 files changed, 484 insertions(+), 174 deletions(-) create mode 100644 src/iocore/hostdb/unit_tests/CMakeLists.txt create mode 100644 src/iocore/hostdb/unit_tests/test_HostDBInfo.cc diff --git a/doc/developer-guide/core-architecture/hostdb.en.rst b/doc/developer-guide/core-architecture/hostdb.en.rst index 18a0e06e61d..0666c8bfb28 100644 --- a/doc/developer-guide/core-architecture/hostdb.en.rst +++ b/doc/developer-guide/core-architecture/hostdb.en.rst @@ -50,12 +50,10 @@ a flag, where a value of ``TS_TIME_ZERO`` indicates a live target and any other down info. If an info is marked down (has a non-zero last failure time) there is a "fail window" during which -no connections are permitted. After this time the info is considered to be a "zombie". If all infos +no connections are permitted. After this time the info is considered to be a "suspect". If all infos for a record are down then a specific error message is generated (body factory tag -"connect#all_down"). Otherwise if the selected info is a zombie, a request is permitted but the -zombie is immediately marked down again, preventing any additional requests until either the fail -window has passed or the single connection succeeds. A successful connection clears the last file -time and the info becomes alive. +"connect#all_down"). Otherwise if the selected info is a suspect, connections are permitted and the +info will transition back to up on success or down on failure. Runtime Structure ================= @@ -152,8 +150,8 @@ Future There is still some work to be done in future PRs. -* The fail window and the zombie window should be separate values. It is quite reasonable to want - to configure a very short fail window (possibly 0) with a moderately long zombie window so that +* The fail window and the suspect window should be separate values. It is quite reasonable to want + to configure a very short fail window (possibly 0) with a moderately long suspect window so that probing connections can immediately start going upstream at a low rate. * Failing an upstream should be more loosely connected to transactions. Currently there is a one @@ -189,7 +187,7 @@ This version has several major architectural changes from the previous version. * State information has been promoted to atomics and updates are immediate rather than scheduled. This also means the data in the state machine is a reference to a shared object, not a local copy. - The promotion was necessary to coordinate zombie connections to upstreams marked down across transactions. + The promotion was necessary to coordinate suspect connections to upstreams marked down across transactions. * The "resolve key" is now a separate data object from the HTTP request. This is a subtle but major change. The effect is requests can be routed to different upstreams without changing diff --git a/include/iocore/hostdb/HostDBProcessor.h b/include/iocore/hostdb/HostDBProcessor.h index fecb1abef7b..15f8133aaed 100644 --- a/include/iocore/hostdb/HostDBProcessor.h +++ b/include/iocore/hostdb/HostDBProcessor.h @@ -123,10 +123,52 @@ enum class HostDBType : uint8_t { }; /** Information about a single target. + * + * Each instance tracks the health state of one upstream address. The state is derived from @c last_failure and the caller-supplied + * @a fail_window: + * + * | State | Description | + * |---------|----------------------------------------------------------------------------------| + * | Up | No known failure; eligible for normal selection. | + * | Down | Blocked; no connections permitted until @c last_failure + @a fail_window elapses | + * | Suspect | Fail window has elapsed; connections are permitted. | + * | | On success transitions to Up (@c mark_up); on failure returns to Down. | + * + * State transition diagram: + * + * @startuml + * hide empty description + * + * [*] --> Up + * Up --> Down : connect failure\n(mark_down) + * Down --> Suspect : fail_window elapses + * Suspect --> Up : connect success\n(mark_up) + * Suspect --> Down : connect failure\n(mark_down) + * @enduml + * + * State transition and `fail_window` time chart: + * + * |<-- fail_window -->| + * -+----------+--------------------+--------------------+----------+----> time + * | Up | Down | Suspect | Up | + * -+----------+--------------------+--------------------+----------+----> + * ^ ^ ^ + * \ \ \ + * (last_failure) (last_failure + fail_window) (connect success) + * */ -struct HostDBInfo { +class HostDBInfo +{ +public: using self_type = HostDBInfo; ///< Self reference type. + /// Health state of this target. + enum class State { + UP, + DOWN, + SUSPECT, + }; + /// Default constructor. HostDBInfo() = default; @@ -134,50 +176,23 @@ struct HostDBInfo { /// Absolute time of when this target failed. /// A value of zero (@c TS_TIME_ZERO ) indicates no failure. - ts_time last_fail_time() const; - - /// Target is alive - no known failure. - bool is_alive(); - - /// Target has failed and is still in the blocked time window. - bool is_down(ts_time now, ts_seconds fail_window); - - /** Select this target. - * - * @param now Current time. - * @param fail_window Failure window. - * @return Status of the selection. - * - * If a zombie is selected the failure time is updated to make it appear down to other threads in a thread safe - * manner. The caller should check @c last_fail_time to see if a zombie was selected. - */ - bool select(ts_time now, ts_seconds fail_window) const; - - /** Mark the entry as down. - * - * @param now Time of the failure. - * @return @c true if @a this was marked down, @c false if not. - * - * This can return @c false if the entry is already marked down, in which case the failure time is not updated. - */ - bool mark_down(ts_time now); + ts_time last_fail_time() const; + uint8_t fail_count() const; + char const *srvname() const; - std::pair increment_fail_count(ts_time now, uint8_t max_retries); + /// Return the current health state of this target. + State state(ts_time now, ts_seconds fail_window) const; - /** Mark the target as up / alive. - * - * @return Previous alive state of the target. - */ - bool mark_up(); + // Sugars of checking state + bool is_up() const; + bool is_down(ts_time now, ts_seconds fail_window) const; + bool is_suspect(ts_time now, ts_seconds fail_window) const; - char const *srvname() const; + // State controllers + bool mark_up(); + bool mark_down(ts_time now, ts_seconds fail_window); + std::pair increment_fail_count(ts_time now, uint8_t max_retries, ts_seconds fail_window); - /** Migrate data after a DNS update. - * - * @param that Source item. - * - * This moves only specific state information, it is not a generic copy. - */ void migrate_from(self_type const &that); /// A target is either an IP address or an SRV record. @@ -187,16 +202,8 @@ struct HostDBInfo { SRVInfo srv; ///< SRV record. } data{IpAddr{}}; - /// Data that migrates after updated DNS records are processed. - /// @see migrate_from - /// @{ - /// Last time a failure was recorded. - std::atomic last_failure{TS_TIME_ZERO}; - /// Count of connection failures - std::atomic fail_count{0}; /// Expected HTTP version of the target based on earlier transactions. HTTPVersion http_version = HTTP_INVALID; - /// @} self_type &assign(IpAddr const &addr); @@ -207,96 +214,12 @@ struct HostDBInfo { HostDBType type = HostDBType::UNSPEC; ///< Invalid data. friend HostDBContinuation; -}; - -inline HostDBInfo & -HostDBInfo::operator=(HostDBInfo const &that) -{ - if (this != &that) { - memcpy(static_cast(this), static_cast(&that), sizeof(*this)); - } - return *this; -} - -inline ts_time -HostDBInfo::last_fail_time() const -{ - return last_failure; -} - -inline bool -HostDBInfo::is_alive() -{ - return this->last_fail_time() == TS_TIME_ZERO; -} - -/** - Check if this HostDBInfo is currently marked DOWN (true) or UP (false). Returns true while within the `fail_window` period after - `last_failure`. Once `fail_window` expires, the host is treated as UP and this function returns false. - - |<-- fail_window -->| - ----------------+-------------------+-----------------> time - UP | DOWN | UP - (is_down=false) | (is_down=true) | (is_down=false) - | | - ^ ^ - \ \ - last_failure last_failure + fail_window - */ -inline bool -HostDBInfo::is_down(ts_time now, ts_seconds fail_window) -{ - auto last_fail = this->last_fail_time(); - return (last_fail != TS_TIME_ZERO) && (now <= last_fail + fail_window); -} - -inline bool -HostDBInfo::mark_up() -{ - auto t = last_failure.exchange(TS_TIME_ZERO); - bool was_down = t != TS_TIME_ZERO; - if (was_down) { - fail_count.store(0); - } - return was_down; -} - -inline bool -HostDBInfo::mark_down(ts_time now) -{ - auto t0{TS_TIME_ZERO}; - return last_failure.compare_exchange_strong(t0, now); -} - -inline std::pair -HostDBInfo::increment_fail_count(ts_time now, uint8_t max_retries) -{ - auto fcount = ++fail_count; - bool marked_down = false; - if (fcount >= max_retries) { - marked_down = mark_down(now); - } - return std::make_pair(marked_down, fcount); -} - -inline bool -HostDBInfo::select(ts_time now, ts_seconds fail_window) const -{ - auto t0 = this->last_fail_time(); - if (t0 == TS_TIME_ZERO) { - return true; // it's alive and so is valid for selection. - } - // Return true and give it a try if enough time is elapsed since the last failure - return (t0 + fail_window < now); -} + // friend HostDBRecord; -inline void -HostDBInfo::migrate_from(HostDBInfo::self_type const &that) -{ - this->last_failure = that.last_failure.load(); - this->fail_count = that.fail_count.load(); - this->http_version = that.http_version; -} +private: + std::atomic _last_failure{TS_TIME_ZERO}; ///< Last time a failure was recorded + std::atomic _fail_count{0}; ///< Count of connection failures +}; // ---- /** Root item for HostDB. @@ -371,15 +294,12 @@ class HostDBRecord : public RefCountObj /** Pick the next round robin and update the record atomically. * - * @note This may select a zombie server and reserve it for the caller, therefore the caller must - * attempt to connect to the selected target if possible. + * @note This may select a suspect server. The caller must attempt to connect to the selected + * target if possible. * - * @param now Current time to use for aliveness calculations. + * @param now Current time to use for HostDBInfo state calculations. * @param fail_window Blackout time for down servers. - * @return Status of the updated target. - * - * If the return value is @c HostDBInfo::Status::DOWN this means all targets are down and there is - * no valid upstream. + * @return The selected target, or @c nullptr if all targets are down. * * @note Concurrency - this is not done under lock and depends on the caller for correct use. * For strict round robin, it is a feature that every call will get a distinct index. For @@ -434,9 +354,9 @@ class HostDBRecord : public RefCountObj * This accounts for the round robin setting. The default is to use "client affinity" in * which case @a hash_addr is as a hash seed to select the target. * - * This may select a zombie target, which can be detected by checking the target's last - * failure time. If it is not @c TS_TIME_ZERO the target is a zombie. Other transactions will - * be blocked from selecting that target until @a fail_window time has passed. + * This may select a suspect target (fail window elapsed, connections permitted again), which can + * be detected by checking the target's last failure time. If it is not @c TS_TIME_ZERO the target + * is a suspect. Multiple threads may concurrently select the same suspect target. * * In cases other than strict round robin, a base target is selected. If valid, that is returned, * but if not then the targets in this record are searched until a valid one is found. The result @@ -588,7 +508,7 @@ struct ResolveInfo { /// Keep a reference to the base HostDB object, so it doesn't get GC'd. Ptr record; - HostDBInfo *active = nullptr; ///< Active host record. + HostDBInfo *active = nullptr; ///< Active HostDBInfo /// Working address. The meaning / source of the value depends on other elements. /// This is the "resolved" address if @a resolved_p is @c true. @@ -646,19 +566,20 @@ struct ResolveInfo { */ bool resolve_immediate(); - /** Mark the active target as down. + /** Mark the active target as DOWN. * - * @param now Time of failure. + * @param now Time of failure. + * @param fail_window The fail window duration (proxy.config.http.down_server.cache_time). * @return @c true if the server was marked as down, @c false if not. * */ - bool mark_active_server_down(ts_time now); + bool mark_active_server_down(ts_time now, ts_seconds fail_window); - /** Mark the active target as alive. + /** Mark the active target as UP. * * @return @c true if the target changed state. */ - bool mark_active_server_alive(); + bool mark_active_server_up(); /// Select / resolve to the next RR entry for the record. bool select_next_rr(); @@ -863,15 +784,15 @@ ResolveInfo::set_active(sockaddr const *s) } inline bool -ResolveInfo::mark_active_server_alive() +ResolveInfo::mark_active_server_up() { return active->mark_up(); } inline bool -ResolveInfo::mark_active_server_down(ts_time now) +ResolveInfo::mark_active_server_down(ts_time now, ts_seconds fail_window) { - return active != nullptr && active->mark_down(now); + return active != nullptr && active->mark_down(now, fail_window); } inline bool diff --git a/src/iocore/hostdb/CMakeLists.txt b/src/iocore/hostdb/CMakeLists.txt index 4e099f8da76..07360c8f2d6 100644 --- a/src/iocore/hostdb/CMakeLists.txt +++ b/src/iocore/hostdb/CMakeLists.txt @@ -45,4 +45,5 @@ if(BUILD_TESTING) ) add_catch2_test(NAME test_hostdb_RefCountCache COMMAND $) + add_subdirectory(unit_tests) endif() diff --git a/src/iocore/hostdb/HostDB.cc b/src/iocore/hostdb/HostDB.cc index 12a8532be6e..ec7473e2929 100644 --- a/src/iocore/hostdb/HostDB.cc +++ b/src/iocore/hostdb/HostDB.cc @@ -1317,14 +1317,14 @@ HostDBRecord::select_best_http(ts_time now, ts_seconds fail_window, sockaddr con // Starting at the current target, search for a valid one. for (unsigned short i = 0; i < rr_count; i++) { auto target = &info[this->rr_idx(i)]; - if (target->select(now, fail_window)) { + if (!target->is_down(now, fail_window)) { best_alive = target; break; } } } } else { - if (info[0].select(now, fail_window)) { + if (!info[0].is_down(now, fail_window)) { best_alive = &info[0]; } } @@ -1647,7 +1647,7 @@ HostDBRecord::select_next_rr(ts_time now, ts_seconds fail_window) auto rr_info = this->rr_info(); for (unsigned idx = 0, limit = rr_info.count(); idx < limit; ++idx) { auto &target = rr_info[this->next_rr()]; - if (target.select(now, fail_window)) { + if (!target.is_down(now, fail_window)) { return ⌖ } } @@ -1711,7 +1711,7 @@ ResolveInfo::select_next_rr() if (active) { if (auto rr_info{this->record->rr_info()}; rr_info.count() > 1) { unsigned limit = active - rr_info.data(), idx = (limit + 1) % rr_info.count(); - while ((idx = (idx + 1) % rr_info.count()) != limit && !rr_info[idx].is_alive()) {} + while ((idx = (idx + 1) % rr_info.count()) != limit && !rr_info[idx].is_up()) {} active = &rr_info[idx]; return idx != limit; // if the active record was actually changed. } diff --git a/src/iocore/hostdb/HostDBInfo.cc b/src/iocore/hostdb/HostDBInfo.cc index db76345a162..944841fe430 100644 --- a/src/iocore/hostdb/HostDBInfo.cc +++ b/src/iocore/hostdb/HostDBInfo.cc @@ -27,6 +27,8 @@ namespace { +DbgCtl dbg_ctl_hostdb_info{"hostdb_info"}; + /** Assign raw storage to an @c IpAddr * * @param ip Destination. @@ -95,3 +97,154 @@ HostDBInfo::srvname() const { return data.srv.srv_offset ? reinterpret_cast(this) + data.srv.srv_offset : nullptr; } + +HostDBInfo & +HostDBInfo::operator=(HostDBInfo const &that) +{ + if (this != &that) { + memcpy(static_cast(this), static_cast(&that), sizeof(*this)); + } + return *this; +} + +ts_time +HostDBInfo::last_fail_time() const +{ + return _last_failure; +} + +uint8_t +HostDBInfo::fail_count() const +{ + return _fail_count; +} + +HostDBInfo::State +HostDBInfo::state(ts_time now, ts_seconds fail_window) const +{ + auto last_fail = this->last_fail_time(); + if (last_fail == TS_TIME_ZERO) { + return State::UP; + } + + if (now <= last_fail + fail_window) { + return State::DOWN; + } else { + return State::SUSPECT; + } +} + +bool +HostDBInfo::is_up() const +{ + return this->last_fail_time() == TS_TIME_ZERO; +} + +bool +HostDBInfo::is_down(ts_time now, ts_seconds fail_window) const +{ + return this->state(now, fail_window) == State::DOWN; +} + +bool +HostDBInfo::is_suspect(ts_time now, ts_seconds fail_window) const +{ + return this->state(now, fail_window) == State::SUSPECT; +} + +/** Mark the target as UP + * + * @return @c true if the target was previously DOWN or SUSPECT (i.e., a state change occurred). + */ +bool +HostDBInfo::mark_up() +{ + auto t = _last_failure.exchange(TS_TIME_ZERO); + bool was_down = t != TS_TIME_ZERO; + if (was_down) { + _fail_count.store(0); + } + return was_down; +} + +/** Mark the entry as DOWN. + * + * @param now Time of the failure. + * @param fail_window The fail window duration (proxy.config.http.down_server.cache_time). + * @return @c true if @a this was marked down, @c false if not. + * + * Handles two transitions: + * - UP → DOWN: @c _last_failure is @c TS_TIME_ZERO; set via CAS. + * - SUSPECT → DOWN: @c fail_window has elapsed since the last failure; @c _last_failure is + * refreshed via CAS to restart the fail window. + * + * On a successful transition @c _fail_count is reset to zero so that the next SUSPECT window + * accumulates failures from a clean baseline. + * + * Returns @c false if the server is already DOWN (within the active fail window), so the + * fail window is not refreshed by concurrent failures. + */ +bool +HostDBInfo::mark_down(ts_time now, ts_seconds fail_window) +{ + // UP -> DOWN + auto t0{TS_TIME_ZERO}; + if (_last_failure.compare_exchange_strong(t0, now)) { + // Reset so the next SUSPECT window starts with a fresh failure count. + _fail_count.store(0); + return true; + } + + // After the failed CAS, t0 holds the current _last_failure value. + // SUSPECT -> DOWN: the fail window has elapsed; refresh _last_failure to restart it. + if (t0 + fail_window < now) { + if (_last_failure.compare_exchange_strong(t0, now)) { + // Reset so the next SUSPECT window starts with a fresh failure count. + _fail_count.store(0); + return true; + } + } + + // Already DOWN; don't refresh the fail window. + return false; +} + +/** Increment the connection failure counter and conditionally mark the target DOWN. + * + * @param now Current time, used as the failure timestamp if the target is marked DOWN. + * @param max_retries Number of failures that triggers a transition to DOWN. + * @param fail_window The fail window duration (proxy.config.http.down_server.cache_time). + * @return A pair { @c marked_down, @c fail_count } where @c marked_down is @c true if this call + * caused the target to transition to DOWN (i.e. @c fail_count just reached @a max_retries + * and the @c mark_down CAS succeeded), and @c fail_count is the updated counter value. + * + * @note @c marked_down can be @c false even when @c fail_count >= @a max_retries if another + * thread concurrently marked the target DOWN first (the CAS on @c _last_failure will fail). + */ +std::pair +HostDBInfo::increment_fail_count(ts_time now, uint8_t max_retries, ts_seconds fail_window) +{ + auto fcount = ++_fail_count; + bool marked_down = false; + + Dbg(dbg_ctl_hostdb_info, "fail_count=%d max_reties=%d", fcount, max_retries); + + if (fcount >= max_retries) { + marked_down = mark_down(now, fail_window); + } + return std::make_pair(marked_down, fcount); +} + +/** Migrate data after a DNS update. + * + * @param that Source item. + * + * This moves only specific state information, it is not a generic copy. + */ +void +HostDBInfo::migrate_from(HostDBInfo::self_type const &that) +{ + this->_last_failure = that._last_failure.load(); + this->_fail_count = that._fail_count.load(); + this->http_version = that.http_version; +} diff --git a/src/iocore/hostdb/unit_tests/CMakeLists.txt b/src/iocore/hostdb/unit_tests/CMakeLists.txt new file mode 100644 index 00000000000..03546414235 --- /dev/null +++ b/src/iocore/hostdb/unit_tests/CMakeLists.txt @@ -0,0 +1,22 @@ +###################### +# +# Licensed to the Apache Software Foundation (ASF) under one or more contributor license +# agreements. See the NOTICE file distributed with this work for additional information regarding +# copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software distributed under the License +# is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express +# or implied. See the License for the specific language governing permissions and limitations under +# the License. +# +###################### + +add_executable(test_hostdb test_HostDBInfo.cc "${CMAKE_CURRENT_SOURCE_DIR}/../HostDBInfo.cc") + +target_link_libraries(test_hostdb PRIVATE Catch2::Catch2WithMain ts::tscore ts::tsutil ts::inkevent configmanager) + +add_catch2_test(NAME test_hostdb COMMAND $) diff --git a/src/iocore/hostdb/unit_tests/test_HostDBInfo.cc b/src/iocore/hostdb/unit_tests/test_HostDBInfo.cc new file mode 100644 index 00000000000..2febc80b3e0 --- /dev/null +++ b/src/iocore/hostdb/unit_tests/test_HostDBInfo.cc @@ -0,0 +1,214 @@ +/** @file + + Unit tests for HostDBInfo state transitions (UP / DOWN / SUSPECT). + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include + +#include "iocore/hostdb/HostDBProcessor.h" +#include "tscore/ink_time.h" + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +// fail_window used throughout the tests (matches down_server.cache_time default). +static const ts_seconds FAIL_WINDOW{300}; + +static const ts_time T0 = ts_clock::now(); ///< A fixed anchor in time; + +static const ts_time T1 = T0 + ts_seconds(1); ///< A time within FAIL_WINDOW from T0 +static const ts_time T2 = T0 + FAIL_WINDOW + ts_seconds(1); ///< A time past FAIL_WINDOW from T0 + +static const ts_time T3 = T2 + ts_seconds(1); ///< A time within FAIL_WINDOW from T2 +static const ts_time T4 = T3 + FAIL_WINDOW + ts_seconds(1); ///< A time past FAIL_WINDOW from T +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +TEST_CASE("HostDBInfo state", "[hostdb]") +{ + SECTION("initial state is UP") + { + HostDBInfo info; + REQUIRE(info.state(T0, FAIL_WINDOW) == HostDBInfo::State::UP); + REQUIRE(info.is_up()); + REQUIRE(info.last_fail_time() == TS_TIME_ZERO); + REQUIRE(info.fail_count() == 0); + } + + SECTION("UP -> DOWN via mark_down; fail_count is reset") + { + HostDBInfo info; + REQUIRE(info.mark_down(T0, FAIL_WINDOW) == true); + REQUIRE(info.last_fail_time() == T0); + REQUIRE(info.fail_count() == 0); // reset so the next SUSPECT window starts fresh + REQUIRE(info.state(T1, FAIL_WINDOW) == HostDBInfo::State::DOWN); + } + + SECTION("mark_down on DOWN server returns false and does not refresh fail window") + { + HostDBInfo info; + info.mark_down(T0, FAIL_WINDOW); + REQUIRE(info.mark_down(T1, FAIL_WINDOW) == false); + REQUIRE(info.last_fail_time() == T0); // unchanged + } + + SECTION("DOWN -> SUSPECT when fail window elapses (time-based, no explicit call)") + { + HostDBInfo info; + info.mark_down(T0, FAIL_WINDOW); + REQUIRE(info.state(T2, FAIL_WINDOW) == HostDBInfo::State::SUSPECT); + REQUIRE(info.is_suspect(T2, FAIL_WINDOW)); + REQUIRE(info.last_fail_time() == T0); // _last_failure is unchanged + } + + SECTION("SUSPECT -> DOWN via mark_down; fail_count is reset; fail window restarts") + { + HostDBInfo info; + info.mark_down(T0, FAIL_WINDOW); + + REQUIRE(info.mark_down(T2, FAIL_WINDOW) == true); + REQUIRE(info.last_fail_time() == T2); // refreshed to the new failure time + REQUIRE(info.fail_count() == 0); // reset so the next SUSPECT window starts fresh + // server is DOWN again with a fresh window anchored at T2 + REQUIRE(info.state(T2 + FAIL_WINDOW / 2, FAIL_WINDOW) == HostDBInfo::State::DOWN); + } + + SECTION("SUSPECT -> UP via mark_up; returns true (was DOWN/SUSPECT)") + { + HostDBInfo info; + info.mark_down(T0, FAIL_WINDOW); + REQUIRE(info.mark_up() == true); + REQUIRE(info.state(T0, FAIL_WINDOW) == HostDBInfo::State::UP); + REQUIRE(info.last_fail_time() == TS_TIME_ZERO); + REQUIRE(info.fail_count() == 0); + } + + SECTION("mark_up on already-UP server returns false") + { + HostDBInfo info; + REQUIRE(info.mark_up() == false); + } + + SECTION("UP -> DOWN via increment_fail_count with max_retries=1") + { + HostDBInfo info; + auto [marked, count] = info.increment_fail_count(T0, 1, FAIL_WINDOW); + REQUIRE(marked == true); + REQUIRE(count == 1); + REQUIRE(info.state(T1, FAIL_WINDOW) == HostDBInfo::State::DOWN); + REQUIRE(info.fail_count() == 0); // reset by mark_down + } + + SECTION("UP -> DOWN via increment_fail_count with max_retries=3 requires 3 failures") + { + HostDBInfo info; + { + auto [marked, count] = info.increment_fail_count(T0, 3, FAIL_WINDOW); + REQUIRE(marked == false); + REQUIRE(count == 1); + } + { + auto [marked, count] = info.increment_fail_count(T0, 3, FAIL_WINDOW); + REQUIRE(marked == false); + REQUIRE(count == 2); + } + { + auto [marked, count] = info.increment_fail_count(T0, 3, FAIL_WINDOW); + REQUIRE(marked == true); + REQUIRE(count == 3); + } + REQUIRE(info.state(T1, FAIL_WINDOW) == HostDBInfo::State::DOWN); + REQUIRE(info.fail_count() == 0); // reset by mark_down + } + + SECTION("SUSPECT -> DOWN via increment_fail_count; fail_count starts from zero each SUSPECT window") + { + HostDBInfo info; + // Drive UP -> DOWN (R=1); fail_count is reset to 0 after mark_down. + info.increment_fail_count(T0, 1, FAIL_WINDOW); + REQUIRE(info.fail_count() == 0); + + // D=3: need exactly 3 fresh failures in the SUSPECT window. + { + auto [marked, count] = info.increment_fail_count(T2, 3, FAIL_WINDOW); + REQUIRE(marked == false); + REQUIRE(count == 1); + } + { + auto [marked, count] = info.increment_fail_count(T2, 3, FAIL_WINDOW); + REQUIRE(marked == false); + REQUIRE(count == 2); + } + { + auto [marked, count] = info.increment_fail_count(T2, 3, FAIL_WINDOW); + REQUIRE(marked == true); + REQUIRE(count == 3); + } + REQUIRE(info.last_fail_time() == T2); + REQUIRE(info.fail_count() == 0); // reset by mark_down SUSPECT->DOWN + } + + SECTION("full cycle: UP -> DOWN -> SUSPECT -> DOWN -> SUSPECT -> UP") + { + HostDBInfo info; + + // UP -> DOWN + info.increment_fail_count(T0, 1, FAIL_WINDOW); + REQUIRE(info.state(T1, FAIL_WINDOW) == HostDBInfo::State::DOWN); + REQUIRE(info.fail_count() == 0); + + // DOWN -> SUSPECT + REQUIRE(info.state(T2, FAIL_WINDOW) == HostDBInfo::State::SUSPECT); + + // SUSPECT -> DOWN (fail window restarts from T2) + info.increment_fail_count(T2, 1, FAIL_WINDOW); + REQUIRE(info.last_fail_time() == T2); + REQUIRE(info.state(T3, FAIL_WINDOW) == HostDBInfo::State::DOWN); + REQUIRE(info.fail_count() == 0); + + // DOWN -> SUSPECT again + REQUIRE(info.state(T4, FAIL_WINDOW) == HostDBInfo::State::SUSPECT); + + // SUSPECT -> UP + REQUIRE(info.mark_up() == true); + REQUIRE(info.state(T4, FAIL_WINDOW) == HostDBInfo::State::UP); + REQUIRE(info.fail_count() == 0); + } + + SECTION("partial failures in SUSPECT then success resets to UP cleanly") + { + HostDBInfo info; + info.increment_fail_count(T0, 1, FAIL_WINDOW); + + // Two failures below the D=3 threshold + info.increment_fail_count(T2, 3, FAIL_WINDOW); + info.increment_fail_count(T2, 3, FAIL_WINDOW); + REQUIRE(info.state(T2, FAIL_WINDOW) == HostDBInfo::State::SUSPECT); + + // Connection succeeds + info.mark_up(); + REQUIRE(info.state(T2, FAIL_WINDOW) == HostDBInfo::State::UP); + REQUIRE(info.last_fail_time() == TS_TIME_ZERO); + REQUIRE(info.fail_count() == 0); + } +} diff --git a/src/mgmt/rpc/handlers/hostdb/HostDB.cc b/src/mgmt/rpc/handlers/hostdb/HostDB.cc index d90343bb1f9..9759a1cd19e 100644 --- a/src/mgmt/rpc/handlers/hostdb/HostDB.cc +++ b/src/mgmt/rpc/handlers/hostdb/HostDB.cc @@ -157,8 +157,8 @@ template <> struct convert { info_node["ip"] = std::string(buf); } - info_node["health"]["last_failure"] = info.last_failure.load().time_since_epoch().count(); - info_node["health"]["fail_count"] = static_cast(info.fail_count.load()); + info_node["health"]["last_failure"] = info.last_fail_time().time_since_epoch().count(); + info_node["health"]["fail_count"] = static_cast(info.fail_count()); node["info"].push_back(info_node); } diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 9dfe7e888cd..f52739f69d0 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -4718,10 +4718,10 @@ HttpSM::do_hostdb_update_if_necessary() if (track_connect_fail()) { this->mark_host_failure(&t_state.dns_info, ts_clock::from_time_t(t_state.client_request_time)); } else { - if (t_state.dns_info.mark_active_server_alive()) { + if (t_state.dns_info.mark_active_server_up()) { char addrbuf[INET6_ADDRPORTSTRLEN]; ats_ip_nptop(&t_state.current.server->dst_addr.sa, addrbuf, sizeof(addrbuf)); - ATS_PROBE2(mark_active_server_alive, sm_id, addrbuf); + ATS_PROBE2(mark_active_server_up, sm_id, addrbuf); if (t_state.dns_info.record->is_srv()) { SMDbg(dbg_ctl_http, "[%" PRId64 "] hostdb update marking SRV: %s(%s) as up", sm_id, t_state.dns_info.record->name(), addrbuf); @@ -5962,7 +5962,8 @@ HttpSM::mark_host_failure(ResolveInfo *info, ts_time time_down) if (time_down != TS_TIME_ZERO) { ats_ip_nptop(&t_state.current.server->dst_addr.sa, addrbuf, sizeof(addrbuf)); // Increment the fail_count - if (auto [down, fail_count] = info->active->increment_fail_count(time_down, t_state.txn_conf->connect_attempts_rr_retries); + if (auto [down, fail_count] = info->active->increment_fail_count(time_down, t_state.txn_conf->connect_attempts_rr_retries, + t_state.txn_conf->down_server_timeout); down) { char *url_str = t_state.hdr_info.client_request.url_string_get_ref(nullptr); std::string_view host_name{t_state.unmapped_url.host_get()}; From 686dea3b25e5251d072ae9bd3fbaea87bd45b71a Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Tue, 21 Apr 2026 11:33:20 +0900 Subject: [PATCH 2/2] Address comments from Copilot --- include/iocore/hostdb/HostDBProcessor.h | 16 ++++++++-------- src/iocore/hostdb/HostDBInfo.cc | 12 +++++------- src/iocore/hostdb/unit_tests/test_HostDBInfo.cc | 4 ++-- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/include/iocore/hostdb/HostDBProcessor.h b/include/iocore/hostdb/HostDBProcessor.h index 15f8133aaed..c18a007638b 100644 --- a/include/iocore/hostdb/HostDBProcessor.h +++ b/include/iocore/hostdb/HostDBProcessor.h @@ -124,15 +124,15 @@ enum class HostDBType : uint8_t { /** Information about a single target. * - * Each instance tracks the health state of one upstream address. The state is derived from @c last_failure and the caller-supplied + * Each instance tracks the health state of one upstream address. The state is derived from @c _last_failure and the caller-supplied * @a fail_window: * - * | State | Description | - * |---------|----------------------------------------------------------------------------------| - * | Up | No known failure; eligible for normal selection. | - * | Down | Blocked; no connections permitted until @c last_failure + @a fail_window elapses | - * | Suspect | Fail window has elapsed; connections are permitted. | - * | | On success transitions to Up (@c mark_up); on failure returns to Down. | + * | State | Description | + * |---------|-----------------------------------------------------------------------------------| + * | Up | No known failure; eligible for normal selection. | + * | Down | Blocked; no connections permitted until @c _last_failure + @a fail_window elapses | + * | Suspect | Fail window has elapsed; connections are permitted. | + * | | On success transitions to Up (@c mark_up); on failure returns to Down. | * * State transition diagram: * @@ -154,7 +154,7 @@ enum class HostDBType : uint8_t { * -+----------+--------------------+--------------------+----------+----> * ^ ^ ^ * \ \ \ - * (last_failure) (last_failure + fail_window) (connect success) + * (_last_failure) (_last_failure + fail_window) (connect success) * */ class HostDBInfo diff --git a/src/iocore/hostdb/HostDBInfo.cc b/src/iocore/hostdb/HostDBInfo.cc index 944841fe430..ffa77c4e101 100644 --- a/src/iocore/hostdb/HostDBInfo.cc +++ b/src/iocore/hostdb/HostDBInfo.cc @@ -159,12 +159,10 @@ HostDBInfo::is_suspect(ts_time now, ts_seconds fail_window) const bool HostDBInfo::mark_up() { - auto t = _last_failure.exchange(TS_TIME_ZERO); - bool was_down = t != TS_TIME_ZERO; - if (was_down) { - _fail_count.store(0); - } - return was_down; + auto t = _last_failure.exchange(TS_TIME_ZERO); + _fail_count.store(0); + + return t != TS_TIME_ZERO; } /** Mark the entry as DOWN. @@ -227,7 +225,7 @@ HostDBInfo::increment_fail_count(ts_time now, uint8_t max_retries, ts_seconds fa auto fcount = ++_fail_count; bool marked_down = false; - Dbg(dbg_ctl_hostdb_info, "fail_count=%d max_reties=%d", fcount, max_retries); + Dbg(dbg_ctl_hostdb_info, "fail_count=%d max_retries=%d", fcount, max_retries); if (fcount >= max_retries) { marked_down = mark_down(now, fail_window); diff --git a/src/iocore/hostdb/unit_tests/test_HostDBInfo.cc b/src/iocore/hostdb/unit_tests/test_HostDBInfo.cc index 2febc80b3e0..431ef1fa1f0 100644 --- a/src/iocore/hostdb/unit_tests/test_HostDBInfo.cc +++ b/src/iocore/hostdb/unit_tests/test_HostDBInfo.cc @@ -30,7 +30,7 @@ // Helpers // --------------------------------------------------------------------------- -// fail_window used throughout the tests (matches down_server.cache_time default). +// fail_window used throughout the tests static const ts_seconds FAIL_WINDOW{300}; static const ts_time T0 = ts_clock::now(); ///< A fixed anchor in time; @@ -39,7 +39,7 @@ static const ts_time T1 = T0 + ts_seconds(1); ///< A time within F static const ts_time T2 = T0 + FAIL_WINDOW + ts_seconds(1); ///< A time past FAIL_WINDOW from T0 static const ts_time T3 = T2 + ts_seconds(1); ///< A time within FAIL_WINDOW from T2 -static const ts_time T4 = T3 + FAIL_WINDOW + ts_seconds(1); ///< A time past FAIL_WINDOW from T +static const ts_time T4 = T2 + FAIL_WINDOW + ts_seconds(1); ///< A time past FAIL_WINDOW from T2 // --------------------------------------------------------------------------- // Tests // ---------------------------------------------------------------------------