Add support for expiring acknowledgements #391

roelvanmeer · 2022-08-16T12:03:18Z

This adds support for acknowledgements that can expire.

I've split everything up in small commits. Please let me know if you'd like to have this differently, or if any improvements can be made. For one thing, I was unsure about the duplicate declaration of the handle_(host|service)_acknowledgement_expire_event functions. It might be better to put these in a header file.

I'm currently testing this out on our systems, but I'd like some preliminary feedback if possible.

Closes #302

Parts of this code were based on work done in Icinga v1 by Ricardo Bartels for Icinga/icinga-core#369. Closes naemon#302. Signed-off-by: Roel van Meer <[email protected]>

Signed-off-by: Roel van Meer <[email protected]>

sni · 2022-08-16T13:55:07Z

very nice. I'll try this out here as well.

jacobbaungard

Thanks a lot for the contribution, and apologies I haven't been able to review this sooner. This is a nice feature. I have code-reviewed and I think it looks really good. I added one minor comment.

Also it would be nice if it is possible to add a test where the expiration of the acknowledgement happens. Create an acknowledgement which expires shortly, check the acknowledgement is there, sleep a a little while, check the acknowledgement has been correctly expired.

@sni did you manage to do some testing of this?

src/naemon/xrddefault.c

roelvanmeer · 2022-09-06T09:57:54Z

Also it would be nice if it is possible to add a test where the expiration of the acknowledgement happens. Create an acknowledgement which expires shortly, check the acknowledgement is there, sleep a a little while, check the acknowledgement has been correctly expired.

I would like to do this, but I wouldn't know how. I also haven't found any tests that checked downtime expiration, so I didn't find anything to base this on. Perhaps someone else can step in here, provide a basis for checking the result of events, or point me to test code that already does this?

Another way may be to test this the light way: schedule an ack that expires in two seconds, wait three, run the expire event manually, and check the results?

sni · 2022-09-06T11:01:09Z

@sni did you manage to do some testing of this?

not yet, i am still on vacation. Will do that next week.

jacobbaungard · 2022-09-06T11:34:00Z

Another way may be to test this the light way: schedule an ack that expires in two seconds, wait three, run the expire event manually, and check the results?

That sounds like a good way forward.

roelvanmeer

This is just a stripped down copy of the test_commands.c file. The test works, but the way the event queue is emptied is not very robust now. If this looks promising, I would like some advice on how to progress.
Ideas: create a test function that empties the event queue, and run that before the test, so after we run the external command we should have to poll the event queue only once. If we do it that way, these tests can also be done in test_commands again, now that I think about it.

roelvanmeer · 2022-09-06T20:40:05Z

Update: function declaration has been de-duplicated, and the test has been added. I'd love to hear what you think.

jacobbaungard

Great, thanks for adding the additional test. I think the code looks good, except for two small indentation issues, and another very minor comment.

If everything works fine following some manual testing, I think this should be fine to merge. I might be able to find some time late in the week to do so, otherwise perhaps sni can do it next week.

Thanks again for the contribution!

src/naemon/nebstructs.h

src/naemon/objects_host.h

jacobbaungard · 2022-09-07T08:03:49Z

t-tap/test_commands.c

+	clear_event_queue();
+	target_host->current_state = STATE_DOWN;
+	time(&current_time);
+	nm_asprintf(&cmdstr, "[1234567890] ACKNOWLEDGE_HOST_PROBLEM_EXPIRE;host1;2;0;0;%llu;myself;expire in two seconds", (unsigned long long int)current_time+2);


Perhaps 1234567890 -> current_time now that we anyway have it.

This isn't done in any of the other tests, but I did update the commit and removed the current_time variable; it's not used in other tests either.

Signed-off-by: Roel van Meer <[email protected]>

If neamon starts and the retention data contains expiring acks that still need to happen, ensure the expire events are properly scheduled. Signed-off-by: Roel van Meer <[email protected]>

Signed-off-by: Roel van Meer <[email protected]>

In tests, we want to be able to remove all events but not destroy the queue.

After destroying the check events, references to those events must be cleared. If not, scheduling a new check event will try to destroy the event that is still referenced, which causes a segfault.

This adds a test that checks if the acknowledgement expiry event does its work. It starts by removing all events from the event queue. We then run the command, with expiration set in two seconds. After waiting three seconds, we process a single event (we should have only one event in the queue at this point). After processing the event, the acknowledgement should be gone.

sni

looks good, thanks a lot

fix number of tests

