Discard likely duplicate problem notifications via Notification#last_notified_state_per_user

[Al2Klimov]

fixes #4503

Commits

1. Whenever a state notification is sent: track user.name=>checkable.state in Notification#last_notified_state_by_user
2. On Notification#last_notified_state_by_user change: sync that over cluster
3. Among the other checks like is-acked, matches-filter, etc. in Notification#BeginExecuteNotification(): in case of the same non-reminder problem already notified before drop the eff. dupl. notification
4. On recovery: clear that state

Test

Tested with https://github.com/Al2Klimov/twintowers and following config in volumes/icinga2/master1/etc/icinga2/zones.d/master/my.conf

 for (i in range(100)) {
    object Host i {
        check_command = "dummy"
        enable_active_checks = false
        #check_interval = "dummy"
        #vars.dummy_state = {{ Math.random() < 0.01 ? 2 : 0 }}
        #vars.dummy_text = "-"
    }
}

 for (i in range(10)) {
apply Service i {
        check_command = "dummy"
        enable_active_checks = false
    assign where true
}
}

object User "me" {
    states = [ OK, Critical, Unknown ]
}

object NotificationCommand "noop" {
    command = [ "true" ]
}

apply Notification "test" to Host {
    command = "noop"
    users = [ "me" ]
    assign where true
}

apply Notification "test" to Service {
    command = "noop"
    users = [ "me" ]
    assign where true
}

• 👍 Initial (passive) warning filtered out as configured
• 👍 Initial critical notified as configured
• 👍 Follow-up warning filtered out as configured
• 👍 Follow-up critical filtered out due to this change
• 👍 Happened while one master was down on 10/10 services, so HA works

[yhabteab]

Apart from the comments below, please also document the new cluster events here.

[yhabteab]

It might also be worth adding this test here:

BOOST_CHECK(helper.n->GetLastNotifiedStatePerUser()->Get(helper.u->GetName()) == Empty);

[Al2Klimov]

Redundant IMAO. It's only the bridge to (not) sending a notification, the latter is important and tested here.

[yhabteab]

Can you please also deduplicate the remaining test cases.

[julianbrost]

The attributes and signals are named inconsistently ("per" vs. "of"). For what it's worth, my preference would be unifying towards "per".

[Al2Klimov]

Guilty. To my defence: The whole thing contains all states, separately per user. But sometimes I want to update only the value of a particular user.

[julianbrost]

But wouldn't it make so much more sense if everything that's specifically related to syncing that attribute was also named like that attribute and not just somewhat similar? Actually, this affects OnLastNotifiedStatesCleared as well.

[yhabteab]

Warning ⚠️:

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 1, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:17:21 +0100] notice/Notification: Attempting to send notifications of type 'Problem' for notification object 'icinga2!test!notify'.
[2023-12-12 16:17:21 +0100] notice/Notification: Not sending notifications for notification object 'icinga2!test!notify and user 'yhabteab': state 'Warning' does not match state filter: Critical, OK and Unknown.

Critical:

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 2, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:18:35 +0100] notice/Notification: Attempting to send notifications of type 'Problem' for notification object 'icinga2!test!notify'.
[2023-12-12 16:18:35 +0100] information/Notification: Sending 'Problem' notification 'icinga2!test!notify' for user 'yhabteab'
[2023-12-12 16:18:35 +0100] information/Notification: Completed sending 'Problem' notification 'icinga2!test!notify' for checkable 'icinga2!test' and user 'yhabteab' using command 'noop'.

Critical -> Warning -> Critical:

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 1, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 2, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:21:48 +0100] notice/Notification: Attempting to send notifications of type 'Problem' for notification object 'icinga2!test!notify'.
[2023-12-12 16:21:48 +0100] notice/Notification: Notification object 'icinga2!test!notify': We already notified user 'yhabteab' for a Critical problem. Likely after that another state change notification was filtered out by config. Not sending duplicate 'Critical' notification.

Recovery -> Critical:

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 0, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:24:00 +0100] information/Notification: Completed sending 'Recovery' notification 'icinga2!test!notify' for checkable 'icinga2!test' and user 'yhabteab' using command 'noop'.


# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 2, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:24:20 +0100] information/Notification: Completed sending 'Problem' notification 'icinga2!test!notify' for checkable 'icinga2!test' and user 'yhabteab' using command 'noop'.

[julianbrost]

What's the purpose of this cast? ConfigObject::OnAllConfigLoaded() is virtual, so shouldn't this just call n->OnAllConfigLoaded() anyways?

And if a cast is actually necessary, we have static_pointer_cast/dynamic_pointer_cast.

[yhabteab]

Config::OnAllConfigLoaded() is virtual, so shouldn't this just call n->OnAllConfigLoaded() anyways?

I suppose it's because Notification::OnAllConfigLoaded() isn't publicly accessible.

[Al2Klimov]

And if a cast is actually necessary, we have static_pointer_cast/dynamic_pointer_cast.

I don't get the point of these. I'm casting to base, not to derived.

[julianbrost]

Is there any good reason why Notification::OnAllConfigLoaded() shouldn't be public if it implements a public virtual method of a base class? So unless there's a good reason, I'd just change the visibility and avoid strange casts altogether.

[Al2Klimov]

Absolutely! IMAO always the overriding method of the most derived class shall be called from external, none of the base methods. Protecting all ex. the base virtual one enforces this. OK, admittedly Notification is the most derived class. But if this changes for Notification (or any other class) in the future, one had to remember these rules and make all methods protected the new derived class overrides. To make some already public methods protected!

[julianbrost]

Either I'm getting you wrong or what you're describing can't happen.

https://en.cppreference.com/w/cpp/language/virtual#In_detail:

If some member function vf is declared as virtual in a class Base, and some class Derived, which is derived, directly or indirectly, from Base, has a declaration for member function with the same [...]
Then this function in the class Derived is also virtual (whether or not the keyword virtual is used in its declaration) and overrides Base::vf (whether or not the word override is used in its declaration).

So Notification::OnAllConfigLoaded() is virtual automatically and if there was another derived class overriding it, it would still be used.

[julianbrost]

I'd also rename this to OnLastNotifiedStatePerUserCleared. Why? Because a simple git grep LastNotifiedStatePerUser will then lead you to all the places that affect this attribute.