Skip to content

Commit dd86100

Browse files
johnstcnjohnstcn
authored
fix(scaletest): fix flake in Test_Runner/Cleanup (coder#10252)
* fix(scaletest/createworkspaces): address flake in Test_Runner/CleanupPendingBuild * fix(scaletest): pass io.Writer to Cleanup() * add some extra logs to workspacebuild cleanup * fixup! fix(scaletest): pass io.Writer to Cleanup() * remove race * fmt * address PR comments
1 parent 1be24dc commit dd86100

File tree

10 files changed

+60
-31
lines changed

10 files changed

+60
-31
lines changed

‎cli/exp_scaletest.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1241,7 +1241,7 @@ func (r *runnableTraceWrapper) Run(ctx context.Context, id string, logs io.Write
12411241
return r.runner.Run(ctx2, id, logs)
12421242
}
12431243

1244-
func (r *runnableTraceWrapper) Cleanup(ctx context.Context, id string) error {
1244+
func (r *runnableTraceWrapper) Cleanup(ctx context.Context, id string, logs io.Writer) error {
12451245
c, ok := r.runner.(harness.Cleanable)
12461246
if !ok {
12471247
return nil
@@ -1253,7 +1253,7 @@ func (r *runnableTraceWrapper) Cleanup(ctx context.Context, id string) error {
12531253
ctx, span := r.tracer.Start(ctx, r.spanName+" cleanup")
12541254
defer span.End()
12551255

1256-
return c.Cleanup(ctx, id)
1256+
return c.Cleanup(ctx, id, logs)
12571257
}
12581258

12591259
// newScaleTestUser returns a random username and email address that can be used

‎scaletest/createworkspaces/run.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,13 +176,14 @@ resourceLoop:
176176
}
177177

178178
// Cleanup implements Cleanable.
179-
func (r *Runner) Cleanup(ctx context.Context, id string) error {
179+
func (r *Runner) Cleanup(ctx context.Context, id string, logs io.Writer) error {
180180
if r.cfg.NoCleanup {
181+
_, _ = fmt.Fprintln(logs, "skipping cleanup")
181182
return nil
182183
}
183184

184185
if r.workspacebuildRunner != nil {
185-
err := r.workspacebuildRunner.Cleanup(ctx, id)
186+
err := r.workspacebuildRunner.Cleanup(ctx, id, logs)
186187
if err != nil {
187188
return xerrors.Errorf("cleanup workspace: %w", err)
188189
}
@@ -191,6 +192,7 @@ func (r *Runner) Cleanup(ctx context.Context, id string) error {
191192
if r.userID != uuid.Nil {
192193
err := r.client.DeleteUser(ctx, r.userID)
193194
if err != nil {
195+
_, _ = fmt.Fprintf(logs, "failed to delete user %q: %v\n", r.userID.String(), err)
194196
return xerrors.Errorf("delete user: %w", err)
195197
}
196198
}

‎scaletest/createworkspaces/run_test.go

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,13 @@ func Test_Runner(t *testing.T) {
177177
require.Contains(t, logsStr, "Opening reconnecting PTY connection to agent")
178178
require.Contains(t, logsStr, "Opening connection to workspace agent")
179179

180-
err = runner.Cleanup(ctx, "1")
180+
cleanupLogs := bytes.NewBuffer(nil)
181+
err = runner.Cleanup(ctx, "1", cleanupLogs)
181182
require.NoError(t, err)
183+
cleanupLogsStr := cleanupLogs.String()
184+
require.Contains(t, cleanupLogsStr, "deleting workspace")
185+
require.NotContains(t, cleanupLogsStr, "canceling workspace build") // The build should have already completed.
186+
require.Contains(t, cleanupLogsStr, "Build succeeded!")
182187

183188
// Ensure the user and workspace were deleted.
184189
users, err = client.Users(ctx, codersdk.UsersRequest{})
@@ -217,7 +222,7 @@ func Test_Runner(t *testing.T) {
217222
},
218223
ProvisionApply: []*proto.Response{
219224
{
220-
Type: &proto.Response_Log{Log: &proto.Log{}},
225+
Type: &proto.Response_Log{Log: &proto.Log{}}, // This provisioner job will never complete.
221226
},
222227
},
223228
})
@@ -257,24 +262,36 @@ func Test_Runner(t *testing.T) {
257262
close(done)
258263
}()
259264

265+
// Wait for the workspace build job to be picked up.
260266
require.Eventually(t, func() bool {
261267
workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{})
262268
if err != nil {
263269
return false
264270
}
271+
if len(workspaces.Workspaces) == 0 {
272+
return false
273+
}
265274

266-
return len(workspaces.Workspaces) > 0
267-
}, testutil.WaitShort, testutil.IntervalFast)
275+
ws := workspaces.Workspaces[0]
276+
t.Logf("checking build: %s | %s", ws.LatestBuild.Transition, ws.LatestBuild.Job.Status)
277+
// There should be only one build at present.
278+
if ws.LatestBuild.Transition != codersdk.WorkspaceTransitionStart {
279+
t.Errorf("expected build transition %s, got %s", codersdk.WorkspaceTransitionStart, ws.LatestBuild.Transition)
280+
return false
281+
}
282+
return ws.LatestBuild.Job.Status == codersdk.ProvisionerJobRunning
283+
}, testutil.WaitShort, testutil.IntervalMedium)
268284

269285
cancelFunc()
270286
<-done
271287

272288
// When we run the cleanup, it should be canceled
289+
cleanupLogs := bytes.NewBuffer(nil)
273290
cancelCtx, cancelFunc = context.WithCancel(ctx)
274291
done = make(chan struct{})
275292
go func() {
276293
// This will return an error as the "delete" operation will never complete.
277-
_ = runner.Cleanup(cancelCtx, "1")
294+
_ = runner.Cleanup(cancelCtx, "1", cleanupLogs)
278295
close(done)
279296
}()
280297

@@ -311,9 +328,11 @@ func Test_Runner(t *testing.T) {
311328
}
312329
}
313330
return false
314-
}, testutil.WaitShort, testutil.IntervalFast)
331+
}, testutil.WaitShort, testutil.IntervalMedium)
315332
cancelFunc()
316333
<-done
334+
cleanupLogsStr := cleanupLogs.String()
335+
require.Contains(t, cleanupLogsStr, "canceling workspace build")
317336
})
318337

