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.

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
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 3 additions & 0 deletions cli/portforward.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ func (r *RootCmd) portForward() *serpent.Command {
listeners[i] = l
}

stopUpdating := client.UpdateWorkspaceUsageContext(ctx, workspace.ID)

// Wait for the context to be canceled or for a signal and close
// all listeners.
var closeErr error
Expand All @@ -156,6 +158,7 @@ func (r *RootCmd) portForward() *serpent.Command {
}

cancel()
stopUpdating()
closeAllListeners()
}()

Expand Down
29 changes: 28 additions & 1 deletion cli/portforward_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbfake"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/pty/ptytest"
"github.com/coder/coder/v2/testutil"
Expand Down Expand Up @@ -96,7 +97,12 @@ func TestPortForward(t *testing.T) {
// Setup agent once to be shared between test-cases (avoid expensive
// non-parallel setup).
var (
client, db = coderdtest.NewWithDatabase(t, nil)
wuTick = make(chan time.Time)
wuFlush = make(chan int, 1)
client, db = coderdtest.NewWithDatabase(t, &coderdtest.Options{
WorkspaceUsageTrackerTick: wuTick,
WorkspaceUsageTrackerFlush: wuFlush,
})
admin = coderdtest.CreateFirstUser(t, client)
member, memberUser = coderdtest.CreateAnotherUser(t, client, admin.OrganizationID)
workspace = runAgent(t, client, memberUser.ID, db)
Expand Down Expand Up @@ -148,6 +154,13 @@ func TestPortForward(t *testing.T) {
cancel()
err = <-errC
require.ErrorIs(t, err, context.Canceled)

flushCtx := testutil.Context(t, testutil.WaitShort)
testutil.RequireSendCtx(flushCtx, t, wuTick, dbtime.Now())
_ = testutil.RequireRecvCtx(flushCtx, t, wuFlush)
updated, err := client.Workspace(context.Background(), workspace.ID)
require.NoError(t, err)
require.Greater(t, updated.LastUsedAt, workspace.LastUsedAt)
})

t.Run(c.name+"_TwoPorts", func(t *testing.T) {
Expand Down Expand Up @@ -196,6 +209,13 @@ func TestPortForward(t *testing.T) {
cancel()
err = <-errC
require.ErrorIs(t, err, context.Canceled)

flushCtx := testutil.Context(t, testutil.WaitShort)
testutil.RequireSendCtx(flushCtx, t, wuTick, dbtime.Now())
_ = testutil.RequireRecvCtx(flushCtx, t, wuFlush)
updated, err := client.Workspace(context.Background(), workspace.ID)
require.NoError(t, err)
require.Greater(t, updated.LastUsedAt, workspace.LastUsedAt)
})
}

Expand Down Expand Up @@ -257,6 +277,13 @@ func TestPortForward(t *testing.T) {
cancel()
err := <-errC
require.ErrorIs(t, err, context.Canceled)

flushCtx := testutil.Context(t, testutil.WaitShort)
testutil.RequireSendCtx(flushCtx, t, wuTick, dbtime.Now())
_ = testutil.RequireRecvCtx(flushCtx, t, wuFlush)
updated, err := client.Workspace(context.Background(), workspace.ID)
require.NoError(t, err)
require.Greater(t, updated.LastUsedAt, workspace.LastUsedAt)
})
}

Expand Down
8 changes: 8 additions & 0 deletions cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ import (
stringutil "github.com/coder/coder/v2/coderd/util/strings"
"github.com/coder/coder/v2/coderd/workspaceapps"
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
"github.com/coder/coder/v2/coderd/workspaceusage"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/codersdk/drpc"
"github.com/coder/coder/v2/cryptorand"
Expand Down Expand Up @@ -968,6 +969,13 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
purger := dbpurge.New(ctx, logger, options.Database)
defer purger.Close()

// Updates workspace usage
tracker := workspaceusage.New(options.Database,
workspaceusage.WithLogger(logger.Named("workspace_usage_tracker")),
)
options.WorkspaceUsageTracker = tracker
defer tracker.Close()

// Wrap the server in middleware that redirects to the access URL if
// the request is not to a local IP.
var handler http.Handler = coderAPI.RootHandler
Expand Down
29 changes: 29 additions & 0 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions coderd/apidoc/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ import (
"github.com/coder/coder/v2/coderd/updatecheck"
"github.com/coder/coder/v2/coderd/util/slice"
"github.com/coder/coder/v2/coderd/workspaceapps"
"github.com/coder/coder/v2/coderd/workspaceusage"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/codersdk/drpc"
"github.com/coder/coder/v2/provisionerd/proto"
Expand Down Expand Up @@ -190,6 +191,9 @@ type Options struct {

// NewTicker is used for unit tests to replace "time.NewTicker".
NewTicker func(duration time.Duration) (tick <-chan time.Time, done func())

// WorkspaceUsageTracker tracks workspace usage by the CLI.
WorkspaceUsageTracker *workspaceusage.Tracker
}

// @title Coder API
Expand Down Expand Up @@ -362,6 +366,12 @@ func New(options *Options) *API {
OIDC: options.OIDCConfig,
}

if options.WorkspaceUsageTracker == nil {
options.WorkspaceUsageTracker = workspaceusage.New(options.Database,
workspaceusage.WithLogger(options.Logger.Named("workspace_usage_tracker")),
)
}

ctx, cancel := context.WithCancel(context.Background())
r := chi.NewRouter()

Expand Down Expand Up @@ -405,6 +415,7 @@ func New(options *Options) *API {
options.Logger.Named("acquirer"),
options.Database,
options.Pubsub),
workspaceUsageTracker: options.WorkspaceUsageTracker,
}

api.AppearanceFetcher.Store(&appearance.DefaultFetcher)
Expand Down Expand Up @@ -972,6 +983,7 @@ func New(options *Options) *API {
})
r.Get("/watch", api.watchWorkspace)
r.Put("/extend", api.putExtendWorkspace)
r.Post("/usage", api.postWorkspaceUsage)
r.Put("/dormant", api.putWorkspaceDormant)
r.Put("/favorite", api.putFavoriteWorkspace)
r.Delete("/favorite", api.deleteFavoriteWorkspace)
Expand Down Expand Up @@ -1179,6 +1191,8 @@ type API struct {
statsBatcher *batchstats.Batcher

Acquirer *provisionerdserver.Acquirer

workspaceUsageTracker *workspaceusage.Tracker
}

