IcingaDB: Make Redis & DB values consistent #10452
Conversation
lib/icingadb/icingadb-utility.cpp
Outdated
| /** | ||
| * Converts the given filter to its Redis value representation. | ||
| * | ||
| * Within the Icinga 2 code base, if the states and types filter bitsets are set to -1, the filter will match | ||
| * on all states/types. However, since sending -1 to Redis would crash the Icinga DB daemon, as both fields are | ||
| * of type Go's uint8/uint16, so the primary purpose of this function is to make sure that no negative values are | ||
| * passed to Redis. | ||
| * | ||
| * @param filter The filter to convert. | ||
| * @param isStatesFilter Whether the given filter is a states filter or a types filter. | ||
| */ | ||
| int IcingaDB::StatesOrTypesFilterToRedisValue(int filter, bool isStatesFilter) | ||
| { | ||
| if (filter >= 0) { | ||
| return filter; | ||
| } | ||
|
|
||
| if (isStatesFilter) { | ||
| return StateFilterOK | StateFilterWarning | StateFilterCritical | StateFilterUnknown | StateFilterUp | StateFilterDown; | ||
| } | ||
|
|
||
| return NotificationDowntimeStart | NotificationDowntimeEnd | NotificationDowntimeRemoved | NotificationCustom | | ||
| NotificationAcknowledgement | NotificationProblem | NotificationRecovery | NotificationFlappingStart | NotificationFlappingEnd; | ||
| } |
There was a problem hiding this comment.
I find this function odd for two reasons:
- It repeats all the
StateFilter*andNotification*values. - The "what does it actually do" bool argument makes it's uses somewhat awkward:
icinga2/lib/icingadb/icingadb-objects.cpp
Lines 1627 to 1628 in 98dee18
Those would be more readable if this was just two different functions:attributes->Set("states", StateFilterToRedisValue(user->GetStateFilter())); attributes->Set("types", TypeFilterToRedisValue(user->GetTypeFilter()));
Where does that -1 actually come from? That's what you get from using ~0 (i.e. all bits set to 1) as an integer as it's used here for example:
Lines 19 to 20 in c253e7e
Given that FilterArrayToInt() already receives all valid values in filterMap, it could also restrict its return value to an integer with only the bits set that are actually used.
There was a problem hiding this comment.
Given that
FilterArrayToInt()already receives all valid values infilterMap, it could also restrict its return value to an integer with only the bits set that are actually used.
That would break this validation! Yes, it's weird, but the -1 value has currently two different interpretations.
Lines 87 to 90 in c253e7e
There was a problem hiding this comment.
Or were you referring with your comment to its callers to provide all bits of the desired filters instead of ~0 and leave the implementation of FilterArrayToInt() unchanged?
There was a problem hiding this comment.
Oh wow, that's a great example why C-style error reporting sucks.
I was thinking of changing FilterArrayToInt() but didn't look into it for long enough that it also returns -1 to report errors. However, that shouldn't be a problem for what I had in mind: in the if (!typeFilters), just unset all bits from defaultValue that are not set in any filterMap value.
Untested diff of what I had in mind
diff --git a/lib/icinga/customvarobject.cpp b/lib/icinga/customvarobject.cpp
index 0edc369d9..d50b8f49d 100644
--- a/lib/icinga/customvarobject.cpp
+++ b/lib/icinga/customvarobject.cpp
@@ -19,12 +19,16 @@ void CustomVarObject::ValidateVars(const Lazy<Dictionary::Ptr>& lvalue, const Va
int icinga::FilterArrayToInt(const Array::Ptr& typeFilters, const std::map<String, int>& filterMap, int defaultValue)
{
- int resultTypeFilter;
+ int resultTypeFilter = 0;
- if (!typeFilters)
- return defaultValue;
-
- resultTypeFilter = 0;
+ if (!typeFilters) {
+ for (const auto & [name, value] : filterMap) {
+ if (defaultValue & value) {
+ resultTypeFilter |= value;
+ }
+ }
+ return resultTypeFilter;
+ }
ObjectLock olock(typeFilters);
for (const Value& typeFilter : typeFilters) {When additionally performing some sanitization in if (typeFilter.IsNumber()), that function would then return -1 to report errors (that error reporting mechanism could be improved though) or a non-negative value where only valid bits are set (valid bits in this case is defined as all bits that are set in any filterMap value).
Given that FilterArrayToInt() seems to be called only with defaultValue = 0 or defaultValue = ~0, there could be room for some simplification by just giving a boolean with the default for all valid bits.
There was a problem hiding this comment.
Do you really want me to refactor that as part of this PR? There're a bunch of consumers that relies on its current behaviour and changing this now, will also require modifying every and each of its consumers. This will just blow up the PR and of course delay it from being done unnecessarily.
icinga2/lib/db_ido/dbconnection.cpp
Lines 535 to 541 in c253e7e
What does this validation even do? Since the desired filters contain DbCatEverything which is set to ~0 => -1, and combining all of them with the bitwise | operator will yield nothing but -1 and flipping the result (-1) with ~ makes it 0 and everything bitwise anded (filter & 0) with 0 will always yield 0. So, I don't know what this validation is used for.
There was a problem hiding this comment.
$ python3
Python 3.13.3 (main, Apr 8 2025, 13:54:08) [Clang 17.0.0 (clang-1700.0.13.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> all = 0 | ~0 | 1 | 2 | 4 | 8 | 16 | 32 | 64 | 128 | 256 | 512 | 1024 | 2048 | 4096 | 8192
>>> all
-1
>>> ~all
0
>>>There was a problem hiding this comment.
2. Those would be more readable if this was just two different functions:
attributes->Set("states", StateFilterToRedisValue(user->GetStateFilter())); attributes->Set("types", TypeFilterToRedisValue(user->GetTypeFilter()));
I can just split the existing Icinga DB mapping function into two as suggested here, and everything else remains unchanged (broken).
There was a problem hiding this comment.
include <itl>
object IdoPgsqlConnection "ido-mysql" {
categories = [2<<16, 2<<17, 2<<18]
}
---
$ icinga2 daemon -c icinga2.conf -C
[2025-05-30 13:24:54 +0200] information/cli: Icinga application loader (version: v2.14.0-677-g9bdbf8823; debug)
[2025-05-30 13:24:54 +0200] information/cli: Loading configuration file(s).
[2025-05-30 13:24:55 +0200] information/ConfigItem: Committing config item(s).
[2025-05-30 13:24:55 +0200] information/ConfigItem: Instantiated 1 Host.
[2025-05-30 13:24:55 +0200] information/ConfigItem: Instantiated 9 CheckCommands.
[2025-05-30 13:24:55 +0200] information/ConfigItem: Instantiated 1 Downtime.
[2025-05-30 13:24:55 +0200] information/ConfigItem: Instantiated 1 IcingaApplication.
[2025-05-30 13:24:55 +0200] information/ConfigItem: Instantiated 1 IdoPgsqlConnection.
[2025-05-30 13:24:55 +0200] information/cli: Finished validating the configuration file(s).There was a problem hiding this comment.
There're a bunch of consumers that relies on its current behaviour
What behavior exactly? Or do you have an example of what would break?
What does this validation even do?
Nothing useful, it's basically dead code at the moment. I believe someone just copied all enum values in there without considering that some of them are special. I don't think DbCatInvalid should actually be in there either.
$ python3 [...]
FYI: you could have verified this more directly just using a static_assert() in the C++ code.
I can just split the existing Icinga DB mapping function into two as suggested here, and everything else remains unchanged
That's also an option, in that case, I'd consider adding a constant similar to DbCatEverything, however defined as only the valid bits, not ~0 to avoid repeating the list of enum values in another place (it's unlikely that we'll add new values, but I think it's still nicer having it in a single place).
lib/icinga/notification.cpp
Outdated
| // Since the types and states attributes are user configurable and allowed to change at runtime, we need to | ||
| // hook into the OnTypesChanged and OnStatesChanged signals to update the actual filter bitsets whenever these | ||
| // attributes change. Otherwise, the filter bitsets would be stale and not reflect their current state. | ||
| OnTypesChanged.connect([](const Notification::Ptr& notification, const MessageOrigin::Ptr&) { | ||
| notification->SetTypeFilter(FilterArrayToInt(notification->GetTypes(), GetTypeFilterMap(), ~0)); | ||
| }); | ||
| OnStatesChanged.connect([](const Notification::Ptr& notification, const MessageOrigin::Ptr&) { | ||
| notification->SetStateFilter(FilterArrayToInt(notification->GetStates(), GetStateFilterMap(), ~0)); | ||
| }); |
There was a problem hiding this comment.
Can't this simply be done as part of the SetTypes() and SetStates() implementation? I don't see why this should need the indirection through these signals.
There was a problem hiding this comment.
Can't this simply be done as part of the
SetTypes()andSetStates()implementation?
Of course, it can be implemented that way but simply isn't the word that would describe it. I just found it easier to use their existing signals instead of having to mess around with the class compiler (making the setters virtual, so that they can be overridden in the User and Notification class accordingly.
There was a problem hiding this comment.
mess around with the class compiler
As in using it or changing something within its implementation?
There was a problem hiding this comment.
As in using it or changing something within its implementation?
It doesn't matter now, already changed it and marked the two attributes as set_virtual in the dubious {user,notfication}.ti files.
There was a problem hiding this comment.
As in using it or changing something within its implementation?
It doesn't matter now, already changed it and marked the two attributes as
set_virtualin the dubious{user,notfication}.tifiles.
Apparently, this introduced a new problem - the SetTypes() and SetStates() methods are called from within the e.g. ObjectImpl<Notification>::ObjectImpl() constructor, but since they're virtual methods, I've overridden them in the Notification class. However, the invokations from within the constructor are resolved at a compile time instead of the dynamic dispatch at runtime 1, and obviously my overrides get never called on the Notification object.
Footnotes
So that Icinga DB (Go) daemon doesn't have to make the mappings again.
…trs change Since the types and states attributes are user configurable and allowed to change at runtime, we need to hook into the `OnTypesChanged` and `OnStatesChanged` signals to update the actual filter bitsets whenever these attributes change. Otherwise, the filter bitsets would be stale and not reflect their current state.
yhabteab
force-pushed
the
streamline-redis-and-db-values
branch
from
98dee18 to
4ffd3e8
Compare
This tries to make some of the values written to Redis and to the Database more consistent. Specifically, previously Icinga DB sent 0 or 1 for the
state_typefield, and the Icinga DB (Go) daemon mapped them tosoftandhardaccordingly before writing them to the database. This is not the case anymore, and Icinga DB (C++) now writessoftandharddirectly to Redis. This also applies to thecomment#entry_type,{user, notification}#types,statesandnotification#typefields. They all are now serialized exactly how they're going to be written to the database, thus eliminating the need for the Go daemon to do any mappings. Apart from that, theis_acknowledgedfield is now serialized astrueorfalseas opposed to1(normal) or2(sticky) previously. From now on that field will be set totrueif the checkable has been acknowledged, and the newis_sticky_acknowledgementfield will be set totrueif the acknowledgement is sticky.One additional bug that's been fixed by this PR but isn't related to the above topic is that the
{user, notification}#types,statesbitsets will correctly be refreshed whenever the user or notificationtypes,statesattributes are changed at runtime. Previously, the bitsets were only updated at startup, and never refreshed again (see 98dee18).Required By
fixes #9427