Fix scheduling of downtimes for all services on child hosts #8990

julianbrost · 2021-09-03T11:11:12Z

Fundamentally, scheduling downtimes for all services on child hosts was implemented, it just iterated over the services of the wrong host resulting in duplicate downtimes being scheduled for services of the parent host. This PR fixes this loop and in addition prevents the creation of duplicate downtimes for individual services in case that both the service and its host are children of the parent checkable.

Tests

Test config: 3 hosts, each with two services, service on on hosts child1 and child2 depends on host parent, additionally, host child1 depends on host parent.

object Host "github-8989-parent" {
	check_command = "dummy"
}

object Host "github-8989-child1" {
	check_command = "dummy"
}

object Host "github-8989-child2" {
	check_command = "dummy"
}

apply Service "github-8989-service" for (name in ["1", "2"]) {
	check_command = "dummy"
	assign where match("github-8989-*", host.name)
}

object Dependency "github-8989-c1" {
	parent_host_name = "github-8989-parent"
	child_host_name = "github-8989-child1"
}

object Dependency "github-8989-c1-s1" {
	parent_host_name = "github-8989-parent"
	child_host_name = "github-8989-child1"
	child_service_name = "github-8989-service1"
}

object Dependency "github-8989-c2-s1" {
	parent_host_name = "github-8989-parent"
	child_host_name = "github-8989-child2"
	child_service_name = "github-8989-service1"
}

Please ignore the service keep-state in the following output, that's just an additional service I apply to every host in my test cluster.

Before

$ curl -iskSu root:icinga -H 'Accept: application/json' -X POST "https://127.0.0.1:5665/v1/actions/schedule-downtime" -d '{"type":"Host", "filter":"host.name == \"github-8989-parent\"", "start_time":'$(date +%s)', "end_time":'$(date -d +5min +%s)', "author":"icingaadmin", "comment":"42", "pretty":true, "all_services":1, "child_options":"DowntimeTriggeredChildren"}'
HTTP/1.1 200 OK
Server: Icinga/v2.13.0-22-g95cdc00ad
Content-Type: application/json
Content-Length: 2172

{
    "results": [
        {
            "child_downtimes": [
                {
                    "legacy_id": 18,
                    "name": "github-8989-child1!65a4a10e-473c-4ae1-9e46-96cf4ce7e463",
                    "service_downtimes": [
                        {
                            "legacy_id": 19,
                            "name": "github-8989-parent!github-8989-service1!8a7ad4e7-cd20-4c4e-8d7c-caeea1249503"
                        },
                        {
                            "legacy_id": 20,
                            "name": "github-8989-parent!github-8989-service2!f563e60f-7b9d-48fb-a92d-c99c51da14b3"
                        },
                        {
                            "legacy_id": 21,
                            "name": "github-8989-parent!keep-state!15a20b8b-7ba6-49c2-882f-440494443f50"
                        }
                    ]
                },
                {
                    "legacy_id": 22,
                    "name": "github-8989-child2!github-8989-service1!1673890d-ed43-43d0-8f19-787657fce90c"
                },
                {
                    "legacy_id": 23,
                    "name": "github-8989-child1!github-8989-service1!bf162feb-f79b-4286-a310-0694073005c8"
                }
            ],
            "code": 200,
            "legacy_id": 14,
            "name": "github-8989-parent!4aca458c-2c6c-49f6-ac87-e9e57f284726",
            "service_downtimes": [
                {
                    "legacy_id": 15,
                    "name": "github-8989-parent!github-8989-service1!df16c710-fd6f-48f6-973b-cbcaaad53738"
                },
                {
                    "legacy_id": 16,
                    "name": "github-8989-parent!github-8989-service2!f3f3b6a5-6b94-4e09-8266-da0ff6bafed2"
                },
                {
                    "legacy_id": 17,
                    "name": "github-8989-parent!keep-state!290c7080-6712-4d3a-befb-fadaaaa4dd99"
                }
            ],
            "status": "Successfully scheduled downtime 'github-8989-parent!4aca458c-2c6c-49f6-ac87-e9e57f284726' for object 'github-8989-parent'."
        }
    ]
}

