Skip to content

Commit dc631f5

Browse files
ammarioammario
committed
Address review comments
1 parent 1986662 commit dc631f5

File tree

10 files changed

+165
-139
lines changed

10 files changed

+165
-139
lines changed

‎agent/agent.go

Lines changed: 55 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -214,9 +214,8 @@ func (a *agent) collectMetadata(ctx context.Context, md codersdk.WorkspaceAgentM
214214
if timeout == 0 {
215215
timeout = md.Interval
216216
}
217-
ctx, cancel := context.WithDeadline(ctx, time.Now().Add(
217+
ctx, cancel := context.WithTimeout(ctx,
218218
time.Duration(timeout)*time.Second,
219-
),
220219
)
221220
defer cancel()
222221

@@ -295,63 +294,65 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
295294
a.logger.Error(ctx, "report metadata", slog.Error(err))
296295
}
297296
case <-baseTicker.C:
298-
if len(metadataResults) > cap(metadataResults)/2 {
299-
// If we're backpressured on sending back results, we risk
300-
// runaway goroutine growth and/or overloading coderd. So,
301-
// we just skip the collection. Since we never update
302-
// the collections map, we'll retry the collection
303-
// on the next tick.
304-
a.logger.Debug(
305-
ctx, "metadata collection backpressured",
306-
slog.F("queue_len", len(metadataResults)),
307-
)
308-
continue
309-
}
297+
break
298+
}
299+
300+
if len(metadataResults) > cap(metadataResults)/2 {
301+
// If we're backpressured on sending back results, we risk
302+
// runaway goroutine growth and/or overloading coderd. So,
303+
// we just skip the collection. Since we never update
304+
// the collections map, we'll retry the collection
305+
// on the next tick.
306+
a.logger.Debug(
307+
ctx, "metadata collection backpressured",
308+
slog.F("queue_len", len(metadataResults)),
309+
)
310+
continue
311+
}
310312

