Skip to content

Commit a19a0e9

Browse files
johnstcnjohnstcn
committed
fix: coderd: decouple ttl and deadline
1 parent 5be52de commit a19a0e9

File tree

8 files changed

+106
-258
lines changed

8 files changed

+106
-258
lines changed

‎cli/ttl.go

+24-41
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,35 @@ 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+
loc, err := time.LoadLocation(sched.Timezone())
112+
if err != nil {
113+
return xerrors.Errorf("schedule has invalid timezone: %w", err)
114+
}
115+
116+
nextShutdown := sched.Next(time.Now()).Add(truncated).In(loc)
117+
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "%q will shut down at %s on %s (%s after start).\n",
118+
workspace.Name,
119+
nextShutdown.Format("15:04:05 MST"),
120+
nextShutdown.Format("2006-02-01"),
121+
truncated,
122+
)
125123
return nil
126124
},
127125
}
@@ -157,18 +155,3 @@ func ttlunset() *cobra.Command {
157155
},
158156
}
159157
}
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

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

+3-5
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

+17-12
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

+22-56
Original file line numberDiff line numberDiff line change
@@ -541,75 +541,41 @@ func (api *API) putWorkspaceTTL(rw http.ResponseWriter, r *http.Request) {
541541
return
542542
}
543543

544-
template, err := api.Database.GetTemplateByID(r.Context(), workspace.TemplateID)
545-
if err != nil {
546-
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
547-
Message: "Error fetching workspace template!",
548-
})
549-
return
550-
}
544+
var validErrs []httpapi.Error
551545

552-
dbTTL, err := validWorkspaceTTLMillis(req.TTLMillis, time.Duration(template.MaxTtl))
553-
if err != nil {
554-
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
555-
Message: "Invalid workspace TTL.",
556-
Detail: err.Error(),
557-
Validations: []httpapi.Error{
558-
{
559-
Field: "ttl_ms",
560-
Detail: err.Error(),
561-
},
562-
},
563-
})
564-
return
565-
}
546+
err := api.Database.InTx(func(s database.Store) error {
547+
template, err := s.GetTemplateByID(r.Context(), workspace.TemplateID)
548+
if err != nil {
549+
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
550+
Message: "Error fetching workspace template!",
551+
})
552+
return xerrors.Errorf("fetch workspace template: %w", err)
553+
}
566554

567-
err = api.Database.InTx(func(s database.Store) error {
555+
dbTTL, err := validWorkspaceTTLMillis(req.TTLMillis, time.Duration(template.MaxTtl))
556+
if err != nil {
557+
validErrs = append(validErrs, httpapi.Error{Field: "ttl_ms", Detail: err.Error()})
558+
return err
559+
}
568560
if err := s.UpdateWorkspaceTTL(r.Context(), database.UpdateWorkspaceTTLParams{
569561
ID: workspace.ID,
570562
Ttl: dbTTL,
571563
}); err != nil {
572564
return xerrors.Errorf("update workspace TTL: %w", err)
573565
}
574566

575-
// Also extend the workspace deadline if the workspace is running
576-
latestBuild, err := s.GetLatestWorkspaceBuildByWorkspaceID(r.Context(), workspace.ID)
577-
if err != nil {
578-
return xerrors.Errorf("get latest workspace build: %w", err)
579-
}
580-
581-
if latestBuild.Transition != database.WorkspaceTransitionStart {
582-
return nil // nothing to do
583-
}
584-
585-
if latestBuild.UpdatedAt.IsZero() {
586-
// Build in progress; provisionerd should update with the new TTL.
587-
return nil
588-
}
589-
590-
var newDeadline time.Time
591-
if dbTTL.Valid {
592-
newDeadline = latestBuild.UpdatedAt.Add(time.Duration(dbTTL.Int64))
593-
}
594-
595-
if err := s.UpdateWorkspaceBuildByID(
596-
r.Context(),
597-
database.UpdateWorkspaceBuildByIDParams{
598-
ID: latestBuild.ID,
599-
UpdatedAt: latestBuild.UpdatedAt,
600-
ProvisionerState: latestBuild.ProvisionerState,
601-
Deadline: newDeadline,
602-
},
603-
); err != nil {
604-
return xerrors.Errorf("update workspace deadline: %w", err)
605-
}
606567
return nil
607568
})
608569

