Partially reverts the changes offeat: update workspace deadline when workspace ttl updated #2165, making the deadline of a running workspace build independant of TTL, once started.
CLI: updating a workspace TTL no longer updates the deadline of the workspace.
UI: updating a workspace TTL no longer updates the deadline of the workspace.

Some additional drive-by changes:

API: When creating a workspace, default TTL to min(12 hours, template max_ttl) if not instructed otherwise.
CLI: list: measure workspace extension correctly (+X in last column) from the time the provisioner job was completed
WorkspaceSchedule: show timezone ofschedule if it is set, defaulting to dayjs guess otherwise.
- 👀 This caused a lot of storybook changes -- if preferred I can move this to another change instead or chromatic-ignore that element.
WorkspaceScheduleForm: fixed an issue where deleting the "TTL" value in the form would show the text "Your workspace will shut down a few seconds after start".

johnstcn self-assigned this

Jun 13, 2022

johnstcn force-pushed thecj/2229/decouple-ws-deadline-from-ttl-again branch from07e97b2 to87c7ffaCompare

June 13, 2022 21:11

johnstcn added4 commits

June 14, 2022 08:10

fix: coderd: decouple ttl and deadline

a19a0e9

yarn lint

3999661

update storybook

f5ce8a3

fmt

987f785

johnstcn force-pushed thecj/2229/decouple-ws-deadline-from-ttl-again branch from87c7ffa to987f785Compare

June 14, 2022 08:10

johnstcn added5 commits

June 14, 2022 08:32

fix ttl command

cd4dce3

drive-by: default ttl to min(12 hours, template max) for new workspaces

91e3171

fix: WorkspaceSchedule: show tz of schedule if available, default to …

00ac2a2

…dayjs guess

cli: list: measure extension from provisioner job completed_at

3faf3c8

WorkspaceScheduleForm: fix "a few seconds after start"

a763d2f

johnstcn marked this pull request as ready for review

June 14, 2022 10:03

johnstcn requested a review froma team as acode owner

June 14, 2022 10:03

johnstcn requested a review froma team

June 14, 2022 10:03

johnstcn commented

Jun 14, 2022

View reviewed changes

cli/list.go

		if ws.LatestBuild.Deadline.IsZero() {
		return false, 0
		}
		if ws.TTLMillis == nil {
		return false, 0
		}
		ttl := time.Duration(ws.TTLMillis) time.Millisecond
		delta := ws.LatestBuild.Deadline.Add(-ttl).Sub(ws.LatestBuild.CreatedAt)
		delta := ws.LatestBuild.Deadline.Add(-ttl).Sub(*ws.LatestBuild.Job.CompletedAt)

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 was causing us to measure deadline extensions incorrectly, resulting in things like8h (+3m) if your workspace took 3 minutes to buidl.

coderd/autobuild/executor/lifecycle_executor.go

Comment on lines +88 to +90

		// NOTE: If a workspace build is created with a given TTL and then the user either
		// changes or unsets the TTL, the deadline for the workspace build will not
		// have changed. This behavior is as expected per #2229.

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 now the expected behaviour until we're told otherwise!

coderd/workspaces.go Outdated

Comment on lines 322 to 323

		// Default tomin(12 hours,template maximum). Just defaulting to template maximum can be surprising.
		dbTTL = sql.NullInt64{Valid: true, Int64:min(template.MaxTtl, int64(12*time.Hour))}

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 the default max_ttl for templates is 168 hours, setting the workspace default to something hopefully more reasonable on a day-to-day basis. Folks can still change this if they want!

Copy link

Member

Choose a reason for hiding this comment

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

Suggestion (non-blocking): Should we make this a constant somewhere, or perhaps define in the db? It could be hard to determine where this limit is coming from.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Constant sounds good!

site/src/components/WorkspaceSchedule/WorkspaceSchedule.tsx

