Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: enforce template-level constraints for TTL and autostart#2018

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

Merged

Conversation

johnstcn
Copy link
Member

This PR adds fields to templates that constrain values for workspaces derived from that template.

  • Autostop: Adds a field max_ttl on the template which limits the maximum value of ttl on all workspaces derived from that template. Defaulting to 168 hours, enforced on edits to workspace metadata.
  • Autostart: Adds a field min_autostart_duration which limits the minimum duration between successive autostarts of a template, measured from a single reference time. Defaulting to 1 hour, enforced on edits to workspace metadata.

@johnstcnjohnstcn self-assigned thisJun 3, 2022
@johnstcnjohnstcn linked an issueJun 3, 2022 that may beclosed by this pull request
@johnstcnjohnstcnforce-pushed the1433-template-constraint-for-workspace-scheduling branch fromd4b43ac to709db5bCompareJune 3, 2022 16:14
@johnstcnjohnstcn marked this pull request as ready for reviewJune 3, 2022 16:14
@johnstcnjohnstcn requested a review froma team as acode ownerJune 3, 2022 16:14
@johnstcnjohnstcn requested review fromgreyscaled anda teamJune 3, 2022 16:15
Comment on lines +25 to +26
MaxTTLMillis int64 `json:"max_ttl_ms"`
MinAutostartIntervalMillis int64 `json:"min_autostart_interval_ms"`
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: following convention ofcodersdk types usingint64 millisecond-timestamps for time.Duration

Emyrk and greyscaled reacted with thumbs up emoji
Comment on lines +65 to +72
// MaxTTLMillis allows optionally specifying the maximum allowable TTL
// for all workspaces created from this template.
MaxTTLMillis *int64 `json:"max_ttl_ms,omitempty"`

// MinAutostartIntervalMillis allows optionally specifying the minimum
// allowable duration between autostarts for all workspaces created from
// this template.
MinAutostartIntervalMillis *int64 `json:"min_autostart_interval_ms,omitempty"`
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Making these optional in creation; if preferred I can make them mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is this so you don't have some autostart config that says start my workspace every minute?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yup!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thought: Can we make he zero values useful here? Or can these be set to zero to disable them entirely? If zero doesn't need to disable, we can treat it as not set, avoiding the use of pointers.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

FE prefers to consume null instead of zero-values; hence the pointers.
It would be nice though; I'll open a follow-up PR to investigate this.

Comment on lines 111 to 140
var t0 = time.Date(1970, 1, 1, 1, 1, 1, 0, time.UTC)
var tMax = t0.Add(168 * time.Hour)

// Min returns the minimum duration of the schedule.
// This is calculated as follows:
// - Let t(0) be a given point in time (1970-01-01T01:01:01Z00:00)
// - Let t(max) be 168 hours after t(0).
// - Let t(1) be the next scheduled time after t(0).
// - Let t(n) be the next scheduled time after t(n-1).
// - Then, the minimum duration of s d(min)
// = min( t(n) - t(n-1) ∀ n ∈ N, t(n) < t(max) )
func (s Schedule) Min() time.Duration {
durMin := tMax.Sub(t0)
tPrev := s.Next(t0)
tCurr := s.Next(tPrev)
for {
dur := tCurr.Sub(tPrev)
if dur < durMin {
durMin = dur
}
tmp := tCurr
tCurr = s.Next(tmp)
tPrev = tmp
if tCurr.After(tMax) {
break
}
}
return durMin
}

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: This is the "simplest" way I could think of to determine the minimum schedule interval without introspecting the actual cron object itself and doing some bit munging.

greyscaled reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This makes sense. What is the significance of 168 hrs (1wk)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@Emyrk it's the maximum ttl (system-constraint)

Emyrk reacted with thumbs up emoji
@@ -2,7 +2,6 @@ package executor_test

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: I ended up making a lot of changes in this test because I had been using a simple* * * * * schedule which is lower than the now-enforced default 🙃

greyscaled reacted with thumbs up emoji
Comment on lines 85 to 137
defer close(doneChan)
err := cmd.Execute()
assert.EqualError(t, err, "Invalid workspace autostart timezone: unknown time zone invalid")
}()
<-doneChan
err := cmd.Execute()
assert.ErrorContains(t, err, "schedule: parse schedule: provided bad location invalid: unknown time zone invalid")
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: the parallelism isn't strictly necessary in this test as I don't need to do I/O with the pty, so just leaving it out for simplicity.

