Skip to content
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

Timeperiods: fix off by one when calculating n-th last weekday of the month #10107

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Timeperiods: fix off by one when calculating n-th last weekday of the month #10107

merged 1 commit into from
Aug 8, 2024

Conversation

julianbrost
Copy link
Contributor

@julianbrost julianbrost commented Aug 2, 2024 •
edited
Loading

A day specification like "monday -1" refers to the last Monday of the month. However, there was an off by one if the first day of the next month is the same day of the week, i.e. a Monday in this example.

LegacyTimePeriod::FindNthWeekday() picks a day to start the search for the day in question. When given a negative n to search for the n-th last day, it wrongly used the first day of the following month as the start and counted it as if it was within the current month. This resulted in a 1/7 chance that the result was one week too late.

This is fixed by using the last day of the current month instead.

fixes #7061

@cla-bot cla-bot bot added the cla/signed label Aug 2, 2024
@icinga-probot icinga-probot bot added area/runtime Downtimes, comments, dependencies, events bug Something isn't working labels Aug 2, 2024
@julianbrost
Copy link
Contributor Author

julianbrost commented Aug 2, 2024 •
edited
Loading

Result of the new test case when reverting the change to lib/icinga/legacytimeperiod.cpp (omitted empty lines and many days in the middle, the same pattern repeats 31 times):

test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=1, n=1] expected: 2019-03-04 00:00:00 , got: 2019-03-04 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=1, n=2] expected: 2019-03-11 00:00:00 , got: 2019-03-11 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=1, n=3] expected: 2019-03-18 00:00:00 , got: 2019-03-18 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=1, n=4] expected: 2019-03-25 00:00:00 , got: 2019-03-25 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-01, wday=1, n=-1] expected: 2019-03-25 00:00:00 , got: 2019-04-01 00:00:00
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-01, wday=1, n=-2] expected: 2019-03-18 00:00:00 , got: 2019-03-25 00:00:00
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-01, wday=1, n=-3] expected: 2019-03-11 00:00:00 , got: 2019-03-18 00:00:00 
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-01, wday=1, n=-4] expected: 2019-03-04 00:00:00 , got: 2019-03-11 00:00:00 
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=1] expected: 2019-03-01 00:00:00 , got: 2019-03-01 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=2] expected: 2019-03-08 00:00:00 , got: 2019-03-08 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=3] expected: 2019-03-15 00:00:00 , got: 2019-03-15 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=4] expected: 2019-03-22 00:00:00 , got: 2019-03-22 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=5] expected: 2019-03-29 00:00:00 , got: 2019-03-29 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=-1] expected: 2019-03-29 00:00:00 , got: 2019-03-29 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=-2] expected: 2019-03-22 00:00:00 , got: 2019-03-22 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=-3] expected: 2019-03-15 00:00:00 , got: 2019-03-15 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=-4] expected: 2019-03-08 00:00:00 , got: 2019-03-08 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-01, wday=5, n=-5] expected: 2019-03-01 00:00:00 , got: 2019-03-01 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=1, n=1] expected: 2019-03-04 00:00:00 , got: 2019-03-04 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=1, n=2] expected: 2019-03-11 00:00:00 , got: 2019-03-11 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=1, n=3] expected: 2019-03-18 00:00:00 , got: 2019-03-18 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=1, n=4] expected: 2019-03-25 00:00:00 , got: 2019-03-25 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-02, wday=1, n=-1] expected: 2019-03-25 00:00:00 , got: 2019-04-01 00:00:00
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-02, wday=1, n=-2] expected: 2019-03-18 00:00:00 , got: 2019-03-25 00:00:00
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-02, wday=1, n=-3] expected: 2019-03-11 00:00:00 , got: 2019-03-18 00:00:00 
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-02, wday=1, n=-4] expected: 2019-03-04 00:00:00 , got: 2019-03-11 00:00:00 
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=1] expected: 2019-03-01 00:00:00 , got: 2019-03-01 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=2] expected: 2019-03-08 00:00:00 , got: 2019-03-08 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=3] expected: 2019-03-15 00:00:00 , got: 2019-03-15 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=4] expected: 2019-03-22 00:00:00 , got: 2019-03-22 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=5] expected: 2019-03-29 00:00:00 , got: 2019-03-29 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=-1] expected: 2019-03-29 00:00:00 , got: 2019-03-29 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=-2] expected: 2019-03-22 00:00:00 , got: 2019-03-22 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=-3] expected: 2019-03-15 00:00:00 , got: 2019-03-15 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=-4] expected: 2019-03-08 00:00:00 , got: 2019-03-08 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-02, wday=5, n=-5] expected: 2019-03-01 00:00:00 , got: 2019-03-01 00:00:00 ' has passed
[...]
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=1, n=1] expected: 2019-03-04 00:00:00 , got: 2019-03-04 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=1, n=2] expected: 2019-03-11 00:00:00 , got: 2019-03-11 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=1, n=3] expected: 2019-03-18 00:00:00 , got: 2019-03-18 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=1, n=4] expected: 2019-03-25 00:00:00 , got: 2019-03-25 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-31, wday=1, n=-1] expected: 2019-03-25 00:00:00 , got: 2019-04-01 00:00:00 
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-31, wday=1, n=-2] expected: 2019-03-18 00:00:00 , got: 2019-03-25 00:00:00 
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-31, wday=1, n=-3] expected: 2019-03-11 00:00:00 , got: 2019-03-18 00:00:00 
test/icinga-legacytimeperiod.cpp(701): error: in "icinga_legacytimeperiod/find_nth_weekday": [ref=2019-03-31, wday=1, n=-4] expected: 2019-03-04 00:00:00 , got: 2019-03-11 00:00:00 
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=1] expected: 2019-03-01 00:00:00 , got: 2019-03-01 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=2] expected: 2019-03-08 00:00:00 , got: 2019-03-08 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=3] expected: 2019-03-15 00:00:00 , got: 2019-03-15 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=4] expected: 2019-03-22 00:00:00 , got: 2019-03-22 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=5] expected: 2019-03-29 00:00:00 , got: 2019-03-29 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=-1] expected: 2019-03-29 00:00:00 , got: 2019-03-29 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=-2] expected: 2019-03-22 00:00:00 , got: 2019-03-22 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=-3] expected: 2019-03-15 00:00:00 , got: 2019-03-15 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=-4] expected: 2019-03-08 00:00:00 , got: 2019-03-08 00:00:00 ' has passed
test/icinga-legacytimeperiod.cpp(701): info: check '[ref=2019-03-31, wday=5, n=-5] expected: 2019-03-01 00:00:00 , got: 2019-03-01 00:00:00 ' has passed

