execution context: refactored the impl to use marco to enable the exe…

…cution context (#36277) Commit Message: execution context: refactored the impl to use marco to enable the execution context Additional Description: The execution context is a feature that was contributed by the @wu-bin from google. But it only makes sense in very limited scenarios. After a discussion with @wu-bin , we decided to refactor it to make it more 'independent'. This PR refactored the implementation to make the custom abstraction `ExecutionContext` only be referenced in limited positions of Envoy core code base. More specifically, this PR: 1. move all ExecutionContext related code to single header file `envoy/common/execution_conetxt.h`. 2. No any directly dependency to the ExecutionContext at the Envoy core code base except the `source/common/common/scope_tracker.h`. 3. compiled the ExecutionContext out by default. 4. removed restart feature flag `envoy.restart_features.enable_execution_context`. Risk Level: low. Testing: unit. Docs Changes: n/a. Release Notes: added. Platform Specific Features: n/a. --------- Signed-off-by: wangbaiping <[email protected]>
envoyproxy · Sep 26, 2024 · 4dd017e · 4dd017e
1 parent 1a15316
commit 4dd017e
Show file tree

Hide file tree

Showing 33 changed files with 188 additions and 189 deletions.
diff --git a/bazel/BUILD b/bazel/BUILD
@@ -517,6 +517,11 @@ config_setting(
     values = {"define": "perf_annotation=enabled"},
 )
 
+config_setting(
+    name = "enable_execution_context",
+    values = {"define": "execution_context=enabled"},
+)
+
 config_setting(
     name = "enable_perf_tracing",
     values = {"define": "perf_tracing=enabled"},

diff --git a/bazel/envoy_internal.bzl b/bazel/envoy_internal.bzl
@@ -125,6 +125,7 @@ def envoy_copts(repository, test = False):
            envoy_select_static_extension_registration(["-DENVOY_STATIC_EXTENSION_REGISTRATION"], repository) + \
            envoy_select_disable_logging(["-DENVOY_DISABLE_LOGGING"], repository) + \
            _envoy_select_perf_annotation(["-DENVOY_PERF_ANNOTATION"]) + \
+           _envoy_select_execution_context() + \
            _envoy_select_perfetto(["-DENVOY_PERFETTO"]) + \
            envoy_select_google_grpc(["-DENVOY_GOOGLE_GRPC"], repository) + \
            envoy_select_signal_trace(["-DENVOY_HANDLE_SIGNALS"], repository) + \
@@ -190,6 +191,12 @@ def _envoy_select_perf_annotation(xs):
         "//conditions:default": [],
     })
 
