Fix broken TimePeriod/ScheduledDowntimes

[yhabteab]

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.

icinga2/lib/icinga/legacytimeperiod.cpp

Line 622 in 01a6c4c

if (!IsInDayDefinition(kv.first, &reference)) {

LegacyTimePeriod::IsInDayDefinition() then parses the specified time range "daydef" with the ParseTimeRange() method and passes the arguments begin and end.

icinga2/lib/icinga/legacytimeperiod.cpp

Lines 392 to 395 in 01a6c4c

	tm begin, end;
	int stride;
	ParseTimeRange(daydef, &begin, &end, &stride, reference);

LegacyTimePeriod::ParseTimeRange() then calls ParseTimeSpec(), which normalises the begin and end arguments if they are set, which is true in this case.

icinga2/lib/icinga/legacytimeperiod.cpp

Lines 183 to 192 in 01a6c4c

	if (begin) {
	*begin = *reference;
	begin->tm_year = year - 1900;
	begin->tm_mon = month - 1;
	begin->tm_mday = day;
	begin->tm_hour = 0;
	begin->tm_min = 0;
	begin->tm_sec = 0;
	begin->tm_isdst = -1;
	}

icinga2/lib/icinga/legacytimeperiod.cpp

Lines 194 to 202 in 01a6c4c

	if (end) {
	*end = *reference;
	end->tm_year = year - 1900;
	end->tm_mon = month - 1;
	end->tm_mday = day;
	end->tm_hour = 24;
	end->tm_min = 0;
	end->tm_sec = 0;
	end->tm_isdst = -1;

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?)

[Al2Klimov]

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.

[yhabteab]

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?

[Al2Klimov]

Your argument is legit, but I think this is a discussion to be continued in #7855 itself. We don't have to decide whether or not to close that PR neither right now, nor along with this one.

[julianbrost]

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)"

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.

[yhabteab]

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.

icinga2/lib/icinga/legacytimeperiod.cpp

Lines 589 to 595 in 01a6c4c

	tm tm_begin = Utility::LocalTime(begin);
	// Always evaluate time periods for full days as their ranges are given per day.
	tm_begin.tm_hour = 0;
	tm_begin.tm_min = 0;
	tm_begin.tm_sec = 0;
	tm_begin.tm_isdst = -1;

[yhabteab]

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.