diff --git a/lib/cli/daemonutility.cpp b/lib/cli/daemonutility.cpp index 9e910f313..5e250d22c 100644 --- a/lib/cli/daemonutility.cpp +++ b/lib/cli/daemonutility.cpp @@ -256,17 +256,6 @@ bool DaemonUtility::LoadConfigFiles(const std::vector& configs, upq.SetName("DaemonUtility::LoadConfigFiles"); bool result = ConfigItem::CommitItems(ascope.GetContext(), upq, newItems); - if (result) { - try { - Dependency::AssertNoCycles(); - } catch (...) { - Log(LogCritical, "config") - << DiagnosticInformation(boost::current_exception(), false); - - result = false; - } - } - if (!result) { ConfigCompilerContext::GetInstance()->CancelObjectsFile(); return false; diff --git a/lib/icinga/dependency.cpp b/lib/icinga/dependency.cpp index 55e681c88..a9a7bf372 100644 --- a/lib/icinga/dependency.cpp +++ b/lib/icinga/dependency.cpp @@ -17,7 +17,12 @@ using namespace icinga; REGISTER_TYPE(Dependency); -bool Dependency::m_AssertNoCyclesForIndividualDeps = false; +INITIALIZE_ONCE(&Dependency::StaticInitialize); + +void Dependency::StaticInitialize() +{ + ConfigType::Get()->BeforeOnAllConfigLoaded.connect(&BeforeOnAllConfigLoadedHandler); +} /** * Helper class to search for cycles in the dependency graph. @@ -31,6 +36,7 @@ class DependencyCycleChecker { bool Visited = false; bool OnStack = false; + std::vector ExtraDependencies; }; std::unordered_map m_Nodes; @@ -40,6 +46,19 @@ class DependencyCycleChecker std::vector> m_Stack; public: + /** + * Add a dependency to this DependencyCycleChecker that will be considered by AssertNoCycle() in addition to + * dependencies already registered to the checkables. This allows checking if additional dependencies would cause + * a cycle before actually registering them to the checkables. + * + * @param dependency Dependency to additionally consider during the cycle search. + */ + void AddExtraDependency(Dependency::Ptr dependency) + { + auto& node = m_Nodes[dependency->GetChild()]; + node.ExtraDependencies.emplace_back(std::move(dependency)); + } + /** * Searches the dependency graph for cycles and throws an exception if one is found. * @@ -110,23 +129,43 @@ public: m_Stack.pop_back(); } + // Additional dependencies to consider + for (const auto& dep : node.ExtraDependencies) { + m_Stack.emplace_back(dep); + AssertNoCycle(dep->GetParent()); + m_Stack.pop_back(); + } + node.OnStack = false; } }; -void Dependency::AssertNoCycles() +/** + * Checks that adding these new dependencies to the configuration does not introduce any cycles. + * + * This is done as an optimization: cycles are checked once for all dependencies in a batch of config objects instead + * of individually per dependency in Dependency::OnAllConfigLoaded(). For runtime updates, this function may still be + * called for single objects. + * + * @param items Config items containing Dependency objects added to the running configuration. + */ +void Dependency::BeforeOnAllConfigLoadedHandler(const ConfigItems& items) { DependencyCycleChecker checker; - for (auto& host : ConfigType::GetObjectsByType()) { - checker.AssertNoCycle(host); - } + // Resolve parent/child names to Checkable::Ptr and temporarily add the edges to the checker. + // The dependencies are later registered to the checkables by Dependency::OnAllConfigLoaded(). + items.ForEachObject([&checker](Dependency::Ptr dependency) { + dependency->InitChildParentReferences(); + checker.AddExtraDependency(std::move(dependency)); + }); - for (auto& service : ConfigType::GetObjectsByType()) { - checker.AssertNoCycle(service); - } - - m_AssertNoCyclesForIndividualDeps = true; + // It's sufficient to search for cycles starting from newly added dependencies only: if a newly added dependency is + // part of a cycle, that cycle is reachable from both the child and the parent of that dependency. The cycle search + // is started from the parent as a slight optimization as that will traverse fewer edges if there is no cycle. + items.ForEachObject([&checker](const Dependency::Ptr& dependency) { + checker.AssertNoCycle(dependency->GetParent()); + }); } String DependencyNameComposer::MakeName(const String& shortName, const Object::Ptr& context) const @@ -178,10 +217,8 @@ void Dependency::OnConfigLoaded() SetStateFilter(FilterArrayToInt(GetStates(), Notification::GetStateFilterMap(), defaultFilter)); } -void Dependency::OnAllConfigLoaded() +void Dependency::InitChildParentReferences() { - ObjectImpl::OnAllConfigLoaded(); - Host::Ptr childHost = Host::GetByName(GetChildHostName()); if (childHost) { @@ -205,21 +242,17 @@ void Dependency::OnAllConfigLoaded() if (!m_Parent) BOOST_THROW_EXCEPTION(ScriptError("Dependency '" + GetName() + "' references a parent host/service which doesn't exist.", GetDebugInfo())); +} + +void Dependency::OnAllConfigLoaded() +{ + ObjectImpl::OnAllConfigLoaded(); + + // InitChildParentReferences() has to be called before. + VERIFY(m_Child && m_Parent); m_Child->AddDependency(this); m_Parent->AddReverseDependency(this); - - if (m_AssertNoCyclesForIndividualDeps) { - DependencyCycleChecker checker; - - try { - checker.AssertNoCycle(m_Parent); - } catch (...) { - m_Child->RemoveDependency(this); - m_Parent->RemoveReverseDependency(this); - throw; - } - } } void Dependency::Stop(bool runtimeRemoved) diff --git a/lib/icinga/dependency.hpp b/lib/icinga/dependency.hpp index 6cebfaab1..32bd8e70d 100644 --- a/lib/icinga/dependency.hpp +++ b/lib/icinga/dependency.hpp @@ -5,6 +5,7 @@ #include "icinga/i2-icinga.hpp" #include "icinga/dependency-ti.hpp" +#include "config/configitem.hpp" namespace icinga { @@ -22,6 +23,8 @@ class Service; class Dependency final : public ObjectImpl { public: + static void StaticInitialize(); + DECLARE_OBJECT(Dependency); DECLARE_OBJECTNAME(Dependency); @@ -36,9 +39,8 @@ public: static void EvaluateApplyRules(const intrusive_ptr& host); static void EvaluateApplyRules(const intrusive_ptr& service); - static void AssertNoCycles(); - /* Note: Only use them for unit test mocks. Prefer OnConfigLoaded(). */ + /* Note: Only use them for unit test mocks. Prefer InitChildParentReferences(). */ void SetParent(intrusive_ptr parent); void SetChild(intrusive_ptr child); @@ -46,15 +48,16 @@ protected: void OnConfigLoaded() override; void OnAllConfigLoaded() override; void Stop(bool runtimeRemoved) override; + void InitChildParentReferences(); private: Checkable::Ptr m_Parent; Checkable::Ptr m_Child; - static bool m_AssertNoCyclesForIndividualDeps; - static bool EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule, bool skipFilter); static bool EvaluateApplyRule(const Checkable::Ptr& checkable, const ApplyRule& rule, bool skipFilter = false); + + static void BeforeOnAllConfigLoadedHandler(const ConfigItems& items); }; }