IcingaDB: better config and state update queueing

[yhabteab]

This pull request introduces a new runtime changes queue to IcingaDB, along with several enhancements to the RedisConnection class. These changes aim to improve the memory footprint and number of duplicate (and thus superfluous) Redis queries. The problem of duplicate queries has been a long-standing issue in IcingaDB, and some hacky workarounds have been implemented in the past to mitigate it. This PR takes a more systematic approach as Julian described in #10186 to address the root cause. I will try to summarize the key changes below:

Changes Queue

A new changes queue has been introduced to IcingaDB, which allows for batching of all runtime updates for a given object in an efficient manner. The changes queue works as outlined below:

Before going into more detail, we should clarify what we mean by "changes". In this context, changes refer to any event
that requires a Redis write operation. This includes, but is not limited to:

• Object creation and deletion
• Object modification (e.g., updating attributes)
• Relationship updates (e.g., adding or removing links between objects)
• State related updates (e.g., state changes, acknowledgements, etc.)
• Dependency config and state updates

This new queue does not cover any history related writes, those types of events follow a different path and are not
affected by this change. The focus here is solely on runtime object changes that affect the normal non-historical operation of IcingaDB. Consequently, history and heartbeat related writes use their own dedicated Redis connection and do not interfere with any of the changes described here.

Now, here is how the changes queue operates:

When an object is modified, instead of immediately writing the changes to Redis, the object pointer is pushed onto the queue with a corresponding flag indicating the type of change required. As long as the object remains in the queue, any subsequent Redis write requests concerning that object are merged into the existing queued dirty bits. This means that no matter how many times e.g., a OnStateChange is triggered for a given object, only a single write operation will be performed when it is finally popped from the queue. Do note that an object can have multiple dirty bits set, so if both its attributes and state are modified while in the queue, a state and config update will be sent when it is processed.

The consumer of the changes queue is a new background worker that pops objects from the queue and performs the necessary Redis write operations. This worker doesn't immediately process objects as they are enqueued; instead, it waits for a short period (currently set to 1000ms) to allow for more changes to accumulate and be merged. After this wait period, the worker serializes the queued objects according to their dirty bits and sends the appropriate Redis commands. Though, there's also another restriction in place: when the used RedisConnection reaches a certain number of pending commands (currently set to 512), the worker won't dequeue any more objects from the changes queue until the pending commands drop below that threshold. This ensures that we don't unnecessarily waste memory by serializing too many objects in advance, if the Redis server isn't able to keep up.

To accommodate this new changes queue, quite a number of existing code has been refactored, so that we no longer perform immediate writes to Redis. Additionally, the RedisConnection class has been enhanced to support this new workflow.

RedisConnection Enhancements

Several enhancements have been made to the RedisConnection class to better support the changes queue and improve overall efficiency:

1. As a consequence of the changes queue, the Redis queries internal queue has significantly been simplified. As opposed to the previous version which used a complex data structure to correctly manage the query priorities, this version uses a std::deque for the write queue and a simple mechanism to insert high-priority items at the front. By default, items are processed in FIFO order, but if someone wants to immediately send a high-priority query it will be placed at the front of the queue (remember std::deque allows efficient insertion at both ends), and will overtake any normal priority items already queued. However, if there are already high-priority items in the queue, the new high-priority item will be inserted after them but still before any normal priority items, ensuring that all high-priority items are processed in the order they were enqueued.

2. And while I'm at it, I also took the chance to improve the WriteQueueItem type by replacing the previously used ridiculously verbose query types by a more compact std::variant based approach. This not only reduces memory usage but also makes clearer that each item represents exactly one of a defined set of query types and nothing else.

Now, IcingaDB is subscribed to the OnNextCheckChanged signal and not the dummy OnNextCheckUpdated signal anymore. Though, that dummy signal is still there since the IDO relies on it. The only behavioural change in IcingaDB as opposed to before is that the oldest pending Redis query is determined only on the primary Redis connection (the one used for history and heartbeats). If you guys think this is a problem, I can look into a way to have IcingaDB consider all connections when determining the oldest pending query.

resolves #10186

[jschmidt-icinga]

I didn't test extensively yet, but from a quick test everything seems to work as expected. I also can't speak much for the logic and performance implications of when to send which events to Redis since I barely touched that until now. I'll continue to look at this in the coming days and see if I can test this more thoroughly.

[jschmidt-icinga]

