autoupdate rollout: honour the maintenance window duration #50745
Conversation
lib/autoupdate/rollout/strategy.go
Outdated
| windowStart := now.Truncate(24 * time.Hour).Add(time.Duration(group.ConfigStartHour) * time.Hour) | ||
| windowEnd := windowStart.Add(duration) | ||
|
|
||
| return now.After(windowStart) && now.Before(windowEnd), nil |
There was a problem hiding this comment.
The leading edge of the window is no longer in the window. Does that matter?
There was a problem hiding this comment.
Nor is the actual start? +1 to Stephen's question.
I suggest adding a comment saying what the intended interval is. For example, this seems to read like "(windowStart, windowEnd)" vs "[windowStart, windowEnd]".
There was a problem hiding this comment.
Good point, I think the opposite would be more natural: with [start, end[ we can set timeoverride to start to start the maintenance and end to end the maintenance
There was a problem hiding this comment.
(Personally I tend to go for "[start,end)", as it's easy to chain sequences without overlap, but no strong opinions in this particular case.)
There was a problem hiding this comment.
TIL about the [a,b) open interval notation. French maths use [a,b[ to avoid the (a,b) ambiguity (open interval or pair).
hugoShaka
added
the
no-changelog
lib/autoupdate/rollout/strategy.go
Outdated
| windowStart := now.Truncate(24 * time.Hour).Add(time.Duration(group.ConfigStartHour) * time.Hour) | ||
| windowEnd := windowStart.Add(duration) | ||
|
|
||
| return now.After(windowStart) && now.Before(windowEnd), nil |
There was a problem hiding this comment.
Nor is the actual start? +1 to Stephen's question.
I suggest adding a comment saying what the intended interval is. For example, this seems to read like "(windowStart, windowEnd)" vs "[windowStart, windowEnd]".
| @@ -500,7 +500,7 @@ func Test_progressGroupsHaltOnError(t *testing.T) { | |||
| State: 0, | |||
| StartTime: tt.rolloutStartTime, | |||
| } | |||
| err := strategy.progressRollout(ctx, status, clock.Now()) | |||
| err := strategy.progressRollout(ctx, nil, status, clock.Now()) | |||
There was a problem hiding this comment.
Is calling progressRollout with a nil spec expected to happen in practice? Should this error?
There was a problem hiding this comment.
There's a single "real" call to this interface and it always sets the spec. Strategies can expect the spec to be always set.
The nil spec in the test is me taking a shortcut because I know that this specific strategy doesn't rely on the spec.
hugoShaka
force-pushed
the
hugo/strict-timebased-rollout
branch
from
6e81555 to
9a07a98
Compare
This PR copies the maintenance duration from the config into the rollout spec. Changing the maintenance duration does not reset the rollout.
The duration from the rollout is now used to estimate if the current time is inside the maintenance window (before we were trucating by the minute, so the maintenance duration was always 1 hour.
Part of: RFD-184
Goal (internal): https://github.com/gravitational/cloud/issues/10289