overload: remove createScaledTimer with OverloadTimerType

include/envoy/event/scaled_timer_minimum.h

@@ -13,26 +13,31 @@ namespace Event {
  * Describes a minimum timer value that is equal to a scale factor applied to the maximum.
  */
 struct ScaledMinimum {
-  explicit ScaledMinimum(UnitFloat scale_factor) : scale_factor_(scale_factor) {}
+  explicit constexpr ScaledMinimum(UnitFloat scale_factor) : scale_factor_(scale_factor) {}
+  inline bool operator==(const ScaledMinimum& other) const {
+    return other.scale_factor_.value() == scale_factor_.value();
+  }
+
   const UnitFloat scale_factor_;
 };
 
 /**
  * Describes a minimum timer value that is an absolute duration.
  */
 struct AbsoluteMinimum {
-  explicit AbsoluteMinimum(std::chrono::milliseconds value) : value_(value) {}
+  explicit constexpr AbsoluteMinimum(std::chrono::milliseconds value) : value_(value) {}
+  inline bool operator==(const AbsoluteMinimum& other) const { return other.value_ == value_; }
   const std::chrono::milliseconds value_;
 };
 
 /**
  * Class that describes how to compute a minimum timeout given a maximum timeout value. It wraps
  * ScaledMinimum and AbsoluteMinimum and provides a single computeMinimum() method.
  */
-class ScaledTimerMinimum : private absl::variant<ScaledMinimum, AbsoluteMinimum> {
+class ScaledTimerMinimum {
 public:
-  // Use base class constructor.
-  using absl::variant<ScaledMinimum, AbsoluteMinimum>::variant;
+  constexpr ScaledTimerMinimum(ScaledMinimum arg) : impl_(arg) {}
+  constexpr ScaledTimerMinimum(AbsoluteMinimum arg) : impl_(arg) {}
 
   // Computes the minimum value for a given maximum timeout. If this object was constructed with a
   // - ScaledMinimum value:
@@ -51,9 +56,13 @@ class ScaledTimerMinimum : private absl::variant<ScaledMinimum, AbsoluteMinimum>
       }
       const std::chrono::milliseconds value_;
     };
-    return absl::visit<Visitor, const absl::variant<ScaledMinimum, AbsoluteMinimum>&>(
-        Visitor(maximum), *this);
+    return absl::visit(Visitor(maximum), impl_);
   }
+
+  inline bool operator==(const ScaledTimerMinimum& other) const { return impl_ == other.impl_; }
+
+private:
+  absl::variant<ScaledMinimum, AbsoluteMinimum> impl_;
 };
 
 } // namespace Event

include/envoy/server/overload/overload_manager.h

@@ -4,6 +4,7 @@
 
 #include "envoy/common/pure.h"
 #include "envoy/event/dispatcher.h"
+#include "envoy/event/scaled_timer_minimum.h"
 #include "envoy/server/overload/thread_local_overload_state.h"
 
 #include "common/singleton/const_singleton.h"
@@ -37,6 +38,17 @@ class OverloadActionNameValues {
 
 using OverloadActionNames = ConstSingleton<OverloadActionNameValues>;
 
+enum class OverloadTimerType {
+  // Timers created with this type will never be scaled. This should only be used for testing.
+  UnscaledRealTimerForTest,
+  // The amount of time an HTTP connection to a downstream client can remain idle (no streams). This
+  // corresponds to the HTTP_DOWNSTREAM_CONNECTION_IDLE TimerType in overload.proto.
+  HttpDownstreamIdleConnectionTimeout,
+  // The amount of time an HTTP stream from a downstream client can remain idle. This corresponds to
+  // the HTTP_DOWNSTREAM_STREAM_IDLE TimerType in overload.proto.
+  HttpDownstreamIdleStreamTimeout,
+};
+
 /**
  * The OverloadManager protects the Envoy instance from being overwhelmed by client
  * requests. It monitors a set of resources and notifies registered listeners if
@@ -69,6 +81,12 @@ class OverloadManager {
    * an alternative to registering a callback for overload action state changes.
    */
   virtual ThreadLocalOverloadState& getThreadLocalOverloadState() PURE;
+
+  /**
+   * Get the configured minimum rule for the given timer type.
+   */
+  virtual Event::ScaledTimerMinimum
+  getConfiguredTimerMinimum(OverloadTimerType timer_type) const PURE;
 };
 
 } // namespace Server

include/envoy/server/overload/thread_local_overload_state.h

