Skip to content

Commit fabacc5

Browse files
coadlercoadler
committed
Merge branch 'main' into colin/metadata-fe-fix
2 parents b6159eb + c8d65de commit fabacc5

File tree

91 files changed

+4874
-922
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

91 files changed

+4874
-922
lines changed

‎.github/workflows/ci.yaml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ jobs:
240240
if: needs.changes.outputs.go == 'true' || needs.changes.outputs.ci == 'true' || github.ref == 'refs/heads/main'
241241
timeout-minutes: 20
242242
strategy:
243+
fail-fast: false
243244
matrix:
244245
os:
245246
- ubuntu-latest
@@ -543,7 +544,7 @@ jobs:
543544
# REMARK: this is only used to build storybook and deploy it to Chromatic.
544545
runs-on: ubuntu-latest
545546
needs: changes
546-
if: needs.changes.outputs.ts == 'true' || needs.changes.outputs.ci == 'true' || github.ref == 'refs/heads/main'
547+
if: needs.changes.outputs.ts == 'true' || needs.changes.outputs.ci == 'true'
547548
steps:
548549
- name: Checkout
549550
uses: actions/checkout@v3

‎agent/agent.go

Lines changed: 60 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -242,15 +242,15 @@ func (a *agent) runLoop(ctx context.Context) {
242242
}
243243
}
244244