sni · 2022-09-15T09:06:31Z

i had to rebase and push it manually. But it's merged now. Thanks a lot for this nice feature.

roelvanmeer added 4 commits August 16, 2022 12:55

Add support for expiring acknowledgements naemon#302

188655c

Parts of this code were based on work done in Icinga v1 by Ricardo Bartels for Icinga/icinga-core#369. Closes naemon#302. Signed-off-by: Roel van Meer <[email protected]>

Add command definitions for expiring acknowledgements

e60e53b

Signed-off-by: Roel van Meer <[email protected]>

Add end_time argument to acknowledge_*_problem functions

b890f51

Signed-off-by: Roel van Meer <[email protected]>

Register the new commands

efbcba1

Signed-off-by: Roel van Meer <[email protected]>

jacobbaungard reviewed Sep 6, 2022

View reviewed changes

src/naemon/xrddefault.c Outdated Show resolved Hide resolved

roelvanmeer commented Sep 6, 2022

View reviewed changes

roelvanmeer force-pushed the expire-acks branch from 7dc5bbf to 6c26ca5 Compare September 6, 2022 20:37

jacobbaungard reviewed Sep 7, 2022

View reviewed changes

roelvanmeer added 13 commits September 7, 2022 11:24

Support end_time in the broker

c0d7d56

Signed-off-by: Roel van Meer <[email protected]>

Set acknowledgement_end_time in host and service structs

7d6af7d

Signed-off-by: Roel van Meer <[email protected]>

Ensure host and service comments know about expiry

079b85b

Signed-off-by: Roel van Meer <[email protected]>

Ensure acknowledgement_end_time is set (or cleared)

ebfe3a6

Signed-off-by: Roel van Meer <[email protected]>

Read and write acknowledgement_end_time in retention data

c32e9a9

Signed-off-by: Roel van Meer <[email protected]>

Create and handle host ack expire event

8b18dc5

Signed-off-by: Roel van Meer <[email protected]>

Reschedule events when starting

72b07a8

If neamon starts and the retention data contains expiring acks that still need to happen, ensure the expire events are properly scheduled. Signed-off-by: Roel van Meer <[email protected]>

Add tests for ACKNOWLEDGE_HOST_PROBLEM_EXPIRE

6881dfa

Signed-off-by: Roel van Meer <[email protected]>

Don't declare functions twice

fad6c90

Separate clearing and destroying event queue

ac16d0f

In tests, we want to be able to remove all events but not destroy the queue.

Properly remove references to events when destroying

739690a

After destroying the check events, references to those events must be cleared. If not, scheduling a new check event will try to destroy the event that is still referenced, which causes a segfault.

Allow event processing when testing commands

2d2e3d1

roelvanmeer force-pushed the expire-acks branch from 6c26ca5 to 9e0bb48 Compare September 7, 2022 09:25

sni approved these changes Sep 14, 2022

View reviewed changes

sni and others added 2 commits September 14, 2022 11:22

Merge branch 'master' into expire-acks

2512b35

Update test_commands.c

4e327e3

fix number of tests

sni closed this Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for expiring acknowledgements #391

Add support for expiring acknowledgements #391

roelvanmeer commented Aug 16, 2022

sni commented Aug 16, 2022

jacobbaungard left a comment •

edited

Loading

roelvanmeer commented Sep 6, 2022

sni commented Sep 6, 2022

jacobbaungard commented Sep 6, 2022

roelvanmeer left a comment

roelvanmeer commented Sep 6, 2022

jacobbaungard left a comment

jacobbaungard Sep 7, 2022

roelvanmeer Sep 7, 2022

sni left a comment

sni commented Sep 15, 2022

Add support for expiring acknowledgements #391

Add support for expiring acknowledgements #391

Conversation

roelvanmeer commented Aug 16, 2022

sni commented Aug 16, 2022

jacobbaungard left a comment • edited Loading

Choose a reason for hiding this comment

roelvanmeer commented Sep 6, 2022

sni commented Sep 6, 2022

jacobbaungard commented Sep 6, 2022

roelvanmeer left a comment

Choose a reason for hiding this comment

roelvanmeer commented Sep 6, 2022

jacobbaungard left a comment

Choose a reason for hiding this comment

jacobbaungard Sep 7, 2022

Choose a reason for hiding this comment

roelvanmeer Sep 7, 2022

Choose a reason for hiding this comment

sni left a comment

Choose a reason for hiding this comment

sni commented Sep 15, 2022

jacobbaungard left a comment •

edited

Loading