Skip to content

Commit c28b7ec

Browse files
johnstcnjohnstcn
authored
fix: coderd: decouple ttl and deadline (coder#2282)
This commit makes the following changes: - Partially reverts the changes of feat: update workspace deadline when workspace ttl updated coder#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. - Drive-by: API: When creating a workspace, default TTL to min(12 hours, template max_ttl) if not instructed otherwise. - Drive-by: CLI: list: measure workspace extension correctly (+X in last column) from the time the provisioner job was completed - Drive-by: WorkspaceSchedule: show timezone of schedule if it is set, defaulting to dayjs guess otherwise. - Drive-by: 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".
1 parent 2513167 commit c28b7ec

File tree

14 files changed

+152
-336
lines changed

14 files changed

+152
-336
lines changed

‎cli/bump.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,9 @@ func bump() *cobra.Command {
5858

5959
_, _ = fmt.Fprintf(
6060
cmd.OutOrStdout(),
61-
"Workspace %q will now stop at %s\n", workspace.Name,
62-
newDeadline.Format(time.RFC822),
61+
"Workspace %q will now stop at %s on %s\n", workspace.Name,
62+
newDeadline.Format(timeFormat),
63+
newDeadline.Format(dateFormat),
6364
)
6465

6566
return nil

‎cli/constants.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
package cli
2+
3+
const (
4+
timeFormat = "3:04:05 PM MST"
5+
dateFormat = "Jan 2, 2006"
6+
)

‎cli/list.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,14 +127,17 @@ func hasExtension(ws codersdk.Workspace) (bool, time.Duration) {
127127
if ws.LatestBuild.Transition != codersdk.WorkspaceTransitionStart {
128128
return false, 0
129129
}
130+
if ws.LatestBuild.Job.CompletedAt == nil {
131+
return false, 0
132+
}
130133
if ws.LatestBuild.Deadline.IsZero() {
131134
return false, 0
132135
}
133136
if ws.TTLMillis == nil {
134137
return false, 0
135138
}
136139
ttl := time.Duration(*ws.TTLMillis) * time.Millisecond
137-
delta := ws.LatestBuild.Deadline.Add(-ttl).Sub(ws.LatestBuild.CreatedAt)
140+
delta := ws.LatestBuild.Deadline.Add(-ttl).Sub(*ws.LatestBuild.Job.CompletedAt)
138141
if delta < time.Minute {
139142
return false, 0
140143
}

‎cli/ttl.go

Lines changed: 21 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
package cli
22

33
import (
4-
"errors"
54
"fmt"
65
"time"
76

87
"github.com/spf13/cobra"
98
"golang.org/x/xerrors"
109

11-
"github.com/coder/coder/cli/cliui"
10+
"github.com/coder/coder/coderd/autobuild/schedule"
11+
"github.com/coder/coder/coderd/util/ptr"
1212
"github.com/coder/coder/codersdk"
1313
)
1414

@@ -91,37 +91,32 @@ func ttlset() *cobra.Command {
9191
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "warning: ttl rounded down to %s\n", truncated)
9292
}
9393

94-
if changed, newDeadline := changedNewDeadline(workspace, truncated); changed {
95-
// For the purposes of the user, "less than a minute" is essentially the same as "immediately".
96-
timeRemaining := time.Until(newDeadline).Truncate(time.Minute)
97-
humanRemaining := "in " + timeRemaining.String()
98-
if timeRemaining <= 0 {
99-
humanRemaining = "immediately"
100-
}
101-
_, err = cliui.Prompt(cmd, cliui.PromptOptions{
102-
Text: fmt.Sprintf(
103-
"Workspace %q will be stopped %s. Are you sure?",
104-
workspace.Name,
105-
humanRemaining,
106-
),
107-
Default: "yes",
108-
IsConfirm: true,
109-
})
110-
if err != nil {
111-
if errors.Is(err, cliui.Canceled) {
112-
return nil
113-
}
114-
return err
115-
}
116-
}
117-
11894
millis := truncated.Milliseconds()
11995
if err = client.UpdateWorkspaceTTL(cmd.Context(), workspace.ID, codersdk.UpdateWorkspaceTTLRequest{
12096
TTLMillis: &millis,
12197
}); err != nil {
12298
return xerrors.Errorf("update workspace ttl: %w", err)
12399
}
124100

101+
if ptr.NilOrEmpty(workspace.AutostartSchedule) {
102+
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "%q will shut down %s after start.\n", workspace.Name, truncated)
103+
return nil
104+
}
105+
106+
sched, err := schedule.Weekly(*workspace.AutostartSchedule)
107+
if err != nil {
108+
return xerrors.Errorf("parse workspace schedule: %w", err)
109+
}
110+
111+
nextShutdown := sched.Next(time.Now()).Add(truncated).In(sched.Location())
112+
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "%q will shut down at %s on %s (%s after start).\n",
113+
workspace.Name,
114+
nextShutdown.Format(timeFormat),
115+
nextShutdown.Format(dateFormat),
116+
truncated,
117+
)
118+
119+
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "NOTE: this will only take effect the next time the workspace is started.\n")
125120
return nil
126121
},
127122
}
@@ -157,18 +152,3 @@ func ttlunset() *cobra.Command {
157152
},
158153
}
159154
}
160-
161-
func changedNewDeadline(ws codersdk.Workspace, newTTL time.Duration) (changed bool, newDeadline time.Time) {
162-
if ws.LatestBuild.Transition != codersdk.WorkspaceTransitionStart {
163-
// not running
164-
return false, newDeadline
165-
}
166-
167-
if ws.LatestBuild.Job.CompletedAt == nil {
168-
// still building
169-
return false, newDeadline
170-
}
171-
172-
newDeadline = ws.LatestBuild.Job.CompletedAt.Add(newTTL)
173-
return true, newDeadline
174-
}

