From b848934d5792178ed89bd4a6d86e664db686ab3e Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Mon, 2 Sep 2024 17:34:47 +0200 Subject: [PATCH 1/4] Introduce Type::GetConfigTypesSortedByLoadDependencies() --- lib/base/initialize.hpp | 1 + lib/base/type.cpp | 65 +++++++++++++++++++++++++++++++++++++++++ lib/base/type.hpp | 1 + 3 files changed, 67 insertions(+) diff --git a/lib/base/initialize.hpp b/lib/base/initialize.hpp index adc995fe1b9..6c50b240875 100644 --- a/lib/base/initialize.hpp +++ b/lib/base/initialize.hpp @@ -23,6 +23,7 @@ enum class InitializePriority { RegisterBuiltinTypes, RegisterFunctions, RegisterTypes, + SortTypes, EvaluateConfigFragments, Default, FreezeNamespaces, diff --git a/lib/base/type.cpp b/lib/base/type.cpp index 1dcef39bfc9..70dd6333ee0 100644 --- a/lib/base/type.cpp +++ b/lib/base/type.cpp @@ -1,9 +1,15 @@ /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ #include "base/type.hpp" +#include "base/atomic.hpp" +#include "base/configobject.hpp" +#include "base/debug.hpp" #include "base/scriptglobal.hpp" #include "base/namespace.hpp" #include "base/objectlock.hpp" +#include +#include +#include using namespace icinga; @@ -32,6 +38,59 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() { Type::Register(type); }, InitializePriority::RegisterTypeType); +static std::vector l_SortedByLoadDependencies; +static Atomic l_SortingByLoadDependenciesDone (false); + +typedef std::unordered_map Visited; // https://stackoverflow.com/a/8942986 + +INITIALIZE_ONCE_WITH_PRIORITY([] { + auto types (Type::GetAllTypes()); + + types.erase(std::remove_if(types.begin(), types.end(), [](auto& type) { + return !ConfigObject::TypeInstance->IsAssignableFrom(type); + }), types.end()); + + // Depth-first search + std::unordered_set unsorted; + Visited visited; + std::vector sorted; + + for (auto type : types) { + unsorted.emplace(type.get()); + } + + std::function visit ([&visit, &unsorted, &visited, &sorted](Type* type) { + if (unsorted.find(type) == unsorted.end()) { + return; + } + + bool& alreadyVisited (visited.at(type)); + VERIFY(!alreadyVisited); + alreadyVisited = true; + + for (auto dep : type->GetLoadDependencies()) { + visit(dep); + } + + unsorted.erase(type); + sorted.emplace_back(type); + }); + + while (!unsorted.empty()) { + for (auto& type : types) { + visited[type.get()] = false; + } + + visit(*unsorted.begin()); + } + + VERIFY(sorted.size() == types.size()); + VERIFY(sorted[0]->GetLoadDependencies().empty()); + + std::swap(sorted, l_SortedByLoadDependencies); + l_SortingByLoadDependenciesDone.store(true); +}, InitializePriority::SortTypes); + String Type::ToString() const { return "type '" + GetName() + "'"; @@ -72,6 +131,12 @@ std::vector Type::GetAllTypes() return types; } +const std::vector& Type::GetConfigTypesSortedByLoadDependencies() +{ + VERIFY(l_SortingByLoadDependenciesDone.load()); + return l_SortedByLoadDependencies; +} + String Type::GetPluralName() const { String name = GetName(); diff --git a/lib/base/type.hpp b/lib/base/type.hpp index 7b8d1cacb64..fa78f858ed0 100644 --- a/lib/base/type.hpp +++ b/lib/base/type.hpp @@ -82,6 +82,7 @@ class Type : public Object static void Register(const Type::Ptr& type); static Type::Ptr GetByName(const String& name); static std::vector GetAllTypes(); + static const std::vector& GetConfigTypesSortedByLoadDependencies(); void SetField(int id, const Value& value, bool suppress_events = false, const Value& cookie = Empty) override; Value GetField(int id) const override; From 31f3acaa137dd124d62c9baea156f19985a12b77 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Thu, 15 Feb 2024 11:46:27 +0100 Subject: [PATCH 2/4] ConfigItem::CommitNewItems(): pre-sort types by their load dependencies once to avoid complicated nested loops, iterating over the same types and checking dependencies over and over, skipping already completed ones. --- lib/config/configitem.cpp | 201 ++++++++++++++------------------------ 1 file changed, 76 insertions(+), 125 deletions(-) diff --git a/lib/config/configitem.cpp b/lib/config/configitem.cpp index bf4da81a4be..e8a50927543 100644 --- a/lib/config/configitem.cpp +++ b/lib/config/configitem.cpp @@ -444,74 +444,47 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue << "Committing " << total << " new items."; #endif /* I2_DEBUG */ - std::set types; - std::set completed_types; int itemsCount {0}; - for (const Type::Ptr& type : Type::GetAllTypes()) { - if (ConfigObject::TypeInstance->IsAssignableFrom(type)) - types.insert(type); - } - - while (types.size() != completed_types.size()) { - for (const Type::Ptr& type : types) { - if (completed_types.find(type) != completed_types.end()) - continue; + for (auto& type : Type::GetConfigTypesSortedByLoadDependencies()) { + std::atomic committed_items(0); - bool unresolved_dep = false; + { + auto items (itemsByType.find(type.get())); - /* skip this type (for now) if there are unresolved load dependencies */ - for (auto pLoadDep : type->GetLoadDependencies()) { - if (types.find(pLoadDep) != types.end() && completed_types.find(pLoadDep) == completed_types.end()) { - unresolved_dep = true; - break; + if (items != itemsByType.end()) { + for (const ItemPair& pair: items->second) { + newItems.emplace_back(pair.first); } - } - - if (unresolved_dep) - continue; - std::atomic committed_items(0); + upq.ParallelFor(items->second, [&committed_items](const ItemPair& ip) { + const ConfigItem::Ptr& item = ip.first; - { - auto items (itemsByType.find(type.get())); + if (!item->Commit(ip.second)) { + if (item->IsIgnoreOnError()) { + item->Unregister(); + } - if (items != itemsByType.end()) { - for (const ItemPair& pair: items->second) { - newItems.emplace_back(pair.first); + return; } - upq.ParallelFor(items->second, [&committed_items](const ItemPair& ip) { - const ConfigItem::Ptr& item = ip.first; + committed_items++; + }); - if (!item->Commit(ip.second)) { - if (item->IsIgnoreOnError()) { - item->Unregister(); - } - - return; - } - - committed_items++; - }); - - upq.Join(); - } + upq.Join(); } + } - itemsCount += committed_items; - - completed_types.insert(type); + itemsCount += committed_items; #ifdef I2_DEBUG - if (committed_items > 0) - Log(LogDebug, "configitem") - << "Committed " << committed_items << " items of type '" << type->GetName() << "'."; + if (committed_items > 0) + Log(LogDebug, "configitem") + << "Committed " << committed_items << " items of type '" << type->GetName() << "'."; #endif /* I2_DEBUG */ - if (upq.HasExceptions()) - return false; - } + if (upq.HasExceptions()) + return false; } #ifdef I2_DEBUG @@ -519,105 +492,83 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue << "Committed " << itemsCount << " items."; #endif /* I2_DEBUG */ - completed_types.clear(); + for (auto& type : Type::GetConfigTypesSortedByLoadDependencies()) { + std::atomic notified_items(0); - while (types.size() != completed_types.size()) { - for (const Type::Ptr& type : types) { - if (completed_types.find(type) != completed_types.end()) - continue; + { + auto items (itemsByType.find(type.get())); - bool unresolved_dep = false; + if (items != itemsByType.end()) { + upq.ParallelFor(items->second, [¬ified_items](const ItemPair& ip) { + const ConfigItem::Ptr& item = ip.first; - /* skip this type (for now) if there are unresolved load dependencies */ - for (auto pLoadDep : type->GetLoadDependencies()) { - if (types.find(pLoadDep) != types.end() && completed_types.find(pLoadDep) == completed_types.end()) { - unresolved_dep = true; - break; - } - } + if (!item->m_Object) + return; - if (unresolved_dep) - continue; - - std::atomic notified_items(0); - - { - auto items (itemsByType.find(type.get())); - - if (items != itemsByType.end()) { - upq.ParallelFor(items->second, [¬ified_items](const ItemPair& ip) { - const ConfigItem::Ptr& item = ip.first; - - if (!item->m_Object) - return; + try { + item->m_Object->OnAllConfigLoaded(); + notified_items++; + } catch (const std::exception& ex) { + if (!item->m_IgnoreOnError) + throw; - try { - item->m_Object->OnAllConfigLoaded(); - notified_items++; - } catch (const std::exception& ex) { - if (!item->m_IgnoreOnError) - throw; + Log(LogNotice, "ConfigObject") + << "Ignoring config object '" << item->m_Name << "' of type '" << item->m_Type->GetName() << "' due to errors: " << DiagnosticInformation(ex); - Log(LogNotice, "ConfigObject") - << "Ignoring config object '" << item->m_Name << "' of type '" << item->m_Type->GetName() << "' due to errors: " << DiagnosticInformation(ex); + item->Unregister(); - item->Unregister(); - - { - std::unique_lock lock(item->m_Mutex); - item->m_IgnoredItems.push_back(item->m_DebugInfo.Path); - } + { + std::unique_lock lock(item->m_Mutex); + item->m_IgnoredItems.push_back(item->m_DebugInfo.Path); } - }); + } + }); - upq.Join(); - } + upq.Join(); } - - completed_types.insert(type); + } #ifdef I2_DEBUG - if (notified_items > 0) - Log(LogDebug, "configitem") - << "Sent OnAllConfigLoaded to " << notified_items << " items of type '" << type->GetName() << "'."; + if (notified_items > 0) + Log(LogDebug, "configitem") + << "Sent OnAllConfigLoaded to " << notified_items << " items of type '" << type->GetName() << "'."; #endif /* I2_DEBUG */ - if (upq.HasExceptions()) - return false; + if (upq.HasExceptions()) + return false; - notified_items = 0; - for (auto loadDep : type->GetLoadDependencies()) { - auto items (itemsByType.find(loadDep)); + notified_items = 0; + for (auto loadDep : type->GetLoadDependencies()) { + auto items (itemsByType.find(loadDep)); - if (items != itemsByType.end()) { - upq.ParallelFor(items->second, [&type, ¬ified_items](const ItemPair& ip) { - const ConfigItem::Ptr& item = ip.first; + if (items != itemsByType.end()) { + upq.ParallelFor(items->second, [&type, ¬ified_items](const ItemPair& ip) { + const ConfigItem::Ptr& item = ip.first; - if (!item->m_Object) - return; + if (!item->m_Object) + return; - ActivationScope ascope(item->m_ActivationContext); - item->m_Object->CreateChildObjects(type); - notified_items++; - }); - } + ActivationScope ascope(item->m_ActivationContext); + item->m_Object->CreateChildObjects(type); + notified_items++; + }); } + } - upq.Join(); + upq.Join(); #ifdef I2_DEBUG - if (notified_items > 0) - Log(LogDebug, "configitem") - << "Sent CreateChildObjects to " << notified_items << " items of type '" << type->GetName() << "'."; + if (notified_items > 0) + Log(LogDebug, "configitem") + << "Sent CreateChildObjects to " << notified_items << " items of type '" << type->GetName() << "'."; #endif /* I2_DEBUG */ - if (upq.HasExceptions()) - return false; + if (upq.HasExceptions()) + return false; - // Make sure to activate any additionally generated items - if (!CommitNewItems(context, upq, newItems)) - return false; - } + // Make sure to activate any additionally generated items + if (!CommitNewItems(context, upq, newItems)) + return false; } return true; From 467e8b18e7ca9b246ca2e8b5afeaa7b5cc6392b3 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 9 Sep 2024 17:25:13 +0200 Subject: [PATCH 3/4] Type: Simplify sort by load dependencies algorithm --- lib/base/type.cpp | 56 ++++++++++++++++------------------------------- lib/base/type.hpp | 14 ++++++++++++ 2 files changed, 33 insertions(+), 37 deletions(-) diff --git a/lib/base/type.cpp b/lib/base/type.cpp index 70dd6333ee0..f6539b7c447 100644 --- a/lib/base/type.cpp +++ b/lib/base/type.cpp @@ -7,9 +7,7 @@ #include "base/scriptglobal.hpp" #include "base/namespace.hpp" #include "base/objectlock.hpp" -#include #include -#include using namespace icinga; @@ -41,53 +39,37 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() { static std::vector l_SortedByLoadDependencies; static Atomic l_SortingByLoadDependenciesDone (false); -typedef std::unordered_map Visited; // https://stackoverflow.com/a/8942986 - INITIALIZE_ONCE_WITH_PRIORITY([] { - auto types (Type::GetAllTypes()); - - types.erase(std::remove_if(types.begin(), types.end(), [](auto& type) { - return !ConfigObject::TypeInstance->IsAssignableFrom(type); - }), types.end()); - - // Depth-first search - std::unordered_set unsorted; - Visited visited; - std::vector sorted; - - for (auto type : types) { - unsorted.emplace(type.get()); - } + std::unordered_set visited; - std::function visit ([&visit, &unsorted, &visited, &sorted](Type* type) { - if (unsorted.find(type) == unsorted.end()) { + std::function visit; + // Please note that this callback does not detect any cyclic load dependencies, + // instead, it relies on the "sort_by_load_after" unit test to fail. + visit = ([&visit, &visited](Type* type) { + if (visited.find(type) != visited.end()) { return; } + visited.emplace(type); - bool& alreadyVisited (visited.at(type)); - VERIFY(!alreadyVisited); - alreadyVisited = true; - - for (auto dep : type->GetLoadDependencies()) { - visit(dep); + for (auto dependency : type->GetLoadDependencies()) { + visit(dependency); } - unsorted.erase(type); - sorted.emplace_back(type); + // We have managed to reach the final/top node in this dependency graph, + // so let's place them in reverse order to their final place. + l_SortedByLoadDependencies.emplace_back(type); }); - while (!unsorted.empty()) { - for (auto& type : types) { - visited[type.get()] = false; + // Sort the types by their load_after dependencies in a Depth-First search manner. + for (const Type::Ptr& type : Type::GetAllTypes()) { + // Note that only those types that are assignable to the dynamic ConfigObject type can have "load_after" + // dependencies, otherwise they are just some Icinga 2 primitive types such as Number, String, etc. and + // we need to ignore them. + if (ConfigObject::TypeInstance->IsAssignableFrom(type)) { + visit(type.get()); } - - visit(*unsorted.begin()); } - VERIFY(sorted.size() == types.size()); - VERIFY(sorted[0]->GetLoadDependencies().empty()); - - std::swap(sorted, l_SortedByLoadDependencies); l_SortingByLoadDependenciesDone.store(true); }, InitializePriority::SortTypes); diff --git a/lib/base/type.hpp b/lib/base/type.hpp index fa78f858ed0..ee8a9b37cd4 100644 --- a/lib/base/type.hpp +++ b/lib/base/type.hpp @@ -82,6 +82,20 @@ class Type : public Object static void Register(const Type::Ptr& type); static Type::Ptr GetByName(const String& name); static std::vector GetAllTypes(); + + /** + * Returns a list of config types sorted by their "load_after" dependencies. + * + * All dependencies of a given type are listed at a lower index than that of the type itself. In other words, + * if a `Service` type load depends on the `Host` and `ApiListener` types, the Host and ApiListener types are + * guaranteed to appear first on the list. Nevertheless, the order of the Host and ApiListener types themselves + * is arbitrary if the two types are not dependent. + * + * It should be noted that this method will fail fatally when used prior to the completion + * of namespace initialization. + * + * @return std::vector + */ static const std::vector& GetConfigTypesSortedByLoadDependencies(); void SetField(int id, const Value& value, bool suppress_events = false, const Value& cookie = Empty) override; From eb97676d699cc326fc7b07b29c60a2482bf35295 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Tue, 10 Sep 2024 09:28:28 +0200 Subject: [PATCH 4/4] Add basic test cases for `Type::GetConfigTypesSortedByLoadDependencies()` --- test/CMakeLists.txt | 58 +++++++++++++++++++++++++++++++++++++++++---- test/base-type.cpp | 33 +++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index e3772886abd..dd9724f0bf0 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -2,6 +2,59 @@ include(BoostTestTargets) +set(types_test_SOURCES + icingaapplication-fixture.cpp + base-type.cpp + ${base_OBJS} + $ + $ + $ + $ +) + +if(ICINGA2_WITH_CHECKER) + list(APPEND types_test_SOURCES $) +endif() + +if(ICINGA2_WITH_MYSQL) + list(APPEND types_test_SOURCES $ $) +endif() + +if(ICINGA2_WITH_PGSQL) + list(APPEND types_test_SOURCES $ $) +endif() + +if(ICINGA2_WITH_ICINGADB) + list(APPEND types_test_SOURCES $) +endif() + +if(ICINGA2_WITH_NOTIFICATION) + list(APPEND types_test_SOURCES $) +endif() + +if(ICINGA2_WITH_PERFDATA) + list(APPEND types_test_SOURCES $) +endif() + +if(ICINGA2_UNITY_BUILD) + mkunity_target(types test types_test_SOURCES) +endif() + +# In order to test the order of all Icinga 2 config type load dependencies, we need to link against all the libraries, +# but this results in boost signals e.g. in dbevents.cpp being triggered by icinga-checkresult.cpp test cases that +# only pass partially initialised objects. Therefore, the types test cases are decoupled from base and moved to a +# separate executable to not crash the base test cases. +add_boost_test(types + SOURCES test-runner.cpp ${types_test_SOURCES} + LIBRARIES ${base_DEPS} + TESTS + types/gettype + types/assign + types/byname + types/instantiate + types/sort_by_load_after +) + set(base_test_SOURCES icingaapplication-fixture.cpp base-array.cpp @@ -21,7 +74,6 @@ set(base_test_SOURCES base-string.cpp base-timer.cpp base-tlsutility.cpp - base-type.cpp base-utility.cpp base-value.cpp config-apply.cpp @@ -117,10 +169,6 @@ add_boost_test(base base_tlsutility/iscertuptodate_ok base_tlsutility/iscertuptodate_expiring base_tlsutility/iscertuptodate_old - base_type/gettype - base_type/assign - base_type/byname - base_type/instantiate base_utility/parse_version base_utility/compare_version base_utility/comparepasswords_works diff --git a/test/base-type.cpp b/test/base-type.cpp index 21bcf439d35..4a8d0deb4b7 100644 --- a/test/base-type.cpp +++ b/test/base-type.cpp @@ -5,11 +5,13 @@ #include "base/objectlock.hpp" #include "base/application.hpp" #include "base/type.hpp" +#include "icinga/host.hpp" +#include "icinga/service.hpp" #include using namespace icinga; -BOOST_AUTO_TEST_SUITE(base_type) +BOOST_AUTO_TEST_SUITE(types) BOOST_AUTO_TEST_CASE(gettype) { @@ -44,4 +46,33 @@ BOOST_AUTO_TEST_CASE(instantiate) BOOST_CHECK(p); } +BOOST_AUTO_TEST_CASE(sort_by_load_after) +{ + int totalDependencies{0}; + std::unordered_set previousTypes; + for (auto type : Type::GetConfigTypesSortedByLoadDependencies()) { + BOOST_CHECK_EQUAL(true, ConfigObject::TypeInstance->IsAssignableFrom(type)); + + totalDependencies += type->GetLoadDependencies().size(); + for (Type* dependency : type->GetLoadDependencies()) { + // Note, Type::GetConfigTypesSortedByLoadDependencies() does not detect any cyclic load dependencies, + // instead, it relies on this test case to fail. + BOOST_CHECK_MESSAGE(previousTypes.find(dependency) != previousTypes.end(), "type '" << type->GetName() + << "' depends on '"<< dependency->GetName() << "' type, but it's not loaded before"); + } + + previousTypes.emplace(type.get()); + } + + // The magic number 12 is the sum of host,service,comment,downtime and endpoint load_after dependencies. + BOOST_CHECK_MESSAGE(totalDependencies >= 12, "total size of load dependency must be at least 12"); + BOOST_CHECK_MESSAGE(previousTypes.find(Host::TypeInstance.get()) != previousTypes.end(), "Host type should be in the list"); + BOOST_CHECK_MESSAGE(previousTypes.find(Service::TypeInstance.get()) != previousTypes.end(), "Service type should be in the list"); + BOOST_CHECK_MESSAGE(previousTypes.find(Downtime::TypeInstance.get()) != previousTypes.end(), "Downtime type should be in the list"); + BOOST_CHECK_MESSAGE(previousTypes.find(Comment::TypeInstance.get()) != previousTypes.end(), "Comment type should be in the list"); + BOOST_CHECK_MESSAGE(previousTypes.find(Notification::TypeInstance.get()) != previousTypes.end(), "Notification type should be in the list"); + BOOST_CHECK_MESSAGE(previousTypes.find(Zone::TypeInstance.get()) != previousTypes.end(), "Zone type should be in the list"); + BOOST_CHECK_MESSAGE(previousTypes.find(Endpoint::TypeInstance.get()) != previousTypes.end(), "Endpoint type should be in the list"); +} + BOOST_AUTO_TEST_SUITE_END()