IcingaDB: send missing state runtime update info & extend acknowledge-problem API action - #11035

Please have a look! Without this, we can't make progress in Icinga DB Icinga/icingadb#1112.

backport-to-support/2.15

Why that though? If Icinga Notifications requires some minimal version of Icinga 2 anyway, why can't/shouldn't this just be ≥ v2.16.x (for some x)? This doesn't sound like a security/critical/important fix to me (which should usually be the rule of thumb for the older support branch).

Why that though? If Icinga Notifications requires some minimal version of Icinga 2 anyway, why can't/shouldn't this just be ≥ v2.16.x (for some x)?

Because Icinga DB v1.4.0 already bumped the required version to v2.15 due to the dependencies support and we don't want to force users to v2.16.x just for this.

This doesn't sound like a security/critical/important fix to me

It is important! After all, the event correlation will be one of the major features available with Icinga Notifications v1.0.

Good that you can't set your ack in the future, but now you can set it in the past. That's not dramatic, but avoidable if the set_time is an Icinga API output, not an input. TBD.

I've checked this and we can't do that as that would require changing the response format.

Can you go into a little more detail on that? Please give me a specific example how it will be used (as someone who isn't as well versed in the Go or PHP sides of things).

Quote from #11006 (comment):

Also, with Icinga/icinga-notifications-web#556 Icinga Notifications/DB Web will hijack the ACK set and clear API actions and will enqueue its own ACK set and clear events into Icinga Notifications job queue. So, in order to allow it to re-construct the ACK set history event IDs (even though I don't know yet how that will work, since there's no an equivalent implementation of Icinga 2's ObjectPacker in PHP), the acknowledge-problem API action will need to allow to provide a custom timestamp for the ACK set change time.

EDIT: See also Icinga/icingadb-web#1419.

Sorry, I had to change the last ACK change check to make it exclusive, i.e., reject timestamp ≤ last ACK time.

Ok, just the changes the previous version of this PR aren't enough for Icinga DB, so I had to add more info to the icinga:runtime:state. These are all the fields required to compute the corresponding history ID hash without persisting them in the database.

• execution_end contains the execution end timestamp of the last check result, so that Icinga DB can correlate it with the corresponding check result history entry.
• flapping_last_change contains the last flapping change timestamp, so that Icinga DB can correlate it with the corresponding flapping history entry.
• last_triggered_downtime contains the last downtime name that triggered the state change update to be sent to the Redis stream.
• last_removed_downtime contains the last downtime name that was removed and triggered the state change update to be sent to the Redis stream.
• icingadb_previous_downtime_state transmits the previous downtime state of the object, so that Icinga DB knows if the update was triggered by a downtime start or end.
• icingadb_previous_acknowledgement_state transmits the previous acknowledgement state of the object, so that Icinga DB knows if the update was triggered by an acknowledgement set or clear.
• icingadb_previous_flapping_state transmits the previous flapping state of the object, so that Icinga DB knows if the update was triggered by a flapping start or stop.

See also Icinga/icingadb#1193 to see how these are going to be used in Icinga DB.

I've now changed the queuing code to use an explicit trigger bits instead of the previously used useless state attributes.

Sorry, still not working as expected.

That should be it, I think now :).

Fixed one last discovered bug while testing it. The acknowledgement_set_time sent to Redis didn't include the correct set time for ACK cleared events. Since the ACK clear event queues a runtime state update that goes through the worker thread, at the time when it serializes that request the acknowledgement_last_change will already be reset to the ACK clear time instead, making it impossible to reproduce the corresponding ACK clear history event ID in Icinga DB Web and Icinga DB (Go). So, in order to fix this without introducing any breaking changes, I've introduced a new state column called last_acknowledgement_set_time that tracks only the set time of an ACK.

I've also dropped the metadata from the initial config dump since there's no way to tell there which state to use instead of a blatant lie claiming that it's a state change, which caused sometimes an event ID to be generated that doesn't and wasn't ever generated by Icinga 2, causing a foreign key violation errors.

One thing I noticed on my second read is that the status code returned when set_time is out of bounds is 400, which IMHO is correct, but for an out of bounds expiry timestamp 409 (conflict?!) is returned. Just as a note, I don't want you to change either.

I noticed that too, and was confused and had to really re-check whether 409 is maybe another code for 400 but na, and I don't know why it uses 409 in that case and I didn't change it because I didn't want to argue about it in this PR.

Apart from that, I have also extended the acknowledge-problem API action to allow Icinga DB Web to provide custom ACK set time instead of having to always use now

What is this for? I didn't see an explanation for this even after going over linked issues and comments.

which no other clients have access to.

What does that mean?

Why that though? If Icinga Notifications requires some minimal version of Icinga 2 anyway, why can't/shouldn't this just be ≥ v2.16.x (for some x)?

Because Icinga DB v1.4.0 already bumped the required version to v2.15 due to the dependencies support and we don't want to force users to v2.16.x just for this.

Is there anything in v2.16.x that should stop you from updating? I mean users will have to update anyway. so if they have to update to v2.15.42 or v2.16.23 shouldn't make much of a difference? According to https://icinga.com/docs/icinga-2/latest/doc/16-upgrading-icinga-2/#upgrading-to-v216, that should be an easy upgrade in that you would only get some deprecation warnings, but no need to change anything.

