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
[dev.icinga.com #13377] Acknowledgements marked with Persistent Comment are not honored #4818
Comments
Updated by TheFlyingCorpse on 2016-12-01 11:46:14 +00:00 It seems it is always set persistent in ido regardless of the flag. However it is removed when the ack expires or the service recovers because it does not use the flag to verify it should be removed or not. icinga2/lib/icinga/checkable-comment.cpp Line 38 in 8fd454f
|
Updated by TheFlyingCorpse on 2016-12-06 21:00:57 +00:00 I've added the code now to store the persistent bit. Would the best approach be to change the comment from an ack to a normal "user" comment when it is no longer an ack(ie, it is recovered, or a clear ack has been sent, thus change it to a normal comment)? |
Updated by TheFlyingCorpse on 2016-12-07 21:59:10 +00:00
Suggested fix attached. |
Updated by mfriedrich on 2016-12-07 22:05:05 +00:00
|
Updated by TheFlyingCorpse on 2016-12-07 22:30:32 +00:00 To me it looks like it wasnt used that way if you read the code itself. There persistent flags the comment (from ack) that the comment should persist when the ack is removed on recovery (or expire). In addition in the code: https://github.com/Icinga/icinga-core/blob/d3158e0140b19bc578556d0172a5ec69c943d4d1/common/comments.c#L354 My googlefu didnt find any good other references, I did see earlier this week that the external command docu for the command was different in nagios, there persistent was flagged as keep ack (and comment) until reload of daemon. Btw, for Ido which hardcodes the persistent flag (to no use other than saying all comments are persistent in the database) I can easily include a fix for correctly adding the "persistent" to the correct data in ido if the definition above is accepted and we start using the real attribute of the comment. |
Updated by TheFlyingCorpse on 2016-12-07 23:24:49 +00:00
Two more patches for edge cases I didnt think of when submitting this. After some thinking these should be included but they are not part of the original bugfix because they are more edge cases than the original patch. 0002 is for storing the correct "is_persistent" in the databases' comment (and comment_history) table. 0003 is for honoring "persistent" also with acknowledgements set to expire on a timer. |
Updated by mfriedrich on 2017-01-13 11:07:11 +00:00 The first patch for the api action isn't entirely correct, it must check and assign the value the caller passed. Just setting the value to "true" because it was added to the request isn't correct. See the other issue with #13939. Can you please apply your patches, rebase against master, and send in a PR on Github? |
Updated by TheFlyingCorpse on 2017-01-16 22:01:37 +00:00 Will do :) Thanks for the update |
Hm, IIRC In case you're using persistent with an Acknowledgement type and comment, it gets a different meaning. Then Your patch attempts to fix this, and by using RemoveCommentsByType(CommentAcknowledgement) this certainly already works, although missing the persistent attribute check. The thing which bugs me the most - how to handle the new For a) - that settings remains useless, for b) if not explicitly set in the config, it must return |
Well, and since |
On using int instead of bool: I used int because that was what came in over the external command processor and already used with the apiaction I looked at for reference, I'll update in a few days. Am I allowed to use commenttype to decide the outcome of persistent? That could decide when to honor the persistent or not, if its any other type than Ack, set to false. Thats the same way for upgraded systems, its false by default since you dont know what state it was set with (which is what this PR tries to fix) |
This issue has been migrated from Redmine: https://dev.icinga.com/issues/13377
Created by TheFlyingCorpse on 2016-12-01 11:12:51 +00:00
Assignee: (none)
Status: New
Target Version: (none)
Last Update: 2017-01-16 22:01:36 +00:00 (in Redmine)
Acknowledgements marked in IcingaWeb2 as a persistent comment are not stored once the service or host recovers.
I have verified the bit is set correctly in the external command submitted to Icinga2.
The code does not seem to use the flag set.
AcknowledgeHostProblem
icinga2/lib/icinga/externalcommandprocessor.cpp
Line 679 in 09658f6
AcknowledgeSvcProblem
icinga2/lib/icinga/externalcommandprocessor.cpp
Line 620 in 09658f6
Attachments
Relations:
The text was updated successfully, but these errors were encountered: