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

johnstcn · 2022-06-03T13:15:12Z

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.

…nterval

johnstcn · 2022-06-03T16:15:57Z

codersdk/templates.go

+	MaxTTLMillis               int64           `json:"max_ttl_ms"`
+	MinAutostartIntervalMillis int64           `json:"min_autostart_interval_ms"`


Review: following convention of codersdk types using int64 millisecond-timestamps for time.Duration

johnstcn · 2022-06-03T16:16:37Z

codersdk/organizations.go

+	// 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"`


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

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

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.

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.

johnstcn · 2022-06-03T16:17:59Z

coderd/autobuild/schedule/schedule.go

+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
+}
+


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.

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

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

johnstcn · 2022-06-03T16:18:54Z

coderd/autobuild/executor/lifecycle_executor_test.go

@@ -2,7 +2,6 @@ package executor_test



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 🙃

johnstcn · 2022-06-03T16:19:52Z

cli/create_test.go

-			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")


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.

johnstcn · 2022-06-03T16:20:19Z

cli/create_test.go

-		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")


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

johnstcn · 2022-06-03T16:20:50Z

cli/create_test.go

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


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

johnstcn · 2022-06-03T16:21:30Z

cli/create.go

@@ -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).")


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

johnstcn · 2022-06-03T16:22:08Z

cli/create.go

+			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)
+			}
+


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

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.

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

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

ammario · 2022-06-03T17:26:28Z

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 the max_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.

mafredri

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

mafredri · 2022-06-06T12:07:54Z

coderd/autobuild/schedule/schedule.go

@@ -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)


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 to templates)?

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

coderd/autobuild/schedule/schedule.go

coderd/workspaces.go

mafredri · 2022-06-06T12:15:43Z

codersdk/organizations.go

+	// 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"`


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.

cli/create.go

greyscaled · 2022-06-06T14:19:06Z

cli/autostart_test.go

@@ -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) {


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

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

greyscaled · 2022-06-06T14:20:42Z

cli/templatecreate.go

@@ -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.")


greyscaled · 2022-06-06T14:20:55Z

cli/templatecreate_test.go

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


greyscaled · 2022-06-06T14:24:19Z

coderd/autobuild/schedule/schedule.go

+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
+}
+


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

greyscaled · 2022-06-06T14:27:25Z

site/src/api/typesGenerated.ts

@@ -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


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

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 ?

I think that's a good idea!

…int-for-workspace-scheduling

…constraint-for-workspace-scheduling

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.

johnstcn self-assigned this Jun 3, 2022

johnstcn linked an issue Jun 3, 2022 that may be closed by this pull request

Template constraint for workspace scheduling #1433

Closed

johnstcn added 12 commits June 3, 2022 16:13

RED: add failing unit tests

1713431

RED: add migrations and make gen

f11f942

ORANGE: fix audit/diff tests

c5c9a7a

fixup! RED: add migrations and make gen

74f4dee

fixup! RED: add migrations and make gen

7da27a0

feat: schedule: add schedule.Min function to determine minimum interval

5ad048b

ORANGE: coderd: fix a whole bunch of tests

65b980e

RED: fast-fail some slow-failing tests

aa298a6

GREEN: fix even more tests

a5ec9bf

GREEN: fix final unit test

a6255d6

cli: templatecreate: add CLI flags --max-ttl --min-autostart-interval

245795e

fixup! cli: templatecreate: add CLI flags --max-ttl --min-autostart-i…

709db5b

…nterval

johnstcn force-pushed the 1433-template-constraint-for-workspace-scheduling branch from d4b43ac to 709db5b Compare June 3, 2022 16:14

johnstcn marked this pull request as ready for review June 3, 2022 16:14

johnstcn requested a review from a team as a code owner June 3, 2022 16:14

johnstcn requested review from greyscaled and a team June 3, 2022 16:15

johnstcn commented Jun 3, 2022

View reviewed changes

johnstcn added 2 commits June 3, 2022 16:45

make gen

c4a0927

update test entities

244e479

mafredri approved these changes Jun 6, 2022

View reviewed changes

greyscaled approved these changes Jun 6, 2022

View reviewed changes

johnstcn added 5 commits June 7, 2022 10:44

Merge remote-tracking branch 'origin/main' into 1433-template-constra…

60ef479

…int-for-workspace-scheduling

fixup! Merge remote-tracking branch 'origin/main' into 1433-template-…

c9aab8b

…constraint-for-workspace-scheduling

coderd: default newly-minted workspaces to template max_ttl

f997af2

address PR comment

d7f697b

fix broken unit test

ebe4c15

johnstcn mentioned this pull request Jun 7, 2022

Refactor (DRY) workspace TTL/Schedule validation logic #2119

Closed

fix some more tests

08ef2eb

johnstcn merged commit 3e419dd into main Jun 7, 2022

johnstcn deleted the 1433-template-constraint-for-workspace-scheduling branch June 7, 2022 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johnstcn commented Jun 3, 2022

johnstcn Jun 3, 2022

johnstcn Jun 3, 2022

f0ssel Jun 3, 2022

johnstcn Jun 3, 2022

mafredri Jun 6, 2022

johnstcn Jun 7, 2022

johnstcn Jun 3, 2022

Emyrk Jun 6, 2022

greyscaled Jun 6, 2022

johnstcn Jun 3, 2022

johnstcn Jun 3, 2022

johnstcn Jun 3, 2022

johnstcn Jun 3, 2022

johnstcn Jun 3, 2022

johnstcn Jun 3, 2022

Emyrk Jun 6, 2022

johnstcn Jun 7, 2022

ammario commented Jun 3, 2022

mafredri left a comment

mafredri Jun 6, 2022

johnstcn Jun 7, 2022

mafredri Jun 6, 2022

greyscaled Jun 6, 2022

johnstcn Jun 7, 2022

greyscaled Jun 6, 2022 •

edited

Loading

greyscaled Jun 6, 2022

greyscaled Jun 6, 2022

greyscaled Jun 6, 2022

johnstcn Jun 7, 2022

		MaxTTLMillis int64 `json:"max_ttl_ms"`
		MinAutostartIntervalMillis int64 `json:"min_autostart_interval_ms"`

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

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

Conversation

johnstcn commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammario commented Jun 3, 2022

mafredri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greyscaled Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greyscaled Jun 6, 2022 •

edited

Loading