@@ -40,17 +40,6 @@ class OverloadActionState {
  */
 using OverloadActionCb = std::function<void(OverloadActionState)>;
 
-enum class OverloadTimerType {
-  // Timers created with this type will never be scaled. This should only be used for testing.
-  UnscaledRealTimerForTest,
-  // The amount of time an HTTP connection to a downstream client can remain idle (no streams). This
-  // corresponds to the HTTP_DOWNSTREAM_CONNECTION_IDLE TimerType in overload.proto.
-  HttpDownstreamIdleConnectionTimeout,
-  // The amount of time an HTTP stream from a downstream client can remain idle. This corresponds to
-  // the HTTP_DOWNSTREAM_STREAM_IDLE TimerType in overload.proto.
-  HttpDownstreamIdleStreamTimeout,
-};
-
 /**
  * Thread-local copy of the state of each configured overload action.
  */
@@ -59,10 +48,6 @@ class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject {
   // Get a thread-local reference to the value for the given action key.
   virtual const OverloadActionState& getState(const std::string& action) PURE;
 
-  // Get a scaled timer whose minimum corresponds to the configured value for the given timer type.
-  virtual Event::TimerPtr createScaledTimer(OverloadTimerType timer_type,
-                                            Event::TimerCb callback) PURE;
-
   // Get a scaled timer whose minimum is determined by the given scaling rule.
   virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerMinimum minimum,
                                             Event::TimerCb callback) PURE;

source/common/http/conn_manager_impl.cc

@@ -101,6 +101,10 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config,
           overload_state_.getState(Server::OverloadActionNames::get().StopAcceptingRequests)),
       overload_disable_keepalive_ref_(
           overload_state_.getState(Server::OverloadActionNames::get().DisableHttpKeepAlive)),
+      downstream_idle_connection_scaled_timeout_(overload_manager.getConfiguredTimerMinimum(
+          Server::OverloadTimerType::HttpDownstreamIdleConnectionTimeout)),
+      downstream_idle_stream_scaled_timeout_(overload_manager.getConfiguredTimerMinimum(
+          Server::OverloadTimerType::HttpDownstreamIdleStreamTimeout)),
       time_source_(time_source) {}
 
 const ResponseHeaderMap& ConnectionManagerImpl::continueHeader() {
@@ -122,8 +126,7 @@ void ConnectionManagerImpl::initializeReadFilterCallbacks(Network::ReadFilterCal
 
   if (config_.idleTimeout()) {
     connection_idle_timer_ = overload_state_.createScaledTimer(
-        Server::OverloadTimerType::HttpDownstreamIdleConnectionTimeout,
-        [this]() -> void { onIdleTimeout(); });
+        downstream_idle_connection_scaled_timeout_, [this]() -> void { onIdleTimeout(); });
     connection_idle_timer_->enableTimer(config_.idleTimeout().value());
   }
 
@@ -638,7 +641,7 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
   if (connection_manager_.config_.streamIdleTimeout().count()) {
     idle_timeout_ms_ = connection_manager_.config_.streamIdleTimeout();
     stream_idle_timer_ = connection_manager_.overload_state_.createScaledTimer(
-        Server::OverloadTimerType::HttpDownstreamIdleStreamTimeout,
+        connection_manager_.downstream_idle_stream_scaled_timeout_,
         [this]() -> void { onIdleTimeout(); });
     resetIdleTimer();
   }
@@ -1063,7 +1066,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
         // If we have a route-level idle timeout but no global stream idle timeout, create a timer.
         if (stream_idle_timer_ == nullptr) {
           stream_idle_timer_ = connection_manager_.overload_state_.createScaledTimer(
-              Server::OverloadTimerType::HttpDownstreamIdleStreamTimeout,
+              connection_manager_.downstream_idle_stream_scaled_timeout_,
               [this]() -> void { onIdleTimeout(); });
         }
       } else if (stream_idle_timer_ != nullptr) {

source/common/http/conn_manager_impl.h

@@ -440,6 +440,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   // map lookup in the hot path of processing each request.
   const Server::OverloadActionState& overload_stop_accepting_requests_ref_;
   const Server::OverloadActionState& overload_disable_keepalive_ref_;
+  // Timer scaling factors.
+  const Event::ScaledTimerMinimum downstream_idle_connection_scaled_timeout_;
+  const Event::ScaledTimerMinimum downstream_idle_stream_scaled_timeout_;
   TimeSource& time_source_;
   bool remote_close_{};
 };

source/server/admin/admin.h

