feat: add user quiet hours schedule and restart requirement feature flag #8115
Conversation
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
coderd/schedule/autostop.go
Outdated
| return autostop, xerrors.New("coder server system clock is incorrect, cannot calculate template restart requirement") | ||
| } | ||
| since := startOfStopDay.Sub(epoch) | ||
| weeksSinceEpoch := int64(since.Hours() / (24 * 7)) |
There was a problem hiding this comment.
This algorithm might be too naive??? More research/testing required to ensure this generates correct dates
There was a problem hiding this comment.
Redid the algorithm completely since this algorithm fails if DST has passed once already during the year. The new algorithm is good in a bunch of timezones for the next 100 years (and this is tested).
coderd/database/migrations/000138_user_quiet_hours_schedule.up.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I have not manually tested this, but the time stuff all looks good 👍
The bitmap feels unnecessary at first, since we always convert it into and out of a list. But it does give us validation that it uses each day at most once, all days are in order, and there is a maximum size to the list. I think LoC wise it's more to use the bitmap.
Example is where we use it here:
coder/coderd/schedule/autostop.go
Line 157 in 87b065b
I have not looked at the schedule package much at all, so there are some things I am not sure about.
This has Sunday as the first day. Is this ok?
Line 190 in 87b065b
coderd/schedule/autostop.go
Outdated
| // Loop until we find a week that doesn't fail. | ||
| var lastErr error | ||
| for i := int64(0); i < 3; i++ { | ||
| monday, err := GetMondayOfWeek(now.Location(), week+i) | ||
| if err != nil { | ||
| lastErr = err | ||
| continue | ||
| } | ||
|
|
||
| return monday, nil | ||
| } |
There was a problem hiding this comment.
Do you have any example of why this would fail?
There was a problem hiding this comment.
To be honest, I'm not sure how this could fail, but the GetMondayOfWeek algorithm just adds 7 * n days to the epoch which seems kinda naive to me. This is tested though for the next 100 years in all of those timezones so I think it's stable, I'll add a comment saying this is probably not needed but here just in case.
There was a problem hiding this comment.
Perfect. Was just making sure I wasn't missing something.
|
You're probably right about the bitmap thing being more LoC. I'll see about making it a string array and validating it in the DB. |
|
Personally I like the bitmap for storing in the DB, it makes the Postgres side pretty simple without the need for arrays. It felt overkill at first, but I think it's a suitable tradeoff. |
It's probably not worth changing. I could go either way in it. |
Yeah we're not using it in this code, that's used for the autostart scheduling. |
|
✔️ Deployed PR 8115 successfully. |
|
I just deployed this, this doesn't include the restart requirement in the Also, thoughts on another name instead of "restart requirement" since the action will be a stop, but the user could intervene and bump or restart if they wish to keep working?
Perhaps "restart requirement" could work, but didn't feel natural to me. I tried documenting it and couldn't find a natural way to explain it or show it in the UI. Feel free to make the final call here @deansheather |
In the interest of not making this PR wait for too much longer I'm going to merge it with the types and field names as they are with I dislike "max session length" as you do not set a duration with this new method. If you were setting a duration in hours like the old max_ttl then it would make sense. Auto-stop schedule doesn't convey that it's a required stop and the workspace can't stay on after this point. Auto-stop requirement could work though. |
|
I like auto-stop requirement 👍 |
Adds feature flag
template_restart_requirementwhich changesmax_deadlineto be calculated using a new "restart requirement" setting on the template, rendering themax_ttlfield unused.Adds a
quiet_hours_schedulefield to the users table which stores a daily cron expression e.g.CRON_TZ=Australia/Sydney 2 0 * * *(every day at 2am Sydney time).Adds
restart_requirement_days_of_weekandrestart_requirement_weeksto templates table. The first value is a uint8 for all 7 days of the week (and an unused bit) governing which days of the week the policy should require restarts on (in the user's quiet hours schedule and timezone). The second value says how many weeks between required restarts. 0-1 means weekly, 2 means fortnightly etc.The week calculations are done as an offset from an "epoch" date, so every workspace from every customer will have the same n-week calculations for consistency.
Admins must configure the default schedule that applies to users who haven't configured a custom schedule yet, and can also configure the window duration. Users can set a custom schedule in any timezone. The cron expression must be a daily expression with a single time each day.
TODO:
Closes #6917