Comment on lines +69 to +72

		scheduleHeader: (workspace: Workspace): string => {
		const tz = workspace.autostart_schedule ? extractTimezone(workspace.autostart_schedule) : dayjs.tz.guess()
		return `Schedule (${tz})`
		},

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 found that if your workspace's schedule was set to a different timezone than your own, it would still show your own. I found that confusing, and figured others would likewise. Still defaulting todayjs.tz.guess() as this should prime the user with what they hopefully want should they need to edit.

site/src/components/WorkspaceScheduleForm/WorkspaceScheduleForm.tsx

		export const ttlShutdownAt = (now: dayjs.Dayjs, workspace: Workspace, tz: string, formTTL: number): string => {
		// a manual shutdown has a deadline of '"0001-01-01T00:00:00Z"'
		// SEE: #1834
		const deadline = dayjs(workspace.latest_build.deadline).utc()
		const hasDeadline = deadline.year() > 1
		const ttl = workspace.ttl_ms ? workspace.ttl_ms / (1000 * 60 * 60) : 0
		const delta = formTTL - ttl

		if (delta === 0 \|\| !isWorkspaceOn(workspace)) {
		return Language.ttlHelperText
		} else if (formTTL === 0) {
		export const ttlShutdownAt = (formTTL: number): string => {
		if (formTTL < 1) {
		// Passing an empty value for TTL in the form results in a number that is not zero but less than 1.
		return Language.ttlCausesNoShutdownHelperText
		} else {
		const newDeadline = dayjs(hasDeadline ? deadline : now).add(delta, "hours")
		if (newDeadline.isSameOrBefore(now)) {
		return `⚠️ ${Language.ttlCausesShutdownHelperText} ${Language.ttlCausesShutdownImmediately} ⚠️`
		} else if (newDeadline.isSameOrBefore(now.add(30, "minutes"))) {
		return `⚠️ ${Language.ttlCausesShutdownHelperText} ${Language.ttlCausesShutdownSoon} ⚠️`
		} else {
		return `${Language.ttlCausesShutdownHelperText} ${Language.ttlCausesShutdownAt} ${newDeadline
		.tz(tz)
		.format("MMM D, YYYY h:mm A")}.`
		}
		return `${Language.ttlCausesShutdownHelperText} ${dayjs.duration(formTTL, "hours").humanize()} ${
		Language.ttlCausesShutdownAfterStart
		}.`

Copy link

MemberAuthor

johnstcnJun 14, 2022

Choose a reason for hiding this comment

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

review: As we no longer can depend on the workspace deadline, I'm just simplifying this a whole bunch and showing the humanized duration.

mafredri approved these changes

Jun 14, 2022

View reviewed changes

Copy link

Member

mafredri left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Some thoughts/suggestions but backend changes look good in general!

cli/ttl.go OutdatedShow resolvedHide resolved

cli/ttl.goShow resolvedHide resolved

coderd/workspaces.go Outdated

Comment on lines 322 to 323

		// Default tomin(12 hours,template maximum). Just defaulting to template maximum can be surprising.
		dbTTL = sql.NullInt64{Valid: true, Int64:min(template.MaxTtl, int64(12*time.Hour))}

Copy link

Member

mafredriJun 14, 2022

Choose a reason for hiding this comment

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

Suggestion (non-blocking): Should we make this a constant somewhere, or perhaps define in the db? It could be hard to determine where this limit is coming from.

Copy link

Member

mafredri commentedJun 14, 2022

Not looking to bike-shed this, but writing down my thoughts.

CLI: updating a workspace TTL no longer updates the deadline of the workspace.
UI: updating a workspace TTL no longer updates the deadline of the workspace.

Intuitively, I don't know if this is what I'd expect, but I think it's close enough. What I might like to see though is a prompt asking me if I want to update the current workspace TTL as well. For example, a yes/no to update tonow + TTL, or perhaps a select box with:created + TTL,now + TTL,Cancel. Just a thought.