‎cli/ttl_test.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package cli_test
33
import (
44
"bytes"
55
"context"
6-
"fmt"
76
"strings"
87
"testing"
98
"time"
@@ -109,9 +108,6 @@ func TestTTL(t *testing.T) {
109108
assert.NoError(t, err, "unexpected error")
110109
}()
111110

112-
pty.ExpectMatch(fmt.Sprintf("warning: ttl rounded down to %s", ttl.Truncate(time.Minute)))
113-
pty.ExpectMatch(fmt.Sprintf("Workspace %q will be stopped in 8h29m0s. Are you sure?", workspace.Name))
114-
pty.WriteLine("yes")
115111
// Ensure ttl updated
116112
updated, err := client.Workspace(ctx, workspace.ID)
117113
require.NoError(t, err, "fetch updated workspace")

‎coderd/autobuild/executor/lifecycle_executor.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,9 @@ func (e *Executor) runOnce(t time.Time) Stats {
8585
// is what we compare against when performing autostop operations, rounded down
8686
// to the minute.
8787
//
88-
// NOTE: Currently, if a workspace build is created with a given TTL and then
89-
// the user either changes or unsets the TTL, the deadline for the workspace
90-
// build will not have changed. So, autostop will still happen at the
91-
// original TTL value from when the workspace build was created.
92-
// Whether this is expected behavior from a user's perspective is not yet known.
88+
// NOTE: If a workspace build is created with a given TTL and then the user either
89+
// changes or unsets the TTL, the deadline for the workspace build will not
90+
// have changed. This behavior is as expected per #2229.
9391
eligibleWorkspaces, err := db.GetWorkspacesAutostart(e.ctx)
9492
if err != nil {
9593
return xerrors.Errorf("get eligible workspaces for autostart or autostop: %w", err)

‎coderd/autobuild/executor/lifecycle_executor_test.go

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -308,8 +308,7 @@ func TestExecutorAutostopNotEnabled(t *testing.T) {
308308
require.NoError(t, err)
309309
require.Nil(t, workspace.TTLMillis)
310310

311-
// TODO(cian): need to stop and start the workspace as we do not update the deadline yet
312-
// see: https://github.com/coder/coder/issues/1783
311+
// TODO(cian): need to stop and start the workspace as we do not update the deadline. See: #2229
313312
coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop)
314313
coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStop, database.WorkspaceTransitionStart)
315314

@@ -440,41 +439,47 @@ func TestExecutorWorkspaceAutostopNoWaitChangedMyMind(t *testing.T) {
440439
err := client.UpdateWorkspaceTTL(ctx, workspace.ID, codersdk.UpdateWorkspaceTTLRequest{TTLMillis: nil})
441440
require.NoError(t, err)
442441

443-
// Then: the deadline should be the zero value
442+
// Then: the deadline should still be the original value
444443
updated := coderdtest.MustWorkspace(t, client, workspace.ID)
445-
assert.Zero(t, updated.LatestBuild.Deadline)
444+
assert.WithinDuration(t, workspace.LatestBuild.Deadline, updated.LatestBuild.Deadline, time.Minute)
446445

447446
// When: the autobuild executor ticks after the original deadline
448447
go func() {
449448
tickCh <- workspace.LatestBuild.Deadline.Add(time.Minute)
450449
}()
451450

452-
// Then: the workspace should not stop
451+
// Then: the workspace should stop
453452
stats := <-statsCh
454453
assert.NoError(t, stats.Error)
455-
assert.Len(t, stats.Transitions, 0)
454+
assert.Len(t, stats.Transitions, 1)
455+
assert.Equal(t, stats.Transitions[workspace.ID], database.WorkspaceTransitionStop)
456+
457+
// Wait for stop to complete
458+
updated = coderdtest.MustWorkspace(t, client, workspace.ID)
459+
_ = coderdtest.AwaitWorkspaceBuildJob(t, client, updated.LatestBuild.ID)
460+
461+
// Start the workspace again
462+
workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStop, database.WorkspaceTransitionStart)
456463

457464
// Given: the user changes their mind again and wants to enable auto-stop
458465
newTTL := 8 * time.Hour
459-
expectedDeadline := workspace.LatestBuild.UpdatedAt.Add(newTTL)
460466
err = client.UpdateWorkspaceTTL(ctx, workspace.ID, codersdk.UpdateWorkspaceTTLRequest{TTLMillis: ptr.Ref(newTTL.Milliseconds())})
461467
require.NoError(t, err)
462468

463-
// Then: the deadline should be updated based on the TTL
469+
// Then: the deadline should remain at the zero value
464470
updated = coderdtest.MustWorkspace(t, client, workspace.ID)
465-
assert.WithinDuration(t, expectedDeadline, updated.LatestBuild.Deadline, time.Minute)
471+
assert.Zero(t, updated.LatestBuild.Deadline)
466472

467473
// When: the relentless onward march of time continues
468474
go func() {
469475
tickCh <- workspace.LatestBuild.Deadline.Add(newTTL + time.Minute)
470476
close(tickCh)
471477
}()
472478

473-
// Then: the workspace should stop
479+
// Then: the workspace should not stop
474480
stats = <-statsCh
475481
assert.NoError(t, stats.Error)
476-
assert.Len(t, stats.Transitions, 1)
477-
assert.Equal(t, stats.Transitions[workspace.ID], database.WorkspaceTransitionStop)
482+
assert.Len(t, stats.Transitions, 0)
478483
}
479484

480485
func TestExecutorAutostartMultipleOK(t *testing.T) {

‎coderd/workspaces.go

Lines changed: 33 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ import (
3131
"github.com/coder/coder/codersdk"
3232
)
3333

34+
const workspaceDefaultTTL = 12 * time.Hour
35+
3436
func (api *API) workspace(rw http.ResponseWriter, r *http.Request) {
3537
workspace := httpmw.WorkspaceParam(r)
3638
if !api.Authorize(r, rbac.ActionRead, workspace) {
@@ -291,8 +293,8 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
291293
}
292294

293295
if !dbTTL.Valid {
294-
// Default to template maximum when creating a new workspace
295-
dbTTL = sql.NullInt64{Valid: true, Int64: template.MaxTtl}
296+
// Default to min(12 hours, template maximum). Just defaulting to template maximum can be surprising.
297+
dbTTL = sql.NullInt64{Valid: true, Int64: min(template.MaxTtl, int64(workspaceDefaultTTL))}
296298
}
297299

298300
workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{
@@ -513,75 +515,41 @@ func (api *API) putWorkspaceTTL(rw http.ResponseWriter, r *http.Request) {
513515
return
514516
}
515517

516-
template, err := api.Database.GetTemplateByID(r.Context(), workspace.TemplateID)
517-
if err != nil {
518-
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
519-
Message: "Error fetching workspace template!",
520-
})
521-
return
522-
}
518+
var validErrs []httpapi.Error
523519

524-
dbTTL, err := validWorkspaceTTLMillis(req.TTLMillis, time.Duration(template.MaxTtl))
525-
if err != nil {
526-
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
527-
Message: "Invalid workspace TTL.",
528-
Detail: err.Error(),
529-
Validations: []httpapi.Error{
530-
{
531-
Field: "ttl_ms",
532-
Detail: err.Error(),
533-
},
534-
},
535-
})
536-
return
537-
}
520+
err := api.Database.InTx(func(s database.Store) error {
521+
template, err := s.GetTemplateByID(r.Context(), workspace.TemplateID)
522+
if err != nil {
523+
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
524+
Message: "Error fetching workspace template!",
525+
})
526+
return xerrors.Errorf("fetch workspace template: %w", err)
527+
}
538528

