[dev.icinga.com #13377] Acknowledgements marked with Persistent Comment are not honored

[icinga-migration]

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)

Icinga Version: 2.5.4
Backport?: Not yet backported
Include in Changelog: 1

---

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

void ExternalCommandProcessor::AcknowledgeHostProblem(double, const std::vector<String>& arguments)

AcknowledgeSvcProblem

icinga2/lib/icinga/externalcommandprocessor.cpp

Line 620 in 09658f6

void ExternalCommandProcessor::AcknowledgeSvcProblem(double, const std::vector<String>& arguments)

Attachments

• 0001-Fix-the-behaviour-of-persistent-comments-from-acknow.patch TheFlyingCorpse - 2016-12-07 21:57:52 +00:00
• 0003-Honor-persistent-comment-from-acks-with-expiry.patch TheFlyingCorpse - 2016-12-07 23:23:24 +00:00 - Honor persistent also from acks with expiry
• 0002-Do-not-expire-comments-set-as-persistent-from-acknow.patch TheFlyingCorpse - 2016-12-07 23:23:24 +00:00 - IDO patch

---

Relations:

• relates #13377

[icinga-migration]

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.
https://github.com/Icinga/icinga2/blob/8fd454fbb1457a34031a53baa16d2bacc77bba1b/lib/db\_ido/dbevents.cpp#L761

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

void Checkable::RemoveCommentsByType(int type)

[icinga-migration]

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)?

[icinga-migration]

Updated by TheFlyingCorpse on 2016-12-07 21:59:10 +00:00

• File added 0001-Fix-the-behaviour-of-persistent-comments-from-acknow.patch

Suggested fix attached.

[icinga-migration]

Updated by mfriedrich on 2016-12-07 22:05:05 +00:00

• Relates set to 9619

[icinga-migration]

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).
Note, in Icinga 1.x, I was told that the documentation said persistent was to remove on reload of the daemon. However icinga 1.x docu says persistent flags means keep comment after recovery.
Ref: https://docs.icinga.com/latest/en/extcommands2.html

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.

[icinga-migration]

Updated by TheFlyingCorpse on 2016-12-07 23:24:49 +00:00

• File added 0002-Do-not-expire-comments-set-as-persistent-from-acknow.patch
• File added 0003-Honor-persistent-comment-from-acks-with-expiry.patch

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.

[icinga-migration]

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?

[icinga-migration]

Updated by TheFlyingCorpse on 2017-01-16 22:01:37 +00:00

Will do :) Thanks for the update

[dnsmichi]

Hm, IIRC persistent means that such a comment will survive a core restart. That is a fairly strange setting in 1.x since you reload the core quite often. That is what you get when you call "ADD_SVC_COMMENT" as an external command. We decided to skip that and make comments persistent over restarts all the time.

In case you're using persistent with an Acknowledgement type and comment, it gets a different meaning. Then persistent means to keep the comment even if the acknowledgement is removed (thus a recovery).

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 persistent attribute for a) all other comments b) upgraded systems.

For a) - that settings remains useless, for b) if not explicitly set in the config, it must return true inside the comment.ti file to keep the same behaviour as known from older versions.

[dnsmichi]

Well, and since persistent is a boolean value type then, it should use bool instead of int everywhere.

[TheFlyingCorpse]

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)