This introduces some tight bidirectional coupling between the IcingaDB (even though it's just the -ti file here) and RedisConnection objects. It would be nice if this could be avoided, though copying all the members doesn't seem elegant either.

[yhabteab]

I personally think, that this is 100% better than the previous version of this, and I don't see a problem with the bidirectional coupling either, since the RedisConnection class is meant to be used by the IcingaDB class only. Obviously, this is not that perfect either but if you have a better approach in mind, then feel free to suggest it.

[jschmidt-icinga]

I had already thought about it and couldn't come up with anything good to suggest, other than designing it that way from the start. One option would be to maybe make the members string_views, which would be fine here, since the connection object has a longer lifetime than the icingadb object, but might become an implementation detail when/if other classes want to use this class. Or just suck up the (small) memory cost and leave things as they were.

It's not the worst thing in the world either way. We have this kind of coupling in many places, like (JsonRpc|HttpServer)Connection<->ApiListener, but it is kind a ugly, design-wise.

Regarding RedisConnection being only used by IcingaDB: Recently I was briefly looking at caching Perfdata in Redis for persistence in case the target services go offline. It's always nice to at least keep the option open of using a class like that for something else in the future.

[yhabteab]

Or just suck up the (small) memory cost and leave things as they were.

It's not just about the memory cost but due to this ridiculously long parameter passing I would have to use. With master branch there are two places like this, and this PR add another one. So, decided simply squash them with approach.

icinga2/lib/icingadb/icingadb.cpp

Lines 84 to 86 in 35fdea8

	m_Rcon = new RedisConnection(GetHost(), GetPort(), GetPath(), GetUsername(), GetPassword(), GetDbIndex(),
	GetEnableTls(), GetInsecureNoverify(), GetCertPath(), GetKeyPath(), GetCaPath(), GetCrlPath(),
	GetTlsProtocolmin(), GetCipherList(), GetConnectTimeout(), GetDebugInfo());

icinga2/lib/icingadb/icingadb.cpp

Lines 94 to 96 in 35fdea8

	RedisConnection::Ptr con = new RedisConnection(GetHost(), GetPort(), GetPath(), GetUsername(), GetPassword(), GetDbIndex(),
	GetEnableTls(), GetInsecureNoverify(), GetCertPath(), GetKeyPath(), GetCaPath(), GetCrlPath(),
	GetTlsProtocolmin(), GetCipherList(), GetConnectTimeout(), GetDebugInfo(), m_Rcon);

Recently I was briefly looking at caching Perfdata in Redis for persistence in case the target services go offline. It's always nice to at least keep the option open of using a class like that for something else in the future.

If we ever end up using RedisConnection for other purposes, then we would definitely have to move it somewhere else. And while doing that, there will be other design decisions to make, I would consider this tiny bit of coupling acceptable for now that can further be improved later.

[yhabteab]

Please the updated code now. I've introduced a helper struct for all the parameters instead and will be copied only once.

[yhabteab]

I'll continue to look at this in the coming days and see if I can test this more thoroughly.

At the moment the integration tests from the Icinga DB repository are the only way to stress test this PR thoroughly. I've been running them ever since the initial implementation and I have almost gone crazy due to a subtle race condition that only showed up when running those tests.

[jschmidt-icinga]

a subtle race condition that only showed up when running those tests.

Can you describe this in a bit more detail so I know what to look out for, i.e. the symptoms of the race condition?

[yhabteab]

Can you describe this in a bit more detail so I know what to look out for, i.e. the symptoms of the race condition?

Well, the obvious symptom is that the integration tests (specifically the Redundancy Group ones) will sporadically fail because either Icinga 2 didn't sent a delete command when it's supposed to, or deleted something that it shouldn't have. Generally speaking, if the tests don't succeed then there is a bug in here.

[julianbrost]

Don't consider this a full review, just what I noticed at first glance.

[yhabteab]

Rebased + cherry-picked commit dc9d40f + addressed some of the still open discusstions.

[yhabteab]

Just fixed a minor style issue and a doc comment. See the diff by clicking ont he compare button.

[julianbrost]

While testing, I noticed that there isn't really a way to observe how the new worker thread is performing (except seeing whether things eventually show up in Redis/Icinga Web).

Show a crude patch that adds what I wanted to know right now to IcingadbCheckTask

diff --git a/lib/icingadb/icingadb.hpp b/lib/icingadb/icingadb.hpp
index ebbaebb50..ddc87cfd6 100644
--- a/lib/icingadb/icingadb.hpp
+++ b/lib/icingadb/icingadb.hpp
@@ -484,6 +484,20 @@ private:
        void EnqueueDependencyChildRegistered(const DependencyGroup::Ptr& depGroup, const Checkable::Ptr& child);
        void EnqueueDependencyChildRemoved(const DependencyGroup::Ptr& depGroup, const std::vector<Dependency::Ptr>& dependencies, bool removeGroup);
        void EnqueueRelationsDeletion(const String& id, icingadb::task_queue::RelationsDeletionItem::RelationsKeySet relations);
+
+public:
+       std::pair<std::size_t, std::chrono::steady_clock::duration> GetPendingItemsStats()
+       {
+               std::unique_lock lock(m_PendingItemsMutex);
+
+               auto& seqView = m_PendingItems.get<1>();
+               std::chrono::steady_clock::duration maxAge{};
+               if (!seqView.empty()) {
+                       maxAge = std::chrono::steady_clock::now() - seqView.front().EnqueueTime;
+               }
+
+               return {m_PendingItems.size(), maxAge};
+       }
 };
 }
 
diff --git a/lib/icingadb/icingadbchecktask.cpp b/lib/icingadb/icingadbchecktask.cpp
index f65583116..9f21d7f15 100644
--- a/lib/icingadb/icingadbchecktask.cpp
+++ b/lib/icingadb/icingadbchecktask.cpp
@@ -183,6 +183,10 @@ void IcingadbCheckTask::ScriptFunc(const Checkable::Ptr& checkable, const CheckR
        std::ostringstream i2okmsgs, idbokmsgs, warnmsgs, critmsgs;
        Array::Ptr perfdata = new Array();
 
+       auto stats = conn->GetPendingItemsStats();
+       perfdata->Add(new PerfdataValue("icinga2_pending_items_count", double(stats.first)));
+       perfdata->Add(new PerfdataValue("icinga2_pending_items_age", std::chrono::duration_cast<std::chrono::duration<double>>(stats.second).count()));
+
        i2okmsgs << std::fixed << std::setprecision(3);
        idbokmsgs << std::fixed << std::setprecision(3);
        warnmsgs << std::fixed << std::setprecision(3);

That obviously needs a lot of cleanup, but should be enough what I need for testing right now. Nonetheless, it would be nice if there were some metrics that allow to verify the proper operation.