Note how the service_downtimes of downtime github-8989-child1!65a4a10e-473c-4ae1-9e46-96cf4ce7e463 lists downtime for github-8989-parent.

After

$ curl -iskSu root:icinga -H 'Accept: application/json' -X POST "https://127.0.0.1:5665/v1/actions/schedule-downtime" -d '{"type":"Host", "filter":"host.name == \"github-8989-parent\"", "start_time":'$(date +%s)', "end_time":'$(date -d +5min +%s)', "author":"icingaadmin", "comment":"42", "pretty":true, "all_services":1, "child_options":"DowntimeTriggeredChildren"}'
HTTP/1.1 200 OK
Server: Icinga/v2.13.0-24-gbb0dcdf0b
Content-Type: application/json
Content-Length: 1991

{
    "results": [
        {
            "child_downtimes": [
                {
                    "legacy_id": 21,
                    "name": "github-8989-child2!github-8989-service1!a8d2f679-1b12-493d-97a1-d080d87a02f1"
                },
                {
                    "legacy_id": 22,
                    "name": "github-8989-child1!82279e74-7225-477b-bfe0-b150c637afaa",
                    "service_downtimes": [
                        {
                            "legacy_id": 23,
                            "name": "github-8989-child1!github-8989-service1!8c488f11-b76e-47f0-b264-7fb1b5cb8fd0"
                        },
                        {
                            "legacy_id": 24,
                            "name": "github-8989-child1!github-8989-service2!5bddb234-a4b6-4957-a42f-c45012e0c9d6"
                        },
                        {
                            "legacy_id": 25,
                            "name": "github-8989-child1!keep-state!d7d3b838-1fa2-4d17-81d1-89c985bc25cd"
                        }
                    ]
                }
            ],
            "code": 200,
            "legacy_id": 17,
            "name": "github-8989-parent!4cc56eef-6d6b-4a00-a575-3489c17e185c",
            "service_downtimes": [
                {
                    "legacy_id": 18,
                    "name": "github-8989-parent!github-8989-service1!7a59a20a-eeb0-442a-8890-aeeb3b2a50a7"
                },
                {
                    "legacy_id": 19,
                    "name": "github-8989-parent!github-8989-service2!b320919a-f888-46e5-940c-9384ab3ff369"
                },
                {
                    "legacy_id": 20,
                    "name": "github-8989-parent!keep-state!65cf7935-adb4-43ec-9605-11720322bf94"
                }
            ],
            "status": "Successfully scheduled downtime 'github-8989-parent!4cc56eef-6d6b-4a00-a575-3489c17e185c' for object 'github-8989-parent'."
        }
    ]
}

Now service_downtimes for github-8989-child1!82279e74-7225-477b-bfe0-b150c637afaa indeed contains downtimes for all services of host github-8989-child1. Also note that github-8989-child2!service1 part of that list and there is no duplicate downtime for it in child_downtimes.

Deleting the downtime for parent also deletes the downtimes created for services of that host but leaves downtimes of other hosts and their services unaffected. Deleting the downtime for child1 also deletes the downtimes for all its serivces (including service1 which is a child itself).

Special thanks to @NicolasGoeddel for pointing out the cause of this bug.

fixes #8989

nilmerg · 2021-09-03T12:14:29Z

Will cancelling the parent downtime now also cancel the service downtimes of children?

julianbrost · 2021-09-03T12:25:42Z

lib/icinga/apiactions.cpp

