Fix broken downtime comment sync

[yhabteab]

All objects must be synced sorted by their load dependency. Otherwise, downtimes and/or comments might get synced before their respective Checkables, which will result in comments and downtimes being ignored by the other endpoint since it does not yet know about their checkables. Given that the runtime config updates event does not trigger a reload on the remote endpoint, these objects won't be synced again until the next reload.

~/master2/icinga2 (bundled-cluster-fixes ✗) ls prefix/var/lib/icinga2/api/packages/_api/*/conf.d/downtimes | wc -l
    3501

~/master2/icinga2 (bundled-cluster-fixes ✗) curl -sSku root:icinga 'https://localhost:5666/v1/objects/downtimes?pretty=1' | grep ' "attrs": {' | wc -l
    1501
~/master1/icinga2 (bundled-cluster-fixes ✗) curl -sSku root:icinga 'https://localhost:5665/v1/objects/downtimes?pretty=1' | grep ' "attrs": {' | wc -l
    3501

After master2 reload:

~/master2/icinga2 (bundled-cluster-fixes ✗) curl -sSku root:icinga 'https://localhost:5666/v1/objects/downtimes?pretty=1' | grep ' "attrs": {' | wc -l
    3501

closes #7786
closes #9873

TODO

• Sort config types by their load dependencies once #10148
• rebase

[log1-c]

Quick question: would this also fix the following issue:
Downtimes scheduled via API (sometimes) get synced/re-created with delay and are doubled #10078

[Mordecaine]

Quick question: would this also fix the following issue: Downtimes scheduled via API (sometimes) get synced/re-created with delay and are doubled #10078

It would be very helpful to get an answer to this.

[yhabteab]

Quick question: would this also fix the following issue: Downtimes scheduled via API (sometimes) get synced/re-created with delay and are doubled #10078

It would be very helpful to get an answer to this.

Hi, we don't know for sure whether this will fix #10078 as we still haven't identified exactly what is going wrong there, other than something is not working as expected. It's unlikely that this PR will fix #10078, but we can't tell you for sure until the cause for #10078 is identified.

[dgiesselbach]

@yhabteab When will this request be completed? Is there a timeline?

[julianbrost]

I believe similar problems will still exist for other types where there's no load_after dependency at the moment: many objects can refer to a time period, however, there's not a single load_after TimePeriod. There are more such examples: Host/Service -> *Command, Notification -> NotificationCommand (not necessarily a complete list).

[yhabteab]

A complete list of the navigable aka navigation types (attributes):

Another list of non-navigable dependencies :):

~/Workspace/icinga2 (broken-downtime-comment-sync ✗) grep -rE 'array\(name.*' lib     
lib/icinga/host.ti:     [config, no_user_modify, required, signal_with_old_value] array(name(HostGroup)) groups {
lib/icinga/notification.ti:     [config, signal_with_old_value] array(name(User)) users (UsersRaw);
lib/icinga/notification.ti:     [config, signal_with_old_value] array(name(UserGroup)) user_groups (UserGroupsRaw);
lib/icinga/servicegroup.ti:     [config, no_user_modify] array(name(ServiceGroup)) groups;
lib/icinga/hostgroup.ti:        [config, no_user_modify] array(name(HostGroup)) groups;
lib/icinga/usergroup.ti:        [config, no_user_modify] array(name(UserGroup)) groups;
lib/icinga/service.ti:  [config, no_user_modify, required, signal_with_old_value] array(name(ServiceGroup)) groups {
lib/icinga/user.ti:     [config, no_user_modify, required, signal_with_old_value] array(name(UserGroup)) groups {
lib/icinga/timeperiod.ti:       [config, required, signal_with_old_value] array(name(TimePeriod)) excludes {
lib/icinga/timeperiod.ti:       [config, required, signal_with_old_value] array(name(TimePeriod)) includes {
lib/remote/zone.ti:     [config] array(name(Endpoint)) endpoints (EndpointsRaw);

[julianbrost]

While cross-checking¹ the dependencies added by this PR, I both noticed another problem as well a different idea to fix this in a hopefully more reliable way.

The problem: There are object types that allow referencing other types of the same type. The first example of this I found is HostGroup: it has a groups attribute where other host groups can be referenced to recursively define groups:

icinga2/lib/icinga/hostgroup.ti

Line 22 in 5e9e0bb

[config, no_user_modify] array(name(HostGroup)) groups;

Something very similar also exists for TimePeriod with the include/exclude attributes:

icinga2/lib/icinga/timeperiod.ti

Lines 27 to 32 in 5e9e0bb

	[config, required, signal_with_old_value] array(name(TimePeriod)) excludes {
	default {{{ return new Array(); }}}
	};
	[config, required, signal_with_old_value] array(name(TimePeriod)) includes {
	default {{{ return new Array(); }}}
	};

So no matter how we reorder the types, the issue that the order of individual objects within a type can also make a difference, i.e. the included/excluded time periods have to be sent, otherwise the same problem can happen there as well (and then continue to affect other objects that reference these time periods like hosts and services).

However, on a positive note, it looks like there's actually an easy solution to this: Icinga 2 already tracks these dependencies between individual config objects on a per-object basis (https://github.com/Icinga/icinga2/blob/master/lib/base/dependencygraph.hpp, https://github.com/Icinga/icinga2/blob/master/lib/base/dependencygraph.cpp). Thus, we should be able to solve both the problem that was the reason for starting this PR as well as the other one I just mentioned by sending the config objects in a topological sort order in respect to DependencyGraph (that should actually remove the requirement to iterate the types in a particular order at all).

Footnotes

1. These two commands were my friends (just so that they are not only forgotten in my shell history): git grep -E 'name\([A-Za-z]+\)' -- '*.ti' and git grep load_after -- '*.ti' ↩

[julianbrost]

For completeness, I already wrote the following inline comment before figuring out #10000 (comment). I'm submitting this for completeness so that it doesn't get forgotten as a pending comment, but there's a good chance it will become obsolete.

[yhabteab]

Thus, we should be able to solve both the problem that was the reason for starting this PR as well as the other one I just mentioned by sending the config objects in a topological sort order in respect to DependencyGraph

No, it won't!

I actually did notice this, but didn't want to go into it too much since it's a relationship on a per object basis and not on the actual types themselves. Icinga 2 happily accepts such a config, and none of the proposed alternatives is going to overcome it.

object HostGroup "foo" {
    groups = [ "bar" ]
}

object HostGroup "bar" {
    groups = [ "foo" ]
}

{
    "attrs": {
        "__name": "bar",
        "display_name": "bar",
        "groups": [
            "foo"
        ],
        "name": "bar",
        "type": "HostGroup",
    },
    "name": "bar",
    "type": "HostGroup"
},
{
    "attrs": {
        "__name": "foo",
        "display_name": "foo",
        "groups": [
          "bar"
        ],
        "name": "foo",
        "type": "HostGroup",
    },
    "name": "foo",
    "type": "HostGroup"
}

[yhabteab]

And this config!

object TimePeriod "included" {
  excludes = ["excluded"]
  ranges = { "2024-10-14" = "14:00-15:00" }
}

object TimePeriod "excluded" {
  excludes = ["included"]
  ranges = { "2024-10-14" = "13:00-15:00" }
}

There is absolutely nothing in the code that prevents two objects from becoming dependent on each other.

[julianbrost]

Thus, we should be able to solve both the problem that was the reason for starting this PR as well as the other one I just mentioned by sending the config objects in a topological sort order in respect to DependencyGraph

No, it won't!

Indeed, it would not make every (currently) possible situation work, but at least for all objects where it's possible.

object HostGroup "foo" {
    groups = [ "bar" ]
}

object HostGroup "bar" {
    groups = [ "foo" ]
}

object TimePeriod "included" {
  excludes = ["excluded"]
  ranges = { "2024-10-14" = "14:00-15:00" }
}

object TimePeriod "excluded" {
  excludes = ["included"]
  ranges = { "2024-10-14" = "13:00-15:00" }
}

With these objects, it's simply impossible to order them such that each object is synced before all of its dependencies. And such config being accepted looks like a bug to me.

Note that this ordering is only relevant for runtime-created objects, so defining something like this in a config file wouldn't even be a problem here. However, you can also create the dependency cycle by modifying attributes at runtime (if allows, for nested host/service groups it isn't, the groups attribute is actually marked no_user_modify, but with the time period example, it's actually possible at the moment).