245-
func (a *agent) collectMetadata(ctx context.Context, md codersdk.WorkspaceAgentMetadataDescription) *codersdk.WorkspaceAgentMetadataResult {
245+
func (a *agent) collectMetadata(ctx context.Context, md codersdk.WorkspaceAgentMetadataDescription, now time.Time) *codersdk.WorkspaceAgentMetadataResult {
246246
var out bytes.Buffer
247247
result := &codersdk.WorkspaceAgentMetadataResult{
248248
// CollectedAt is set here for testing purposes and overrode by
249249
// coderd to the time of server receipt to solve clock skew.
250250
//
251251
// In the future, the server may accept the timestamp from the agent
252252
// if it can guarantee the clocks are synchronized.
253-
CollectedAt: time.Now(),
253+
CollectedAt: now,
254254
}
255255
cmdPty, err := a.sshServer.CreateCommand(ctx, md.Script, nil)
256256
if err != nil {
@@ -298,38 +298,48 @@ type metadataResultAndKey struct {
298298
}
299299

300300
type trySingleflight struct {
301-
m sync.Map
301+
mu sync.Mutex
302+
m map[string]struct{}
302303
}
303304

304305
func (t *trySingleflight) Do(key string, fn func()) {
305-
_, loaded := t.m.LoadOrStore(key, struct{}{})
306-
if !loaded {
307-
// There is already a goroutine running for this key.
306+
t.mu.Lock()
307+
_, ok := t.m[key]
308+
if ok {
309+
t.mu.Unlock()
308310
return
309311
}
310312

311-
defer t.m.Delete(key)
313+
t.m[key] = struct{}{}
314+
t.mu.Unlock()
315+
defer func() {
316+
t.mu.Lock()
317+
delete(t.m, key)
318+
t.mu.Unlock()
319+
}()
320+
312321
fn()
313322
}
314323

315324
func (a *agent) reportMetadataLoop(ctx context.Context) {
316325
const metadataLimit = 128
317326

318327
var (
319-
baseTicker = time.NewTicker(a.reportMetadataInterval)
320-
lastCollectedAts = make(map[string]time.Time)
321-
metadataResults = make(chan metadataResultAndKey, metadataLimit)
328+
baseTicker = time.NewTicker(a.reportMetadataInterval)
329+
lastCollectedAtMu sync.RWMutex
330+
lastCollectedAts = make(map[string]time.Time)
331+
metadataResults = make(chan metadataResultAndKey, metadataLimit)
332+
logger = a.logger.Named("metadata")
322333
)
323334
defer baseTicker.Stop()
324335

325336
// We use a custom singleflight that immediately returns if there is already
326337
// a goroutine running for a given key. This is to prevent a build-up of
327338
// goroutines waiting on Do when the script takes many multiples of
328339
// baseInterval to run.
329-
var flight trySingleflight
340+
flight := trySingleflight{m: map[string]struct{}{}}
330341

331342
postMetadata := func(mr metadataResultAndKey) {
332-
lastCollectedAts[mr.key] = mr.result.CollectedAt
333343
err := a.client.PostMetadata(ctx, mr.key, *mr.result)
334344
if err != nil {
335345
a.logger.Error(ctx, "agent failed to report metadata", slog.Error(err))
@@ -349,8 +359,7 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
349359
if len(metadataResults) > 0 {
350360
// The inner collection loop expects the channel is empty before spinning up
351361
// all the collection goroutines.
352-
a.logger.Debug(
353-
ctx, "metadata collection backpressured",
362+
logger.Debug(ctx, "metadata collection backpressured",
354363
slog.F("queue_len", len(metadataResults)),
355364
)
356365
continue
@@ -362,7 +371,7 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
362371
}
363372

364373
if len(manifest.Metadata) > metadataLimit {
365-
a.logger.Error(
374+
logger.Error(
366375
ctx, "metadata limit exceeded",
367376
slog.F("limit", metadataLimit), slog.F("got", len(manifest.Metadata)),
368377
)
@@ -372,66 +381,79 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
372381
// If the manifest changes (e.g. on agent reconnect) we need to
373382
// purge old cache values to prevent lastCollectedAt from growing
374383
// boundlessly.
384+
lastCollectedAtMu.Lock()
375385
for key := range lastCollectedAts {
376386
if slices.IndexFunc(manifest.Metadata, func(md codersdk.WorkspaceAgentMetadataDescription) bool {
377387
return md.Key == key
378388
}) < 0 {
389+
logger.Debug(ctx, "deleting lastCollected key, missing from manifest",
390+
slog.F("key", key),
391+
)
379392
delete(lastCollectedAts, key)
380393
}
381394
}
395+
lastCollectedAtMu.Unlock()
382396

383397
// Spawn a goroutine for each metadata collection, and use a
384398
// channel to synchronize the results and avoid both messy
385399
// mutex logic and overloading the API.
386400
for _, md := range manifest.Metadata {
387-
collectedAt, ok := lastCollectedAts[md.Key]
388-
if ok {
389-
// If the interval is zero, we assume the user just wants
390-
// a single collection at startup, not a spinning loop.
391-
if md.Interval == 0 {
392-
continue
393-
}
394-
395-
intervalUnit := time.Second
396-
// reportMetadataInterval is only less than a second in tests,
397-
// so adjust the interval unit for them.
398-
if a.reportMetadataInterval < time.Second {
399-
intervalUnit = 100 * time.Millisecond
400-
}
401-
// The last collected value isn't quite stale yet, so we skip it.
402-
if collectedAt.Add(time.Duration(md.Interval) * intervalUnit).After(time.Now()) {
403-
continue
404-
}
405-
}
406-
407401
md := md
408402
// We send the result to the channel in the goroutine to avoid
409403
// sending the same result multiple times. So, we don't care about
410404
// the return values.
411405
go flight.Do(md.Key, func() {
406+
ctx := slog.With(ctx, slog.F("key", md.Key))
407+
lastCollectedAtMu.RLock()
408+
collectedAt, ok := lastCollectedAts[md.Key]
409+
lastCollectedAtMu.RUnlock()
410+
if ok {
411+
// If the interval is zero, we assume the user just wants
412+
// a single collection at startup, not a spinning loop.
413+
if md.Interval == 0 {
414+
return
415+
}
416+
intervalUnit := time.Second
417+
// reportMetadataInterval is only less than a second in tests,
418+
// so adjust the interval unit for them.
419+
if a.reportMetadataInterval < time.Second {
420+
intervalUnit = 100 * time.Millisecond
421+
}
422+
// The last collected value isn't quite stale yet, so we skip it.
423+
if collectedAt.Add(time.Duration(md.Interval) * intervalUnit).After(time.Now()) {
424+
return
425+
}
426+
}
427+
412428
timeout := md.Timeout
413429
if timeout == 0 {
414430
if md.Interval != 0 {
415431
timeout = md.Interval
416432
} else if interval := int64(a.reportMetadataInterval.Seconds()); interval != 0 {
417433
// Fallback to the report interval
418-
timeout = interval
434+
timeout = interval * 3
419435
} else {
420436
// If the interval is still 0 (possible if the interval
421437
// is less than a second), default to 5. This was
422438
// randomly picked.
423439
timeout = 5
424440
}
425441
}
426-
ctx, cancel := context.WithTimeout(ctx, time.Duration(timeout)*time.Second)
442+
ctxTimeout := time.Duration(timeout) * time.Second
443+
ctx, cancel := context.WithTimeout(ctx, ctxTimeout)
427444
defer cancel()
428445

446+
now := time.Now()
429447
select {
430448
case <-ctx.Done():
449+
logger.Warn(ctx, "metadata collection timed out", slog.F("timeout", ctxTimeout))
431450
case metadataResults <- metadataResultAndKey{
432451
key: md.Key,
433-
result: a.collectMetadata(ctx, md),
452+
result: a.collectMetadata(ctx, md, now),
434453
}:
454+
lastCollectedAtMu.Lock()
455+
lastCollectedAts[md.Key] = now
456+
lastCollectedAtMu.Unlock()
435457
}
436458
})
437459
}

‎agent/agent_test.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1066,6 +1066,7 @@ func TestAgent_StartupScript(t *testing.T) {
10661066
t.Parallel()
10671067
logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug)
10681068
client := agenttest.NewClient(t,
1069+
logger,
10691070
uuid.New(),
10701071
agentsdk.Manifest{
10711072
StartupScript: command,
@@ -1097,6 +1098,7 @@ func TestAgent_StartupScript(t *testing.T) {
10971098
t.Parallel()
10981099
logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug)
10991100
client := agenttest.NewClient(t,
1101+
logger,
11001102
uuid.New(),
11011103
agentsdk.Manifest{
11021104
StartupScript: command,
@@ -1470,6 +1472,7 @@ func TestAgent_Lifecycle(t *testing.T) {
14701472
derpMap, _ := tailnettest.RunDERPAndSTUN(t)
14711473

14721474
client := agenttest.NewClient(t,
1475+
logger,
14731476
uuid.New(),
14741477
agentsdk.Manifest{
14751478
DERPMap: derpMap,
@@ -1742,6 +1745,7 @@ func TestAgent_Reconnect(t *testing.T) {
17421745
statsCh := make(chan *agentsdk.Stats, 50)
17431746
derpMap, _ := tailnettest.RunDERPAndSTUN(t)
17441747
client := agenttest.NewClient(t,
1748+
logger,
17451749
agentID,
17461750
agentsdk.Manifest{
17471751
DERPMap: derpMap,
@@ -1776,6 +1780,7 @@ func TestAgent_WriteVSCodeConfigs(t *testing.T) {
17761780
defer coordinator.Close()
17771781

17781782
client := agenttest.NewClient(t,
1783+
logger,
17791784
uuid.New(),
17801785
agentsdk.Manifest{
17811786
GitAuthConfigs: 1,
@@ -1900,7 +1905,7 @@ func setupAgent(t *testing.T, metadata agentsdk.Manifest, ptyTimeout time.Durati
19001905
})
19011906
statsCh := make(chan *agentsdk.Stats, 50)
19021907
fs := afero.NewMemMapFs()
1903-
c := agenttest.NewClient(t, metadata.AgentID, metadata, statsCh, coordinator)
1908+
c := agenttest.NewClient(t, logger.Named("agent"), metadata.AgentID, metadata, statsCh, coordinator)
19041909

19051910
options := agent.Options{
19061911
Client: c,

‎agent/agenttest/client.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
)
1919

2020
func NewClient(t testing.TB,
21+
logger slog.Logger,
2122
agentID uuid.UUID,
2223
manifest agentsdk.Manifest,
2324
statsChan chan *agentsdk.Stats,
@@ -28,6 +29,7 @@ func NewClient(t testing.TB,
2829
}
2930
return &Client{
3031
t: t,
32+
logger: logger.Named("client"),
3133
agentID: agentID,
3234
manifest: manifest,
3335
statsChan: statsChan,
@@ -37,6 +39,7 @@ func NewClient(t testing.TB,
3739

3840
type Client struct {
3941
t testing.TB
42+
logger slog.Logger
4043
agentID uuid.UUID
4144
manifest agentsdk.Manifest
4245
metadata map[string]agentsdk.PostMetadataRequest
@@ -110,14 +113,16 @@ func (c *Client) GetLifecycleStates() []codersdk.WorkspaceAgentLifecycle {
110113
return c.lifecycleStates
111114
}
112115

113-
func (c *Client) PostLifecycle(_ context.Context, req agentsdk.PostLifecycleRequest) error {
116+
func (c *Client) PostLifecycle(ctx context.Context, req agentsdk.PostLifecycleRequest) error {
114117
c.mu.Lock()
115118
defer c.mu.Unlock()
116119
c.lifecycleStates = append(c.lifecycleStates, req.State)
120+
c.logger.Debug(ctx, "post lifecycle", slog.F("req", req))
117121
return nil
118122
}
119123

120-
func (*Client) PostAppHealth(_ context.Context, _ agentsdk.PostAppHealthsRequest) error {
124+
func (c *Client) PostAppHealth(ctx context.Context, req agentsdk.PostAppHealthsRequest) error {
125+
c.logger.Debug(ctx, "post app health", slog.F("req", req))
121126
return nil
122127
}
123128

@@ -133,20 +138,22 @@ func (c *Client) GetMetadata() map[string]agentsdk.PostMetadataRequest {
133138
return maps.Clone(c.metadata)
134139
}
135140

136-
func (c *Client) PostMetadata(_ context.Context, key string, req agentsdk.PostMetadataRequest) error {
141+
func (c *Client) PostMetadata(ctx context.Context, key string, req agentsdk.PostMetadataRequest) error {
137142
c.mu.Lock()
138143
defer c.mu.Unlock()
139144
if c.metadata == nil {
140145
c.metadata = make(map[string]agentsdk.PostMetadataRequest)
141146
}
142147
c.metadata[key] = req
148+
c.logger.Debug(ctx, "post metadata", slog.F("key", key), slog.F("req", req))
143149
return nil
144150
}
145151

146-
func (c *Client) PostStartup(_ context.Context, startup agentsdk.PostStartupRequest) error {
152+
func (c *Client) PostStartup(ctx context.Context, startup agentsdk.PostStartupRequest) error {
147153
c.mu.Lock()
148154
defer c.mu.Unlock()
149155
c.startup = startup
156+
c.logger.Debug(ctx, "post startup", slog.F("req", startup))
150157
return nil
151158
}
152159

@@ -156,13 +163,14 @@ func (c *Client) GetStartupLogs() []agentsdk.StartupLog {
156163
return c.logs
157164
}
158165

159-
func (c *Client) PatchStartupLogs(_ context.Context, logs agentsdk.PatchStartupLogs) error {
166+
func (c *Client) PatchStartupLogs(ctx context.Context, logs agentsdk.PatchStartupLogs) error {
160167
c.mu.Lock()
161168
defer c.mu.Unlock()
162169
if c.PatchWorkspaceLogs != nil {
163170
return c.PatchWorkspaceLogs()
164171
}
165172
c.logs = append(c.logs, logs.Logs...)
173+
c.logger.Debug(ctx, "patch startup logs", slog.F("req", logs))
166174
return nil
167175
}
168176

@@ -173,9 +181,10 @@ func (c *Client) SetServiceBannerFunc(f func() (codersdk.ServiceBannerConfig, er
173181
c.GetServiceBannerFunc = f
174182
}
175183

176-
func (c *Client) GetServiceBanner(_ context.Context) (codersdk.ServiceBannerConfig, error) {
184+
func (c *Client) GetServiceBanner(ctx context.Context) (codersdk.ServiceBannerConfig, error) {
177185
c.mu.Lock()
178186
defer c.mu.Unlock()
187+
c.logger.Debug(ctx, "get service banner")
179188
if c.GetServiceBannerFunc != nil {
180189
return c.GetServiceBannerFunc()
181190
}

‎cli/create.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,6 @@ func (r *RootCmd) create() *clibase.Cmd {
149149
var ttlMillis *int64
150150
if stopAfter > 0 {
151151
ttlMillis = ptr.Ref(stopAfter.Milliseconds())
152-
} else if template.MaxTTLMillis > 0 {
153-
ttlMillis = &template.MaxTTLMillis
154152
}
155153

156154
workspace, err := client.CreateWorkspace(inv.Context(), organization.ID, workspaceOwner, codersdk.CreateWorkspaceRequest{

‎cli/portforward.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func (r *RootCmd) portForward() *clibase.Cmd {
3232
client := new(codersdk.Client)
3333
cmd := &clibase.Cmd{
3434
Use: "port-forward <workspace>",
35-
Short: "Forward ports from machine to a workspace",
35+
Short: `Forward ports from a workspace to the local machine. Forward ports from a workspace to the local machine. For reverse port forwarding, use "coder ssh -R".`,
3636
Aliases: []string{"tunnel"},
3737
Long: formatExamples(
3838
example{

0 commit comments

Comments
 (0)