539-
err = api.Database.InTx(func(s database.Store) error {
529+
dbTTL, err := validWorkspaceTTLMillis(req.TTLMillis, time.Duration(template.MaxTtl))
530+
if err != nil {
531+
validErrs = append(validErrs, httpapi.Error{Field: "ttl_ms", Detail: err.Error()})
532+
return err
533+
}
540534
if err := s.UpdateWorkspaceTTL(r.Context(), database.UpdateWorkspaceTTLParams{
541535
ID: workspace.ID,
542536
Ttl: dbTTL,
543537
}); err != nil {
544538
return xerrors.Errorf("update workspace TTL: %w", err)
545539
}
546540

547-
// Also extend the workspace deadline if the workspace is running
548-
latestBuild, err := s.GetLatestWorkspaceBuildByWorkspaceID(r.Context(), workspace.ID)
549-
if err != nil {
550-
return xerrors.Errorf("get latest workspace build: %w", err)
551-
}
552-
553-
if latestBuild.Transition != database.WorkspaceTransitionStart {
554-
return nil // nothing to do
555-
}
556-
557-
if latestBuild.UpdatedAt.IsZero() {
558-
// Build in progress; provisionerd should update with the new TTL.
559-
return nil
560-
}
561-
562-
var newDeadline time.Time
563-
if dbTTL.Valid {
564-
newDeadline = latestBuild.UpdatedAt.Add(time.Duration(dbTTL.Int64))
565-
}
566-
567-
if err := s.UpdateWorkspaceBuildByID(
568-
r.Context(),
569-
database.UpdateWorkspaceBuildByIDParams{
570-
ID: latestBuild.ID,
571-
UpdatedAt: latestBuild.UpdatedAt,
572-
ProvisionerState: latestBuild.ProvisionerState,
573-
Deadline: newDeadline,
574-
},
575-
); err != nil {
576-
return xerrors.Errorf("update workspace deadline: %w", err)
577-
}
578541
return nil
579542
})
580543

581544
if err != nil {
582-
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
583-
Message: "Error updating workspace time until shutdown!",
584-
Detail: err.Error(),
545+
code := http.StatusInternalServerError
546+
if len(validErrs) > 0 {
547+
code = http.StatusBadRequest
548+
}
549+
httpapi.Write(rw, code, httpapi.Response{
550+
Message: "Error updating workspace time until shutdown!",
551+
Validations: validErrs,
552+
Detail: err.Error(),
585553
})
586554
return
587555
}
@@ -1028,3 +996,10 @@ func splitQueryParameterByDelimiter(query string, delimiter rune, maintainQuotes
1028996

1029997
return parts
1030998
}
999+
1000+
func min(x, y int64) int64 {
1001+
if x < y {
1002+
return x
1003+
}
1004+
return y
1005+
}

0 commit comments

Comments
 (0)