Merge pull request #10148 from Icinga/enhanced-sort-types-by-load-dependencies

Sort config types by their load dependencies once

lib/base/initialize.hpp

@@ -23,6 +23,7 @@ enum class InitializePriority {
 	RegisterBuiltinTypes,
 	RegisterFunctions,
 	RegisterTypes,
+	SortTypes,
 	EvaluateConfigFragments,
 	Default,
 	FreezeNamespaces,

lib/base/type.cpp

@@ -1,9 +1,13 @@
 /* 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 <functional>
 
 using namespace icinga;
 
@@ -32,6 +36,43 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() {
 	Type::Register(type);
 }, InitializePriority::RegisterTypeType);
 
+static std::vector<Type::Ptr> l_SortedByLoadDependencies;
+static Atomic l_SortingByLoadDependenciesDone (false);
+
+INITIALIZE_ONCE_WITH_PRIORITY([] {
+	std::unordered_set<Type*> visited;
+
+	std::function<void(Type*)> 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);
+
+		for (auto dependency : type->GetLoadDependencies()) {
+			visit(dependency);
+		}
+
+		// 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);
+	});
+
+	// 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());
+		}
+	}
+
+	l_SortingByLoadDependenciesDone.store(true);
+}, InitializePriority::SortTypes);
+
 String Type::ToString() const
 {
 	return "type '" + GetName() + "'";
@@ -72,6 +113,12 @@ std::vector<Type::Ptr> Type::GetAllTypes()
 	return types;
 }
 
+const std::vector<Type::Ptr>& Type::GetConfigTypesSortedByLoadDependencies()
+{
+	VERIFY(l_SortingByLoadDependenciesDone.load());
+	return l_SortedByLoadDependencies;
+}
+
 String Type::GetPluralName() const
 {
 	String name = GetName();

lib/base/type.hpp

@@ -83,6 +83,21 @@ public:
 	static Type::Ptr GetByName(const String& name);
 	static std::vector<Type::Ptr> 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<Type::Ptr>
+	 */
+	static const std::vector<Ptr>& GetConfigTypesSortedByLoadDependencies();
+
 	void SetField(int id, const Value& value, bool suppress_events = false, const Value& cookie = Empty) override;
 	Value GetField(int id) const override;
 

lib/config/configitem.cpp

@@ -444,33 +444,9 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue
 		<< "Committing " << total << " new items.";
 #endif /* I2_DEBUG */
 
-	std::set<Type::Ptr> types;
-	std::set<Type::Ptr> completed_types;
 	int itemsCount {0};
 
-	for (const Type::Ptr& type : Type::GetAllTypes()) {
+	for (auto& type : Type::GetConfigTypesSortedByLoadDependencies()) {
-		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;
-
-			bool unresolved_dep = false;
-
-			/* 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 (unresolved_dep)
-				continue;
-
 		std::atomic<int> committed_items(0);
 
 		{
@@ -501,8 +477,6 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue
 
 		itemsCount += committed_items;
 
-			completed_types.insert(type);
-
 #ifdef I2_DEBUG
 		if (committed_items > 0)
 			Log(LogDebug, "configitem")
@@ -512,33 +486,13 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue
 		if (upq.HasExceptions())
 			return false;
 	}
-	}
 
 #ifdef I2_DEBUG
 	Log(LogDebug, "configitem")
 		<< "Committed " << itemsCount << " items.";
 #endif /* I2_DEBUG */
 
-	completed_types.clear();
+	for (auto& type : Type::GetConfigTypesSortedByLoadDependencies()) {
-
-	while (types.size() != completed_types.size()) {
-		for (const Type::Ptr& type : types) {
-			if (completed_types.find(type) != completed_types.end())
-				continue;
-
-			bool unresolved_dep = false;
-
-			/* 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 (unresolved_dep)
-				continue;
-
 		std::atomic<int> notified_items(0);
 
 		{
@@ -574,8 +528,6 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue
 			}
 		}
 
-			completed_types.insert(type);
-
 #ifdef I2_DEBUG
 		if (notified_items > 0)
 			Log(LogDebug, "configitem")
@@ -618,7 +570,6 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue
 		if (!CommitNewItems(context, upq, newItems))
 			return false;
 	}
-	}
 
 	return true;
 }

test/CMakeLists.txt

@@ -2,6 +2,59 @@
 
 include(BoostTestTargets)
 
+set(types_test_SOURCES
+  icingaapplication-fixture.cpp
+  base-type.cpp
+  ${base_OBJS}
+  $<TARGET_OBJECTS:config>
+  $<TARGET_OBJECTS:remote>
+  $<TARGET_OBJECTS:icinga>
+  $<TARGET_OBJECTS:methods>
+)
+
+if(ICINGA2_WITH_CHECKER)
+  list(APPEND types_test_SOURCES $<TARGET_OBJECTS:checker>)
+endif()
+
+if(ICINGA2_WITH_MYSQL)
+  list(APPEND types_test_SOURCES $<TARGET_OBJECTS:db_ido> $<TARGET_OBJECTS:db_ido_mysql>)
+endif()
+
+if(ICINGA2_WITH_PGSQL)
+  list(APPEND types_test_SOURCES $<TARGET_OBJECTS:db_ido> $<TARGET_OBJECTS:db_ido_pgsql>)
+endif()
+
+if(ICINGA2_WITH_ICINGADB)
+  list(APPEND types_test_SOURCES $<TARGET_OBJECTS:icingadb>)
+endif()
+
+if(ICINGA2_WITH_NOTIFICATION)
+  list(APPEND types_test_SOURCES $<TARGET_OBJECTS:notification>)
+endif()
+
+if(ICINGA2_WITH_PERFDATA)
+  list(APPEND types_test_SOURCES $<TARGET_OBJECTS:perfdata>)
+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

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 <BoostTestTargetConfig.h>
 
 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<Type*> 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()