]> git.ipfire.org Git - thirdparty/pdns.git/commitdiff
dnsdist: Refactor the exponential back-off timer code 13523/head
authorRemi Gacogne <remi.gacogne@powerdns.com>
Mon, 20 Nov 2023 19:38:57 +0000 (20:38 +0100)
committerRemi Gacogne <remi.gacogne@powerdns.com>
Tue, 21 Nov 2023 10:33:35 +0000 (11:33 +0100)
The existing code could overflow in some cases, leading to a
potentially endless busy-loop.

(cherry picked from commit d629f5b02ee6de8bf94592980472337fdbf301ad)

pdns/dnsdist-carbon.cc
pdns/dnsdistdist/Makefile.am
pdns/dnsdistdist/dnsdist-backend.cc
pdns/dnsdistdist/dnsdist-backoff.hh [new file with mode: 0644]
pdns/dnsdistdist/test-dnsdistbackoff.cc [new file with mode: 0644]

index f72301a4506ee380843dccb05e90eac381d06be5..5a5014d8d17da4f7f5de2d206ee0b065bcba6976 100644 (file)
@@ -25,6 +25,7 @@
 
 #include "dnsdist-carbon.hh"
 #include "dnsdist.hh"
+#include "dnsdist-backoff.hh"
 
 #ifndef DISABLE_CARBON
 #include "dolog.hh"
@@ -272,6 +273,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;
@@ -290,16 +293,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<double>(consecutiveFailures));
-        if (backOffTmp != HUGE_VAL && static_cast<uint64_t>(backOffTmp) <= static_cast<uint64_t>(std::numeric_limits<time_t>::max())) {
-          backOff = static_cast<time_t>(backOffTmp);
-          if (backOff > maxBackOff) {
-            backOff = maxBackOff;
-          }
-        }
+        const auto backOff = backOffTimer.get(consecutiveFailures);
         if (consecutiveFailures < std::numeric_limits<decltype(consecutiveFailures)>::max()) {
           consecutiveFailures++;
         }
index 9d87f68b821629bdbeafdc9cfbcebcc589b03a55..f81f8bfb469f44ec7c7a6a2e20bed89128d58435 100644 (file)
@@ -138,6 +138,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 \
@@ -250,6 +251,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 \
@@ -316,6 +318,7 @@ testrunner_SOURCES = \
        test-dnsdist_cc.cc \
        test-dnsdistasync.cc \
        test-dnsdistbackend_cc.cc \
+       test-dnsdistbackoff.cc \
        test-dnsdistdynblocks_hh.cc \
        test-dnsdistkvs_cc.cc \
        test-dnsdistlbpolicies_cc.cc \
index 97cd0e46ec5c8fef36e7f5f3b87d999572495cf1..eca469a2bc69503fc566070bacec8eef299a7844 100644 (file)
@@ -21,6 +21,7 @@
  */
 
 #include "dnsdist.hh"
+#include "dnsdist-backoff.hh"
 #include "dnsdist-nghttp2.hh"
 #include "dnsdist-random.hh"
 #include "dnsdist-rings.hh"
@@ -676,14 +677,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<uint64_t>(backOffCoeffTmp) <= static_cast<uint64_t>(std::numeric_limits<time_t>::max())) {
-        time_t backOffCoeff = static_cast<time_t>(backOffCoeffTmp);
-        if ((std::numeric_limits<time_t>::max() / d_config.d_lazyHealthCheckFailedInterval) >= backOffCoeff) {
-          backOff = d_config.d_lazyHealthCheckFailedInterval * backOffCoeff;
-          if (backOff > d_config.d_lazyHealthCheckMaxBackOff || (std::numeric_limits<time_t>::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<time_t>(backOffCoeffTmp);
+      if ((std::numeric_limits<time_t>::max() / d_config.d_lazyHealthCheckFailedInterval) >= backOffCoeff) {
+        backOff = d_config.d_lazyHealthCheckFailedInterval * backOffCoeff;
+        if (backOff > d_config.d_lazyHealthCheckMaxBackOff || (std::numeric_limits<time_t>::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 (file)
index 0000000..3e3081f
--- /dev/null
@@ -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 (file)
index 0000000..173d102
--- /dev/null
@@ -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 <boost/test/unit_test.hpp>
+
+#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<std::pair<size_t, unsigned int>> 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()