This doesn't sound like a security/critical/important fix to me

It is important! After all, the event correlation will be one of the major features available with Icinga Notifications v1.0.

Overall, Icinga Notifications sounds more like a feature than a bug fix for me. So for me, backporting it to 2.16 is already an exception and backporting to 2.15 would be very odd.

Edit: Also, are you aware that backporting this to 2.15 would be very nasty given the reworks in Icinga DB in v2.16.0?

What is this for? I didn't see an explanation for this even after going over linked issues and comments.

Really #11035 (comment)? I think, I've explained this more than enough and provided enough sources for it, so I don't know what else should I include to make that clear.

which no other clients have access to.

What does that mean?

It means, only Icinga 2 knows the ACK set time since it just uses NOW() when acknowledging the checkable. Now, Icinga DB as well as Icinga DB Web needs that timestamp, so I've extended the API action to optionally provide own set time, so Icinga DB Web knows which ACK set is Icinga 2 going to use.

Is there anything in v2.16.x that should stop you from updating? I mean users will have to update anyway. so if they have to update to v2.15.42 or v2.16.23 shouldn't make much of a difference?

Nothing, except, well, they are two different major versions. Aren't you guys currently struggling to help a customer upgrade from v2.13.x or something to v2.15.x instead of directly to v2.16.x because of some problems v2.16 had/has while v2.15 doesn't? I don't really care whether this should really be back ported to v2.15 because ultimately I'm not the one who has/will have the last word on this, so 🤷‍♂️. //cc @nilmerg

Edit: Also, are you aware that backporting this to 2.15 would be very nasty given the reworks in Icinga DB in v2.16.0?

No, I actually wasn't aware of that, but see my previous paragraph.

What is this for? I didn't see an explanation for this even after going over linked issues and comments.

Really #11035 (comment)? I think, I've explained this more than enough and provided enough sources for it, so I don't know what else should I include to make that clear.

Most of this PR adds information to a Redis stream. What makes this particular value special so that it needs this additional treatment? Nothing in that comment explains the big picture.

Also, with Icinga/icinga-notifications-web#556 Icinga Notifications/DB Web will hijack the ACK set and clear API actions and will enqueue its own ACK set and clear events into Icinga Notifications job queue. So, in order to allow it to re-construct the ACK set history event IDs (even though I don't know yet how that will work, since there's no an equivalent implementation of Icinga 2's ObjectPacker in PHP), the acknowledge-problem API action will need to allow to provide a custom timestamp for the ACK set change time.

EDIT: See also Icinga/icingadb-web#1419.

Why does it need to hijack something? What exactly does that mean? As in: if I click this thing in Icinga Web, this is supposed to cause action A, B, and C in the future. The linked issues are very sparse on information unfortunately.

which no other clients have access to.

What does that mean?

It means, only Icinga 2 knows the ACK set time since it just uses NOW() when acknowledging the checkable. Now, Icinga DB as well as Icinga DB Web needs that timestamp, so I've extended the API action to optionally provide own set time, so Icinga DB Web knows which ACK set is Icinga 2 going to use.

Ah, I was reading that as "access to that API" and "clients other than Icinga Web".

Most of this PR adds information to a Redis stream. What makes this particular value special so that it needs this additional treatment? Nothing in that comment explains the big picture

Why does it need to hijack something? What exactly does that mean? As in: if I click this thing in Icinga Web, this is supposed to cause action A, B, and C in the future. The linked issues are very sparse on information unfortunately.

If you didn't forget that already, but ACKing a host/service problem promotes the very same Icinga Web 2 user also to an Incident manager in Icinga Notifications, and causes a one time notification to be sent to all existing incident recipients stating that the incident has been ACKed and that they won't receive any new notifications about that anymore until the manager is demoted. Now, we want to correlate the resulting notifications with corresponding Icinga 2/Icinga DB ACK set history entry, and that will only work if Icinga DB Web uses the actual history.id as a job_queue.ID to enqueue the promote this user to incident manager Icinga Notifications event, and Icinga DB Web can't just guess the history.id hash without the ACK set timestamp because that's what Icinga 2 uses to generate the corresponding history ID.

In short, when using compatible Icinga DB and Icinga Notifications, then the ACK quick action in Icinga DB Web will indeed cause two separate actions, one to send the actual ACK request to Icinga 2, and second, a separate event to enqueue a special event to the Icinga Notifications queue.

So that thing is basically special because Icinga Web wants to trigger related actions in Icinga 2 and directly in Icinga Notifications, instead of the action in Icinga Notifications being triggered from the Redis stream (as it's the case for all other actions that involve Icinga 2)?

So that thing is basically special because Icinga Web wants to trigger related actions in Icinga 2 and directly in Icinga Notifications, instead of the action in Icinga Notifications being triggered from the Redis stream (as it's the case for all other actions that involve Icinga 2)?

Yes. Because there's only one way to promote a user to manager, and that's from within Icinga Notifications Web, which Icinga DB Web makes use of through some kind of hooks, I guess. Though, Icinga DB will still the related event to Icinga Notifications, but only to mute an existing incident and nothing more.