311-
manifest := a.manifest.Load()
312-
if manifest == nil {
313-
continue
313+
manifest := a.manifest.Load()
314+
if manifest == nil {
315+
continue
316+
}
317+
// If the manifest changes (e.g. on agent reconnect) we need to
318+
// purge old cache values to prevent lastCollectedAt from growing
319+
// boundlessly.
320+
for key := range lastCollectedAts {
321+
if slices.IndexFunc(manifest.Metadata, func(md codersdk.WorkspaceAgentMetadataDescription) bool {
322+
return md.Key == key
323+
}) < 0 {
324+
delete(lastCollectedAts, key)
314325
}
315-
// If the manifest changes (e.g. on agent reconnect) we need to
316-
// purge old cache values to prevent lastCollectedAt from growing
317-
// boundlessly.
318-
for key := range lastCollectedAts {
319-
if slices.IndexFunc(manifest.Metadata, func(md codersdk.WorkspaceAgentMetadataDescription) bool {
320-
return md.Key == key
321-
}) < 0 {
322-
delete(lastCollectedAts, key)
326+
}
327+
328+
// Spawn a goroutine for each metadata collection, and use a
329+
// channel to synchronize the results and avoid both messy
330+
// mutex logic and overloading the API.
331+
for _, md := range manifest.Metadata {
332+
collectedAt, ok := lastCollectedAts[md.Key]
333+
if ok {
334+
// If the interval is zero, we assume the user just wants
335+
// a single collection at startup, not a spinning loop.
336+
if md.Interval == 0 {
337+
continue
338+
}
339+
if collectedAt.Add(
340+
convertInterval(md.Interval),
341+
).After(time.Now()) {
342+
continue
323343
}
324344
}
325345

326-
// Spawn a goroutine for each metadata collection, and use a
327-
// channel to synchronize the results and avoid both messy
328-
// mutex logic and overloading the API.
329-
for _, md := range manifest.Metadata {
330-
collectedAt, ok := lastCollectedAts[md.Key]
331-
if ok {
332-
// If the interval is zero, we assume the user just wants
333-
// a single collection at startup, not a spinning loop.
334-
if md.Interval == 0 {
335-
continue
336-
}
337-
if collectedAt.Add(
338-
convertInterval(md.Interval),
339-
).After(time.Now()) {
340-
continue
341-
}
346+
go func(md codersdk.WorkspaceAgentMetadataDescription) {
347+
select {
348+
case <-ctx.Done():
349+
return
350+
case metadataResults <- metadataResultAndKey{
351+
key: md.Key,
352+
result: a.collectMetadata(ctx, md),
353+
}:
342354
}
343-
344-
go func(md codersdk.WorkspaceAgentMetadataDescription) {
345-
select {
346-
case <-ctx.Done():
347-
return
348-
case metadataResults <- metadataResultAndKey{
349-
key: md.Key,
350-
result: a.collectMetadata(ctx, md),
351-
}:
352-
}
353-
}(md)
354-
}
355+
}(md)
355356
}
356357
}
357358
}
@@ -1077,7 +1078,7 @@ func (a *agent) init(ctx context.Context) {
10771078
}
10781079

10791080
// createCommand processes raw command input with OpenSSH-like behavior.
1080-
// If the rawScript provided is empty, it will default to the users shell.
1081+
// If the createCommand provided is empty, it will default to the users shell.
10811082
// This injects environment variables specified by the user at launch too.
10821083
func (a *agent) createCommand(ctx context.Context, script string, env []string) (*exec.Cmd, error) {
10831084
currentUser, err := user.Current()

‎coderd/coderd.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,9 @@ func New(options *Options) *API {
603603
r.Route("/me", func(r chi.Router) {
604604
r.Use(httpmw.ExtractWorkspaceAgent(options.Database))
605605
r.Get("/manifest", api.workspaceAgentManifest)
606+
// This route is deprecated and will be removed in a future release.
607+
// New agents will use /me/manifest instead.
608+
r.Get("/metadata", api.workspaceAgentManifest)
606609
r.Post("/startup", api.postWorkspaceAgentStartup)
607610
r.Patch("/startup-logs", api.patchWorkspaceAgentStartupLogs)
608611
r.Post("/app-health", api.postWorkspaceAppHealth)

‎coderd/database/dbauthz/querier.go

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1556,13 +1556,9 @@ func (q *querier) InsertWorkspaceAgentStat(ctx context.Context, arg database.Ins
15561556
}
15571557

15581558
func (q *querier) InsertWorkspaceAgentMetadata(ctx context.Context, arg database.InsertWorkspaceAgentMetadataParams) error {
1559-
workspace, err := q.db.GetWorkspaceByAgentID(ctx, arg.WorkspaceAgentID)
1560-
if err != nil {
1561-
return xerrors.Errorf("find workspace by agent %v: %v", arg.WorkspaceAgentID, err)
1562-
}
1563-
1564-
err = q.authorizeContext(ctx, rbac.ActionUpdate, workspace)
1565-
if err != nil {
1559+
// We don't check for workspace ownership here since the agent metadata may
1560+
// be associated with an orphaned agent used by a dry run build.
1561+
if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceSystem); err != nil {
15661562
return err
15671563
}
15681564

‎coderd/database/dump.sql

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000111_workspace_agent_metadata.up.sql

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@
22
-- key enough?
33
CREATE TABLE workspace_agent_metadata (
44
workspace_agent_id uuid NOT NULL,
5-
display_name text NOT NULL,
6-
key character varying(128) NOT NULL,
7-
script text NOT NULL,
8-
value text NOT NULL DEFAULT '',
9-
error text NOT NULL DEFAULT '',
5+
display_name varchar(127) NOT NULL,
6+
key varchar(127) NOT NULL,
7+
script varchar(65535) NOT NULL,
8+
value varchar(65535) NOT NULL DEFAULT '',
9+
error varchar(65535) NOT NULL DEFAULT '',
1010
timeout bigint NOT NULL,
1111
interval bigint NOT NULL,
1212
collected_at timestamp with time zone NOT NULL DEFAULT '0001-01-01 00:00:00+00',

‎coderd/provisionerdserver/provisionerdserver.go

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1276,27 +1276,21 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid.
12761276
}
12771277
snapshot.WorkspaceAgents = append(snapshot.WorkspaceAgents, telemetry.ConvertWorkspaceAgent(dbAgent))
12781278

1279-
// We don't need to insert metadata if the agent is not for a workspace.
1280-
// This is probably an agent made during the provisioning process.
1281-
_, err = db.GetWorkspaceByAgentID(ctx, agentID)
1282-
if err == nil {
1283-
for _, md := range prAgent.Metadata {
1284-
p := database.InsertWorkspaceAgentMetadataParams{
1285-
WorkspaceAgentID: agentID,
1286-
DisplayName: md.DisplayName,
1287-
Script: md.Script,
1288-
Key: md.Key,
1289-
Timeout: md.Timeout,
1290-
Interval: md.Interval,
1291-
}
1292-
err := db.InsertWorkspaceAgentMetadata(ctx, p)
1293-
if err != nil {
1294-
return xerrors.Errorf("insert agent metadata: %w, params: %+v", err, p)
1295-
}
1279+
for _, md := range prAgent.Metadata {
1280+
p := database.InsertWorkspaceAgentMetadataParams{
1281+
WorkspaceAgentID: agentID,
1282+
DisplayName: md.DisplayName,
1283+
Script: md.Script,
1284+
Key: md.Key,
1285+
Timeout: md.Timeout,
1286+
Interval: md.Interval,
1287+
}
1288+
err := db.InsertWorkspaceAgentMetadata(ctx, p)
1289+
if err != nil {
1290+
return xerrors.Errorf("insert agent metadata: %w, params: %+v", err, p)
12961291
}
1297-
} else if !errors.Is(err, sql.ErrNoRows) {
1298-
return xerrors.Errorf("get workspace by agent ID: %w", err)
12991292
}
1293+
13001294
for _, app := range prAgent.Apps {
13011295
slug := app.Slug
13021296
if slug == "" {

‎coderd/workspaceagents.go

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1373,13 +1373,6 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques
13731373
})
13741374
}
13751375

1376-
func ellipse(s string, maxLength int) string {
1377-
if len(s) > maxLength {
1378-
return s[:maxLength] + "..."
1379-
}
1380-
return s
1381-
}
1382-
13831376
// @Summary Submit workspace agent metadata
13841377
// @ID submit-workspace-agent-metadata
13851378
// @Security CoderSessionToken
@@ -1411,12 +1404,30 @@ func (api *API) workspaceAgentPostMetadata(rw http.ResponseWriter, r *http.Reque
14111404

14121405
key := chi.URLParam(r, "key")
14131406

1407+
const (
1408+
maxValueLen = 64 << 10
1409+
maxErrorLen = maxValueLen
1410+
)
1411+
1412+
metadataError := req.Error
1413+
1414+
// We overwrite the error if the provided payload is too long.
1415+
if len(req.Value) > maxValueLen {
1416+
metadataError = fmt.Sprintf("value of %d bytes exceeded %d bytes", len(req.Value), maxValueLen)
1417+
req.Value = req.Value[:maxValueLen]
1418+
}
1419+
1420+
if len(req.Error) > maxErrorLen {
1421+
metadataError = fmt.Sprintf("error of %d bytes exceeded %d bytes", len(req.Error), maxErrorLen)
1422+
req.Error = req.Error[:maxErrorLen]
1423+
}
1424+
14141425
datum := database.UpdateWorkspaceAgentMetadataParams{
14151426
WorkspaceAgentID: workspaceAgent.ID,
14161427
// We don't want a misconfigured agent to fill the database.
1417-
Key: ellipse(key, 128),
1418-
Value: ellipse(req.Value, 10<<10),
1419-
Error: ellipse(req.Error, 10<<10),
1428+
Key: key,
1429+
Value: req.Value,
1430+
Error: metadataError,
14201431
// We ignore the CollectedAt from the agent to avoid bugs caused by
14211432
// clock skew.
14221433
CollectedAt: time.Now(),
@@ -1434,7 +1445,6 @@ func (api *API) workspaceAgentPostMetadata(rw http.ResponseWriter, r *http.Reque
14341445
slog.F("workspace", workspace.ID),
14351446
slog.F("collected_at", datum.CollectedAt),
14361447
slog.F("key", datum.Key),
1437-
slog.F("value", ellipse(datum.Value, 5)),
14381448
)
14391449

14401450
err = api.Pubsub.Publish(watchWorkspaceAgentMetadataChannel(workspaceAgent.ID), []byte(datum.Key))
@@ -1535,15 +1545,17 @@ func (api *API) watchWorkspaceAgentMetadata(rw http.ResponseWriter, r *http.Requ
15351545

15361546
for {
15371547
select {
1538-
case <-refreshTicker.C:
1539-
// Avoid spamming the DB with reads we know there are no updates. We want
1540-
// to continue sending updates to the frontend so that "Result.Age"
1541-
// is always accurate. This way, the frontend doesn't need to
1542-
// sync its own clock with the backend.
1543-
sendMetadata(false)
15441548
case <-senderClosed:
15451549
return
1550+
case <-refreshTicker.C:
1551+
break
15461552
}
1553+
1554+
// Avoid spamming the DB with reads we know there are no updates. We want
1555+
// to continue sending updates to the frontend so that "Result.Age"
1556+
// is always accurate. This way, the frontend doesn't need to
1557+
// sync its own clock with the backend.
1558+
sendMetadata(false)
15471559
}
15481560
}
15491561

‎coderd/workspaceagents_test.go

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1291,6 +1291,13 @@ func TestWorkspaceAgent_Metadata(t *testing.T) {
12911291
Interval: 10,
12921292
Timeout: 3,
12931293
},
1294+
{
1295+
DisplayName: "TooLong",
1296+
Key: "foo3",
1297+
Script: "echo howdy",
1298+
Interval: 10,
1299+
Timeout: 3,
1300+
},
12941301
},
12951302
Id: uuid.NewString(),
12961303
Auth: &proto.Agent_Token{
@@ -1370,25 +1377,35 @@ func TestWorkspaceAgent_Metadata(t *testing.T) {
13701377
}
13711378

13721379
update = recvUpdate()
1373-
require.Len(t, update, 2)
1380+
require.Len(t, update, 3)
13741381
check(wantMetadata1, update[0])
13751382
// The second metadata result is not yet posted.
13761383
require.Zero(t, update[1].Result.CollectedAt)
13771384

13781385
wantMetadata2 := wantMetadata1
13791386
post("foo2", wantMetadata2)
13801387
update = recvUpdate()
1381-
require.Len(t, update, 2)
1388+
require.Len(t, update, 3)
13821389
check(wantMetadata1, update[0])
13831390
check(wantMetadata2, update[1])
13841391

13851392
wantMetadata1.Error = "error"
13861393
post("foo1", wantMetadata1)
13871394
update = recvUpdate()
1388-
require.Len(t, update, 2)
1395+
require.Len(t, update, 3)
13891396
check(wantMetadata1, update[0])
13901397

1391-
badMetadata := wantMetadata1
1392-
err = agentClient.PostMetadata(ctx, "unknown", badMetadata)
1398+
const maxValueLen = 64 << 10
1399+
tooLongValueMetadata := wantMetadata1
1400+
tooLongValueMetadata.Value = strings.Repeat("a", maxValueLen*2)
1401+
tooLongValueMetadata.Error = ""
1402+
tooLongValueMetadata.CollectedAt = time.Now()
1403+
post("foo3", tooLongValueMetadata)
1404+
got := recvUpdate()[2]
1405+
require.Len(t, got.Result.Value, maxValueLen)
1406+
require.NotEmpty(t, got.Result.Error)
1407+
1408+
unknownKeyMetadata := wantMetadata1
1409+
err = agentClient.PostMetadata(ctx, "unknown", unknownKeyMetadata)
13931410
require.Error(t, err)
13941411
}

0 commit comments

Comments
 (0)