- Notifications
You must be signed in to change notification settings - Fork928
feat: autostop workspaces owned by suspended users#13790
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.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| @@ -387,6 +387,10 @@ func isEligibleForAutostop(ws database.Workspace, build database.WorkspaceBuild, | |||
| return false | |||
| } | |||
| if build.Transition == database.WorkspaceTransitionStart && user.Status == database.UserStatusSuspended { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we care about any other user statuses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, I don't want to mess with others - just suspended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was going to mentiondormant but figured it was out of scope;should dormant users be allowed to run workloads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IIRC on the first interaction with API the dormant user will be switched to active, so yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
By definition... they shouldn't be able to...? Once they log in, they're active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, what is your preference - should I extend the logic to dormant users too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it could be a follow-up PR since there may be additional considerations.
| // When: the autobuild executor ticks after the scheduled time | ||
| go func() { | ||
| tickCh <- sched.Next(workspace.LatestBuild.CreatedAt) | ||
| close(tickCh) | ||
| }() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we want the executor toimmediately stop workspaces of suspended users, or wait until the workspace has its time to shut down? This is testing the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I assume that the lifecycle executor stops the workspace right after the conditional logic is satisfied? Do you have a specific idea in mind on how we can improve testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No matter what time you send down the channel, it should result in the workspace being stopped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for more context. I excludedsched from the test.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
7c41f95 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixed:#8051
This PR modifies autostop rules to stop workspaces owned by suspended users.