- Notifications
You must be signed in to change notification settings - Fork928
chore: cover deadline crossing autostart border#13115
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| //- This crosses the autostart deadline, so the deadline is extended to 9pm | ||
| nextAutostart, ok := NextAutostartSchedule(params.Now, params.WorkspaceAutostart, templateSchedule) | ||
| if ok && autostop.Deadline.After(nextAutostart) { | ||
| autostop.Deadline = nextAutostart.Add(ttl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need to handle themin(deadline, max_deadline) here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, that is handled down below.
coder/coderd/schedule/autostop.go
Line 238 in94872aa
| autostop.Deadline=autostop.MaxDeadline |
I'd rather not duplicate that logic since it also handles the case where MaxDeadline is not set.
| templateAutostopRequirement: schedule.TemplateAutostopRequirement{}, | ||
| userQuietHoursSchedule: "", | ||
| expectedDeadline: time.Date(pastDateNight.Year(), pastDateNight.Month(), pastDateNight.Day()+1, 21, 0, 0, 0, chicago), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could you usepastDateNight.AddDate() here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AddDate cannot add hours. So it would beAddDate(0,0,1).Add(time.Hour*X) But the number of hours is 23.25 hours, which is such a strange amount.
pastDateNight = 9:45pm on WednesdayexpectedDeadline = 9pm Thursday, which is 12hrs + autostart (which is a cron string, not a Gotime.Time).
It was easier for me to reason because the offset being 23.25 hours is not really the assertion I am making. I want to assert it's 12hrs after autostart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fair!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, especially with the new test cases!
Uh oh!
There was an error while loading.Please reload this page.
Closes#12864
This PR adds the second case in this image. If a user starts a workspace, and that TTL crosses the next autostart, the deadline is set to autostart + TTL