Fix persistent comments for Acknowledgements

[TheFlyingCorpse]

refs #4818

[TheFlyingCorpse]

Note: I used the same way of setting the bool persistent / false as commented earlier as is already used for the ApiAction::ScheduleDowntime command.

[dnsmichi]

That should check for a boolean, not just for the attribute being set.

Similar fix to be applied: 8196523

if (params->Contains("sticky") && HttpUtility::GetLastParameter(params, "sticky"))

[TheFlyingCorpse]

Updated to convert the number to bool properly.

[gunnarbeutner]

Can you remove the unnecessary explicit '== true', '> 0', etc. checks?

Also, aren't all non-persistent comments in Icinga 1.x removed on restart (by virtue of not being persisted at all)? Your patch seems to not remove expired comments at all if they're persistent. Is that the correct behavior?

[dnsmichi]

The naming with persistent just for acknowledgement comments is somewhat ambiguous. For "normal" comments there's no exact meaning, you cannot change or tell anything. Comments of the type Acknowledgement will only require an additional state to determine whether the comment is being deleted upon ack removal or not.

[TheFlyingCorpse]

Right on the updated documentation, I see it now when you point out the ambiguity. So remove the document update and squash commits? Anything else?

…

Den 10. feb. 2017 kl. 09.10 skrev Michael Friedrich ***@***.***>: The naming with persistent just for acknowledgement comments is somewhat ambiguous. For "normal" comments there's no exact meaning, you cannot change or tell anything. Comments of the type Acknowledgement will only require an additional state to determine whether the comment is being deleted upon ack removal or not. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

[dnsmichi]

Squashing would certainly help an additional review, not sure about the docs. I would opt for finding a better name for persistent. That is for the changes inside the .ti file as well as the function parameters inside the source code.

[TheFlyingCorpse]

Updated, let me know if I didnt do it correctly. :-)

[TheFlyingCorpse]

Rebased based on master, there was a conflict with doc and apiactions after an update to notify.

[TheFlyingCorpse]

Feedback from @dnsmichi is that he does not like the proposed name of comment_persistance.

Other suggestions:

• comment_from_ack_persistent
• acknowledgement_persistance
• acknowledgement_expiry

I'd really like to see this in 2.7 as the bug causes us a lot of headaches when comments from acknowledgements disappear with ticket numbers that was the comment for the ack.

[dnsmichi]

Review

I don't have a better name. I'd go with the same name as known from 1.x and external commands, just persistent. In contrast to the old world, our comments are never removed upon daemon restart (that is really nonsense).

The docs should be clear about that, and if one tends to fetch the values from i.e. the API, he/she needs to be sure to check entry_type == 4 && persistent == ? then.

In terms of upgrade safety, one would say, persistent should default to false. All comments inside the _api package won't have that flag set, and would likely to continue being removed on host recovery.
On the other hand queries will look strange with persistent returning false. But that is purely cosmetic and can only be explained in the docs.

I wouldn't change that behaviour and leave your patch as is.

Patch

I've modified some code parts following our code style, and removed a duplicate check. Rebased against master and amended my changes into your commit. If've also taken the liberty of force pushing your remote branch to cleanly merge this PR.

Sorry for keeping this so long on my TODO list. I wanted to make sure I've fully understand your concern and proposed patch. The description with ticket ids in comments totally make sense to me now.

Thanks for the patch 👍

Tests

Non-persistent

Add

curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/acknowledge-problem' -d '{ "author": "michi", "comment": "jo eh.", "type": "Service", "filter": "match(\"mbmif*disk /\", service.__name) && service.state != ServiceOK" }'

curl -k -s -u root:icinga -H 'Accept: application/json' -H 'X-HTTP-Method-Override: GET' -X POST 'https://localhost:5665/v1/objects/comments' -d '{ "joins": [ "service.__name", "service.acknowledgement" ], "filter": "comment.entry_type == 4", "attrs": [ "__name", "author", "persistent", "text" ] }' | python -m json.tool
{
    "results": [
        {
            "attrs": {
                "__name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428256-9",
                "author": "michi",
                "persistent": false,
                "text": "jo eh."
            },
            "joins": {
                "service": {
                    "__name": "mbmif.int.netways.de!disk /",
                    "acknowledgement": 1.0
                }
            },
            "meta": {},
            "name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428256-9",
            "type": "Comment"
        }
    ]
}