+def _envoy_select_execution_context():
+    return select({
+        "@envoy//bazel:enable_execution_context": ["-DENVOY_ENABLE_EXECUTION_CONTEXT"],
+        "//conditions:default": [],
+    })
+
 def _envoy_select_perfetto(xs):
     return select({
         "@envoy//bazel:enable_perf_tracing": xs,

diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -54,6 +54,10 @@ behavior_changes:
     <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.access_log>`.
     This change can be disabled by setting the runtime guard flag
     ``envoy.reloadable_features.filter_access_loggers_first`` to ``false``.
+- area: monitoring
+  change: |
+    Removed runtime feature flag ``envoy.restart_features.enable_execution_context``. The execution context feature
+    now could be enabled only by setting compile option ``--define=execution_context=enabled``.
 
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*

diff --git a/envoy/common/BUILD b/envoy/common/BUILD
@@ -124,15 +124,19 @@ envoy_cc_library(
 envoy_cc_library(
     name = "execution_context",
     hdrs = ["execution_context.h"],
-    deps = [":pure_lib"],
+    deps = [
+        ":pure_lib",
+        ":scope_tracker_interface",
+    ],
 )
 
 envoy_cc_library(
     name = "scope_tracker_interface",
     hdrs = ["scope_tracker.h"],
     deps = [
-        ":execution_context",
+        ":optref_lib",
         ":pure_lib",
+        "//envoy/stream_info:stream_info_interface",
     ],
 )
 

diff --git a/envoy/common/execution_context.h b/envoy/common/execution_context.h
@@ -3,22 +3,25 @@
 #include <cstddef>
 
 #include "envoy/common/pure.h"
+#include "envoy/common/scope_tracker.h"
+#include "envoy/stream_info/stream_info.h"
 
 #include "source/common/common/non_copyable.h"
 
 namespace Envoy {
 
+#ifdef ENVOY_ENABLE_EXECUTION_CONTEXT
+
+static constexpr absl::string_view kConnectionExecutionContextFilterStateName =
+    "envoy.network.connection_execution_context";
+
 class ScopedExecutionContext;
 
 // ExecutionContext can be inherited by subclasses to represent arbitrary information associated
 // with the execution of a piece of code. activate/deactivate are called when the said execution
 // starts/ends. For an example usage, please see
 // https://github.com/envoyproxy/envoy/issues/32012.
-class ExecutionContext : NonCopyable {
-public:
-  ExecutionContext() = default;
-  virtual ~ExecutionContext() = default;
-
+class ExecutionContext : public StreamInfo::FilterState::Object, NonCopyable {
 protected:
   // Called when the current thread starts to run code on behalf of the owner of this object.
   // protected because it should only be called by ScopedExecutionContext.
@@ -43,7 +46,8 @@ class ExecutionContext : NonCopyable {
 class ScopedExecutionContext : NonCopyable {
 public:
   ScopedExecutionContext() : ScopedExecutionContext(nullptr) {}
-  ScopedExecutionContext(ExecutionContext* context) : context_(context) {
+  ScopedExecutionContext(const ScopeTrackedObject* object)
+      : context_(object != nullptr ? getExecutionContext(object->trackedStream()) : nullptr) {
     if (context_ != nullptr) {
       context_->activate();
     }
@@ -62,7 +66,18 @@ class ScopedExecutionContext : NonCopyable {
   bool isNull() const { return context_ == nullptr; }
 
 private:
+  ExecutionContext* getExecutionContext(OptRef<const StreamInfo::StreamInfo> info) {
+    if (!info.has_value()) {
+      return nullptr;
+    }
+    const auto* const_context = info->filterState().getDataReadOnly<ExecutionContext>(
+        kConnectionExecutionContextFilterStateName);
+    return const_cast<ExecutionContext*>(const_context);
+  }
+
   ExecutionContext* context_;
 };
 
+#endif
+
 } // namespace Envoy
diff --git a/envoy/common/scope_tracker.h b/envoy/common/scope_tracker.h
@@ -2,16 +2,17 @@
 
 #include <ostream>
 
-#include "envoy/common/execution_context.h"
+#include "envoy/common/optref.h"
 #include "envoy/common/pure.h"
+#include "envoy/stream_info/stream_info.h"
 
 namespace Envoy {
 
 /*
  * An interface for tracking the scope of work. Implementors of this interface
  * can be registered to the dispatcher when they're active on the stack. If a
- * fatal error occurs while they were active, the dumpState method will be
- * called.
+ * fatal error occurs while they were active, the dumpState() method will be
+ * called to output the active state.
  *
  * Currently this is only used for the L4 network connection and L7 stream.
  */
@@ -20,9 +21,11 @@ class ScopeTrackedObject {
   virtual ~ScopeTrackedObject() = default;
 
   /**
-   * If the tracked object has a ExecutionContext, returns it. Returns nullptr otherwise.
+   * Return the tracked stream info that related to the scope tracked object (L4
+   * network connection or L7 stream).
+   * @return optional reference to stream info of stream (L4 connection or L7 stream).
    */
-  virtual ExecutionContext* executionContext() const { return nullptr; }
+  virtual OptRef<const StreamInfo::StreamInfo> trackedStream() const { return {}; }
 
   /**
    * Dump debug state of the object in question to the provided ostream.

diff --git a/envoy/network/BUILD b/envoy/network/BUILD
@@ -25,6 +25,7 @@ envoy_cc_library(
         ":filter_interface",
         ":listen_socket_interface",
         "//envoy/buffer:buffer_interface",
+        "//envoy/common:scope_tracker_interface",
         "//envoy/event:deferred_deletable",
         "//envoy/ssl:connection_interface",
         "//envoy/stream_info:stream_info_interface",
@@ -174,7 +175,6 @@ envoy_cc_library(
     deps = [
         ":io_handle_interface",
         ":socket_interface",
-        "//envoy/common:scope_tracker_interface",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
     ],
 )

diff --git a/envoy/network/listen_socket.h b/envoy/network/listen_socket.h
@@ -7,7 +7,6 @@
 
 #include "envoy/common/exception.h"
 #include "envoy/common/pure.h"
-#include "envoy/common/scope_tracker.h"
 #include "envoy/config/core/v3/base.pb.h"
 #include "envoy/network/address.h"
 #include "envoy/network/io_handle.h"
@@ -26,7 +25,7 @@ namespace Network {
  * TODO(jrajahalme): Hide internals (e.g., fd) from listener filters by providing callbacks filters
  * may need (set/getsockopt(), peek(), recv(), etc.)
  */
-class ConnectionSocket : public virtual Socket, public virtual ScopeTrackedObject {
+class ConnectionSocket : public virtual Socket {
 public:
   /**
    * Set detected transport protocol (e.g. RAW_BUFFER, TLS).
@@ -83,6 +82,16 @@ class ConnectionSocket : public virtual Socket, public virtual ScopeTrackedObjec
    * return value is cwnd(in packets) times the connection's MSS.
    */
   virtual absl::optional<uint64_t> congestionWindowInBytes() const PURE;
+
+  /**
+   * Dump debug state of the object in question to the provided ostream.
+   *
+   * This is called on Envoy fatal errors, so should do minimal memory allocation.
+   *
+   * @param os the ostream to output to.
+   * @param indent_level how far to indent, for pretty-printed classes and subclasses.
+   */
+  virtual void dumpState(std::ostream& os, int indent_level = 0) const PURE;
 };
 
 using ConnectionSocketPtr = std::unique_ptr<ConnectionSocket>;

diff --git a/source/common/common/scope_tracker.h b/source/common/common/scope_tracker.h
@@ -18,9 +18,7 @@ namespace Envoy {
 class ScopeTrackerScopeState {
 public:
   ScopeTrackerScopeState(const ScopeTrackedObject* object, Event::ScopeTracker& tracker)
-      : registered_object_(object),
-        scoped_execution_context_(executionContextEnabled() ? object->executionContext() : nullptr),
-        tracker_(tracker) {
+      : registered_object_(object), tracker_(tracker) {
     tracker_.pushTrackedObject(registered_object_);
   }
 
@@ -36,14 +34,13 @@ class ScopeTrackerScopeState {
 
 private:
   friend class ScopeTrackerScopeStateTest;
-  static bool& executionContextEnabled() {
-    static bool enabled =
-        Runtime::runtimeFeatureEnabled("envoy.restart_features.enable_execution_context");
-    return enabled;
-  }
+
   const ScopeTrackedObject* registered_object_;
-  ScopedExecutionContext scoped_execution_context_;
   Event::ScopeTracker& tracker_;
+
+#ifdef ENVOY_ENABLE_EXECUTION_CONTEXT
+  ScopedExecutionContext scoped_execution_context_{registered_object_};
+#endif
 };
 
 } // namespace Envoy
diff --git a/source/common/http/BUILD b/source/common/http/BUILD
@@ -392,7 +392,6 @@ envoy_cc_library(
         "//source/common/config:utility_lib",
         "//source/common/http/http1:codec_lib",
         "//source/common/http/http2:codec_lib",
-        "//source/common/network:common_connection_filter_states_lib",
         "//source/common/network:proxy_protocol_filter_state_lib",
         "//source/common/network:utility_lib",
         "//source/common/quic:quic_server_factory_stub_lib",

diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h
@@ -45,7 +45,6 @@
 #include "source/common/http/user_agent.h"
 #include "source/common/http/utility.h"
 #include "source/common/local_reply/local_reply.h"
-#include "source/common/network/common_connection_filter_states.h"
 #include "source/common/network/proxy_protocol_filter_state.h"
 #include "source/common/stream_info/stream_info_impl.h"
 #include "source/common/tracing/http_tracer_impl.h"
@@ -218,10 +217,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     }
 
     // ScopeTrackedObject
-    ExecutionContext* executionContext() const override {
-      return getConnectionExecutionContext(connection_manager_.read_callbacks_->connection());
+    OptRef<const StreamInfo::StreamInfo> trackedStream() const override {
+      return filter_manager_.trackedStream();
     }
-
     void dumpState(std::ostream& os, int indent_level = 0) const override {
       const char* spaces = spacesForLevel(indent_level);
       os << spaces << "ActiveStream " << this << DUMP_MEMBER(stream_id_);

diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h
@@ -651,6 +651,7 @@ class FilterManager : public ScopeTrackedObject,
   }
 
   // ScopeTrackedObject
+  OptRef<const StreamInfo::StreamInfo> trackedStream() const override { return streamInfo(); }
   void dumpState(std::ostream& os, int indent_level = 0) const override {
     const char* spaces = spacesForLevel(indent_level);
     os << spaces << "FilterManager " << this << DUMP_MEMBER(state_.has_1xx_headers_)

diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD
@@ -60,7 +60,6 @@ envoy_cc_library(
         "//source/common/http:headers_lib",
         "//source/common/http:status_lib",
         "//source/common/http:utility_lib",
-        "//source/common/network:common_connection_filter_states_lib",
         "//source/common/runtime:runtime_features_lib",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
     ],

diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -24,7 +24,6 @@
 #include "source/common/http/http1/header_formatter.h"
 #include "source/common/http/http1/legacy_parser_impl.h"
 #include "source/common/http/utility.h"
-#include "source/common/network/common_connection_filter_states.h"
 #include "source/common/runtime/runtime_features.h"
 
 #include "absl/container/fixed_array.h"
@@ -976,8 +975,8 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) {
   onResetStream(reason);
 }
 
-ExecutionContext* ConnectionImpl::executionContext() const {
-  return getConnectionExecutionContext(connection_);
+OptRef<const StreamInfo::StreamInfo> ConnectionImpl::trackedStream() const {
+  return connection_.trackedStream();
 }
 
 void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const {

diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h
@@ -272,7 +272,7 @@ class ConnectionImpl : public virtual Connection,
   Envoy::Http::Status codec_status_;
 
   // ScopeTrackedObject
-  ExecutionContext* executionContext() const override;
+  OptRef<const StreamInfo::StreamInfo> trackedStream() const override;
   void dumpState(std::ostream& os, int indent_level) const override;
 
 protected:

diff --git a/source/common/http/http2/BUILD b/source/common/http/http2/BUILD
@@ -56,7 +56,6 @@ envoy_cc_library(
         "//source/common/http:headers_lib",
         "//source/common/http:status_lib",
         "//source/common/http:utility_lib",
-        "//source/common/network:common_connection_filter_states_lib",
         "//source/common/runtime:runtime_features_lib",
         "@com_github_google_quiche//:http2_adapter",
         "@com_google_absl//absl/algorithm",

diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc
@@ -25,7 +25,6 @@
 #include "source/common/http/headers.h"
 #include "source/common/http/http2/codec_stats.h"
 #include "source/common/http/utility.h"
-#include "source/common/network/common_connection_filter_states.h"
 #include "source/common/runtime/runtime_features.h"
 
 #include "absl/cleanup/cleanup.h"
@@ -2066,8 +2065,8 @@ ConnectionImpl::ClientHttp2Options::ClientHttp2Options(
 #endif
 }
 
-ExecutionContext* ConnectionImpl::executionContext() const {
-  return getConnectionExecutionContext(connection_);
+OptRef<const StreamInfo::StreamInfo> ConnectionImpl::trackedStream() const {
+  return connection_.trackedStream();
 }
 
 void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const {

diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h
@@ -179,7 +179,7 @@ class ConnectionImpl : public virtual Connection,
   }
 
   // ScopeTrackedObject
-  ExecutionContext* executionContext() const override;
+  OptRef<const StreamInfo::StreamInfo> trackedStream() const override;
   void dumpState(std::ostream& os, int indent_level) const override;
 
 protected:

diff --git a/source/common/network/BUILD b/source/common/network/BUILD
@@ -80,7 +80,6 @@ envoy_cc_library(
     srcs = ["connection_impl_base.cc"],
     hdrs = ["connection_impl_base.h"],
     deps = [
-        ":common_connection_filter_states_lib",
         ":connection_socket_lib",
         ":filter_manager_lib",
         "//envoy/common:scope_tracker_interface",
@@ -624,14 +623,3 @@ envoy_cc_library(
         "//envoy/network:filter_interface",
     ],
 )
-
-envoy_cc_library(
-    name = "common_connection_filter_states_lib",
-    srcs = ["common_connection_filter_states.cc"],
-    hdrs = ["common_connection_filter_states.h"],
-    deps = [
-        "//envoy/common:execution_context",
-        "//envoy/network:connection_interface",
-        "//envoy/stream_info:filter_state_interface",
-    ],
-)
diff --git a/source/common/network/common_connection_filter_states.cc b/source/common/network/common_connection_filter_states.cc