Recognize PENDING -> UNKNOWN as state change

[Al2Klimov]

... to get notified about that state change.

fixes #7807
fixes #10217

[dnsmichi]

This changes the current behavior, and will possibly influence the way states are counted and notifications are sent. Imho this needs a deeper discussion with @lippserd

[dnsmichi]

I've also discussed this with @N-o-X today. Changing the behavior of ProcessCheckResult is not allowed in this regard.

If IcingaDB needs this specific state change from "pending" (not known to Icinga 2 itself) to another state, you can add a new signal where the IcingaDbWriter can subscribe to. Something like

if (!old_cr)
  OnFirstCheckCheckResult(cr, origin);

[Al2Klimov]

Just for understanding: Why exactly is it not allowed?

[dnsmichi]

It is a change in the behavior of the state machine. Now, !cr (pending) -> first state is not treated as a state change. As such, people know exactly when event handlers and notifications are fired. If this is shifted by one, making this !cr (pending) -> first state a first state change, the counting logic changes and leads to unexpected behaviour with regard to event handlers and notifications.

If you really need to know about the change from pending (which isn't a state by definition in Icinga 2) to the first check result, our decision was to add a new signal where IcingaDB and other features may subscribe upon, leaving the current state machine intact.

[Al2Klimov]

For me that notification behavior is absolutely expected, i.e. I if a service with one check attempt changes from a non-problem state (e.g. PENDING) to a problem state (e.g. UNKNOWN) I expect a notification.

By the way we've already changed that state machine indirectly. I opt for merging this at least after v2.12.

[dnsmichi]

I can only repeat myself, pending is not a state in Icinga 2. This is something which has been invented by the web views, and is now slowly moved into the core without re-evaluation. Imho this needs a proper thought and conceptual review on all the affected components.

Anyhow, pinging @Thomas-Gelf since this affects the whole architecture.

[Al2Klimov]

Sigh. Okay, then it's not PENDING, but UNKNOWN w/o any check result so far which have been changed to not a problem by you and me.

As a user I'd like to get notifications on a change from UNKNOWN w/o any check result so far to UNKNOWN the same way as e.g. on a change from UNKNOWN w/o any check result so far to CRITICAL.

[dnsmichi]

Let's simplify this.

Please write a new proposal, similar to a draft concept with the label TBD, how the current state logic works, and how you'd like it being changed. Illustrate that in all its detail in a new issue, including the core and web components. 1000 words should be a good orientation for that. Then everyone can discuss based on your ideas.

That being said, I'll close this PR.

[Thomas-Gelf]

IMHO the expected behaviour for related state transitions is as simple as follows:

From	To	Notify	State Change
Pending	Ok	-	x
Pending	Up	-	x
Pending	Warning	x	x
Pending	Critical	x	x
Pending	Unknown	x	x
Pending	Down	x	x

State change should result in IDO/IcingaDB State & History updates, former (last) state fields should be NULL where a transition from Pending/NoState occurred. If you need more words please let me know.

[Al2Klimov]

Thank you, @Thomas-Gelf, for your opinion. Yes, I need a bit more words, but I'll just add them by myself:

1. Just to be clear, the thing you called Pending is actually UNKNOWN w/o any check result so far on our side
2. You seem to expect an actual state change on transition from UNKNOWN w/o any check result so far to ANY w/ a check result
3. This PR does no less and no more that that and should be re-opened for merging in the future

[Al2Klimov]

Re-opening as there seem to be discussion potential. @dnsmichi and @N-o-X ("our decision") are against and @Thomas-Gelf and me are not (2 vs. 2).

[N-o-X]

I'm not directly against it, I'm just not sure if this change would break stuff. I really think the behaviour @Thomas-Gelf proposed would be nice to have, but might break something during implementation. As @dnsmichi said, it would be nice to have a summary of what that change would touch (Core & Web) and if we're able to implement it without breaking those things.

[Al2Klimov]

Even better. 1 vs. 2.5.

[dnsmichi]

A decision can only be made with a document which covers all the details. Therefore I'd suggest that @Al2Klimov creates a new issue and does exactly that.

[Al2Klimov]

1. We really don't need more than one issue for one PR.
2. IMAO @Thomas-Gelf has already covered all details.

[dnsmichi]

I'm talking about a document, which includes the table, and describes the current situation, adds the changes and includes all dependencies. It should also discuss the possible impact on backends, features and the end-user. If you need an inspiration, look into #7814 how to write such a document/concept. Depending on the work load of yours, we should find a time slot in your schedule with @lippserd.

[Thomas-Gelf]

To me this seems "slightly" exaggerated for a fix that adds a missing state change event. I do not know whether the pull request would lead to notifications being treated differently than state changes in the DB, so this is something @Al2Klimov could eventually verify and explain. It is important that we're not going to send notifications when transitioning from "pending" (no former result) to a non-problem state.

Web frontend: @dnsmichi, could you please elaborate why you're expecting this change to break anything there?

IDO: no new state type is being introduced. Fixing the NULL-issue I mentioned would be a change with potential impact. Skip that part if you feel unsure. SLA functions didn't trust this (last_state) column from the beginning, so who cares.

Notification commands: some might work with last_state. Could they fail when sending a notification for an penging-to-unknown transition? Only if they also failed for a pending-to-critical transition. Who is to blame? The script author. Is this worse than sending no notification at all? Definitively no.

[Al2Klimov]

To me this seems "slightly" exaggerated for a fix that adds a missing state change event.

Full ack.

[lippserd]

I opt for changing this but we need a test protocol with all possible states, state changes and triggered notifications please.

[Al2Klimov]

1. Yes and no. Now it applies also to PENDING -> UNKNOWN, in addition to PENDING -> X.
2. Hard state change? Why hard state change? (see below)
3. -
4. See Checkable#GetProblem(): consider PENDING not a problem #7685 and 2.
5. Yes. (But see also 1.)

[2020-11-10 13:20:55 +0100] warning/PluginCheckTask: Check command for object 'Alexanders-MacBook-Pro.local!http' (PID: 88691, arguments: '/Users/aklimov/NET/WS/icinga2/prefix/usr/lib/nagios/plugins/check_http' '-I' '127.0.0.1' '-u' '/') terminated with exit code 128, output: execvpe(/Users/aklimov/NET/WS/icinga2/prefix/usr/lib/nagios/plugins/check_http) failed: No such file or directory

[2020-11-10 13:20:55 +0100] notice/Checkable: State Change: Checkable 'Alexanders-MacBook-Pro.local!http' soft state change from UNKNOWN to UNKNOWN detected.

[lippserd]

@Al2Klimov I find it a little confusing that the first state without cr is also referred to as UNKNOWN in the logs. Is there any chance that we log something like "first state change to UNKNOWN" instead of "UNKNOWN to UNKNOWN"?

[Al2Klimov]

@cla-bot check

[Al2Klimov]

ref/IP/44082
ref/NC/774135