// Close waits for all WebSocket connections to drain before returning.
Expand All @@ -1200,6 +1214,7 @@ func (api *API) Close() error {
_ = (*coordinator).Close()
}
_ = api.agentProvider.Close()
api.workspaceUsageTracker.Close()
return nil
}

Expand Down
34 changes: 34 additions & 0 deletions coderd/coderdtest/coderdtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import (
"github.com/coder/coder/v2/coderd/util/ptr"
"github.com/coder/coder/v2/coderd/workspaceapps"
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
"github.com/coder/coder/v2/coderd/workspaceusage"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/codersdk/agentsdk"
"github.com/coder/coder/v2/codersdk/drpc"
Expand Down Expand Up @@ -146,6 +147,8 @@ type Options struct {
WorkspaceAppsStatsCollectorOptions workspaceapps.StatsCollectorOptions
AllowWorkspaceRenames bool
NewTicker func(duration time.Duration) (<-chan time.Time, func())
WorkspaceUsageTrackerFlush chan int
WorkspaceUsageTrackerTick chan time.Time
}

// New constructs a codersdk client connected to an in-memory API instance.
Expand Down Expand Up @@ -306,6 +309,36 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
hangDetector.Start()
t.Cleanup(hangDetector.Close)

// Did last_used_at not update? Scratching your noggin? Here's why.
// Workspace usage tracking must be triggered manually in tests.
// The vast majority of existing tests do not depend on last_used_at
// and adding an extra time-based background goroutine to all existing
// tests may lead to future flakes and goleak complaints.
// Instead, pass in your own flush and ticker like so:
//
// tickCh = make(chan time.Time)
// flushCh = make(chan int, 1)
// client = coderdtest.New(t, &coderdtest.Options{
// WorkspaceUsageTrackerFlush: flushCh,
// WorkspaceUsageTrackerTick: tickCh
// })
//
// Now to trigger a tick, just write to `tickCh`.
// Reading from `flushCh` will ensure that workspaceusage.Tracker flushed.
// See TestPortForward or TestTracker_MultipleInstances for how this works in practice.
if options.WorkspaceUsageTrackerFlush == nil {
options.WorkspaceUsageTrackerFlush = make(chan int, 1) // buffering just in case
}
if options.WorkspaceUsageTrackerTick == nil {
options.WorkspaceUsageTrackerTick = make(chan time.Time, 1) // buffering just in case
}
// Close is called by API.Close()
wuTracker := workspaceusage.New(
options.Database,
workspaceusage.WithLogger(options.Logger.Named("workspace_usage_tracker")),
workspaceusage.WithTickFlush(options.WorkspaceUsageTrackerTick, options.WorkspaceUsageTrackerFlush),
)

var mutex sync.RWMutex
var handler http.Handler
srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -454,6 +487,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
WorkspaceAppsStatsCollectorOptions: options.WorkspaceAppsStatsCollectorOptions,
AllowWorkspaceRenames: options.AllowWorkspaceRenames,
NewTicker: options.NewTicker,
WorkspaceUsageTracker: wuTracker,
}
}

Expand Down
4 changes: 4 additions & 0 deletions coderd/database/dbmem/dbmem.go
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,10 @@ func (q *FakeQuerier) BatchUpdateWorkspaceLastUsedAt(_ context.Context, arg data
if _, found := m[q.workspaces[i].ID]; !found {
continue
}
// WHERE last_used_at < @last_used_at
if !q.workspaces[i].LastUsedAt.Before(arg.LastUsedAt) {
continue
}
q.workspaces[i].LastUsedAt = arg.LastUsedAt
n++
}
Expand Down
3 changes: 3 additions & 0 deletions coderd/database/queries.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion coderd/database/queries/workspaces.sql
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,10 @@ UPDATE
SET
last_used_at = @last_used_at
WHERE
id = ANY(@ids :: uuid[]);
id = ANY(@ids :: uuid[])
AND
-- Do not overwrite with older data
last_used_at < @last_used_at;

-- name: GetDeploymentWorkspaceStats :one
WITH workspaces_with_jobs AS (
Expand Down
18 changes: 18 additions & 0 deletions coderd/workspaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -1096,6 +1096,24 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) {
httpapi.Write(ctx, rw, code, resp)
}

// @Summary Post Workspace Usage by ID
// @ID post-workspace-usage-by-id
// @Security CoderSessionToken
// @Tags Workspaces
// @Param workspace path string true "Workspace ID" format(uuid)
// @Success 204
// @Router /workspaces/{workspace}/usage [post]
func (api *API) postWorkspaceUsage(rw http.ResponseWriter, r *http.Request) {
workspace := httpmw.WorkspaceParam(r)
if !api.Authorize(r, rbac.ActionUpdate, workspace) {
httpapi.Forbidden(rw)
return
}

api.workspaceUsageTracker.Add(workspace.ID)
rw.WriteHeader(http.StatusNoContent)
}

// @Summary Favorite workspace by ID.
// @ID favorite-workspace-by-id
// @Security CoderSessionToken
Expand Down
Loading