@@ -255,9 +255,6 @@ class AdminImpl : public Admin,
     struct NullThreadLocalOverloadState : public ThreadLocalOverloadState {
       NullThreadLocalOverloadState(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {}
       const OverloadActionState& getState(const std::string&) override { return inactive_; }
-      Event::TimerPtr createScaledTimer(OverloadTimerType, Event::TimerCb callback) override {
-        return dispatcher_.createTimer(callback);
-      }
       Event::TimerPtr createScaledTimer(Event::ScaledTimerMinimum,
                                         Event::TimerCb callback) override {
         return dispatcher_.createTimer(callback);
@@ -286,6 +283,10 @@ class AdminImpl : public Admin,
       return false;
     }
 
+    Event::ScaledTimerMinimum getConfiguredTimerMinimum(OverloadTimerType) const override {
+      return Event::ScaledMinimum(UnitFloat::max());
+    }
+
     ThreadLocal::SlotPtr tls_;
   };
 

source/server/overload_manager_impl.cc

@@ -26,11 +26,9 @@ namespace Server {
  */
 class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState {
 public:
-  ThreadLocalOverloadStateImpl(
-      Event::ScaledRangeTimerManagerPtr scaled_timer_manager,
-      const NamedOverloadActionSymbolTable& action_symbol_table,
-      const absl::flat_hash_map<OverloadTimerType, Event::ScaledTimerMinimum>& timer_minimums)
-      : action_symbol_table_(action_symbol_table), timer_minimums_(timer_minimums),
+  ThreadLocalOverloadStateImpl(Event::ScaledRangeTimerManagerPtr scaled_timer_manager,
+                               const NamedOverloadActionSymbolTable& action_symbol_table)
+      : action_symbol_table_(action_symbol_table),
         actions_(action_symbol_table.size(), OverloadActionState(UnitFloat::min())),
         scaled_timer_action_(action_symbol_table.lookup(OverloadActionNames::get().ReduceTimeouts)),
         scaled_timer_manager_(std::move(scaled_timer_manager)) {}
@@ -42,16 +40,6 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState {
     return always_inactive_;
   }
 
-  Event::TimerPtr createScaledTimer(OverloadTimerType timer_type,
-                                    Event::TimerCb callback) override {
-    auto minimum_it = timer_minimums_.find(timer_type);
-    const Event::ScaledTimerMinimum minimum =
-        minimum_it != timer_minimums_.end()
-            ? minimum_it->second
-            : Event::ScaledTimerMinimum(Event::ScaledMinimum(UnitFloat::max()));
-    return scaled_timer_manager_->createTimer(minimum, std::move(callback));
-  }
-
   Event::TimerPtr createScaledTimer(Event::ScaledTimerMinimum minimum,
                                     Event::TimerCb callback) override {
     return scaled_timer_manager_->createTimer(minimum, std::move(callback));
@@ -67,7 +55,6 @@ class ThreadLocalOverloadStateImpl : public ThreadLocalOverloadState {
 private:
   static const OverloadActionState always_inactive_;
   const NamedOverloadActionSymbolTable& action_symbol_table_;
-  const absl::flat_hash_map<OverloadTimerType, Event::ScaledTimerMinimum>& timer_minimums_;
   std::vector<OverloadActionState> actions_;
   absl::optional<NamedOverloadActionSymbolTable::Symbol> scaled_timer_action_;
   const Event::ScaledRangeTimerManagerPtr scaled_timer_manager_;
@@ -340,7 +327,7 @@ void OverloadManagerImpl::start() {
 
   tls_.set([this](Event::Dispatcher& dispatcher) {
     return std::make_shared<ThreadLocalOverloadStateImpl>(createScaledRangeTimerManager(dispatcher),
-                                                          action_symbol_table_, timer_minimums_);
+                                                          action_symbol_table_);
   });
 
   if (resources_.empty()) {
@@ -393,6 +380,14 @@ bool OverloadManagerImpl::registerForAction(const std::string& action,
 
 ThreadLocalOverloadState& OverloadManagerImpl::getThreadLocalOverloadState() { return *tls_; }
 
+Event::ScaledTimerMinimum
+OverloadManagerImpl::getConfiguredTimerMinimum(OverloadTimerType timer_type) const {
+  if (auto it = timer_minimums_.find(timer_type); it != timer_minimums_.end()) {
+    return it->second;
+  }
+  return Event::ScaledMinimum(UnitFloat::max());
+}
+
 Event::ScaledRangeTimerManagerPtr
 OverloadManagerImpl::createScaledRangeTimerManager(Event::Dispatcher& dispatcher) const {
   return std::make_unique<Event::ScaledRangeTimerManagerImpl>(dispatcher);

source/server/overload_manager_impl.h

@@ -114,6 +114,7 @@ class OverloadManagerImpl : Logger::Loggable<Logger::Id::main>, public OverloadM
   bool registerForAction(const std::string& action, Event::Dispatcher& dispatcher,
                          OverloadActionCb callback) override;
   ThreadLocalOverloadState& getThreadLocalOverloadState() override;
+  Event::ScaledTimerMinimum getConfiguredTimerMinimum(OverloadTimerType timer_type) const override;
 
   // Stop the overload manager timer and wait for any pending resource updates to complete.
   // After this returns, overload manager clients should not receive any more callbacks

test/common/http/conn_manager_impl_test_base.cc

@@ -53,7 +53,7 @@ void HttpConnectionManagerImplTest::setup(bool ssl, const std::string& server_na
   server_name_ = server_name;
   ON_CALL(filter_callbacks_.connection_, ssl()).WillByDefault(Return(ssl_connection_));
   ON_CALL(Const(filter_callbacks_.connection_), ssl()).WillByDefault(Return(ssl_connection_));
-  ON_CALL(overload_manager_.overload_state_, createScaledTypedTimer_)
+  ON_CALL(overload_manager_.overload_state_, createScaledMinimumTimer_)
       .WillByDefault([&](auto, auto callback) {
         return filter_callbacks_.connection_.dispatcher_.createTimer(callback).release();
       });

test/mocks/server/overload_manager.cc

@@ -11,28 +11,25 @@ namespace Envoy {
 namespace Server {
 
 using ::testing::NiceMock;
+using ::testing::Return;
 using ::testing::ReturnNew;
 using ::testing::ReturnRef;
 
 MockThreadLocalOverloadState::MockThreadLocalOverloadState()
     : disabled_state_(OverloadActionState::inactive()) {
   ON_CALL(*this, getState).WillByDefault(ReturnRef(disabled_state_));
-  ON_CALL(*this, createScaledTypedTimer_).WillByDefault(ReturnNew<NiceMock<Event::MockTimer>>());
   ON_CALL(*this, createScaledMinimumTimer_).WillByDefault(ReturnNew<NiceMock<Event::MockTimer>>());
 }
 
-Event::TimerPtr MockThreadLocalOverloadState::createScaledTimer(OverloadTimerType timer_type,
-                                                                Event::TimerCb callback) {
-  return Event::TimerPtr{createScaledTypedTimer_(timer_type, std::move(callback))};
-}
-
 Event::TimerPtr MockThreadLocalOverloadState::createScaledTimer(Event::ScaledTimerMinimum minimum,
                                                                 Event::TimerCb callback) {
   return Event::TimerPtr{createScaledMinimumTimer_(minimum, std::move(callback))};
 }
 
 MockOverloadManager::MockOverloadManager() {
   ON_CALL(*this, getThreadLocalOverloadState()).WillByDefault(ReturnRef(overload_state_));
+  ON_CALL(*this, getConfiguredTimerMinimum)
+      .WillByDefault(Return(Event::ScaledMinimum(UnitFloat::max())));
 }
 
 MockOverloadManager::~MockOverloadManager() = default;

test/mocks/server/overload_manager.h

@@ -14,10 +14,8 @@ class MockThreadLocalOverloadState : public ThreadLocalOverloadState {
 public:
   MockThreadLocalOverloadState();
   MOCK_METHOD(const OverloadActionState&, getState, (const std::string&), (override));
-  Event::TimerPtr createScaledTimer(OverloadTimerType timer_type, Event::TimerCb callback) override;
   Event::TimerPtr createScaledTimer(Event::ScaledTimerMinimum minimum,
                                     Event::TimerCb callback) override;
-  MOCK_METHOD(Event::Timer*, createScaledTypedTimer_, (OverloadTimerType, Event::TimerCb));
   MOCK_METHOD(Event::Timer*, createScaledMinimumTimer_,
               (Event::ScaledTimerMinimum, Event::TimerCb));
 
@@ -36,6 +34,8 @@ class MockOverloadManager : public OverloadManager {
               (const std::string& action, Event::Dispatcher& dispatcher,
                OverloadActionCb callback));
   MOCK_METHOD(ThreadLocalOverloadState&, getThreadLocalOverloadState, ());
+  MOCK_METHOD(Event::ScaledTimerMinimum, getConfiguredTimerMinimum, (OverloadTimerType timer_type),
+              (const));
 
   testing::NiceMock<MockThreadLocalOverloadState> overload_state_;
 };