From: Remi Gacogne Date: Mon, 20 Nov 2023 19:38:57 +0000 (+0100) Subject: dnsdist: Refactor the exponential back-off timer code X-Git-Tag: rec-5.0.0-rc1~26^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F13520%2Fhead;p=thirdparty%2Fpdns.git dnsdist: Refactor the exponential back-off timer code The existing code could overflow in some cases, leading to a potentially endless busy-loop. --- diff --git a/pdns/dnsdist-carbon.cc b/pdns/dnsdist-carbon.cc index 90957ece94..1b1c0fe552 100644 --- a/pdns/dnsdist-carbon.cc +++ b/pdns/dnsdist-carbon.cc @@ -25,6 +25,7 @@ #include "dnsdist-carbon.hh" #include "dnsdist.hh" +#include "dnsdist-backoff.hh" #include "dnsdist-metrics.hh" #ifndef DISABLE_CARBON @@ -279,6 +280,8 @@ static void carbonHandler(Carbon::Endpoint&& endpoint) { setThreadName("dnsdist/carbon"); const auto intervalUSec = endpoint.interval * 1000 * 1000; + /* maximum interval between two attempts is 10 minutes */ + const ExponentialBackOffTimer backOffTimer(10 * 60); try { uint8_t consecutiveFailures = 0; @@ -297,16 +300,7 @@ static void carbonHandler(Carbon::Endpoint&& endpoint) consecutiveFailures = 0; } else { - /* maximum interval between two attempts is 10 minutes */ - const time_t maxBackOff = 10 * 60; - time_t backOff = 1; - double backOffTmp = std::pow(2.0, static_cast(consecutiveFailures)); - if (backOffTmp != HUGE_VAL && static_cast(backOffTmp) <= static_cast(std::numeric_limits::max())) { - backOff = static_cast(backOffTmp); - if (backOff > maxBackOff) { - backOff = maxBackOff; - } - } + const auto backOff = backOffTimer.get(consecutiveFailures); if (consecutiveFailures < std::numeric_limits::max()) { consecutiveFailures++; } diff --git a/pdns/dnsdistdist/Makefile.am b/pdns/dnsdistdist/Makefile.am index d463c5bfb8..b95fa48526 100644 --- a/pdns/dnsdistdist/Makefile.am +++ b/pdns/dnsdistdist/Makefile.am @@ -146,6 +146,7 @@ dnsdist_SOURCES = \ dnscrypt.cc dnscrypt.hh \ dnsdist-async.cc dnsdist-async.hh \ dnsdist-backend.cc \ + dnsdist-backoff.hh \ dnsdist-cache.cc dnsdist-cache.hh \ dnsdist-carbon.cc dnsdist-carbon.hh \ dnsdist-concurrent-connections.hh \ @@ -264,6 +265,7 @@ testrunner_SOURCES = \ dnscrypt.cc dnscrypt.hh \ dnsdist-async.cc dnsdist-async.hh \ dnsdist-backend.cc \ + dnsdist-backoff.hh \ dnsdist-cache.cc dnsdist-cache.hh \ dnsdist-concurrent-connections.hh \ dnsdist-dnsparser.cc dnsdist-dnsparser.hh \ @@ -335,6 +337,7 @@ testrunner_SOURCES = \ test-dnsdist_cc.cc \ test-dnsdistasync.cc \ test-dnsdistbackend_cc.cc \ + test-dnsdistbackoff.cc \ test-dnsdistdynblocks_hh.cc \ test-dnsdistedns.cc \ test-dnsdistkvs_cc.cc \ diff --git a/pdns/dnsdistdist/dnsdist-backend.cc b/pdns/dnsdistdist/dnsdist-backend.cc index d3d70abc84..8c3eefc239 100644 --- a/pdns/dnsdistdist/dnsdist-backend.cc +++ b/pdns/dnsdistdist/dnsdist-backend.cc @@ -21,6 +21,7 @@ */ #include "dnsdist.hh" +#include "dnsdist-backoff.hh" #include "dnsdist-metrics.hh" #include "dnsdist-nghttp2.hh" #include "dnsdist-random.hh" @@ -677,14 +678,14 @@ void DownstreamState::updateNextLazyHealthCheck(LazyHealthCheckStats& stats, boo } time_t backOff = d_config.d_lazyHealthCheckMaxBackOff; - double backOffCoeffTmp = std::pow(2.0, failedTests); - if (backOffCoeffTmp != HUGE_VAL && static_cast(backOffCoeffTmp) <= static_cast(std::numeric_limits::max())) { - time_t backOffCoeff = static_cast(backOffCoeffTmp); - if ((std::numeric_limits::max() / d_config.d_lazyHealthCheckFailedInterval) >= backOffCoeff) { - backOff = d_config.d_lazyHealthCheckFailedInterval * backOffCoeff; - if (backOff > d_config.d_lazyHealthCheckMaxBackOff || (std::numeric_limits::max() - now) <= backOff) { - backOff = d_config.d_lazyHealthCheckMaxBackOff; - } + const ExponentialBackOffTimer backOffTimer(d_config.d_lazyHealthCheckMaxBackOff); + auto backOffCoeffTmp = backOffTimer.get(failedTests); + /* backOffCoeffTmp cannot be higher than d_config.d_lazyHealthCheckMaxBackOff */ + const auto backOffCoeff = static_cast(backOffCoeffTmp); + if ((std::numeric_limits::max() / d_config.d_lazyHealthCheckFailedInterval) >= backOffCoeff) { + backOff = d_config.d_lazyHealthCheckFailedInterval * backOffCoeff; + if (backOff > d_config.d_lazyHealthCheckMaxBackOff || (std::numeric_limits::max() - now) <= backOff) { + backOff = d_config.d_lazyHealthCheckMaxBackOff; } } diff --git a/pdns/dnsdistdist/dnsdist-backoff.hh b/pdns/dnsdistdist/dnsdist-backoff.hh new file mode 100644 index 0000000000..3e3081ffe7 --- /dev/null +++ b/pdns/dnsdistdist/dnsdist-backoff.hh @@ -0,0 +1,44 @@ +/* + * This file is part of PowerDNS or dnsdist. + * Copyright -- PowerDNS.COM B.V. and its contributors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * In addition, for the avoidance of any doubt, permission is granted to + * link this program with OpenSSL and to (re)distribute the binaries + * produced as the result of such linking. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ +#pragma once + +class ExponentialBackOffTimer +{ +public: + ExponentialBackOffTimer(unsigned int maxBackOff) : + d_maxBackOff(maxBackOff) + { + } + + unsigned int get(size_t consecutiveFailures) const + { + unsigned int backOff = d_maxBackOff; + if (consecutiveFailures <= 31) { + backOff = 1U << consecutiveFailures; + backOff = std::min(d_maxBackOff, backOff); + } + return backOff; + } + +private: + const unsigned int d_maxBackOff; +}; diff --git a/pdns/dnsdistdist/test-dnsdistbackoff.cc b/pdns/dnsdistdist/test-dnsdistbackoff.cc new file mode 100644 index 0000000000..173d102694 --- /dev/null +++ b/pdns/dnsdistdist/test-dnsdistbackoff.cc @@ -0,0 +1,58 @@ +/* + * This file is part of PowerDNS or dnsdist. + * Copyright -- PowerDNS.COM B.V. and its contributors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * In addition, for the avoidance of any doubt, permission is granted to + * link this program with OpenSSL and to (re)distribute the binaries + * produced as the result of such linking. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#define BOOST_TEST_DYN_LINK +#define BOOST_TEST_NO_MAIN + +#include + +#include "dnsdist-backoff.hh" + +BOOST_AUTO_TEST_SUITE(dnsdistbackoff) + +BOOST_AUTO_TEST_CASE(test_ExponentialBackOffTimer) +{ + const unsigned int maxBackOff = 10 * 60; + const ExponentialBackOffTimer ebot(maxBackOff); + const std::vector> testVector{ + {0U, 1U}, + {1U, 2U}, + {2U, 4U}, + {3U, 8U}, + {4U, 16U}, + {5U, 32U}, + {6U, 64U}, + {7U, 128U}, + {8U, 256U}, + {9U, 512U}, + {10U, maxBackOff}}; + for (const auto& entry : testVector) { + BOOST_CHECK_EQUAL(ebot.get(entry.first), entry.second); + } + + /* the behaviour is identical after 32 but let's go to 1024 to be safe */ + for (size_t consecutiveFailures = testVector.size(); consecutiveFailures < 1024; consecutiveFailures++) { + BOOST_CHECK_EQUAL(ebot.get(consecutiveFailures), maxBackOff); + } +} + +BOOST_AUTO_TEST_SUITE_END()