Skip to content

Commit 7fcf319

Browse files
johnstcnjohnstcn
authored
fix(cli)!: protect client Logger and refactor cli scaletest tests (#8317)
- (breaking) Protects Logger and LogBodies fields of codersdk.Client with its mutex. This addresses a data race in cli/scaletest. - Fillets the existing cli/createworkspaces unit test and moves the testing logic there into the tests under scaletest/createworkspaces. - Adds testutil.RaceEnabled bool const and conditionaly skips previously-skipped tests under scaletest/ if the race detector is enabled. This is unfortunate and sad, but I would prefer to have these tests at least running without the race detector than not running at all. - Adds IgnoreErrors option to fake in-memory agent loggers; having the agents fail the test immediately when they encounter any sort of error isn't really helpful.
1 parent 1d746b9 commit 7fcf319

File tree

20 files changed

+309
-254
lines changed

20 files changed

+309
-254
lines changed

‎cli/agent.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ func (r *RootCmd) workspaceAgent() *clibase.Cmd {
167167
slog.F("version", version),
168168
)
169169
client := agentsdk.New(r.agentURL)
170-
client.SDK.Logger = logger
170+
client.SDK.SetLogger(logger)
171171
// Set a reasonable timeout so requests can't hang forever!
172172
// The timeout needs to be reasonably long, because requests
173173
// with large payloads can take a bit. e.g. startup scripts

‎cli/root.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,7 @@ func (r *RootCmd) InitClient(client *codersdk.Client) clibase.MiddlewareFunc {
543543

544544
if r.debugHTTP {
545545
client.PlainLogger = os.Stderr
546-
client.LogBodies = true
546+
client.SetLogBodies(true)
547547
}
548548
client.DisableDirectConnections = r.disableDirect
549549

‎cli/scaletest_test.go

Lines changed: 30 additions & 178 deletions
Original file line numberDiff line numberDiff line change
@@ -3,201 +3,53 @@ package cli_test
33
import (
44
"bytes"
55
"context"
6-
"encoding/json"
7-
"os"
86
"path/filepath"
97
"testing"
108

11-
"github.com/stretchr/testify/assert"
129
"github.com/stretchr/testify/require"
1310

1411
"github.com/coder/coder/cli/clitest"
1512
"github.com/coder/coder/coderd/coderdtest"
16-
"github.com/coder/coder/codersdk"
1713
"github.com/coder/coder/pty/ptytest"
18-
"github.com/coder/coder/scaletest/harness"
1914
"github.com/coder/coder/testutil"
2015
)
2116

2217
func TestScaleTestCreateWorkspaces(t *testing.T) {
23-
t.Skipf("This test is flakey. See https://github.com/coder/coder/issues/4942")
2418
t.Parallel()
2519

26-
// This test does a create-workspaces scale test with --no-cleanup, checks
27-
// that the created resources are OK, and then runs a cleanup.
28-
t.Run("WorkspaceBuildNoCleanup", func(t *testing.T) {
29-
t.Parallel()
30-
31-
ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitLong)
32-
defer cancelFunc()
33-
34-
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
35-
user := coderdtest.CreateFirstUser(t, client)
36-
37-
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
38-
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
39-
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
40-
41-
// Write a parameters file.
42-
tDir := t.TempDir()
43-
paramsFile := filepath.Join(tDir, "params.yaml")
44-
outputFile := filepath.Join(tDir, "output.json")
45-
46-
f, err := os.Create(paramsFile)
47-
require.NoError(t, err)
48-
defer f.Close()
49-
_, err = f.WriteString(`---
50-
param1: foo
51-
param2: true
52-
param3: 1
53-
`)
54-
require.NoError(t, err)
55-
err = f.Close()
56-
require.NoError(t, err)
57-
58-
inv, root := clitest.New(t, "scaletest", "create-workspaces",
59-
"--count", "2",
60-
"--template", template.Name,
61-
"--parameters-file", paramsFile,
62-
"--parameter", "param1=bar",
63-
"--parameter", "param4=baz",
64-
"--no-cleanup",
65-
// This flag is important for tests because agents will never be
66-
// started.
67-
"--no-wait-for-agents",
68-
// Run and connect flags cannot be tested because they require an
69-
// agent.
70-
"--concurrency", "2",
71-
"--timeout", "30s",
72-
"--job-timeout", "15s",
73-
"--cleanup-concurrency", "1",
74-
"--cleanup-timeout", "30s",
75-
"--cleanup-job-timeout", "15s",
76-
"--output", "text",
77-
"--output", "json:"+outputFile,
78-
)
79-
clitest.SetupConfig(t, client, root)
80-
pty := ptytest.New(t)
81-
inv.Stdout = pty.Output()
82-
inv.Stderr = pty.Output()
83-
84-
done := make(chan any)
85-
go func() {
86-
err := inv.WithContext(ctx).Run()
87-
assert.NoError(t, err)
88-
close(done)
89-
}()
90-
pty.ExpectMatch("Test results:")
91-
pty.ExpectMatch("Pass: 2")
92-
select {
93-
case <-done:
94-
case <-ctx.Done():
95-
}
96-
cancelFunc()
97-
<-done
98-
99-
// Recreate the context.
100-
ctx, cancelFunc = context.WithTimeout(context.Background(), testutil.WaitLong)
101-
defer cancelFunc()
102-
103-
// Verify the output file.
104-
f, err = os.Open(outputFile)
105-
require.NoError(t, err)
106-
defer f.Close()
107-
var res harness.Results
108-
err = json.NewDecoder(f).Decode(&res)
109-
require.NoError(t, err)
110-
111-
require.EqualValues(t, 2, res.TotalRuns)
112-
require.EqualValues(t, 2, res.TotalPass)
113-
114-
// Find the workspaces and users and check that they are what we expect.
115-
workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{
116-
Offset: 0,
117-
Limit: 100,
118-
})
119-
require.NoError(t, err)
120-
require.Len(t, workspaces.Workspaces, 2)
121-
122-
seenUsers := map[string]struct{}{}
123-
for _, w := range workspaces.Workspaces {
124-
// Sadly we can't verify params as the API doesn't seem to return
125-
// them.
126-
127-
// Verify that the user is a unique scaletest user.
128-
u, err := client.User(ctx, w.OwnerID.String())
129-
require.NoError(t, err)
130-
131-
_, ok := seenUsers[u.ID.String()]
132-
require.False(t, ok, "user has more than one workspace")
133-
seenUsers[u.ID.String()] = struct{}{}
134-
135-
require.Contains(t, u.Username, "scaletest-")
136-
require.Contains(t, u.Email, "scaletest")
137-
}
138-
139-
require.Len(t, seenUsers, len(workspaces.Workspaces))
140-
141-
// Check that there are exactly 3 users.
142-
users, err := client.Users(ctx, codersdk.UsersRequest{
143-
Pagination: codersdk.Pagination{
144-
Offset: 0,
145-
Limit: 100,
146-
},
147-
})
148-
require.NoError(t, err)
149-
require.Len(t, users.Users, len(seenUsers)+1)
150-
151-
// Cleanup.
152-
inv, root = clitest.New(t, "scaletest", "cleanup",
153-
"--cleanup-concurrency", "1",
154-
"--cleanup-timeout", "30s",
155-
"--cleanup-job-timeout", "15s",
156-
)
157-
clitest.SetupConfig(t, client, root)
158-
pty = ptytest.New(t)
159-
inv.Stdout = pty.Output()
160-
inv.Stderr = pty.Output()
161-
162-
done = make(chan any)
163-
go func() {
164-
err := inv.WithContext(ctx).Run()
165-
assert.NoError(t, err)
166-
close(done)
167-
}()
168-
pty.ExpectMatch("Test results:")
169-
pty.ExpectMatch("Pass: 2")
170-
pty.ExpectMatch("Test results:")
171-
pty.ExpectMatch("Pass: 2")
172-
select {
173-
case <-done:
174-
case <-ctx.Done():
175-
}
176-
cancelFunc()
177-
<-done
20+
// This test only validates that the CLI command accepts known arguments.
21+
// More thorough testing is done in scaletest/createworkspaces/run_test.go.
22+
ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitLong)
23+
defer cancelFunc()
17824

179-
// Recreate the context (again).
180-
ctx, cancelFunc = context.WithTimeout(context.Background(), testutil.WaitLong)
181-
defer cancelFunc()
25+
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
26+
_ = coderdtest.CreateFirstUser(t, client)
18227

183-
// Verify that the workspaces are gone.
184-
workspaces, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
185-
Offset: 0,
186-
Limit: 100,
187-
})
188-
require.NoError(t, err)
189-
require.Len(t, workspaces.Workspaces, 0)
28+
// Write a parameters file.
29+
tDir := t.TempDir()
30+
outputFile := filepath.Join(tDir, "output.json")
31+
32+
inv, root := clitest.New(t, "scaletest", "create-workspaces",
33+
"--count", "2",
34+
"--template", "doesnotexist",
35+
"--no-cleanup",
36+
"--no-wait-for-agents",
37+
"--concurrency", "2",
38+
"--timeout", "30s",
39+
"--job-timeout", "15s",
40+
"--cleanup-concurrency", "1",
41+
"--cleanup-timeout", "30s",
42+
"--cleanup-job-timeout", "15s",
43+
"--output", "text",
44+
"--output", "json:"+outputFile,
45+
)
46+
clitest.SetupConfig(t, client, root)
47+
pty := ptytest.New(t)
48+
inv.Stdout = pty.Output()
49+
inv.Stderr = pty.Output()
19050

