diff --git a/api/envoy/extensions/watchdog/profile_action/v3alpha/profile_action.proto b/api/envoy/extensions/watchdog/profile_action/v3alpha/profile_action.proto index 2bb03f6031322..02636d0fb25f0 100644 --- a/api/envoy/extensions/watchdog/profile_action/v3alpha/profile_action.proto +++ b/api/envoy/extensions/watchdog/profile_action/v3alpha/profile_action.proto @@ -25,11 +25,8 @@ message ProfileActionConfig { // File path to the directory to output profiles. string profile_path = 2 [(validate.rules).string = {min_bytes: 1}]; - // Limits the max number of profiles that can be generated by a thread over - // its lifetime to avoid filling the disk. We keep a map of - // to track the number of profiles triggered by a particular thread. Only one - // thread is counted as triggering the profile even though multiple threads - // might have been eligible for triggering the profile. + // Limits the max number of profiles that can be generated by this action + // over its lifetime to avoid filling the disk. // If not set (i.e. it's 0), a default of 10 will be used. - uint64 max_profiles_per_thread = 3; + uint64 max_profiles = 3; } diff --git a/generated_api_shadow/envoy/extensions/watchdog/profile_action/v3alpha/profile_action.proto b/generated_api_shadow/envoy/extensions/watchdog/profile_action/v3alpha/profile_action.proto index 2bb03f6031322..02636d0fb25f0 100644 --- a/generated_api_shadow/envoy/extensions/watchdog/profile_action/v3alpha/profile_action.proto +++ b/generated_api_shadow/envoy/extensions/watchdog/profile_action/v3alpha/profile_action.proto @@ -25,11 +25,8 @@ message ProfileActionConfig { // File path to the directory to output profiles. string profile_path = 2 [(validate.rules).string = {min_bytes: 1}]; - // Limits the max number of profiles that can be generated by a thread over - // its lifetime to avoid filling the disk. We keep a map of - // to track the number of profiles triggered by a particular thread. Only one - // thread is counted as triggering the profile even though multiple threads - // might have been eligible for triggering the profile. + // Limits the max number of profiles that can be generated by this action + // over its lifetime to avoid filling the disk. // If not set (i.e. it's 0), a default of 10 will be used. - uint64 max_profiles_per_thread = 3; + uint64 max_profiles = 3; } diff --git a/source/extensions/watchdog/profile_action/BUILD b/source/extensions/watchdog/profile_action/BUILD index 0a028627ccd4c..6add86fd8479c 100644 --- a/source/extensions/watchdog/profile_action/BUILD +++ b/source/extensions/watchdog/profile_action/BUILD @@ -32,7 +32,7 @@ envoy_cc_extension( name = "config", srcs = ["config.cc"], hdrs = ["config.h"], - security_posture = "robust_to_untrusted_downstream_and_upstream", + security_posture = "data_plane_agnostic", status = "alpha", deps = [ ":profile_action_lib", diff --git a/source/extensions/watchdog/profile_action/config.h b/source/extensions/watchdog/profile_action/config.h index 39ad21a4ff2c0..0ccce8a03b7cd 100644 --- a/source/extensions/watchdog/profile_action/config.h +++ b/source/extensions/watchdog/profile_action/config.h @@ -12,7 +12,7 @@ namespace ProfileAction { class ProfileActionFactory : public Server::Configuration::GuardDogActionFactory { public: - ProfileActionFactory() : name_("envoy.watchdog.profile_action"){}; + ProfileActionFactory() = default; Server::Configuration::GuardDogActionPtr createGuardDogActionFromProto( const envoy::config::bootstrap::v3::Watchdog::WatchdogAction& config, @@ -22,13 +22,11 @@ class ProfileActionFactory : public Server::Configuration::GuardDogActionFactory return std::make_unique(); } - std::string name() const override { return name_; } + std::string name() const override { return "envoy.watchdog.profile_action"; } private: using ProfileActionConfig = envoy::extensions::watchdog::profile_action::v3alpha::ProfileActionConfig; - - const std::string name_; }; } // namespace ProfileAction diff --git a/source/extensions/watchdog/profile_action/profile_action.cc b/source/extensions/watchdog/profile_action/profile_action.cc index 6d63c3cf23f97..7b268d1911fff 100644 --- a/source/extensions/watchdog/profile_action/profile_action.cc +++ b/source/extensions/watchdog/profile_action/profile_action.cc @@ -14,7 +14,7 @@ namespace Extensions { namespace Watchdog { namespace ProfileAction { namespace { -static constexpr uint64_t DefaultMaxProfilePerTid = 10; +static constexpr uint64_t DefaultMaxProfiles = 10; std::string generateProfileFilePath(const std::string& directory, const SystemTime& now) { auto timestamp = std::chrono::duration_cast(now.time_since_epoch()).count(); @@ -31,9 +31,7 @@ ProfileAction::ProfileAction( : path_(config.profile_path()), duration_( std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, profile_duration, 5000))), - max_profiles_per_tid_(config.max_profiles_per_thread() == 0 - ? DefaultMaxProfilePerTid - : config.max_profiles_per_thread()), + max_profiles_(config.max_profiles() == 0 ? DefaultMaxProfiles : config.max_profiles()), running_profile_(false), profiles_started_(0), context_(context), timer_cb_(context_.dispatcher_.createTimer([this] { if (Profiler::Cpu::profilerEnabled()) { @@ -58,9 +56,15 @@ void ProfileAction::run( } // Check if there's a tid that justifies profiling - auto trigger_tid = getTidTriggeringProfile(thread_ltt_pairs); - if (!trigger_tid.has_value()) { - ENVOY_LOG_MISC(warn, "Profile Action: None of the provided tids justify profiling"); + if (thread_ltt_pairs.empty()) { + ENVOY_LOG_MISC(warn, "Profile Action: No tids were provided."); + return; + } + + if (profiles_started_ >= max_profiles_) { + ENVOY_LOG_MISC(warn, + "Profile Action: Unable to profile: enabled but already wrote {} profiles.", + profiles_started_); return; } @@ -78,7 +82,6 @@ void ProfileAction::run( // Update state running_profile_ = true; ++profiles_started_; - tid_to_profile_count_[*trigger_tid] += 1; // Schedule callback to stop timer_cb_->enableTimer(duration_); @@ -90,19 +93,6 @@ void ProfileAction::run( } } -// Helper to determine if we have a valid tid to start profiling. -absl::optional ProfileAction::getTidTriggeringProfile( - const std::vector>& thread_ltt_pairs) { - - // Find a TID not over the max_profiles threshold - for (const auto& [tid, ltt] : thread_ltt_pairs) { - if (tid_to_profile_count_[tid] < max_profiles_per_tid_) { - return tid; - } - } - - return absl::nullopt; -} } // namespace ProfileAction } // namespace Watchdog } // namespace Extensions diff --git a/source/extensions/watchdog/profile_action/profile_action.h b/source/extensions/watchdog/profile_action/profile_action.h index e91a2cfd3615e..24dea5b592d26 100644 --- a/source/extensions/watchdog/profile_action/profile_action.h +++ b/source/extensions/watchdog/profile_action/profile_action.h @@ -26,17 +26,12 @@ class ProfileAction : public Server::Configuration::GuardDogAction { MonotonicTime now) override; private: - // Helper to determine if we should run the profiler. - absl::optional getTidTriggeringProfile( - const std::vector>& thread_ltt_pairs); - const std::string path_; const std::chrono::milliseconds duration_; - const uint64_t max_profiles_per_tid_; + const uint64_t max_profiles_; bool running_profile_; std::string profile_filename_; uint64_t profiles_started_; - absl::flat_hash_map tid_to_profile_count_; Server::Configuration::GuardDogActionFactoryContext& context_; Event::TimerPtr timer_cb_; }; diff --git a/test/extensions/watchdog/profile_action/config_test.cc b/test/extensions/watchdog/profile_action/config_test.cc index ba29113fdc1b4..2b40dfa68f1f5 100644 --- a/test/extensions/watchdog/profile_action/config_test.cc +++ b/test/extensions/watchdog/profile_action/config_test.cc @@ -34,7 +34,7 @@ TEST(ProfileActionFactoryTest, CanCreateAction) { "value": { "profile_duration": "2s", "profile_path": "/tmp/envoy/", - "max_profiles_per_thread": "20" + "max_profiles": "20" } } }, diff --git a/test/extensions/watchdog/profile_action/profile_action_test.cc b/test/extensions/watchdog/profile_action/profile_action_test.cc index 2dade866a329b..01503c07efbb0 100644 --- a/test/extensions/watchdog/profile_action/profile_action_test.cc +++ b/test/extensions/watchdog/profile_action/profile_action_test.cc @@ -292,12 +292,12 @@ TEST_F(ProfileActionTest, ShouldNotProfileIfNoTids) { EXPECT_EQ(countNumberOfProfileInPath(test_path_), 0); } -TEST_F(ProfileActionTest, ShouldSaturateTids) { +TEST_F(ProfileActionTest, ShouldSaturatedMaxProfiles) { // Create configuration that we'll run until it saturates. envoy::extensions::watchdog::profile_action::v3alpha::ProfileActionConfig config; config.set_profile_path(test_path_); config.mutable_profile_duration()->set_seconds(1); - config.set_max_profiles_per_thread(1); + config.set_max_profiles(1); // Create the ProfileAction before we start running the dispatcher // otherwise the timer created will in ProfileActions ctor will