Skip to content

Commit 1ceac51

Browse files
spikecurtisspikecurtis
committed
code review improvements & fixes
Signed-off-by: Spike Curtis <spike@coder.com>
1 parent 1e53245 commit 1ceac51

File tree

2 files changed

+38
-34
lines changed

2 files changed

+38
-34
lines changed

‎coderd/provisionerdserver/acquirer.go

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,9 @@ import (
2121
)
2222

2323
const (
24-
EventJobPosted = "provisioner_job_posted"
25-
dbMaxBackoff = 10 * time.Second
24+
EventJobPosted = "provisioner_job_posted"
25+
dbMaxBackoff = 10 * time.Second
26+
// backPollDuration is the period for the backup polling described in Acquirer comment
2627
backupPollDuration = 30 * time.Second
2728
)
2829

@@ -201,17 +202,17 @@ func (a *Acquirer) cancel(dk dKey, clearance chan<- struct{}) error {
201202
defer a.mu.Unlock()
202203
d, ok := a.q[dk]
203204
if !ok {
204-
// this is a code error, as something removed the dKey early, or cancel
205+
// this is a code error, as something removed the domain early, or cancel
205206
// was called twice.
206-
err := xerrors.New("canceled non-existent job acquisition")
207+
err := xerrors.New("cancel for domain that doesn't exist")
207208
a.logger.Critical(a.ctx, "internal error", slog.Error(err))
208209
return err
209210
}
210211
w, ok := d.acquirees[clearance]
211212
if !ok {
212-
// this is a code error, as something removed the dKey early, or cancel
213+
// this is a code error, as something removed the acquiree early, or cancel
213214
// was called twice.
214-
err := xerrors.New("canceled non-existent job acquisition")
215+
err := xerrors.New("cancel for an acquiree that doesn't exist")
215216
a.logger.Critical(a.ctx, "internal error", slog.Error(err))
216217
return err
217218
}
@@ -245,17 +246,17 @@ func (a *Acquirer) done(dk dKey, clearance chan struct{}) error {
245246
defer a.mu.Unlock()
246247
d, ok := a.q[dk]
247248
if !ok {
248-
// this is a code error, as something removed the dKey early, or done
249+
// this is a code error, as something removed the domain early, or done
249250
// was called twice.
250-
err := xerrors.New("done with non-existent job acquisition")
251+
err := xerrors.New("done for a domain that doesn't exist")
251252
a.logger.Critical(a.ctx, "internal error", slog.Error(err))
252253
return err
253254
}
254255
w, ok := d.acquirees[clearance]
255256
if !ok {
256257
// this is a code error, as something removed the dKey early, or done
257258
// was called twice.
258-
err := xerrors.New("canceled non-existent job acquisition")
259+
err := xerrors.New("done for an acquiree that doesn't exist")
259260
a.logger.Critical(a.ctx, "internal error", slog.Error(err))
260261
return err
261262
}
@@ -401,9 +402,19 @@ func (*Acquirer) clearOrPendLocked(d domain) {
401402

402403
type dKey string
403404

405+
// domainKey generates a canonical map key for the given provisioner types and
406+
// tags. It uses the null byte (0x00) as a delimiter because it is an
407+
// unprintable control character and won't show up in any "reasonable" set of
408+
// string tags, even in non-Latin scripts. It is important that Tags are
409+
// validated not to contain this control character prior to use.
404410
func domainKey(pt []database.ProvisionerType, tags Tags) dKey {
411+
// make a copy of pt before sorting, so that we don't mutate the original
412+
// slice or underlying array.
413+
pts := make([]database.ProvisionerType, len(pt))
414+
copy(pts, pt)
415+
slices.Sort(pts)
405416
sb := strings.Builder{}
406-
for _, t := range pt {
417+
for _, t := range pts {
407418
_, _ = sb.WriteString(string(t))
408419
_ = sb.WriteByte(0x00)
409420
}

‎coderd/provisionerdserver/acquirer_test.go

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,8 @@ func TestAcquirer_Single(t *testing.T) {
5454
}
5555
acquiree := newTestAcquiree(t, workerID, pt, tags)
5656
jobID := uuid.New()
57-
go func() {
58-
err := fs.sendCtx(ctx, database.ProvisionerJob{ID: jobID}, nil)
59-
assert.NoError(t, err)
60-
}()
57+
err := fs.sendCtx(ctx, database.ProvisionerJob{ID: jobID}, nil)
58+
require.NoError(t, err)
6159
acquiree.startAcquire(ctx, uut)
6260
job := acquiree.success(ctx)
6361
require.Equal(t, jobID, job.ID)
@@ -89,14 +87,12 @@ func TestAcquirer_MultipleSameDomain(t *testing.T) {
8987
acquirees = append(acquirees, a)
9088
a.startAcquire(ctx, uut)
9189
}
92-
go func() {
93-
for i := 0; i < 10; i++ {
94-
jobID := uuid.New()
95-
jobIDs[jobID] = true
96-
err := fs.sendCtx(ctx, database.ProvisionerJob{ID: jobID}, nil)
97-
assert.NoError(t, err)
98-
}
99-
}()
90+
for i := 0; i < 10; i++ {
91+
jobID := uuid.New()
92+
jobIDs[jobID] = true
93+
err := fs.sendCtx(ctx, database.ProvisionerJob{ID: jobID}, nil)
94+
require.NoError(t, err)
95+
}
10096
gotJobIDs := make(map[uuid.UUID]bool)
10197
for i := 0; i < 10; i++ {
10298
j := acquirees[i].success(ctx)
@@ -129,12 +125,10 @@ func TestAcquirer_WaitsOnNoJobs(t *testing.T) {
129125
}
130126
acquiree := newTestAcquiree(t, workerID, pt, tags)
131127
jobID := uuid.New()
132-
go func() {
133-
err := fs.sendCtx(ctx, database.ProvisionerJob{}, sql.ErrNoRows)
134-
assert.NoError(t, err)
135-
err = fs.sendCtx(ctx, database.ProvisionerJob{ID: jobID}, nil)
136-
assert.NoError(t, err)
137-
}()
128+
err := fs.sendCtx(ctx, database.ProvisionerJob{}, sql.ErrNoRows)
129+
require.NoError(t, err)
130+
err = fs.sendCtx(ctx, database.ProvisionerJob{ID: jobID}, nil)
131+
require.NoError(t, err)
138132
acquiree.startAcquire(ctx, uut)
139133
require.Eventually(t, func() bool {
140134
fs.mu.Lock()
@@ -274,12 +268,10 @@ func TestAcquirer_BackupPoll(t *testing.T) {
274268
}
275269
acquiree := newTestAcquiree(t, workerID, pt, tags)
276270
jobID := uuid.New()
277-
go func() {
278-
err := fs.sendCtx(ctx, database.ProvisionerJob{}, sql.ErrNoRows)
279-
assert.NoError(t, err)
280-
err = fs.sendCtx(ctx, database.ProvisionerJob{ID: jobID}, nil)
281-
assert.NoError(t, err)
282-
}()
271+
err := fs.sendCtx(ctx, database.ProvisionerJob{}, sql.ErrNoRows)
272+
require.NoError(t, err)
273+
err = fs.sendCtx(ctx, database.ProvisionerJob{ID: jobID}, nil)
274+
require.NoError(t, err)
283275
acquiree.startAcquire(ctx, uut)
284276
job := acquiree.success(ctx)
285277
require.Equal(t, jobID, job.ID)
@@ -323,6 +315,7 @@ func TestAcquirer_UnblockOnCancel(t *testing.T) {
323315
}
324316

325317
func postJob(t *testing.T, ps pubsub.Pubsub, pt database.ProvisionerType, tags provisionerdserver.Tags) {
318+
t.Helper()
326319
msg, err := json.Marshal(provisionerdserver.JobPosting{
327320
ProvisionerType: pt,
328321
Tags: tags,

0 commit comments

Comments
 (0)