-
Notifications
You must be signed in to change notification settings - Fork 923
feat: implement scheduling mechanism for prebuilds #18126
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement scheduling mechanism for prebuilds #18126
Conversation
606894f
to
ff0e813
Compare
9af5e02
to
bcfbb04
Compare
3a25178
to
e0d1de7
Compare
80e52ee
to
47cb6fc
Compare
@@ -0,0 +1,12 @@ | |||
-- Add autoscaling_timezone column to template_version_presets table | |||
ALTER TABLE template_version_presets | |||
ADD COLUMN autoscaling_timezone TEXT DEFAULT 'UTC' NOT NULL; |
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.
Why do we need a default here, if the provider is not defining a default?
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.
It's not null
, so it requires default. You're suggesting DEFAULT ''
?
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.
it was defensive mechanism to make sure we don't fail here on time.LoadLocation(p.Preset.SchedulingTimezone)
:
func (p PresetSnapshot) CalculateDesiredInstances(at time.Time) int32 {
if len(p.PrebuildSchedules) == 0 {
// If no schedules are defined, fall back to the default desired instance count
return p.Preset.DesiredInstances.Int32
}
// Validate that the provided timezone is valid
_, err := time.LoadLocation(p.Preset.SchedulingTimezone)
if err != nil {
p.logger.Error(context.Background(), "invalid timezone in prebuild scheduling configuration",
slog.F("preset_id", p.Preset.ID),
slog.F("timezone", p.Preset.SchedulingTimezone),
slog.Error(err))
// If timezone is invalid, fall back to the default desired instance count
return p.Preset.DesiredInstances.Int32
}
}
It shouldn't happen, because if len(p.PrebuildSchedules) > 0
- SchedulingTimezone
should be set and valid according to validation rules in tf-provider-coder
. Even if this happens we should return default - so we should be safe.
But I added DEFAULT 'UTC'
as another layer of defense.
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.
Final decision: changed to ''
instead of UTC
coderd/database/migrations/000335_add_autoscaling_to_presets.up.sql
Outdated
Show resolved
Hide resolved
The "continuous time" interpretation of the crontab makes it, I guess, relatively simple to specify spans that start and end on the hour, but consider attempting to program a span like 7:45am - 9:10am. You'd need 3 spans
Are we just kind of assuming that operators likely only want hourly precision? |
Co-authored-by: Danny Kopping <danny@coder.com>
…toscaling-mechanism
Co-authored-by: Danny Kopping <danny@coder.com>
Minutes must always be If we allow specific minute ranges (e.g., Therefore, the minimum supported granularity is one hour. There was a long discussion, and majority voted for this approach. Alternative approaches are described here: https://www.notion.so/coderhq/Implement-autoscaling-mechanism-201d579be5928054837dceb8358bfed3?source=copy_link#207d579be5928059b5ded1778841fba2 |
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.
Excellent work @evgeniy-scherbina!
I suspect coder/terraform-provider-coder#408 might prompt a naming change, but I'm happy to approve as-is.
RunningPrebuilds []database.GetRunningPrebuiltWorkspacesRow | ||
PrebuildsInProgress []database.CountInProgressPrebuildsRow | ||
Backoffs []database.GetPresetsBackoffRow | ||
HardLimitedPresetsMap map[uuid.UUID]database.GetPresetsAtFailureLimitRow | ||
clock quartz.Clock |
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.
I don't mind either approach, but let's pick one 👍
…toscaling-mechanism
Closes coder/internal#312
Depends on coder/terraform-provider-coder#408
This PR adds support for defining an autoscaling block for prebuilds, allowing number of desired instances to scale dynamically based on a schedule.
Example usage:
Behavior
schedule
blocks perprebuilds
block are supported.prebuilds.instances
) is used as a fallback.Why
This feature allows prebuild instance capacity to adapt to predictable usage patterns, such as:
Cron specification
The cron specification is interpreted as a continuous time range.
For example, the expression:
is intended to represent a continuous range from 09:00 to 18:59, Monday through Friday.
However, due to minor implementation imprecision, it is currently interpreted as a range from 08:59:00 to 18:58:59, Monday through Friday.
This slight discrepancy arises because the evaluation is based on whether a specific point in time falls within the range, using the
github.com/coder/coder/v2/coderd/schedule/cron
library, which performs per-minute matching rather than strict range evaluation.