Comment on lines -111 to +160
go func() {
defer close(doneChan)
err := cmd.Execute()
assert.EqualError(t, err, "TTL must be at least 1 minute")
}()
<-doneChan
err := cmd.Execute()
assert.EqualError(t, err, "TTL must be at least 1 minute")
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: as above, removing parallelism from this test as it's not strictly necessary.

"my-workspace",
"--template", template.Name,
"--ttl", "12h1m",
"-y", // don't bother with waiting
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: I'm slightly side-stepping here because I don't want to deal with confirms

greyscaled reacted with thumbs up emoji
@@ -262,7 +265,22 @@ func create() *cobra.Command {
cliflag.StringVarP(cmd.Flags(), &autostartMinute, "autostart-minute", "", "CODER_WORKSPACE_AUTOSTART_MINUTE", "0", "Specify the minute(s) at which the workspace should autostart (e.g. 0).")
cliflag.StringVarP(cmd.Flags(), &autostartHour, "autostart-hour", "", "CODER_WORKSPACE_AUTOSTART_HOUR", "9", "Specify the hour(s) at which the workspace should autostart (e.g. 9).")
cliflag.StringVarP(cmd.Flags(), &autostartDow, "autostart-day-of-week", "", "CODER_WORKSPACE_AUTOSTART_DOW", "MON-FRI", "Specify the days(s) on which the workspace should autostart (e.g. MON,TUE,WED,THU,FRI)")
cliflag.StringVarP(cmd.Flags(), &tzName, "tz", "", "TZ", "", "Specify your timezone location for workspace autostart (e.g. US/Central).")
cliflag.StringVarP(cmd.Flags(), &tzName, "tz", "", "TZ", "UTC", "Specify your timezone location for workspace autostart (e.g. US/Central).")
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: defaulting TZ to UTC here explicitly. Will hopefully have some fancier auto-detection in a separate issue.

greyscaled reacted with thumbs up emoji
Comment on lines +118 to +134
schedSpec, err := validSchedule(
autostartMinute,
autostartHour,
autostartDow,
tzName,
time.Duration(template.MinAutostartIntervalMillis)*time.Millisecond,
)
if err != nil {
return xerrors.Errorf("Invalid autostart schedule: %w", err)
}
if ttl < time.Minute {
return xerrors.Errorf("TTL must be at least 1 minute")
}
if ttlMax := time.Duration(template.MaxTTLMillis) * time.Millisecond; ttl > ttlMax {
return xerrors.Errorf("TTL must be below template maximum %s", ttlMax)
}

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: We're duplicating some validations here because I want the errors to show upbefore the whole workspace plan happens.

greyscaled reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Interesting to do it in the cli. Can we somehow put the validation on a datastructure and reuse it for the backend + frontend? Like reuse the same code?

Not a huge deal.

johnstcn reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It gets validated on the BE as well but if we don't at least validate in thecreate command the user ends up seeing the workspace dry run, confirming the create, andthen getting the validation error, which is annoying.

And yes, this is definitely a good candidate to be DRYed up a bit.

@ammario
Copy link
Member

Thinking about this more, would an engineer really want an Auto-Off that is less than the template-configured amount? Since the engineer is not accountable to infrastructure costs like the template admin, I think not. Perhaps you've made themax_ttl already the default TTL for new workspaces, but if not I think it's a good idea as the rare case is choosing a shorter TTL.

greyscaled and johnstcn reacted with thumbs up emoji

Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Other than the one correctness question, I think the code changes look good. 👍🏻

@@ -108,6 +108,36 @@ func (s Schedule) Next(t time.Time) time.Time {
return s.sched.Next(t)
}

var t0 = time.Date(1970, 1, 1, 1, 1, 1, 0, time.UTC)
var tMax = t0.Add(168 * time.Hour)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would this work correctly (or as expected) if e.g. max TTL is set to a different value in the database (i.e. the column that's added totemplates)?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We don't let people set themonth orday-of-month field currently, so a schedule currently maxes out at 1 week.

mafredri reacted with thumbs up emoji
Comment on lines +65 to +72
// MaxTTLMillis allows optionally specifying the maximum allowable TTL
// for all workspaces created from this template.
MaxTTLMillis *int64 `json:"max_ttl_ms,omitempty"`

// MinAutostartIntervalMillis allows optionally specifying the minimum
// allowable duration between autostarts for all workspaces created from
// this template.
MinAutostartIntervalMillis *int64 `json:"min_autostart_interval_ms,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thought: Can we make he zero values useful here? Or can these be set to zero to disable them entirely? If zero doesn't need to disable, we can treat it as not set, avoiding the use of pointers.

@@ -158,4 +159,26 @@ func TestAutostart(t *testing.T) {
require.NoError(t, err, "fetch updated workspace")
require.Equal(t, expectedSchedule, *updated.AutostartSchedule, "expected default autostart schedule")
})

t.Run("BelowTemplateConstraint", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Question(gut-check): Should there be a similar case forAboveTemplateConstraint ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't see any reason to enforce an upper bound on the interval between successive autostarts.

@@ -115,6 +122,8 @@ func templateCreate() *cobra.Command {
cmd.Flags().StringVarP(&directory, "directory", "d", currentDirectory, "Specify the directory to create from")
cmd.Flags().StringVarP(&provisioner, "test.provisioner", "", "terraform", "Customize the provisioner backend")
cmd.Flags().StringVarP(&parameterFile, "parameter-file", "", "", "Specify a file path with parameter values.")
cmd.Flags().DurationVarP(&maxTTL, "max-ttl", "", 168*time.Hour, "Specify a maximum TTL for worksapces created from this template.")
cmd.Flags().DurationVarP(&minAutostartInterval, "min-autostart-interval", "", time.Hour, "Specify a minimum autostart interval for workspaces created from this template.")
Copy link
Contributor

@greyscaledgreyscaledJun 6, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍🏻

"--directory", source,
"--test.provisioner", string(database.ProvisionerTypeEcho),
"--max-ttl", "24h",
"--min-autostart-interval", "2h",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍🏻

Comment on lines 111 to 140
var t0 = time.Date(1970, 1, 1, 1, 1, 1, 0, time.UTC)
var tMax = t0.Add(168 * time.Hour)

// Min returns the minimum duration of the schedule.
// This is calculated as follows:
// - Let t(0) be a given point in time (1970-01-01T01:01:01Z00:00)
// - Let t(max) be 168 hours after t(0).
// - Let t(1) be the next scheduled time after t(0).
// - Let t(n) be the next scheduled time after t(n-1).
// - Then, the minimum duration of s d(min)
// = min( t(n) - t(n-1) ∀ n ∈ N, t(n) < t(max) )
func (s Schedule) Min() time.Duration {
durMin := tMax.Sub(t0)
tPrev := s.Next(t0)
tCurr := s.Next(tPrev)
for {
dur := tCurr.Sub(tPrev)
if dur < durMin {
durMin = dur
}
tmp := tCurr
tCurr = s.Next(tmp)
tPrev = tmp
if tCurr.After(tMax) {
break
}
}
return durMin
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@Emyrk it's the maximum ttl (system-constraint)

Emyrk reacted with thumbs up emoji
@@ -243,6 +245,8 @@ export interface Template {
readonly active_version_id: string
readonly workspace_owner_count: number
readonly description: string
readonly max_ttl_ms: number
readonly min_autostart_interval_ms: number
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Question(for FE): Could I use these to set min/max and validations on theWorkspaceScheduleForm ?

I think I can pass the template down into those components and add that logic. Is there any concerns or gotchas or notes about me doing that@johnstcn ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think that's a good idea!

@johnstcnjohnstcn merged commit3e419dd intomainJun 7, 2022
@johnstcnjohnstcn deleted the 1433-template-constraint-for-workspace-scheduling branchJune 7, 2022 12:37
kylecarbs pushed a commit that referenced this pull requestJun 10, 2022
This PR adds fields to templates that constrain values for workspaces derived from that template.- Autostop: Adds a field max_ttl on the template which limits the maximum value of ttl on all workspaces derived from that template. Defaulting to 168 hours, enforced on edits to workspace metadata. New workspaces will default to the templates's `max_ttl` if not specified.- Autostart: Adds a field min_autostart_duration which limits the minimum duration between successive autostarts of a template, measured from a single reference time. Defaulting to 1 hour, enforced on edits to workspace metadata.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@EmyrkEmyrkEmyrk left review comments

@f0sself0sself0ssel left review comments

@mafredrimafredrimafredri approved these changes

@greyscaledgreyscaledgreyscaled approved these changes

Assignees

@johnstcnjohnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Template constraint for workspace scheduling
6 participants
@johnstcn@ammario@mafredri@Emyrk@greyscaled@f0ssel
[8]ページ先頭
©2009-2025 Movatter.jp