-					Downtime::Ptr serviceDowntime = Downtime::AddDowntime(hostService, author, comment, startTime, endTime,
+					Downtime::Ptr serviceDowntime = Downtime::AddDowntime(childService, author, comment, startTime, endTime,
 						fixed, triggerName, duration);


Will cancelling the parent downtime now also cancel the service downtimes of children?

Looks like it won't as #8913 didn't add the information about that relation to this call. But I think it should, so I'll change that.

Looking at the code, it's not only missing in that place but also in line 442. @N-o-X was this a deliberate decision to only set the parent relation for those downtimes created by all_services but not child_options?

Thinking about it, I think I can remember the discussion we had about that (looks like we haven't written down the result over there in the PR).

We decided on not propagating the deletion down to dependencies as it's not clear what should happen if a downtime somewhere in the middle of a dependency chain is deleted. Intuitively, one could say that this should delete all the downtimes in that sub-tree, however dependencies are a graph, not a tree, so a dependant child could be in there due to multiple dependencies.

What's definitely an oversight in the current PR that service downtimes on child hosts don't have a relation to the downtime on the host. This should be addressed in any case. I'm no longer sure about dependencies. If we want to also delete then, this should be handled separately as this needs larger changes to what we store and how we handle deletes (as described in the second paragraph) and also a downtime can only one parent right now, so it can't have a relation to both the downtime on the child host and the root downtime.

Aye fine, but then should the documentation get updated (https://icinga.com/docs/icinga-2/latest/doc/16-upgrading-icinga-2/#deletion-of-child-downtimes-on-services) before this fix is propagated. Otherwise the expected result is false.

I can't see anything wrong with the documentation. It says all_services flag and nothing about child_options.

But with this change the all_services flag applies also to childs. If that's the case, and I re-read the documentation with this in mind, I expect that these service downtimes of children also get removed.

Deleting a downtime of a child host will now also delete downtimes for service of that particular child host.

Al2Klimov

LGTM

The loop iterated over the services of the wrong host resulting in duplicate downtimes scheduled for services of the parent host instead of downtimes for services of the child host.

…ices

julianbrost · 2021-09-03T14:06:41Z

I've pushed a new version that handles the deletion of child downtimes for the all_services case. Please have another look.

@nilmerg Please ack if that behavior is ok for you. Otherwise please create an issue for also handling the propagation of deletion for child_options, but be aware that this would require some larger rework of how dependencies are handled.

nilmerg · 2021-09-03T14:36:11Z

I just want that creating and removing downtimes behaves the same regarding the all_services flag. If a child's services get a downtime due to that flag and the documentation tells me that service downtimes created with this flag get automatically removed, and if this is in fact the case.., then it's fine. If that's not the case the documentation has to reflect that.

julianbrost · 2021-09-03T14:46:30Z

Does the current behavior (i.e. downtime removal on any hosts propagates to the services of that very host) match the documentation? The only problem I could see is that the term "child" is sometimes used for the relation between a service and the host it belongs to and sometimes for the relation given by an explicit Dependency object.

nilmerg · 2021-09-03T14:58:53Z

From: https://icinga.com/docs/icinga-2/latest/doc/12-icinga2-api/#schedule-downtime

Sets downtime for all services for the matched host objects. If child_options are set, all child hosts and their services will schedule a downtime too.

From: https://icinga.com/docs/icinga-2/latest/doc/16-upgrading-icinga-2/#deletion-of-child-downtimes-on-services

Service downtimes created while using the all_services flag on the schedule-downtime API action will now automatically be deleted when deleting the hosts downtime.

The latter only speaks about the parent host's services. It needs to clarify what happens to child hosts's services.

Btw: I miss that part here: https://icinga.com/docs/icinga-2/latest/doc/12-icinga2-api/#remove-downtime

xenuser · 2021-10-12T11:31:50Z

@nilmerg Can you provide a short update when this will be moving forward? Thanks in advance.

…time

julianbrost · 2021-11-19T13:48:52Z

I've added another commit that documents how downtime removals using /v1/actions/remove-downtime propagate to other downtimes.

nilmerg

The docs are fine now, thanks @julianbrost!

NicolasGoeddel · 2021-12-03T15:22:18Z

Can anyone tell me when the new release will be out? @lippserd told me he thinks it will be out in late November.
If I understand this correctly it should be available in v2.13.3 and v2.12.7. I would be happy if there is already a final date.

cla-bot bot added the cla/signed label Sep 3, 2021

icinga-probot bot added area/runtime Downtimes, comments, dependencies, events bug Something isn't working ref/IP labels Sep 3, 2021

julianbrost added the consider backporting Should be considered for inclusion in a bugfix release label Sep 3, 2021

julianbrost requested a review from Al2Klimov September 3, 2021 12:10

julianbrost commented Sep 3, 2021

View reviewed changes

Al2Klimov approved these changes Sep 3, 2021

View reviewed changes

julianbrost marked this pull request as draft September 3, 2021 12:53

julianbrost added 2 commits September 3, 2021 15:19

Fix scheduling of downtimes for all services on child hosts

e556d3c

The loop iterated over the services of the wrong host resulting in duplicate downtimes scheduled for services of the parent host instead of downtimes for services of the child host.

Prevent duplicate donwtimes when combining child_options and all_serv…

bb0dcdf

…ices

julianbrost force-pushed the bugfix/downtime-all-services-on-child-hosts branch from b762af7 to bb0dcdf Compare September 3, 2021 13:53

julianbrost requested review from Al2Klimov and nilmerg September 3, 2021 14:04

julianbrost marked this pull request as ready for review September 3, 2021 14:04

Al2Klimov approved these changes Sep 3, 2021

View reviewed changes

Document which other downtimes are deleted by /v1/actions/remove-down…

da08d97

…time

nilmerg approved these changes Nov 22, 2021

View reviewed changes

Al2Klimov assigned lippserd Dec 3, 2021

Al2Klimov merged commit 31c5641 into master Dec 7, 2021

icinga-probot bot deleted the bugfix/downtime-all-services-on-child-hosts branch December 7, 2021 11:48

julianbrost added this to the 2.14.0 milestone Dec 17, 2021

Al2Klimov mentioned this pull request Jan 3, 2022

Fix scheduling of downtimes for all services on child hosts #9159

Merged

Al2Klimov mentioned this pull request Jan 20, 2022

Fix scheduling of downtimes for all services on child hosts #9184

Merged

julianbrost added backported Fix was included in a bugfix release and removed consider backporting Should be considered for inclusion in a bugfix release labels Mar 30, 2022

acp-jsz mentioned this pull request Apr 14, 2022

Bug in 2.12.7 release related to downtimes for all services on child hosts #9345

Closed

julianbrost mentioned this pull request Apr 19, 2022

Fix mixed up arguments for AddDowntime/RemoveDowntime in 2.12.x #9349

Merged

Fix scheduling of downtimes for all services on child hosts #8990

Fix scheduling of downtimes for all services on child hosts #8990

Uh oh!

Conversation

julianbrost commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Before

After

Uh oh!

nilmerg commented Sep 3, 2021

Uh oh!

julianbrost Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

julianbrost Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

julianbrost Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

nilmerg Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

N-o-X Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

nilmerg Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

julianbrost Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

Al2Klimov left a comment

Choose a reason for hiding this comment

Uh oh!

julianbrost commented Sep 3, 2021

Uh oh!

nilmerg commented Sep 3, 2021

Uh oh!

julianbrost commented Sep 3, 2021

Uh oh!

nilmerg commented Sep 3, 2021

Uh oh!

xenuser commented Oct 12, 2021

Uh oh!

julianbrost commented Nov 19, 2021

Uh oh!

nilmerg left a comment

Choose a reason for hiding this comment

Uh oh!

NicolasGoeddel commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

julianbrost commented Sep 3, 2021 •

edited

Loading

NicolasGoeddel commented Dec 3, 2021 •

edited

Loading