@julianbrost julianbrost force-pushed the timeperiod-nth-day-of-month-off-by-one branch 2 times, most recently from 4f2b840 to 323652e Compare August 6, 2024 09:59
@yhabteab yhabteab added this to the 2.15.0 milestone Aug 7, 2024
@yhabteab yhabteab added the consider backporting Should be considered for inclusion in a bugfix release label Aug 7, 2024
@julianbrost
Timeperiods: fix off by one when calculating n-th last weekday of the…
c45829b 
… month

A day specification like "monday -1" refers to the last Monday of the month.
However, there was an off by one if the first day of the next month is the same
day of the week, i.e. a Monday in this example.

LegacyTimePeriod::FindNthWeekday() picks a day to start the search for the day
in question. When given a negative n to search for the n-th last day, it
wrongly used the first day of the following month as the start and counted it
as if it was within the current month. This resulted in a 1/7 chance that the
result was one week too late.

This is fixed by using the last day of the current month instead.
@julianbrost julianbrost force-pushed the timeperiod-nth-day-of-month-off-by-one branch from 323652e to c45829b Compare August 7, 2024 10:06
yhabteab
yhabteab approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

LFTM!

Al2Klimov
Al2Klimov approved these changes Aug 7, 2024
View reviewed changes
test/icinga-legacytimeperiod.cpp Show resolved Hide resolved
@julianbrost julianbrost merged commit 2bfa1f1 into master Aug 8, 2024
26 checks passed
@julianbrost julianbrost deleted the timeperiod-nth-day-of-month-off-by-one branch August 8, 2024 12:40
This was referenced Aug 21, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yhabteab yhabteab yhabteab approved these changes

@Al2Klimov Al2Klimov Al2Klimov approved these changes

Assignees
No one assigned
Labels
area/runtime Downtimes, comments, dependencies, events bug Something isn't working cla/signed consider backporting Should be considered for inclusion in a bugfix release
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

Recuring Downtime wrong dates specific month
3 participants
@julianbrost @Al2Klimov @yhabteab