icinga2
icinga2 copied to clipboard
Icinga
Fix broken `TimePeriod/ScheduledDowntime`s
This PR fixes a broken behaviour of the LegacyTimePeriod::IsInTimeRange() method that affects the overall time period and scheduled downtime handling and frustrates so many users just due to this nasty little logic error.
LegacyTimePeriod::ScriptFunc() calls IsInDayDefinition() with the current time range e.g. (2024-01-29) and a reference time, which is the local time of the Icinga 2 daemon being started, i.e the time being Icinga 2 started at.
https://github.com/Icinga/icinga2/blob/01a6c4c1ce15184f465a7f0fc309bcf6779cdeb3/lib/icinga/legacytimeperiod.cpp#L622
LegacyTimePeriod::IsInDayDefinition() then parses the specified time range "daydef" with the ParseTimeRange() method and passes the arguments begin and end.
https://github.com/Icinga/icinga2/blob/01a6c4c1ce15184f465a7f0fc309bcf6779cdeb3/lib/icinga/legacytimeperiod.cpp#L392-L395
LegacyTimePeriod::ParseTimeRange() then calls ParseTimeSpec(), which normalises the begin and end arguments if they are set, which is true in this case.
https://github.com/Icinga/icinga2/blob/01a6c4c1ce15184f465a7f0fc309bcf6779cdeb3/lib/icinga/legacytimeperiod.cpp#L183-L192
https://github.com/Icinga/icinga2/blob/01a6c4c1ce15184f465a7f0fc309bcf6779cdeb3/lib/icinga/legacytimeperiod.cpp#L194-L202
At this point, the end time is normalised the same way as the start date, but its tm_hour is set to 24 (~which is incorrect anyway, since mtkime(3) wraps around at 23~), and passing it through mtkime(3) produces the exactly the same timestamp as the given reference time, which is erroneously considered to be within range.
~Irrespective of this PR, we should consider changing this. According to the method description, the end date is supposed to be set to 0 o'clock in the following day, instead it is set to 1 o'clock due to the wrapping around.~
mktime(3)
#include <ctime>
#include <iomanip>
#include <iostream>
#include <sstream>
int main()
{
setenv("TZ", "/usr/share/zoneinfo/Europe/Berlin", 1);
std::tm tm{};
tm.tm_year = 2024 - 1900;
tm.tm_mon = 4 - 1;
tm.tm_mday = 4;
tm.tm_hour = 24;
tm.tm_min = 0;
tm.tm_isdst = -1;
std::time_t t = std::mktime(&tm);
std::tm local = *std::localtime(&t);
std::cout << "Is DST -1: " << std::put_time(&local, "%c %Z") << '\n';
tm.tm_isdst = 0;
t = std::mktime(&tm);
local = *std::localtime(&t);
std::cout << "Is DST 0: " << std::put_time(&local, "%c %Z") << '\n';
tm.tm_isdst = 1;
t = std::mktime(&tm);
local = *std::localtime(&t);
std::cout << "Is DST 1: " << std::put_time(&local, "%c %Z") << '\n';
}
-----------------
Is DST -1: Fri Apr 5 00:00:00 2024 CEST
Is DST 0: Fri Apr 5 01:00:00 2024 CEST
Is DST 1: Fri Apr 5 01:00:00 2024 CEST
Tests
Test cases for #9781
object TimePeriod "broken_calendar" {
ranges = {
"2024-01-29 - 2024-01-29" = "07:00-08:00"
}
}
Before:
"is_inside": false,
"name": "broken_calendar",
"prefer_includes": true,
"ranges": {
"2024-01-29 - 2024-01-29": "07:00-08:00"
},
"segments": [
{
"begin": 1706508000,
"end": 1706511600
},
{
"begin": 1706594400,
"end": 1706598000
}
],
---
~/Workspace/icinga2 (master ✗) date -r 1706508000
Mon Jan 29 07:00:00 CET 2024
~/Workspace/icinga2 (master ✗) date -r 1706511600
Mon Jan 29 08:00:00 CET 2024
~/Workspace/icinga2 (master ✗) date -r 1706594400
Tue Jan 30 07:00:00 CET 2024
~/Workspace/icinga2 (master ✗) date -r 1706598000
Tue Jan 30 08:00:00 CET 2024
Even if the time period is not active, it has exceeded the time span and still includes the next day after the end of the range!
After:
"is_inside": false,
"name": "broken_calendar",
"ranges": {
"2024-01-29 - 2024-01-29": "07:00-08:00"
},
"segments": [
{
"begin": 1706508000,
"end": 1706511600
}
],
---
~/Workspace/icinga2 (broken-timeperiod ✗) date -r 1706508000
Mon Jan 29 07:00:00 CET 2024
~/Workspace/icinga2 (broken-timeperiod ✗) date -r 1706511600
Mon Jan 29 08:00:00 CET 2024
Test cases for #9388
object TimePeriod "broken_calendar" {
ranges = {
"2024-01-28" = "00:00-24:00"
}
}
Before:
"is_inside": true,
"name": "broken_calendar",
"ranges": {
"2024-01-28": "00:00-24:00"
},
"segments": [
{
"begin": 1706482800,
"end": 1706569200
}
],
---
~/Workspace/icinga2 (master ✗) date -r 1706482800
Mon Jan 29 00:00:00 CET 2024
~/Workspace/icinga2 (master ✗) date -r 1706569200
Tue Jan 30 00:00:00 CET 2024
There should be no segments at all, as the time period only covers 28 January.
After:
"is_inside": false,
"name": "broken_calendar",
"ranges": {
"2024-01-28": "00:00-24:00"
},
"segments": [],
---
Test cases for #8741 and #7398
object TimePeriod "holidays" {
ranges = {
"january 28" = "00:00-24:00"
}
}
object TimePeriod "broken_calendar" {
excludes = [ "holidays" ]
ranges = {
"monday" = "09:00-17:00"
"tuesday" = "09:00-17:00"
}
}
Before:
"excludes": [
"holidays"
],
"is_inside": false,
"name": "broken_calendar",
"ranges": {
"monday": "09:00-17:00",
"tuesday": "09:00-17:00"
},
"segments": [
{
"begin": 1706601600,
"end": 1706630400
}
],
---
~/Workspace/icinga2 (master ✗) date -r 1706601600
Tue Jan 30 09:00:00 CET 2024
~/Workspace/icinga2 (master ✗) date -r 1706630400
Tue Jan 30 17:00:00 CET 2024
Today (monday) is just getting skipped!
After:
"excludes": [
"holidays"
],
"is_inside": true,
"name": "broken_calendar",
"prefer_includes": true,
"ranges": {
"monday": "09:00-17:00",
"tuesday": "09:00-17:00"
},
"segments": [
{
"begin": 1706515200,
"end": 1706544000
},
{
"begin": 1706601600,
"end": 1706630400
}
],
---
~/Workspace/icinga2 (broken-timeperiod ✗) date -r 1706515200 // First segment start date
Mon Jan 29 09:00:00 CET 2024
~/Workspace/icinga2 (broken-timeperiod ✗) date -r 1706544000 // First segment end date
Mon Jan 29 17:00:00 CET 2024
~/Workspace/icinga2 (broken-timeperiod ✗) date -r 1706601600 // Second segment start date
Tue Jan 30 09:00:00 CET 2024
~/Workspace/icinga2 (broken-timeperiod ✗) date -r 1706630400. // Second segment end date
Tue Jan 30 17:00:00 CET 2024
Test cases for #7239
object TimePeriod "holidays" {
ranges = {
"january 28" = "00:00-24:00"
}
}
object TimePeriod "offDuty" {
ranges = {
"monday" = "00:00-09:00"
}
includes = [ "holidays" ]
}
Before:
---
"includes": [
"holidays"
],
"is_inside": true,
"ranges": {
"monday": "00:00-09:00"
},
"segments": [
{
"begin": 1706482800,
"end": 1706569200
}
],
---
~/Workspace/icinga2 (master ✗) date -r 1706482800
Mon Jan 29 00:00:00 CET 2024
~/Workspace/icinga2 (master ✗) date -r 1706569200
Tue Jan 30 00:00:00 CET 2024
Firstly, the period shouldn't be active and secondly, the end date spans across the entire day.
After:
"includes": [
"holidays"
],
"is_inside": false,
"name": "offDuty",
"ranges": {
"monday": "00:00-09:00"
},
"segments": [
{
"begin": 1706482800,
"end": 1706515200
}
],
---
~/Workspace/icinga2 (broken-timeperiod ✗) date -r 1706482800
Mon Jan 29 00:00:00 CET 2024
~/Workspace/icinga2 (broken-timeperiod ✗) date -r 1706515200
Mon Jan 29 09:00:00 CET 2024
fixes #9781 fixes #9388 fixes #8741 fixes #7398 fixes #7239 fixes also https://community.icinga.com/t/i-get-notifications-for-users-outside-their-timeperiod/13126/3
May also fix #7061 (I haven't verified it, but since both object types use the same code, I would expect this to be fixed as well.)
closes #7855 (There is nothing wrong with the current implementation of the time period inclusion/exclusion, so why should it be changed?)
First of all, I disagree with this one:
closes #7855
That PR even describes what's not optimal with the current implementation of the time period inclusion/exclusion.
It's okay to disagree! As I said before, I don't see any serious problems with the current implementation and it doesn't introduce any known bugs. So why are you willing to change this "working" path with a "better" implementation that might introduce another bug?
Your argument is legit, but I think this is a discussion to be continued in https://github.com/Icinga/icinga2/pull/7855 itself. We don't have to decide whether or not to close that PR neither right now, nor along with this one.
At this point, the end time is normalised the same way as the start date, but its tm_hour is set to 24 (which is incorrect anyway, since
mtkime(3)wraps around at23),
All that timeperiod code relies heavily on the following behavior of mktime() (as stated in the man page you linked): "if structure members are outside their valid interval, they will be normalized (so that, for example, 40 October is changed into 9 November)"
and passing it through
mtkime(3)produces the exactly the same timestamp as the given reference time, which is erroneously considered to be within range.
So that shouldn't be the same time (also, if there reference time may be the startup time, hours/minutes/seconds should not be from the reference time). I'm not sure whether you're implying that begin and end is set to the same time, they should be a day apart.
At this point, the end time is normalised the same way as the start date, but its tm_hour is set to 24 (which is incorrect anyway, since mtkime(3) wraps around at 23),
All that timeperiod code relies heavily on the following behavior of mktime() (as stated in the man page you linked): "if structure members are outside their valid interval, they will be normalized (so that, for example, 40 October is changed into 9 November)"
Ja! NVM!
I'm not sure whether you're implying that begin and end is set to the same time
No, I'm not!
So that shouldn't be the same time (also, if there reference time may be the startup time, hours/minutes/seconds should not be from the reference time)
Yes, they are the same, as the "hours/minutes/seconds" are set to 0 here, i.e. the reference time becomes the same as the actual end date of for example "2024-02-04" = "15:00-16:00", which is "Mon 5 Feb 2024 00:00:00 CET", since the time periods will always be evaluated for the next 24 hours starting from 00 of the current day.
https://github.com/Icinga/icinga2/blob/01a6c4c1ce15184f465a7f0fc309bcf6779cdeb3/lib/icinga/legacytimeperiod.cpp#L589-L595
So that shouldn't be the same time (also, if there reference time may be the startup time, hours/minutes/seconds should not be from the reference time)
Yes, they are the same, as the "hours/minutes/seconds" are set to 0 here, i.e. the reference time becomes the same as the actual end date of for example
"2024-02-04" = "15:00-16:00", which is "Mon 5 Feb 2024 00:00:00 CET", since the time periods will always be evaluated for the next 24 hours starting from 00 of the current day.
I have also updated the PR description and added an example of how mktime(3) wraps around based on the tm_isdst field, then it should be clear why reference and end range do not differ.
Irrespective of this PR, we should consider changing this. According to the method description, the end date is supposed to be set to 0 o'clock in the following day, instead it is set to 1 o'clock due to the wrapping around.
No, it's not. Your test program does not account for mktime() changing it's parameter. Try it again with calling a function for each case that uses a freshly initialized std::tm:
#include <ctime>
#include <iomanip>
#include <iostream>
#include <sstream>
void f(int isdst) {
std::tm tm{};
tm.tm_year = 2024 - 1900;
tm.tm_mon = 4 - 1;
tm.tm_mday = 4;
tm.tm_hour = 24;
tm.tm_min = 0;
tm.tm_isdst = isdst;
std::time_t t = std::mktime(&tm);
std::tm local = *std::localtime(&t);
std::cout << "Is DST " << isdst << ": " << std::put_time(&local, "%c %Z") << '\n';
}
int main() {
setenv("TZ", "/usr/share/zoneinfo/Europe/Berlin", 1);
f(-1);
f(0);
f(1);
}
You'll see the following result instead:
Is DST -1: Fri Apr 5 00:00:00 2024 CEST
Is DST 0: Fri Apr 5 01:00:00 2024 CEST
Is DST 1: Fri Apr 5 00:00:00 2024 CEST
If tm_isdst is set to -1 (asking mktime() to determine whether this is during DST, i.e. whether it's CET or CEST) or 1 (telling mktime() you're passing a time in the DST zone of the location, i.e. CEST), you get the midnight as expected.
Only if you set tm_isdst = 0, i.e. saying you're giving values in CET, the non-DST timezone for Europe/Berlin, that's corrected to CEST and thus moved by an hour.
Irrespective of this PR, we should consider changing this. According to the method description, the end date is supposed to be set to 0 o'clock in the following day, instead it is set to 1 o'clock due to the wrapping around.
No, it's not. Your test program does not account for
mktime()changing it's parameter. Try it again with calling a function for each case that uses a freshly initializedstd::tm:
I have already said it in https://github.com/Icinga/icinga2/pull/9983#issuecomment-1927551080 that this was my misunderstanding and that example program is not supposed to prove this claim of mine, but your objection that the range start and the end date should never be the same.
So that shouldn't be the same time (also, if there reference time may be the startup time, hours/minutes/seconds should not be from the reference time). I'm not sure whether you're implying that begin and end is set to the same time, they should be a day apart.
First of all, I disagree with this one:
closes #7855
I agree insofar that the changes in both PRs are unrelated, so it's not a "which of these two do we want" question. We can decide for or against both independently. So I don't see a reason why the merge of this PR should close the other one.
That PR even describes what's not optimal with the current implementation of the time period inclusion/exclusion.
However, just that I don't want to close #7855 by merging this PR doesn't mean that I want to merge both. The reason for the connection between the two is that initially, you opened it with a claim that it would fix one of the issues now fixed by this PR. It turned out that it didn't fix the issue, so now the question is, why do we still want that PR? To me, the description looks chaotic and doesn't make that clear.
May also fix #7061 (I haven't verified it, but since both object types use the same code, I would expect this to be fixed as well.)
I don't think it will. As I already commented over there, I suspect something else to be the cause of that issue.
At this point, the end time is normalised the same way as the start date, but its tm_hour is set to 24, and passing it through
mtkime(3)produces the exactly the same timestamp as the given reference time, which is erroneously considered to be within range.
I'm still not 100% sure what this sentence is trying to say. Doesn't the problem boil down to the following:
This is about the day definitions, i.e. the left hand side in the ranges definitions, so the exact time of day is irrelevant for the evaluation. However, there are segments for the whole day, i.e. covering the whole day. The code hat inconsistent handling whether the start and end of the segment should be considered as part of the segment and as one place consider the end as part of it. So it would consider the next day at midnight part of the segment and this incorrectly extended the time period to the next day.
As I just had a discussion about this with @yhabteab, here's another attempt at describing the problem with a bit more context and hopefully easier to understand:
If you have a time period with ranges = {"2024-08-09" = "10:00-18:00"}, the following code will parse the day definition ("2024-08-09"):
https://github.com/Icinga/icinga2/blob/2bfa1f1649e4d4f5ea5960b2bafb78169a3432a3/lib/icinga/legacytimeperiod.cpp#L173-L209
This will result in the day definition being expanded to begin = 2024-08-09 00:00:00 and end = 2024-08-10 00:00:00, i.e. the whole day. Note that as there's a fixed day given, that will be the result, no matter for which reference time it is evaluated (the year, month, day variables are extracted from the string input).
Now during the evaluation of the time period for the next day, i.e. 2024-08-10, there's a check whether the day specification is applicable to that day. For this, it checks whether 2024-08-10 00:00:00 is inside the range given by begin and end. If end is treated as inclusive, this wrongly concludes that the day is included in the day definition and adds the time ranges for that day as well. The PR simply changes that end itself is no longer treated as part of the range.
At this point, the end time is normalised the same way as the start date, but its tm_hour is set to 24, and passing it through
mtkime(3)produces the exactly the same timestamp as the given reference time, which is erroneously considered to be within range.
Now I also understand what this is trying to say: end refers to the end of the day parsed from the day definition and reference refers to the beginning of the day the time period is evaluated for.