Skip to content

PENG-6092: attempt to deal with race condition by returning success on duplication error #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fformica merged 3 commits into from
Mar 11, 2025
Merged

PENG-6092: attempt to deal with race condition by returning success on duplication error #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99f9d59
PENG-6092: check if we can tolerate the race condition by returning s…
fformica Mar 10, 2025
3b4bbc0
Force policy name to std::string for logging
fformica Mar 10, 2025
bebbb38
Align unit test
fformica Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/CoreServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,15 +353,18 @@ void CoreServer::_setup_routes(const PrometheusConfig &prom_config)
});
_svr.Delete(fmt::format("/api/v1/policies/({})", AbstractModule::MODULE_ID_REGEX).c_str(), [&](const httplib::Request &req, httplib::Response &res) {
json j = json::object();
auto name = req.matches[1];
std::string name = req.matches[1];
if (!_registry->policy_manager()->module_exists(name)) {
res.status = 404;
j["error"] = "policy does not exists";
res.set_content(j.dump(), "text/json");
_logger->info("delete: policy not found: {}", name);
return;
}
try {
_logger->info("delete: removing policy: {}", name);
_registry->policy_manager()->remove_policy(name);
_logger->info("delete: removed policy: {}", name);
res.status = 200;
res.set_content(j.dump(), "text/json");
} catch (const std::exception &e) {
Expand Down
10 changes: 6 additions & 4 deletions src/Policies.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,13 @@ std::vector<Policy *> PolicyManager::load(const YAML::Node &policy_yaml, bool si
// serialized policy loads
std::unique_lock lock(_load_mutex);

// Ensure policy name isn't already defined
auto policy_name = _get_policy_name(it);
if (module_exists(policy_name)) {
// ignore
spdlog::get("visor")->warn("ignoring policy with name '{}' already defined", policy_name);
continue;
}
// Input Section
auto input_node = it->second["input"];
auto [input_config, input_filter] = _registry->input_manager()->get_config_and_filter(input_node);
Expand Down Expand Up @@ -225,10 +231,6 @@ std::string PolicyManager::_get_policy_name(YAML::const_iterator it)
if (!it->second.IsMap()) {
throw PolicyException("expecting policy configuration map");
}
// Ensure policy name isn't already defined
if (module_exists(policy_name)) {
throw PolicyException(fmt::format("policy with name '{}' already defined", policy_name));
}

// Policy kind defines schema
if (!it->second["kind"] || !it->second["kind"].IsScalar()) {
Expand Down
6 changes: 4 additions & 2 deletions src/tests/test_policies.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wmissing-declarations"
#endif
#include "inputs/static_plugins.h"
#include "handlers/static_plugins.h"
#include "inputs/static_plugins.h"
#ifdef __GNUC__
#pragma GCC diagnostic pop
#endif
Expand Down Expand Up @@ -901,7 +901,9 @@ TEST_CASE("Policies", "[policies]")

REQUIRE_NOTHROW(registry.tap_manager()->load(config_file["visor"]["taps"]));
REQUIRE_NOTHROW(registry.policy_manager()->load(config_file["visor"]["policies"]));
REQUIRE_THROWS_WITH(registry.policy_manager()->load(config_file["visor"]["policies"]), "policy with name 'default_view' already defined");
// tolerated for the time being
REQUIRE_NOTHROW(registry.policy_manager()->load(config_file["visor"]["policies"]));
// REQUIRE_THROWS_WITH(registry.policy_manager()->load(config_file["visor"]["policies"]), "policy with name 'default_view' already defined");

REQUIRE(registry.policy_manager()->module_exists("default_view"));
auto [policy, lock] = registry.policy_manager()->module_get_locked("default_view");
Expand Down