319338
t.Run("NoCleanup", func(t *testing.T) {
@@ -447,7 +466,8 @@ func Test_Runner(t *testing.T) {
447466
require.Contains(t, logsStr, "Opening reconnecting PTY connection to agent")
448467
require.Contains(t, logsStr, "Opening connection to workspace agent")
449468

450-
err = runner.Cleanup(ctx, "1")
469+
cleanupLogs := bytes.NewBuffer(nil)
470+
err = runner.Cleanup(ctx, "1", cleanupLogs)
451471
require.NoError(t, err)
452472

453473
// Ensure the user and workspace were not deleted.

‎scaletest/dashboard/run.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,6 @@ func (r *Runner) runUntilDeadlineExceeded(ctx context.Context) error {
125125
}
126126
}
127127

128-
func (*Runner) Cleanup(_ context.Context, _ string) error {
128+
func (*Runner) Cleanup(_ context.Context, _ string, _ io.Writer) error {
129129
return nil
130130
}

‎scaletest/harness/harness_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ func Test_TestHarness(t *testing.T) {
112112
RunFn: func(_ context.Context, _ string, _ io.Writer) error {
113113
return nil
114114
},
115-
CleanupFn: func(_ context.Context, _ string) error {
115+
CleanupFn: func(_ context.Context, _ string, _ io.Writer) error {
116116
panic(testPanicMessage)
117117
},
118118
})
@@ -150,7 +150,7 @@ func Test_TestHarness(t *testing.T) {
150150
RunFn: func(_ context.Context, _ string, _ io.Writer) error {
151151
return nil
152152
},
153-
CleanupFn: func(_ context.Context, _ string) error {
153+
CleanupFn: func(_ context.Context, _ string, _ io.Writer) error {
154154
return nil
155155
},
156156
})
@@ -295,7 +295,7 @@ func fakeTestFns(err, cleanupErr error) testFns {
295295
RunFn: func(_ context.Context, _ string, _ io.Writer) error {
296296
return err
297297
},
298-
CleanupFn: func(_ context.Context, _ string) error {
298+
CleanupFn: func(_ context.Context, _ string, _ io.Writer) error {
299299
return cleanupErr
300300
},
301301
}

‎scaletest/harness/run.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ type Runnable interface {
2828
type Cleanable interface {
2929
Runnable
3030
// Cleanup should clean up any lingering resources from the test.
31-
Cleanup(ctx context.Context, id string) error
31+
Cleanup(ctx context.Context, id string, logs io.Writer) error
3232
}
3333

3434
// AddRun creates a new *TestRun with the given name, ID and Runnable, adds it
@@ -131,7 +131,7 @@ func (r *TestRun) Cleanup(ctx context.Context) (err error) {
131131
}
132132
}()
133133

134-
err = c.Cleanup(ctx, r.id)
134+
err = c.Cleanup(ctx, r.id, r.logs)
135135
//nolint:revive // we use named returns because we mutate it in a defer
136136
return
137137
}

‎scaletest/harness/run_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import (
1616
type testFns struct {
1717
RunFn func(ctx context.Context, id string, logs io.Writer) error
1818
// CleanupFn is optional if no cleanup is required.
19-
CleanupFn func(ctx context.Context, id string) error
19+
CleanupFn func(ctx context.Context, id string, logs io.Writer) error
2020
}
2121

2222
// Run implements Runnable.
@@ -25,12 +25,12 @@ func (fns testFns) Run(ctx context.Context, id string, logs io.Writer) error {
2525
}
2626

2727
// Cleanup implements Cleanable.
28-
func (fns testFns) Cleanup(ctx context.Context, id string) error {
28+
func (fns testFns) Cleanup(ctx context.Context, id string, logs io.Writer) error {
2929
if fns.CleanupFn == nil {
3030
return nil
3131
}
3232

33-
return fns.CleanupFn(ctx, id)
33+
return fns.CleanupFn(ctx, id, logs)
3434
}
3535

3636
func Test_TestRun(t *testing.T) {
@@ -49,7 +49,7 @@ func Test_TestRun(t *testing.T) {
4949
atomic.AddInt64(&runCalled, 1)
5050
return nil
5151
},
52-
CleanupFn: func(ctx context.Context, id string) error {
52+
CleanupFn: func(ctx context.Context, id string, logs io.Writer) error {
5353
atomic.AddInt64(&cleanupCalled, 1)
5454
return nil
5555
},
@@ -93,7 +93,7 @@ func Test_TestRun(t *testing.T) {
9393
RunFn: func(ctx context.Context, id string, logs io.Writer) error {
9494
return nil
9595
},
96-
CleanupFn: func(ctx context.Context, id string) error {
96+
CleanupFn: func(ctx context.Context, id string, logs io.Writer) error {
9797
atomic.AddInt64(&cleanupCalled, 1)
9898
return nil
9999
},

‎scaletest/workspacebuild/run.go

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"io"
7+
"net/http"
78
"time"
89

910
"github.com/google/uuid"
@@ -106,25 +107,31 @@ func NewCleanupRunner(client *codersdk.Client, workspaceID uuid.UUID) *CleanupRu
106107

107108
// Run implements Runnable.
108109
func (r *CleanupRunner) Run(ctx context.Context, _ string, logs io.Writer) error {
109-
if r.workspaceID == uuid.Nil {
110-
return nil
111-
}
112110
ctx, span := tracing.StartSpan(ctx)
113111
defer span.End()
114-
115112
logs = loadtestutil.NewSyncWriter(logs)
116113
logger := slog.Make(sloghuman.Sink(logs)).Leveled(slog.LevelDebug)
114+
if r.workspaceID == uuid.Nil {
115+
return nil
116+
}
117+
logger.Info(ctx, "deleting workspace", slog.F("workspace_id", r.workspaceID))
117118
r.client.SetLogger(logger)
118119
r.client.SetLogBodies(true)
119120

120121
ws, err := r.client.Workspace(ctx, r.workspaceID)
121122
if err != nil {
123+
var sdkErr *codersdk.Error
124+
if xerrors.As(err, &sdkErr) && sdkErr.StatusCode() == http.StatusNotFound {
125+
logger.Info(ctx, "workspace not found, skipping delete", slog.F("workspace_id", r.workspaceID))
126+
return nil
127+
}
122128
return err
123129
}
124130

125131
build, err := r.client.WorkspaceBuild(ctx, ws.LatestBuild.ID)
126132
if err == nil && build.Job.Status.Active() {
127133
// mark the build as canceled
134+
logger.Info(ctx, "canceling workspace build", slog.F("build_id", build.ID), slog.F("workspace_id", r.workspaceID))
128135
if err = r.client.CancelWorkspaceBuild(ctx, build.ID); err == nil {
129136
// Wait for the job to cancel before we delete it
130137
_ = waitForBuild(ctx, logs, r.client, build.ID) // it will return a "build canceled" error
@@ -151,12 +158,11 @@ func (r *CleanupRunner) Run(ctx context.Context, _ string, logs io.Writer) error
151158
}
152159

153160
// Cleanup implements Cleanable by wrapping CleanupRunner.
154-
func (r *Runner) Cleanup(ctx context.Context, id string) error {
155-
// TODO: capture these logs
161+
func (r *Runner) Cleanup(ctx context.Context, id string, w io.Writer) error {
156162
return (&CleanupRunner{
157163
client: r.client,
158164
workspaceID: r.workspaceID,
159-
}).Run(ctx, id, io.Discard)
165+
}).Run(ctx, id, w)
160166
}
161167

162168
func waitForBuild(ctx context.Context, w io.Writer, client *codersdk.Client, buildID uuid.UUID) error {

‎scaletest/workspacebuild/run_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,8 @@ func Test_Runner(t *testing.T) {
180180
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspaces[0].LatestBuild.ID)
181181
coderdtest.AwaitWorkspaceAgents(t, client, workspaces[0].ID)
182182

183-
err = runner.Cleanup(ctx, "1")
183+
cleanupLogs := bytes.NewBuffer(nil)
184+
err = runner.Cleanup(ctx, "1", cleanupLogs)
184185
require.NoError(t, err)
185186
})
186187

‎scaletest/workspacetraffic/run.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ func (r *Runner) Run(ctx context.Context, _ string, logs io.Writer) error {
169169
}
170170

171171
// Cleanup does nothing, successfully.
172-
func (*Runner) Cleanup(context.Context, string) error {
172+
func (*Runner) Cleanup(context.Context, string, io.Writer) error {
173173
return nil
174174
}
175175

0 commit comments

Comments
 (0)