From 93217de515de5a524dde04475ba8c9db9cbf8e0d Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 26 Oct 2021 13:21:49 +0200 Subject: [PATCH 1/2] LegacyTimePeriod::ScriptFunc: fix DST edge-cases This change fixes two problems: * The internal functions used by ScriptFunc more or less expect to operate on full days, but ScriptFunc may have called them with some random timestamp during the day. This is fixed by always using midnight of the day as reference time. * Previously, the code advanced a timestamp to the next day by adding 24 hours. On days with DST changes, this could either still be on the same day (a day may have 25 hours) or skip an entire day (a day may have 23 hours). This is fixed by using a struct tm to advance the time to the next day. --- lib/icinga/legacytimeperiod.cpp | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/lib/icinga/legacytimeperiod.cpp b/lib/icinga/legacytimeperiod.cpp index d8355110f..33e666544 100644 --- a/lib/icinga/legacytimeperiod.cpp +++ b/lib/icinga/legacytimeperiod.cpp @@ -586,13 +586,35 @@ Array::Ptr LegacyTimePeriod::ScriptFunc(const TimePeriod::Ptr& tp, double begin, Dictionary::Ptr ranges = tp->GetRanges(); if (ranges) { - for (int i = 0; i <= (end - begin) / (24 * 60 * 60); i++) { - time_t refts = begin + i * 24 * 60 * 60; - tm reference = Utility::LocalTime(refts); + 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; + + // Helper to move a struct tm to midnight of the next day for the loop below. + // Due to DST changes, this may move the time by something else than 24 hours. + auto advance_to_next_day = [](tm *t) { + t->tm_mday++; + t->tm_hour = 0; + t->tm_min = 0; + t->tm_sec = 0; + t->tm_isdst = -1; + + // Normalize fields using mktime. + mktime(t); + + // Reset tm_isdst so that future calls figure out the correct time zone after setting tm_hour/tm_min/tm_sec. + t->tm_isdst = -1; + }; + + for (tm reference = tm_begin; mktime_const(&reference) <= end; advance_to_next_day(&reference)) { #ifdef I2_DEBUG Log(LogDebug, "LegacyTimePeriod") - << "Checking reference time " << refts; + << "Checking reference time " << mktime_const(&reference); #endif /* I2_DEBUG */ ObjectLock olock(ranges); From 26d78231dd718d0ac7fae66b7a7fc57d0288fb9c Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Tue, 26 Oct 2021 10:28:07 +0200 Subject: [PATCH 2/2] Add tests for LegacyTimePeriod::ScriptFunc when used by TimePeriod::IsInside --- test/CMakeLists.txt | 1 + test/icinga-legacytimeperiod.cpp | 71 +++++++++++++++++++++++++++----- 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index c1087448d..3c8c2bbaf 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -136,6 +136,7 @@ add_boost_test(base icinga_legacytimeperiod/simple icinga_legacytimeperiod/advanced icinga_legacytimeperiod/dst + icinga_legacytimeperiod/dst_isinside icinga_perfdata/empty icinga_perfdata/simple icinga_perfdata/quotes diff --git a/test/icinga-legacytimeperiod.cpp b/test/icinga-legacytimeperiod.cpp index dc7a139fe..e1150be57 100644 --- a/test/icinga-legacytimeperiod.cpp +++ b/test/icinga-legacytimeperiod.cpp @@ -49,6 +49,18 @@ struct GlobalTimezoneFixture BOOST_GLOBAL_FIXTURE(GlobalTimezoneFixture); +// DST changes in America/Los_Angeles: +// 2021-03-14: 01:59:59 PST (UTC-8) -> 03:00:00 PDT (UTC-7) +// 2021-11-07: 01:59:59 PDT (UTC-7) -> 01:00:00 PST (UTC-8) +#ifndef _WIN32 +static const char *dst_test_timezone = "America/Los_Angeles"; +#else /* _WIN32 */ +// Tests are using pacific time because Windows only really supports timezones following US DST rules with the TZ +// environment variable. Format is "[Standard TZ][negative UTC offset][DST TZ]". +// https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/tzset?view=msvc-160#remarks +static const char *dst_test_timezone = "PST8PDT"; +#endif /* _WIN32 */ + BOOST_AUTO_TEST_CASE(simple) { tm tm_beg, tm_end, tm_ref; @@ -356,17 +368,7 @@ std::ostream& operator<<(std::ostream& o, const boost::optional& s) BOOST_AUTO_TEST_CASE(dst) { - // DST changes in America/Los_Angeles: - // 2021-03-14: 01:59:59 PST (UTC-8) -> 03:00:00 PDT (UTC-7) - // 2021-11-07: 01:59:59 PDT (UTC-7) -> 01:00:00 PST (UTC-8) -#ifndef _WIN32 - GlobalTimezoneFixture tz("America/Los_Angeles"); -#else /* _WIN32 */ - // Tests are using pacific time because Windows only really supports timezones following US DST rules with the TZ - // environment variable. Format is "[Standard TZ][negative UTC offset][DST TZ]". - // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/tzset?view=msvc-160#remarks - GlobalTimezoneFixture tz("PST8PDT"); -#endif /* _WIN32 */ + GlobalTimezoneFixture tz(dst_test_timezone); // Self-tests for helper functions BOOST_CHECK_EQUAL(make_tm("2021-11-07 02:30:00").tm_isdst, -1); @@ -642,4 +644,51 @@ BOOST_AUTO_TEST_CASE(dst) } } +// This tests checks that TimePeriod::IsInside() always returns true for a 24x7 period, even around DST changes. +BOOST_AUTO_TEST_CASE(dst_isinside) +{ + GlobalTimezoneFixture tz(dst_test_timezone); + + Function::Ptr update = new Function("LegacyTimePeriod", LegacyTimePeriod::ScriptFunc, {"tp", "begin", "end"}); + Dictionary::Ptr ranges = new Dictionary({ + {"monday", "00:00-24:00"}, + {"tuesday", "00:00-24:00"}, + {"wednesday", "00:00-24:00"}, + {"thursday", "00:00-24:00"}, + {"friday", "00:00-24:00"}, + {"saturday", "00:00-24:00"}, + {"sunday", "00:00-24:00"}, + }); + + // Vary begin from Sat 06 Nov 2021 00:00:00 PDT to Mon 08 Nov 2021 00:00:00 PST in 30 minute intervals. + for (time_t t_begin = 1636182000; t_begin <= 1636358400; t_begin += 30*60) { + // Test varying interval lengths: 60 minutes, 24 hours, 48 hours. + for (time_t len : {60*60, 24*60*60, 48*60*60}) { + time_t t_end = t_begin + len; + + TimePeriod::Ptr p = new TimePeriod(); + p->SetUpdate(update, true); + p->SetRanges(ranges, true); + + p->UpdateRegion(double(t_begin), double(t_end), true); + + { + // Print resulting segments for easier debugging. + Array::Ptr segments = p->GetSegments(); + ObjectLock lock(segments); + for (Dictionary::Ptr segment: segments) { + BOOST_TEST_MESSAGE("t_begin=" << t_begin << " t_end=" << t_end + << " segment.begin=" << segment->Get("begin") << " segment.end=" << segment->Get("end")); + } + } + + time_t step = 10*60; + for (time_t t = t_begin+step; t < t_end; t += step) { + BOOST_CHECK_MESSAGE(p->IsInside(double(t)), + t << " should be inside for t_begin=" << t_begin << " t_end=" << t_end); + } + } + } +} + BOOST_AUTO_TEST_SUITE_END()