Skip to content

Commit 8b6e286

Browse files
johnstcnjohnstcn
authored
refactor(coderd): collapse activityBumpWorkspace into a single query (#9652)
* Adds unit-style tests for activityBumpWorkspace * Ports logic of activityBumpWorkspace to a SQL query * Updates activityBumpWorkspace to call above query
1 parent 38560dd commit 8b6e286

File tree

9 files changed

+424
-69
lines changed

9 files changed

+424
-69
lines changed

‎coderd/activitybump.go

+1-69
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,13 @@ package coderd
22

33
import (
44
"context"
5-
"database/sql"
6-
"errors"
75
"time"
86

97
"github.com/google/uuid"
108
"golang.org/x/xerrors"
119

1210
"cdr.dev/slog"
1311
"github.com/coder/coder/v2/coderd/database"
14-
"github.com/coder/coder/v2/coderd/database/dbtime"
1512
)
1613

1714
// activityBumpWorkspace automatically bumps the workspace's auto-off timer
@@ -21,72 +18,7 @@ func activityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Sto
2118
// low priority operations fail first.
2219
ctx, cancel := context.WithTimeout(ctx, time.Second*15)
2320
defer cancel()
24-
25-
err := db.InTx(func(s database.Store) error {
26-
build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspaceID)
27-
if errors.Is(err, sql.ErrNoRows) {
28-
return nil
29-
} else if err != nil {
30-
return xerrors.Errorf("get latest workspace build: %w", err)
31-
}
32-
33-
job, err := s.GetProvisionerJobByID(ctx, build.JobID)
34-
if err != nil {
35-
return xerrors.Errorf("get provisioner job: %w", err)
36-
}
37-
38-
if build.Transition != database.WorkspaceTransitionStart || !job.CompletedAt.Valid {
39-
return nil
40-
}
41-
42-
if build.Deadline.IsZero() {
43-
// Workspace shutdown is manual
44-
return nil
45-
}
46-
47-
workspace, err := s.GetWorkspaceByID(ctx, workspaceID)
48-
if err != nil {
49-
return xerrors.Errorf("get workspace: %w", err)
50-
}
51-
52-
var (
53-
// We bump by the original TTL to prevent counter-intuitive behavior
54-
// as the TTL wraps. For example, if I set the TTL to 12 hours, sign off
55-
// work at midnight, come back at 10am, I would want another full day
56-
// of uptime. In the prior implementation, the workspace would enter
57-
// a state of always expiring 1 hour in the future
58-
bumpAmount = time.Duration(workspace.Ttl.Int64)
59-
// DB writes are expensive so we only bump when 5% of the deadline
60-
// has elapsed.
61-
bumpEvery = bumpAmount / 20
62-
timeSinceLastBump = bumpAmount - time.Until(build.Deadline)
63-
)
64-
65-
if timeSinceLastBump < bumpEvery {
66-
return nil
67-
}
68-
69-
if bumpAmount == 0 {
70-
return nil
71-
}
72-
73-
newDeadline := dbtime.Now().Add(bumpAmount)
74-
if !build.MaxDeadline.IsZero() && newDeadline.After(build.MaxDeadline) {
75-
newDeadline = build.MaxDeadline
76-
}
77-
78-
if err := s.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{
79-
ID: build.ID,
80-
UpdatedAt: dbtime.Now(),
81-
ProvisionerState: build.ProvisionerState,
82-
Deadline: newDeadline,
83-
MaxDeadline: build.MaxDeadline,
84-
}); err != nil {
85-
return xerrors.Errorf("update workspace build: %w", err)
86-
}
87-
return nil
88-
}, nil)
89-
if err != nil {
21+
if err := db.ActivityBumpWorkspace(ctx, workspaceID); err != nil {
9022
if !xerrors.Is(err, context.Canceled) && !database.IsQueryCanceledError(err) {
9123
// Bump will fail if the context is canceled, but this is ok.
9224
log.Error(ctx, "bump failed", slog.Error(err),

‎coderd/activitybump_internal_test.go

+233
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,233 @@
1+
package coderd
2+
3+
import (
4+
"database/sql"
5+
"runtime"
6+
"testing"
7+
"time"
8+
9+
"github.com/google/uuid"
10+
11+
"cdr.dev/slog/sloggers/slogtest"
12+
"github.com/coder/coder/v2/coderd/database"
13+
"github.com/coder/coder/v2/coderd/database/dbgen"
14+
"github.com/coder/coder/v2/coderd/database/dbtestutil"
15+
"github.com/coder/coder/v2/coderd/database/dbtime"
16+
"github.com/coder/coder/v2/coderd/util/ptr"
17+
"github.com/coder/coder/v2/testutil"
18+
19+
"github.com/stretchr/testify/require"
20+
)
21+
22+
func Test_ActivityBumpWorkspace(t *testing.T) {
23+
t.Parallel()
24+
25+
for _, tt := range []struct {
26+
name string
27+
transition database.WorkspaceTransition
28+
jobCompletedAt sql.NullTime
29+
buildDeadlineOffset *time.Duration
30+
maxDeadlineOffset *time.Duration
31+
workspaceTTL time.Duration
32+
expectedBump time.Duration
33+
}{
34+
{
35+
name: "NotFinishedYet",
36+
transition: database.WorkspaceTransitionStart,
37+
jobCompletedAt: sql.NullTime{},
38+
buildDeadlineOffset: ptr.Ref(8 * time.Hour),
39+
workspaceTTL: 8 * time.Hour,
40+
expectedBump: 0,
41+
},
42+
{
43+
name: "ManualShutdown",
44+
transition: database.WorkspaceTransitionStart,
45+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now()},
46+
buildDeadlineOffset: nil,
47+
expectedBump: 0,
48+
},
49+
{
50+
name: "NotTimeToBumpYet",
51+
transition: database.WorkspaceTransitionStart,
52+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now()},
53+
buildDeadlineOffset: ptr.Ref(8 * time.Hour),
54+
workspaceTTL: 8 * time.Hour,
55+
expectedBump: 0,
56+
},
57+
{
58+
name: "TimeToBump",
59+
transition: database.WorkspaceTransitionStart,
60+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
61+
buildDeadlineOffset: ptr.Ref(8*time.Hour - 24*time.Minute),
62+
workspaceTTL: 8 * time.Hour,
63+
expectedBump: 8 * time.Hour,
64+
},
65+
{
66+
name: "MaxDeadline",
67+
transition: database.WorkspaceTransitionStart,
68+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
69+
buildDeadlineOffset: ptr.Ref(time.Minute), // last chance to bump!
70+
maxDeadlineOffset: ptr.Ref(time.Hour),
71+
workspaceTTL: 8 * time.Hour,
72+
expectedBump: 1 * time.Hour,
73+
},
74+
{
75+
// A workspace that is still running, has passed its deadline, but has not
76+
// yet been auto-stopped should still bump the deadline.
77+
name: "PastDeadlineStillBumps",
78+
transition: database.WorkspaceTransitionStart,
79+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-24 * time.Minute)},
80+
buildDeadlineOffset: ptr.Ref(-time.Minute),
81+
workspaceTTL: 8 * time.Hour,
82+
expectedBump: 8 * time.Hour,
83+
},
84+
{
85+
// A stopped workspace should never bump.
86+
name: "StoppedWorkspace",
87+
transition: database.WorkspaceTransitionStop,
88+
jobCompletedAt: sql.NullTime{Valid: true, Time: dbtime.Now().Add(-time.Minute)},
89+
buildDeadlineOffset: ptr.Ref(-time.Minute),
90+
workspaceTTL: 8 * time.Hour,
91+
expectedBump: 0,
92+
},
93+
} {
94+
tt := tt
95+
t.Run(tt.name, func(t *testing.T) {
96+
t.Parallel()
97+
98+
var (
99+
now = dbtime.Now()
100+
ctx = testutil.Context(t, testutil.WaitShort)
101+
log = slogtest.Make(t, nil)
102+
db, _ = dbtestutil.NewDB(t)
103+
org = dbgen.Organization(t, db, database.Organization{})
104+
user = dbgen.User(t, db, database.User{
105+
Status: database.UserStatusActive,
106+
})
107+
_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
108+
UserID: user.ID,
109+
OrganizationID: org.ID,
110+
})
111+
templateVersion = dbgen.TemplateVersion(t, db, database.TemplateVersion{
112+
OrganizationID: org.ID,
113+
CreatedBy: user.ID,
114+
})
115+
template = dbgen.Template(t, db, database.Template{
116+
OrganizationID: org.ID,
117+
ActiveVersionID: templateVersion.ID,
118+
CreatedBy: user.ID,
119+
})
120+
ws = dbgen.Workspace(t, db, database.Workspace{
121+
OwnerID: user.ID,
122+
OrganizationID: org.ID,
123+
TemplateID: template.ID,
124+
Ttl: sql.NullInt64{Valid: true, Int64: int64(tt.workspaceTTL)},
125+
})
126+
job = dbgen.ProvisionerJob(t, db, database.ProvisionerJob{
127+
OrganizationID: org.ID,
128+
CompletedAt: tt.jobCompletedAt,
129+
})
130+
_ = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
131+
JobID: job.ID,
132+
})
133+
buildID = uuid.New()
134+
)
135+
136+
var buildNumber int32 = 1
137+
// Insert a number of previous workspace builds.
138+
for i := 0; i < 5; i++ {
139+
insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStart, buildNumber)
140+
buildNumber++
141+
insertPrevWorkspaceBuild(t, db, org.ID, templateVersion.ID, ws.ID, database.WorkspaceTransitionStop, buildNumber)
142+
buildNumber++
143+
}
144+
145+
// dbgen.WorkspaceBuild automatically sets deadline to now+1 hour if not set
146+
var buildDeadline time.Time
147+
if tt.buildDeadlineOffset != nil {
148+
buildDeadline = now.Add(*tt.buildDeadlineOffset)
149+
}
150+
var maxDeadline time.Time
151+
if tt.maxDeadlineOffset != nil {
152+
maxDeadline = now.Add(*tt.maxDeadlineOffset)
153+
}
154+
err := db.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{
155+
ID: buildID,
156+
CreatedAt: dbtime.Now(),
157+
UpdatedAt: dbtime.Now(),
158+
BuildNumber: buildNumber,
159+
InitiatorID: user.ID,
160+
Reason: database.BuildReasonInitiator,
161+
WorkspaceID: ws.ID,
162+
JobID: job.ID,
163+
TemplateVersionID: templateVersion.ID,
164+
Transition: tt.transition,
165+
Deadline: buildDeadline,
166+
MaxDeadline: maxDeadline,
167+
})
168+
require.NoError(t, err, "unexpected error inserting workspace build")
169+
bld, err := db.GetWorkspaceBuildByID(ctx, buildID)
170+
require.NoError(t, err, "unexpected error fetching inserted workspace build")
171+
172+
// Validate our initial state before bump
173+
require.Equal(t, tt.transition, bld.Transition, "unexpected transition before bump")
174+
require.Equal(t, tt.jobCompletedAt.Time.UTC(), job.CompletedAt.Time.UTC(), "unexpected job completed at before bump")
175+
require.Equal(t, buildDeadline.UTC(), bld.Deadline.UTC(), "unexpected build deadline before bump")
176+
require.Equal(t, maxDeadline.UTC(), bld.MaxDeadline.UTC(), "unexpected max deadline before bump")
177+
require.Equal(t, tt.workspaceTTL, time.Duration(ws.Ttl.Int64), "unexpected workspace TTL before bump")
178+
179+
workaroundWindowsTimeResolution(t)
180+
181+
// Bump duration is measured from the time of the bump, so we measure from here.
182+
start := dbtime.Now()
183+
activityBumpWorkspace(ctx, log, db, bld.WorkspaceID)
184+
elapsed := time.Since(start)
185+
if elapsed > 15*time.Second {
186+
t.Logf("warning: activityBumpWorkspace took longer than 15 seconds: %s", elapsed)
187+
}
188+
// The actual bump could have happened anywhere in the elapsed time, so we
189+
// guess at the approximate time of the bump.
190+
approxBumpTime := start.Add(elapsed / 2)
191+
192+
// Validate our state after bump
193+
updatedBuild, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, bld.WorkspaceID)
194+
require.NoError(t, err, "unexpected error getting latest workspace build")
195+
if tt.expectedBump == 0 {
196+
require.Equal(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should not have bumped updated_at")
197+
require.Equal(t, bld.Deadline.UTC(), updatedBuild.Deadline.UTC(), "should not have bumped deadline")
198+
} else {
199+
require.NotEqual(t, bld.UpdatedAt.UTC(), updatedBuild.UpdatedAt.UTC(), "should have bumped updated_at")
200+
expectedDeadline := approxBumpTime.Add(tt.expectedBump).UTC()
201+
// Note: if CI is especially slow, this test may fail. There is an internal 15-second
202+
// deadline in activityBumpWorkspace, so we allow the same window here.
203+
require.WithinDuration(t, expectedDeadline, updatedBuild.Deadline.UTC(), 15*time.Second, "unexpected deadline after bump")
204+
}
205+
})
206+
}
207+
}
208+
209+
func insertPrevWorkspaceBuild(t *testing.T, db database.Store, orgID, tvID, workspaceID uuid.UUID, transition database.WorkspaceTransition, buildNumber int32) {
210+
t.Helper()
211+
212+
job := dbgen.ProvisionerJob(t, db, database.ProvisionerJob{
213+
OrganizationID: orgID,
214+
})
215+
_ = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
216+
JobID: job.ID,
217+
})
218+
_ = dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
219+
BuildNumber: buildNumber,
220+
WorkspaceID: workspaceID,
221+
JobID: job.ID,
222+
TemplateVersionID: tvID,
223+
Transition: transition,
224+
})
225+
}
226+
227+
func workaroundWindowsTimeResolution(t *testing.T) {
228+
t.Helper()
229+
if runtime.GOOS == "windows" {
230+
t.Logf("workaround: sleeping for a short time to avoid time resolution issues on Windows")
231+
<-time.After(testutil.IntervalSlow)
232+
}
233+
}

‎coderd/database/dbauthz/dbauthz.go

+7
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,13 @@ func (q *querier) AcquireProvisionerJob(ctx context.Context, arg database.Acquir
657657
return q.db.AcquireProvisionerJob(ctx, arg)
658658
}
659659

660+
func (q *querier) ActivityBumpWorkspace(ctx context.Context, arg uuid.UUID) error {
661+
fetch := func(ctx context.Context, arg uuid.UUID) (database.Workspace, error) {
662+
return q.db.GetWorkspaceByID(ctx, arg)
663+
}
664+
return update(q.log, q.auth, fetch, q.db.ActivityBumpWorkspace)(ctx, arg)
665+
}
666+
660667
func (q *querier) CleanTailnetCoordinators(ctx context.Context) error {
661668
if err := q.authorizeContext(ctx, rbac.ActionDelete, rbac.ResourceTailnetCoordinator); err != nil {
662669
return err

0 commit comments

Comments
 (0)