From b346e7a9faed718dcb6ffdd2db68ca2cf98c79d7 Mon Sep 17 00:00:00 2001 From: Mauro Ezequiel Moltrasio Date: Mon, 30 Sep 2024 17:39:24 +0200 Subject: [PATCH] Refactor RunSingle and RunSingleAfterglow into a single method (#1867) --- collector/lib/CollectorConfig.cpp | 47 +++++------- collector/lib/CollectorConfig.h | 4 +- collector/lib/EnvVar.h | 8 ++ collector/lib/NetworkStatusNotifier.cpp | 76 +++++-------------- collector/lib/NetworkStatusNotifier.h | 1 - .../schedule-curls/schedule-curls.sh | 2 + 6 files changed, 48 insertions(+), 90 deletions(-) diff --git a/collector/lib/CollectorConfig.cpp b/collector/lib/CollectorConfig.cpp index 38c283c7f0..a33b456782 100644 --- a/collector/lib/CollectorConfig.cpp +++ b/collector/lib/CollectorConfig.cpp @@ -36,7 +36,8 @@ StringListEnvVar ignored_networks("ROX_IGNORE_NETWORKS", std::vector()); -BoolEnvVar set_enable_afterglow("ROX_ENABLE_AFTERGLOW", true); +BoolEnvVar enable_afterglow("ROX_ENABLE_AFTERGLOW", true); +FloatEnvVar afterglow_period("ROX_AFTERGLOW_PERIOD", 300.0); BoolEnvVar set_enable_core_dump("ENABLE_CORE_DUMP", false); @@ -287,41 +288,27 @@ void CollectorConfig::InitCollectorConfig(CollectorArgs* args) { } void CollectorConfig::HandleAfterglowEnvVars() { - if (!set_enable_afterglow) { - enable_afterglow_ = false; - } - - if (const char* afterglow_period = std::getenv("ROX_AFTERGLOW_PERIOD")) { - afterglow_period_micros_ = static_cast(atof(afterglow_period) * 1000000); - } + constexpr int64_t SECOND = 1'000'000; + constexpr int64_t max_afterglow_period_micros = 300 * SECOND; // 5 minutes - const int64_t max_afterglow_period_micros = 300000000; // 5 minutes - - if (afterglow_period_micros_ > max_afterglow_period_micros) { - CLOG(ERROR) << "User set afterglow period of " << afterglow_period_micros_ / 1000000 - << "s is greater than the maximum allowed afterglow period of " << max_afterglow_period_micros / 1000000 << "s"; - CLOG(ERROR) << "Setting the afterglow period to " << max_afterglow_period_micros / 1000000 << "s"; - afterglow_period_micros_ = max_afterglow_period_micros; - } - - if (enable_afterglow_ && afterglow_period_micros_ > 0) { - CLOG(INFO) << "Afterglow is enabled"; - return; - } - - if (!enable_afterglow_) { - CLOG(INFO) << "Afterglow is disabled"; - return; - } + afterglow_period_micros_ = static_cast(afterglow_period.value() * SECOND); if (afterglow_period_micros_ < 0) { - CLOG(ERROR) << "Invalid afterglow period " << afterglow_period_micros_ / 1000000 << ". ROX_AFTERGLOW_PERIOD must be positive."; + CLOG(ERROR) << "Invalid afterglow period " << afterglow_period_micros_ / SECOND << ". ROX_AFTERGLOW_PERIOD must be positive."; + } else if (afterglow_period_micros_ == 0) { + CLOG(ERROR) << "Afterglow period set to 0."; } else { - CLOG(ERROR) << "Afterglow period set to 0"; + if (afterglow_period_micros_ > max_afterglow_period_micros) { + CLOG(WARNING) << "User set afterglow period of " << afterglow_period_micros_ / SECOND + << "s is greater than the maximum allowed afterglow period of " << max_afterglow_period_micros / SECOND << "s"; + CLOG(WARNING) << "Setting the afterglow period to " << max_afterglow_period_micros / SECOND << "s"; + afterglow_period_micros_ = max_afterglow_period_micros; + } + + enable_afterglow_ = enable_afterglow.value(); } - enable_afterglow_ = false; - CLOG(INFO) << "Disabling afterglow"; + CLOG(INFO) << "Afterglow is " << (enable_afterglow_ ? "enabled" : "disabled"); } void CollectorConfig::HandleConnectionStatsEnvVars() { diff --git a/collector/lib/CollectorConfig.h b/collector/lib/CollectorConfig.h index e62cdf8dca..81a885da92 100644 --- a/collector/lib/CollectorConfig.h +++ b/collector/lib/CollectorConfig.h @@ -118,8 +118,8 @@ class CollectorConfig { std::vector non_aggregated_networks_; HostConfig host_config_; - int64_t afterglow_period_micros_ = 300000000; // 5 minutes in microseconds - bool enable_afterglow_ = true; + int64_t afterglow_period_micros_ = 300'000'000; // 5 minutes in microseconds + bool enable_afterglow_ = false; bool enable_core_dump_ = false; bool enable_processes_listening_on_ports_; bool import_users_; diff --git a/collector/lib/EnvVar.h b/collector/lib/EnvVar.h index 9477dab0eb..61bd83aaba 100644 --- a/collector/lib/EnvVar.h +++ b/collector/lib/EnvVar.h @@ -110,6 +110,13 @@ struct ParsePath { return true; } }; + +struct ParseFloat { + bool operator()(float* out, const std::string& str_val) { + *out = std::stof(str_val); + return true; + } +}; } // namespace internal using BoolEnvVar = EnvVar; @@ -117,6 +124,7 @@ using StringListEnvVar = EnvVar, internal::ParseStringL using StringEnvVar = EnvVar; using IntEnvVar = EnvVar; using PathEnvVar = EnvVar; +using FloatEnvVar = EnvVar; } // namespace collector diff --git a/collector/lib/NetworkStatusNotifier.cpp b/collector/lib/NetworkStatusNotifier.cpp index d6f93c1567..f95b0c6902 100644 --- a/collector/lib/NetworkStatusNotifier.cpp +++ b/collector/lib/NetworkStatusNotifier.cpp @@ -120,11 +120,7 @@ void NetworkStatusNotifier::Run() { auto client_writer = comm_->PushNetworkConnectionInfoOpenStream([this](const sensor::NetworkFlowsControlMessage* msg) { OnRecvControlMessage(msg); }); - if (enable_afterglow_) { - RunSingleAfterglow(client_writer.get()); - } else { - RunSingle(client_writer.get()); - } + RunSingle(client_writer.get()); if (thread_.should_stop()) { return; } @@ -222,61 +218,17 @@ bool NetworkStatusNotifier::UpdateAllConnsAndEndpoints() { void NetworkStatusNotifier::RunSingle(IDuplexClientWriter* writer) { WaitUntilWriterStarted(writer, 10); - ConnMap old_conn_state; - AdvertisedEndpointMap old_cep_state; - auto next_scrape = std::chrono::system_clock::now(); - - while (writer->Sleep(next_scrape)) { - next_scrape = std::chrono::system_clock::now() + std::chrono::seconds(scrape_interval_); - - if (!UpdateAllConnsAndEndpoints()) { - continue; - } - - ReportConnectionStats(); - - const sensor::NetworkConnectionInfoMessage* msg; - ConnMap new_conn_state; - AdvertisedEndpointMap new_cep_state; - WITH_TIMER(CollectorStats::net_fetch_state) { - new_conn_state = conn_tracker_->FetchConnState(true, true); - ConnectionTracker::ComputeDelta(new_conn_state, &old_conn_state); - - new_cep_state = conn_tracker_->FetchEndpointState(true, true); - ConnectionTracker::ComputeDelta(new_cep_state, &old_cep_state); - } - - WITH_TIMER(CollectorStats::net_create_message) { - msg = CreateInfoMessage(old_conn_state, old_cep_state); - old_conn_state = std::move(new_conn_state); - old_cep_state = std::move(new_cep_state); - } - - if (!msg) { - continue; - } - - WITH_TIMER(CollectorStats::net_write_message) { - if (!writer->Write(*msg, next_scrape)) { - CLOG(ERROR) << "Failed to write network connection info"; - return; - } - } - } -} - -void NetworkStatusNotifier::RunSingleAfterglow(IDuplexClientWriter* writer) { - WaitUntilWriterStarted(writer, 10); - ConnMap old_conn_state; AdvertisedEndpointMap old_cep_state; auto next_scrape = std::chrono::system_clock::now(); int64_t time_at_last_scrape = NowMicros(); while (writer->Sleep(next_scrape)) { + CLOG(DEBUG) << "Starting network status notification"; next_scrape = std::chrono::system_clock::now() + std::chrono::seconds(scrape_interval_); if (!UpdateAllConnsAndEndpoints()) { + CLOG(DEBUG) << "No connection or endpoint to report"; continue; } @@ -284,26 +236,34 @@ void NetworkStatusNotifier::RunSingleAfterglow(IDuplexClientWriterFetchConnState(true, true); - ConnectionTracker::ComputeDeltaAfterglow(new_conn_state, old_conn_state, delta_conn, time_micros, time_at_last_scrape, afterglow_period_micros_); + if (enable_afterglow_) { + ConnectionTracker::ComputeDeltaAfterglow(new_conn_state, old_conn_state, delta_conn, time_micros, time_at_last_scrape, afterglow_period_micros_); + } else { + ConnectionTracker::ComputeDelta(new_conn_state, &old_conn_state); + } new_cep_state = conn_tracker_->FetchEndpointState(true, true); ConnectionTracker::ComputeDelta(new_cep_state, &old_cep_state); } WITH_TIMER(CollectorStats::net_create_message) { - // Report the deltas - msg = CreateInfoMessage(delta_conn, old_cep_state); - // Add new connections to the old_state and remove inactive connections that are older than the afterglow period. - ConnectionTracker::UpdateOldState(&old_conn_state, new_conn_state, time_micros, afterglow_period_micros_); + if (enable_afterglow_) { + msg = CreateInfoMessage(delta_conn, old_cep_state); + ConnectionTracker::UpdateOldState(&old_conn_state, new_conn_state, time_micros, afterglow_period_micros_); + } else { + msg = CreateInfoMessage(old_conn_state, old_cep_state); + old_conn_state = std::move(new_conn_state); + } old_cep_state = std::move(new_cep_state); time_at_last_scrape = time_micros; } if (!msg) { + CLOG(DEBUG) << "No update to report"; continue; } @@ -313,6 +273,8 @@ void NetworkStatusNotifier::RunSingleAfterglow(IDuplexClientWriter* writer, int wait_time); bool UpdateAllConnsAndEndpoints(); void RunSingle(IDuplexClientWriter* writer); - void RunSingleAfterglow(IDuplexClientWriter* writer); void ReceivePublicIPs(const sensor::IPAddressList& public_ips); void ReceiveIPNetworks(const sensor::IPNetworkList& networks); diff --git a/integration-tests/container/schedule-curls/schedule-curls.sh b/integration-tests/container/schedule-curls/schedule-curls.sh index 0e24e72d15..503d1e0b8e 100755 --- a/integration-tests/container/schedule-curls/schedule-curls.sh +++ b/integration-tests/container/schedule-curls/schedule-curls.sh @@ -1,5 +1,7 @@ #!/usr/bin/env sh +set -ex + num_meta_iter=$1 num_iter=$2 sleep_between_curl_time=$3