Skip to content

Icinga DB: Add downtime.duration & service_state.host_id to Redis #9061

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 11, 2021
Merged

Icinga DB: Add downtime.duration & service_state.host_id to Redis #9061

merged 2 commits into from
Nov 11, 2021

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Nov 3, 2021 •
edited
Loading

resolves #9049

Corresponding Icinga DB PR: Icinga/icingadb#403
Corresponding Icinga DB Web PR(s): Icinga/icingadb-web#405, Icinga/icingadb-web#421

@cla-bot cla-bot bot added the cla/signed label Nov 3, 2021
@julianbrost julianbrost mentioned this pull request Nov 4, 2021
Merged
@yhabteab yhabteab force-pushed the add-downtime-duration-and-service-state-host-id-streams branch from b73b43f to b9abfdb Compare November 4, 2021 09:58
@yhabteab yhabteab requested a review from Al2Klimov November 4, 2021 10:25
@yhabteab yhabteab force-pushed the add-downtime-duration-and-service-state-host-id-streams branch from b9abfdb to 73ce897 Compare November 4, 2021 13:42
@yhabteab yhabteab requested review from julianbrost and removed request for julianbrost and Al2Klimov November 4, 2021 13:43
@yhabteab yhabteab force-pushed the add-downtime-duration-and-service-state-host-id-streams branch 2 times, most recently from c507b96 to 3b348de Compare November 4, 2021 14:43
@yhabteab yhabteab requested a review from julianbrost November 4, 2021 14:43
julianbrost
julianbrost reviewed Nov 5, 2021
View reviewed changes
lib/icingadb/icingadb-objects.cpp Outdated
Comment on lines 1342 to 1347
auto duration = downtime->GetDuration();
if (downtime->GetFixed() || ! downtime->IsInEffect()) {
duration = downtime->GetEndTime() - downtime->GetStartTime();
}
attributes->Set("duration", TimestampToMilliseconds(duration));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain why this is set this way, in particular why a flexible downtime should use the scheduled duration when not in effect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed this with Yonas yesterday. We have decided on the scheduled duration here, as the flexible downtime actually ends after this duration if it is not / never in effect. This duration will of course need to be updated when it comes into effect. I don't know if that's part of this PR (as it should).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we go back a step? #9049 says that you want to sort by this column. Is this just for when a user decides to sort downtimes by duration, or is this needed for some specific view? Maybe it helps to know what this will be used for in the end.

Anyways, so if you have a flexible 1h downtime in a 3h window, you want to day that the downtime has a duration of 1h while it's active, but 3h before and after that? This sounds strange to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duration refers to the actual downtime duration, i.e for fixed downtimes duration is always scheduled_end - scheduled_start, but for flexible downtime it is slightly different. If a flexible downtime is not in effect, then the actual duration is still scheduled_end - scheduled_start, but if the downtime is triggered, then the duration is the same as flexible_duration which you specified when creating the downtime. And yes this column is used for sorting downtimes by duration and for creating the downtime timeline progress.

Bildschirmfoto 2021-11-05 um 15 54 48

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why should the duration be reset to scheduled_end - scheduled_start after a flexible downtime has ended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is, obviously, wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't get why it would be a problem if the duration was always the flexible_duration for flexible downtimes. I'd still say it's a 1 hour downtime in a 3 hour window all the time.

However, even if you'd want to go for scheduled_duration before it came into effect and flexible_duration afterwards, I don't think you would want to check IsInEffect() here as this tells you if the current time is within the effective period. I think you might want to check if the downtime has triggered at all. Check for that would be downtime->GetTriggerTime() != 0.

@yhabteab yhabteab requested a review from julianbrost November 5, 2021 15:23
julianbrost
julianbrost requested changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the comments below, have you tested that once a flexible downtime triggers, it sends a runtime update for the downtime config object?

lib/icingadb/icingadb-objects.cpp Outdated
Comment on lines 1342 to 1347
auto duration = downtime->GetDuration();
if (downtime->GetFixed() || ! downtime->IsInEffect()) {
duration = downtime->GetEndTime() - downtime->GetStartTime();
}
attributes->Set("duration", TimestampToMilliseconds(duration));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't get why it would be a problem if the duration was always the flexible_duration for flexible downtimes. I'd still say it's a 1 hour downtime in a 3 hour window all the time.

However, even if you'd want to go for scheduled_duration before it came into effect and flexible_duration afterwards, I don't think you would want to check IsInEffect() here as this tells you if the current time is within the effective period. I think you might want to check if the downtime has triggered at all. Check for that would be downtime->GetTriggerTime() != 0.

@yhabteab
Copy link
Member Author

yhabteab commented Nov 5, 2021

Apart from the comments below, have you tested that once a flexible downtime triggers, it sends a runtime update for the downtime config object?

Service is in OK state and downtime is not started yet.

Bildschirmfoto 2021-11-05 um 17 21 27

After submitting process check result: Downtime is triggered automatically.

Bildschirmfoto 2021-11-05 um 17 22 59

@yhabteab yhabteab requested a review from julianbrost November 5, 2021 16:29
@yhabteab
Copy link
Member Author

yhabteab commented Nov 5, 2021

Should the downtime be automatically canceled if the service object recovers before the flexible downtime expires?

@julianbrost
Copy link
Contributor

No, should exist until it ends or is canceled manually.

@yhabteab yhabteab mentioned this pull request Nov 5, 2021
Merged
@yhabteab yhabteab force-pushed the add-downtime-duration-and-service-state-host-id-streams branch from ed164c1 to 75a691c Compare November 5, 2021 18:01
@lippserd lippserd changed the title IcingaDB: Add downtime.duration & service_state.host_id to redis Icinga DB: Add downtime.duration & service_state.host_id to Redis Nov 8, 2021
@lippserd lippserd force-pushed the add-downtime-duration-and-service-state-host-id-streams branch from 75a691c to fe5aa1e Compare November 9, 2021 10:08
@lippserd lippserd requested review from N-o-X and lippserd and removed request for lippserd November 9, 2021 10:09
@lippserd
Copy link
Member

lippserd commented Nov 9, 2021

I removed the new columns from the history streams. These are not required in the history tables.

@N-o-X N-o-X added area/icingadb New backend consider backporting Should be considered for inclusion in a bugfix release labels Nov 9, 2021
@N-o-X N-o-X merged commit c1098be into master Nov 11, 2021
@N-o-X N-o-X mentioned this pull request Nov 11, 2021
Merged
@julianbrost julianbrost mentioned this pull request Nov 12, 2021
Closed
@julianbrost julianbrost added backported Fix was included in a bugfix release and removed consider backporting Should be considered for inclusion in a bugfix release labels Nov 19, 2021
@Al2Klimov Al2Klimov added this to the 2.14.0 milestone Dec 3, 2021
@Al2Klimov Al2Klimov deleted the add-downtime-duration-and-service-state-host-id-streams branch July 26, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lippserd lippserd lippserd left review comments

@nilmerg nilmerg nilmerg left review comments

@julianbrost julianbrost julianbrost approved these changes

@N-o-X N-o-X N-o-X approved these changes

Assignees
No one assigned
Labels
area/icingadb New backend backported Fix was included in a bugfix release cla/signed
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

Icinga DB: Add new mysql column downtime.duration and service_state.host_id
6 participants
@yhabteab @julianbrost @lippserd @N-o-X @nilmerg @Al2Klimov