-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Recognize PENDING -> UNKNOWN as state change #7815
base: master
Are you sure you want to change the base?
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
Just for understanding: Why exactly is it not allowed? |
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. |
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. |
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. |
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. |
Let's simplify this. Please write a new proposal, similar to a draft concept with the label That being said, I'll close this PR. |
IMHO the expected behaviour for related state transitions is as simple as follows:
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. |
Thank you, @Thomas-Gelf, for your opinion. Yes, I need a bit more words, but I'll just add them by myself:
|
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). |
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. |
Even better. 1 vs. 2.5. |
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. |
|
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. |
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. |
Full ack. |
I opt for changing this but we need a test protocol with all possible states, state changes and triggered notifications please. |
|
fa758e2
to
4fe677b
Compare
@Al2Klimov I find it a little confusing that the first state without cr is also referred to as |
... to get notified about that state change. refs #7807
4fe677b
to
7cd1dd8
Compare
@cla-bot check |
ref/IP/44082 |
... to get notified about that state change.
fixes #7807
fixes #10217