191-
// Verify that the users are gone.
192-
users, err = client.Users(ctx, codersdk.UsersRequest{
193-
Pagination: codersdk.Pagination{
194-
Offset: 0,
195-
Limit: 100,
196-
},
197-
})
198-
require.NoError(t, err)
199-
require.Len(t, users.Users, 1)
200-
})
51+
err := inv.WithContext(ctx).Run()
52+
require.ErrorContains(t, err, "could not find template \"doesnotexist\" in any organization")
20153
}
20254

20355
// This test just validates that the CLI command accepts its known arguments.

‎cli/ssh.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ func (r *RootCmd) ssh() *clibase.Cmd {
119119
}
120120

121121
// log HTTP requests
122-
client.Logger = logger
122+
client.SetLogger(logger)
123123
}
124124

125125
workspace, workspaceAgent, err := getWorkspaceAndAgent(ctx, inv, client, codersdk.Me, inv.Args[0])

‎cli/ssh_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ func setupWorkspaceForAgent(t *testing.T, mutate func([]*proto.Agent) []*proto.A
4949
}
5050
}
5151
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
52-
client.Logger = slogtest.Make(t, nil).Named("client").Leveled(slog.LevelDebug)
52+
client.SetLogger(slogtest.Make(t, nil).Named("client").Leveled(slog.LevelDebug))
5353
user := coderdtest.CreateFirstUser(t, client)
5454
agentToken := uuid.NewString()
5555
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{

‎coderd/deployment_test.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"testing"
66

7+
"github.com/stretchr/testify/assert"
78
"github.com/stretchr/testify/require"
89

910
"github.com/coder/coder/coderd/coderdtest"
@@ -47,10 +48,13 @@ func TestDeploymentValues(t *testing.T) {
4748

4849
func TestDeploymentStats(t *testing.T) {
4950
t.Parallel()
51+
t.Log("This test is time-sensitive. It may fail if the deployment is not ready in time.")
5052
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
5153
defer cancel()
5254
client := coderdtest.New(t, &coderdtest.Options{})
5355
_ = coderdtest.CreateFirstUser(t, client)
54-
_, err := client.DeploymentStats(ctx)
55-
require.NoError(t, err)
56+
assert.True(t, testutil.Eventually(ctx, t, func(tctx context.Context) bool {
57+
_, err := client.DeploymentStats(tctx)
58+
return err == nil
59+
}, testutil.IntervalMedium), "failed to get deployment stats in time")
5660
}

‎codersdk/agentsdk/agentsdk.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ func (c *Client) Listen(ctx context.Context) (net.Conn, error) {
194194
ticker := time.NewTicker(tick)
195195
defer ticker.Stop()
196196
defer func() {
197-
c.SDK.Logger.Debug(ctx, "coordinate pinger exited")
197+
c.SDK.Logger().Debug(ctx, "coordinate pinger exited")
198198
}()
199199
for {
200200
select {
@@ -205,18 +205,18 @@ func (c *Client) Listen(ctx context.Context) (net.Conn, error) {
205205

206206
err := conn.Ping(ctx)
207207
if err != nil {
208-
c.SDK.Logger.Error(ctx, "workspace agent coordinate ping", slog.Error(err))
208+
c.SDK.Logger().Error(ctx, "workspace agent coordinate ping", slog.Error(err))
209209

210210
err := conn.Close(websocket.StatusGoingAway, "Ping failed")
211211
if err != nil {
212-
c.SDK.Logger.Error(ctx, "close workspace agent coordinate websocket", slog.Error(err))
212+
c.SDK.Logger().Error(ctx, "close workspace agent coordinate websocket", slog.Error(err))
213213
}
214214

215215
cancel()
216216
return
217217
}
218218

219-
c.SDK.Logger.Debug(ctx, "got coordinate pong", slog.F("took", time.Since(start)))
219+
c.SDK.Logger().Debug(ctx, "got coordinate pong", slog.F("took", time.Since(start)))
220220
cancel()
221221
}
222222
}

0 commit comments

Comments
 (0)