Remove

curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/remove-acknowledgement' -d '{ "type": "Service", "filter": "match(\"mbmif*disk /\", service.__name) && service.state != ServiceOK" }'

$ curl -k -s -u root:icinga -H 'Accept: application/json' -H 'X-HTTP-Method-Override: GET' -X POST 'https://localhost:5665/v1/objects/comments' -d '{ "joins": [ "service.__name", "service.acknowledgement" ], "filter": "comment.entry_type == 4", "attrs": [ "__name", "author", "persistent", "text" ] }' | python -m json.tool
{
    "results": []
}

Persistent

Add

curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/acknowledge-problem' -d '{ "author": "michi", "comment": "jo eh.", "persistent": true, "type": "Service", "filter": "match(\"mbmif*disk /\", service.__name) && service.state != ServiceOK" }

curl -k -s -u root:icinga -H 'Accept: application/json' -H 'X-HTTP-Method-Override: GET' -X POST 'https://localhost:5665/v1/objects/comments' -d '{ "joins": [ "service.__name", "service.acknowledgement" ], "filter": "comment.entry_type == 4", "attrs": [ "__name", "author", "persistent", "text" ] }' | python -m json.tool
{
    "results": [
        {
            "attrs": {
                "__name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428328-10",
                "author": "michi",
                "persistent": true,
                "text": "jo eh."
            },
            "joins": {
                "service": {
                    "__name": "mbmif.int.netways.de!disk /",
                    "acknowledgement": 1.0
                }
            },
            "meta": {},
            "name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428328-10",
            "type": "Comment"
        }
    ]
}

Remove

curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/remove-acknowledgement' -d '{ "type": "Service", "filter": "match(\"mbmif*disk /\", service.__name) && service.state != ServiceOK" }'

curl -k -s -u root:icinga -H 'Accept: application/json' -H 'X-HTTP-Method-Override: GET' -X POST 'https://localhost:5665/v1/objects/comments' -d '{ "joins": [ "service.__name", "service.acknowledgement" ], "filter": "comment.entry_type == 4", "attrs": [ "__name", "author", "persistent", "text" ] }' | python -m json.tool
{
    "results": [
        {
            "attrs": {
                "__name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428328-10",
                "author": "michi",
                "persistent": true,
                "text": "jo eh."
            },
            "joins": {
                "service": {
                    "__name": "mbmif.int.netways.de!disk /",
                    "acknowledgement": 0.0
                }
            },
            "meta": {},
            "name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428328-10",
            "type": "Comment"
        }
    ]
}

All fine ❤️

Icinga Web 2

Keep in mind that Icinga Web 2 doesn't send the persistent flag when the API command transport is used - that needs to be adopted too once this patch is merged and released. The API doesn't accept the flag in <= 2.6, while the external command listener silently ignores the passed value (and it doesn't return an error to the caller anyways).

I haven't tested the patch, but it should work. Maybe you can test that and send in a PR for Icinga Web 2 then :)

mbmif /usr/local/share/icingaweb2 (master *=) # git diff
diff --git a/modules/monitoring/library/Monitoring/Command/Renderer/IcingaApiCommandRenderer.php b/modules/monitoring/library/Monitoring/Command/Renderer/IcingaApiCommandRenderer.php
index 30dc414dc..edc5946ed 100644
--- a/modules/monitoring/library/Monitoring/Command/Renderer/IcingaApiCommandRenderer.php
+++ b/modules/monitoring/library/Monitoring/Command/Renderer/IcingaApiCommandRenderer.php
@@ -181,7 +181,8 @@ class IcingaApiCommandRenderer implements IcingaCommandRendererInterface
             'comment'       => $command->getComment(),
             'expiry'        => $command->getExpireTime(),
             'sticky'        => $command->getSticky(),
-            'notify'        => $command->getNotify()
+            'notify'        => $command->getNotify(),
+            'persistent'    => $command->getPersistent()
         );
         $this->applyFilter($data, $command->getObject());
         return IcingaApiCommand::create($endpoint, $data);

[dnsmichi]

@lippserd see my note about the Icinga Web 2 API command transport :)