609570
if err != nil {
610-
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
611-
Message: "Error updating workspace time until shutdown!",
612-
Detail: err.Error(),
571+
code := http.StatusInternalServerError
572+
if len(validErrs) > 0 {
573+
code = http.StatusBadRequest
574+
}
575+
httpapi.Write(rw, code, httpapi.Response{
576+
Message: "Error updating workspace time until shutdown!",
577+
Validations: validErrs,
578+
Detail: err.Error(),
613579
})
614580
return
615581
}

‎coderd/workspaces_test.go

+16-24
Original file line numberDiff line numberDiff line change
@@ -668,40 +668,35 @@ func TestWorkspaceUpdateTTL(t *testing.T) {
668668
t.Parallel()
669669

670670
testCases := []struct {
671-
name string
672-
ttlMillis *int64
673-
expectedError string
674-
expectedDeadline *time.Time
675-
modifyTemplate func(*codersdk.CreateTemplateRequest)
671+
name string
672+
ttlMillis *int64
673+
expectedError string
674+
modifyTemplate func(*codersdk.CreateTemplateRequest)
676675
}{
677676
{
678-
name: "disable ttl",
679-
ttlMillis: nil,
680-
expectedError: "",
681-
expectedDeadline: ptr.Ref(time.Time{}),
677+
name: "disable ttl",
678+
ttlMillis: nil,
679+
expectedError: "",
682680
},
683681
{
684-
name: "update ttl",
685-
ttlMillis: ptr.Ref(12 * time.Hour.Milliseconds()),
686-
expectedError: "",
687-
expectedDeadline: ptr.Ref(time.Now().Add(12*time.Hour + time.Minute)),
682+
name: "update ttl",
683+
ttlMillis: ptr.Ref(12 * time.Hour.Milliseconds()),
684+
expectedError: "",
688685
},
689686
{
690687
name: "below minimum ttl",
691688
ttlMillis: ptr.Ref((30 * time.Second).Milliseconds()),
692689
expectedError: "ttl must be at least one minute",
693690
},
694691
{
695-
name: "minimum ttl",
696-
ttlMillis: ptr.Ref(time.Minute.Milliseconds()),
697-
expectedError: "",
698-
expectedDeadline: ptr.Ref(time.Now().Add(2 * time.Minute)),
692+
name: "minimum ttl",
693+
ttlMillis: ptr.Ref(time.Minute.Milliseconds()),
694+
expectedError: "",
699695
},
700696
{
701-
name: "maximum ttl",
702-
ttlMillis: ptr.Ref((24 * 7 * time.Hour).Milliseconds()),
703-
expectedError: "",
704-
expectedDeadline: ptr.Ref(time.Now().Add(24*7*time.Hour + time.Minute)),
697+
name: "maximum ttl",
698+
ttlMillis: ptr.Ref((24 * 7 * time.Hour).Milliseconds()),
699+
expectedError: "",
705700
},
706701
{
707702
name: "above maximum ttl",
@@ -754,9 +749,6 @@ func TestWorkspaceUpdateTTL(t *testing.T) {
754749
require.NoError(t, err, "fetch updated workspace")
755750

756751
require.Equal(t, testCase.ttlMillis, updated.TTLMillis, "expected autostop ttl to equal requested")
757-
if testCase.expectedDeadline != nil {
758-
require.WithinDuration(t, *testCase.expectedDeadline, updated.LatestBuild.Deadline, time.Minute, "expected autostop deadline to be equal expected")
759-
}
760752
})
761753
}
762754

0 commit comments

Comments
 (0)