Skip to content

fix(cli): port-forward: update workspace last_used_at #12659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Mar 20, 2024
Merged

fix(cli): port-forward: update workspace last_used_at #12659

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
279f874
add tests to assert last used at updated on port-forward
johnstcn Mar 18, 2024
8f9b945
add workspaceusage package
johnstcn Mar 18, 2024
e8c842c
add workspace usager tracking to coderd, add endpoint
johnstcn Mar 18, 2024
86704a1
add workspace usage tracking to cli/portforward, fix tests
johnstcn Mar 19, 2024
c99327c
make gen
johnstcn Mar 19, 2024
5876edd
workspaceusage: improve locking and tests
johnstcn Mar 19, 2024
e4e0311
address more PR comments
johnstcn Mar 19, 2024
958d1d1
try to race harder
johnstcn Mar 19, 2024
a36aeb9
add danny's suggestions
johnstcn Mar 20, 2024
692f666
add big big comments
johnstcn Mar 20, 2024
d794e00
fix(database): BatchUpdateWorkspaceLastUsedAt: avoid overwriting olde…
johnstcn Mar 20, 2024
45a0eef
fix(coderd/workspaceusage): log number of consecutive flush errors
johnstcn Mar 20, 2024
8e40efd
upgrade to error log on multiple flush failures
johnstcn Mar 20, 2024
591e1ab
chore(coderd/workspaceusage): add integration-style test with multipl…
johnstcn Mar 20, 2024
0caaf3a
fix(cli/portforward_test.go): use testutil.RequireRecv/SendCtx
johnstcn Mar 20, 2024
cc72868
just use default flush interval
johnstcn Mar 20, 2024
f5f8d75
rename receiver
johnstcn Mar 20, 2024
a2e716d
defer close doneCh
johnstcn Mar 20, 2024
5b64f96
defer instead of cleanup, avoid data race in real pubsub
johnstcn Mar 20, 2024
23ccf21
fix(coderdtest): buffer just in case
johnstcn Mar 20, 2024
c9ac9d2
refactor: unexport Loop, remove panic, simplify external API
johnstcn Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
address more PR comments
  • Loading branch information
@johnstcn
johnstcn committed Mar 19, 2024
commit e4e031163723225c14c9b494c138d40d71dccf8c
34 changes: 25 additions & 9 deletions coderd/workspaceusage/tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package workspaceusage

import (
"bytes"
"context"
"flag"
"os"
"sort"
"strings"
"sync"
"time"

Expand Down Expand Up @@ -84,16 +85,22 @@ func WithFlushInterval(d time.Duration) Option {

// WithFlushChannel allows passing a channel that receives
// the number of marked workspaces every time Tracker flushes.
// For testing only.
// For testing only and will panic if used outside of tests.
func WithFlushChannel(c chan int) Option {
if flag.Lookup("test.v") == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice strictness check! Feels like this could be a testutil func, even if obvious.

panic("developer error: WithFlushChannel is not to be used outside of tests.")
}
return func(h *Tracker) {
h.flushCh = c
}
}

// WithTickChannel allows passing a channel to replace a ticker.
// For testing only.
// For testing only and will panic if used outside of tests.
func WithTickChannel(c chan time.Time) Option {
if flag.Lookup("test.v") == nil {
panic("developer error: WithTickChannel is not to be used outside of tests.")
}
return func(h *Tracker) {
h.tickCh = c
h.stopTick = func() {}
Expand Down Expand Up @@ -125,11 +132,6 @@ func (wut *Tracker) flush(now time.Time) {
return
}

// For ease of testing, sort the IDs lexically
sort.Slice(ids, func(i, j int) bool {
// For some unfathomable reason, byte arrays are not comparable?
return strings.Compare(ids[i].String(), ids[j].String()) < 0
})
// Set a short-ish timeout for this. We don't want to hang forever.
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
Expand All @@ -147,7 +149,15 @@ func (wut *Tracker) flush(now time.Time) {
wut.log.Info(ctx, "updated workspaces last_used_at", slog.F("count", count), slog.F("now", now))
}

// Loop periodically flushes every tick.
// If Loop is called after Close, it will panic. Don't do this.
func (wut *Tracker) Loop() {
// Calling Loop after Close() is an error.
select {
case <-wut.doneCh:
panic("developer error: Loop called after Close")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to do this instead of just returning, I'd prefer returning an error here, wouldn't want this to happen for a production instance after some changes to the code.

Feels like this mostly exists for tests (otherwise could be launched from New), so more reason not to add a possibility of panic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we return an error, then we have two options:

  1. Exit with the error, which brings us back to potentially impacting prod, but just in a nicer-looking way.
  2. Just log the error and do nothing, which means the error will likely pass unnoticed until folks wonder why their workspace usage isn't being tracked.

I don't think 2) is a valid option. We're not guaranteed to catch it in tests especially because of the possibility of ignoring errors in slogtest.

There's an alternative possibility 1a) where we check the error and recreate the tracker if non-nil. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot about option c) - have New start the loop and unexport the function.
Now it's un-possible!

default:
}
defer func() {
wut.log.Debug(context.Background(), "workspace usage tracker loop exited")
}()
Expand All @@ -165,7 +175,8 @@ func (wut *Tracker) Loop() {
}
}

// Close stops Tracker and performs a final flush.
// Close stops Tracker and returns once Loop has exited.
// After calling Close(), Loop must not be called.
func (wut *Tracker) Close() {
wut.stopOnce.Do(func() {
wut.stopCh <- struct{}{}
Expand Down Expand Up @@ -202,6 +213,11 @@ func (s *uuidSet) UniqueAndClear() []uuid.UUID {
for k := range s.m {
l = append(l, k)
}
// For ease of testing, sort the IDs lexically
sort.Slice(l, func(i, j int) bool {
// For some unfathomable reason, byte arrays are not comparable?
return bytes.Compare(l[i][:], l[j][:]) < 0
})
s.m = make(map[uuid.UUID]struct{})
return l
}
7 changes: 7 additions & 0 deletions coderd/workspaceusage/tracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@ func TestTracker(t *testing.T) {

wg.Wait()
require.Equal(t, 11, count, "expected one flush with eleven ids")

// 4. Closing multiple times should not be a problem.
wut.Close()
wut.Close()

// 5. Running Loop() again should panic.
require.Panics(t, wut.Loop)
}

func TestMain(m *testing.M) {
Expand Down