Fix wrapped cron day-of-week step ranges matching the wrong days

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/fleeting/taskscaler:jmrp-cron-dow-wrap-step into main

What does this MR do?

Makes a wrapped day-of-week range in the cron package count Sunday once. For a day-of-week range whose start is after its end, expression() now passes 6 instead of the field maximum of 7 to setStep, so the range goes from Saturday straight to Sunday (0) instead of visiting 7 and then 0.

Why was this MR needed?

The day-of-week field accepts 0 to 7 because 7 is another name for Sunday (dowExpr). For a wrapped range, expression() passes that maximum to setStep (parser.go#L179), which counts up to 7 and then restarts at 0 (parser.go#L111-135). Sunday is visited twice, so the week is 8 days long. Without a step that only sets one extra bit, and Contains already matches 0 and 7 as the same Sunday (expression.go#L40-45). With a step, the extra day shifts every stepped day after Sunday one day earlier:

Expression Matches on main Expected
FRI-TUE/2 Fri, Sun, Mon Fri, Sun, Tue
SAT-MON/2 Sat, Sun Sat, Mon
7-3/2 Sun, Mon, Wed Sun, Tue

The hour field already has wrapped ranges with a step under test (20-4/2, 21-3/2, 22-2/5); the day-of-week field is the only one where the maximum and the minimum name the same value. The wrap in setStep came with !94 (merged); before it, at the !93 (merged) merge, these ranges were wrong for the step offset reason !94 (merged) fixed (FRI-TUE/2 matched Sat, Sun, Tue). taskscaler parses each schedule period with cron.Parse (schedule.go#L78), and GitLab Runner passes the periods of each [[runners.autoscaler.policy]] to it (provider.go#L273-275), so a policy with such a period applies its idle count, idle time and scale factor on the wrong days. Runner's Kubernetes executor also parses the periods of each [[runners.kubernetes.autoscaler.policy]] with cron.Parse (policy.go#L43), so its pause pod policies are active on the wrong days too.

Behavior change: a wrapped day-of-week range with a step matches the expected days above. A wrapped range without a step matches the same days as before. One existing expectation changes: TestDaysOfWeek/overflow_range now expects 7-1 as bits 0 and 1 instead of 7, 0 and 1, because a wrapped range that starts at 7 now stores its Sunday as 0. The bitfield is internal to the package and only Contains reads it; the new TestContains case parse_check_wrap_around_dow_range_from_seven checks that 7-1 still matches Sunday and Monday and not Tuesday or Saturday, and it passes on main too. Keeping 7 for a range that starts at 7, so that overflow_range stays as it is, would leave 7-3/2 matching Sun, Mon, Wed. Bounding only ranges with a step would also leave overflow_range as it is and still match the right days, but then a wrapped day-of-week range would be expanded one way or the other depending on its step, so I kept one rule for every wrapped day-of-week range.

What's the best way to test this MR?

go test ./cron/ -count=1 -run 'TestDaysOfWeek|TestContains' -v

The new cases are overflow_range_step, overflow_range_step_from_sat and overflow_range_step_from_seven in TestDaysOfWeek, and the four parse_check_wrap_around_dow_* cases in TestContains (2020-01-03 is a Friday). On main the three stepped cases fail in both tests and overflow_range fails on the bitfield; with this change they all pass.

I ran everything in golang:1.26.7-trixie, the image of the test job. make test passes on this branch and on this branch merged with the current main (the root package takes about 7 minutes), mocks and modules included. go test -race ./cron/, go vet ./... and GOOS=windows go vet ./cron/ pass, and gofmt reports nothing in the lines this MR adds; cron/expression_test.go already has a few comments gofmt would realign, which I left alone. Setting the wrap bound back to 7 or to 5, or keeping 7 for a range that starts at 7, makes the new tests fail; applying the bound to every field fails the existing minute, hour, day-of-month and month cases.

Test output on main

go test ./cron/ -count=1 -run 'TestDaysOfWeek|TestContains' -v on main with this MR's tests, trimmed to the cases it adds or changes:

--- FAIL: TestContains (0.00s)
    --- FAIL: TestContains/parse_check_wrap_around_dow_range_step (0.00s)
    --- FAIL: TestContains/parse_check_wrap_around_dow_range_step_from_sat (0.00s)
    --- FAIL: TestContains/parse_check_wrap_around_dow_range_step_from_seven (0.00s)
    --- PASS: TestContains/parse_check_wrap_around_dow_range_from_seven (0.00s)
    parser_test.go:371: input "7-1": expected bits 0000000000000000000000000000000000000000000000000000000000000011, got 0000000000000000000000000000000000000000000000000000000010000011
    parser_test.go:374: input "FRI-TUE/2": expected bits 0000000000000000000000000000000000000000000000000000000000100101, got 0000000000000000000000000000000000000000000000000000000010100010
    parser_test.go:377: input "SAT-MON/2": expected bits 0000000000000000000000000000000000000000000000000000000001000010, got 0000000000000000000000000000000000000000000000000000000001000001
    parser_test.go:380: input "7-3/2": expected bits 0000000000000000000000000000000000000000000000000000000000000101, got 0000000000000000000000000000000000000000000000000000000010001010
--- FAIL: TestDaysOfWeek (0.00s)
    --- FAIL: TestDaysOfWeek/overflow_range (0.00s)
    --- FAIL: TestDaysOfWeek/overflow_range_step (0.00s)
    --- FAIL: TestDaysOfWeek/overflow_range_step_from_sat (0.00s)
    --- FAIL: TestDaysOfWeek/overflow_range_step_from_seven (0.00s)
FAIL
FAIL	gitlab.com/gitlab-org/fleeting/taskscaler/cron	0.003s

The TestContains failures name the days: FRI-TUE/2 matches 2020-01-06 (Monday) and not 2020-01-07 (Tuesday), SAT-MON/2 matches 2020-01-05 (Sunday) and not 2020-01-06 (Monday), and 7-3/2 matches 2020-01-06 and 2020-01-08 (Monday and Wednesday) and not 2020-01-07 (Tuesday).

make test with this change merged into the current main:

ok  	gitlab.com/gitlab-org/fleeting/taskscaler	422.170s
ok  	gitlab.com/gitlab-org/fleeting/taskscaler/cron	0.009s
ok  	gitlab.com/gitlab-org/fleeting/taskscaler/internal	2.725s
ok  	gitlab.com/gitlab-org/fleeting/taskscaler/internal/capacity	0.004s
ok  	gitlab.com/gitlab-org/fleeting/taskscaler/internal/ratelimit	0.003s
ok  	gitlab.com/gitlab-org/fleeting/taskscaler/storage	0.006s
Mocks are up-to-date!
Go modules are tidy and complete!

Merge request reports