- Notifications
You must be signed in to change notification settings - Fork928
feat: bump workspace deadline on user activity#4119
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
10a877bc43c2ff52f68e624e42e1bd15243ef8e29df6c555c585358d60a27150c48a67File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| package coderd | ||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "errors" | ||
| "time" | ||
| "golang.org/x/xerrors" | ||
| "cdr.dev/slog" | ||
| "github.com/coder/coder/coderd/database" | ||
| ) | ||
| // activityBumpWorkspace automatically bumps the workspace's auto-off timer | ||
| // if it is set to expire soon. | ||
| func activityBumpWorkspace(log slog.Logger, db database.Store, workspace database.Workspace) { | ||
| // We set a short timeout so if the app is under load, these | ||
| // low priority operations fail first. | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second*15) | ||
| defer cancel() | ||
| err := db.InTx(func(s database.Store) error { | ||
| build, err := s.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Is there a reason we don't want to surface an error? It seems like an inconsistency to me that a workspace is being bumped but doesn't exist. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Since the bump occurs asynchronously, there is no guarantee the workspace exists when this line executes. But, that is not an error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. That makes sense 👍🏻. Should we btw consider moving the log above to blow the error handling? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. That was an artifact from testing so I will remove. | ||
| } else if err != nil { | ||
| return xerrors.Errorf("get latest workspace build: %w", err) | ||
| } | ||
| job, err := s.GetProvisionerJobByID(ctx, build.JobID) | ||
| if err != nil { | ||
| return xerrors.Errorf("get provisioner job: %w", err) | ||
| } | ||
| if build.Transition != database.WorkspaceTransitionStart || !job.CompletedAt.Valid { | ||
| return nil | ||
| } | ||
| if build.Deadline.IsZero() { | ||
| // Workspace shutdown is manual | ||
| return nil | ||
| } | ||
| // We sent bumpThreshold slightly under bumpAmount to minimize DB writes. | ||
| const ( | ||
| bumpAmount = time.Hour | ||
| bumpThreshold = time.Hour - (time.Minute * 10) | ||
| ) | ||
ammario marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| if !build.Deadline.Before(time.Now().Add(bumpThreshold)) { | ||
| return nil | ||
| } | ||
| newDeadline := database.Now().Add(bumpAmount) | ||
| if err := s.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{ | ||
| ID: build.ID, | ||
| UpdatedAt: database.Now(), | ||
| ProvisionerState: build.ProvisionerState, | ||
| Deadline: newDeadline, | ||
| }); err != nil { | ||
| return xerrors.Errorf("update workspace build: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| log.Error( | ||
| ctx, "bump failed", | ||
| slog.Error(err), | ||
| slog.F("workspace_id", workspace.ID), | ||
| ) | ||
| } else { | ||
| log.Debug( | ||
| ctx, "bumped deadline from activity", | ||
| slog.F("workspace_id", workspace.ID), | ||
| ) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| package coderd_test | ||
| import ( | ||
| "context" | ||
| "testing" | ||
| "time" | ||
| "github.com/stretchr/testify/require" | ||
| "cdr.dev/slog/sloggers/slogtest" | ||
| "github.com/coder/coder/coderd/coderdtest" | ||
| "github.com/coder/coder/coderd/database" | ||
| "github.com/coder/coder/codersdk" | ||
| "github.com/coder/coder/testutil" | ||
| ) | ||
| func TestWorkspaceActivityBump(t *testing.T) { | ||
| t.Parallel() | ||
| ctx := context.Background() | ||
| setupActivityTest := func(t *testing.T) (client *codersdk.Client, workspace codersdk.Workspace, assertBumped func(want bool)) { | ||
| var ttlMillis int64 = 60 * 1000 | ||
| client, _, workspace, _ = setupProxyTest(t, func(cwr *codersdk.CreateWorkspaceRequest) { | ||
| cwr.TTLMillis = &ttlMillis | ||
| }) | ||
| // Sanity-check that deadline is near. | ||
| workspace, err := client.Workspace(ctx, workspace.ID) | ||
| require.NoError(t, err) | ||
| require.WithinDuration(t, | ||
| time.Now().Add(time.Duration(ttlMillis)*time.Millisecond), | ||
| workspace.LatestBuild.Deadline.Time, testutil.WaitShort, | ||
| ) | ||
| firstDeadline := workspace.LatestBuild.Deadline.Time | ||
| _ = coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) | ||
| return client, workspace, func(want bool) { | ||
| if !want { | ||
| // It is difficult to test the absence of a call in a non-racey | ||
| // way. In general, it is difficult for the API to generate | ||
| // false positive activity since Agent networking event | ||
| // is required. The Activity Bump behavior is also coupled with | ||
| // Last Used, so it would be obvious to the user if we | ||
| // are falsely recognizing activity. | ||
| time.Sleep(testutil.IntervalMedium) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This might not trigger a CI flake currently, but its behavior is racy. We can't guarantee that the goroutine has run to completion here so we also can't guarantee we're testing the correct thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah, it sucks, but I couldn't think of a clean alternative. Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. One option would be dependency injection, e.g. store the activity bumping method on This would be a bit cleaner as an internal test so that nothing needs to be exported. Another idea: I'm not sure what we're sending on the websocket, but could we write a payload informing the other side about what we did (e.g. event: bumped workspace)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more.
In this particular test we're looking for an absence of a call. So, it's not as simple as replacing the activity bump method. We should also strive to test behavior from the user's perspective. The more we dependency inject and replace internals in our tests the further away we're getting from the user. We may be better off accepting a little bit of raciness for more test accuracy here.
I, too, enjoy internal tests, but that is a codebase decision larger than this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I added an explanatory comment to the code. Let me know your thoughts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more.
I thought we would be testing for the presence of a call, but the absence of modification? What’s the purpose of testing for the absence of a call? The only case that the call wouldn’t happen is if the In my personal opinion we should avoid test racyness as much as possible because those lead to CI flakes that in turn result in loss of developer productivity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. In the current implementation, since | ||
| workspace, err = client.Workspace(ctx, workspace.ID) | ||
| require.NoError(t, err) | ||
| require.Equal(t, workspace.LatestBuild.Deadline.Time, firstDeadline) | ||
| return | ||
| } | ||
| // The Deadline bump occurs asynchronously. | ||
| require.Eventuallyf(t, | ||
| func() bool { | ||
| workspace, err = client.Workspace(ctx, workspace.ID) | ||
| require.NoError(t, err) | ||
| return workspace.LatestBuild.Deadline.Time != firstDeadline | ||
| }, | ||
| testutil.WaitShort, testutil.IntervalFast, | ||
| "deadline %v never updated", firstDeadline, | ||
| ) | ||
| require.WithinDuration(t, database.Now().Add(time.Hour), workspace.LatestBuild.Deadline.Time, time.Second) | ||
| } | ||
| } | ||
| t.Run("Dial", func(t *testing.T) { | ||
| t.Parallel() | ||
| client, workspace, assertBumped := setupActivityTest(t) | ||
| resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) | ||
| conn, err := client.DialWorkspaceAgentTailnet(ctx, slogtest.Make(t, nil), resources[0].Agents[0].ID) | ||
| require.NoError(t, err) | ||
| defer conn.Close() | ||
| sshConn, err := conn.SSHClient() | ||
| require.NoError(t, err) | ||
| _ = sshConn.Close() | ||
| assertBumped(true) | ||
| }) | ||
| t.Run("NoBump", func(t *testing.T) { | ||
| t.Parallel() | ||
| client, workspace, assertBumped := setupActivityTest(t) | ||
| // Benign operations like retrieving resources must not | ||
| // bump the deadline. | ||
| _, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID) | ||
| require.NoError(t, err) | ||
| assertBumped(false) | ||
| }) | ||
| } | ||