Skip to content
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

Jump to bottom

[bridge] Set stoppedTime while workspace is stopping #4833

Merged
merged 2 commits into from
Jul 20, 2021
Merged

[bridge] Set stoppedTime while workspace is stopping #4833

merged 2 commits into from
Jul 20, 2021

Conversation

csweichel
Copy link
Contributor

there's no need to include the time our workspaces take to stop into account when computing workspace runtime (which is accounting relevant).

fixes #4818

@geropl
Copy link
Member

geropl commented Jul 15, 2021

Code-wise this looks ok.

But to me this feels like accumulating debt: Why not have a 2nd field called stoppingTime? 🤔

@csweichel csweichel force-pushed the csweichel/revise-setting-workspace-4818 branch from 9a85cc9 to 8974148 Compare July 15, 2021 12:11
@csweichel
Copy link
Contributor Author

csweichel commented Jul 15, 2021 •
edited by werft-gitpod-dev-com bot
Loading

Code-wise this looks ok.

But to me this feels like accumulating debt: Why not have a 2nd field called stoppingTime? 🤔

done

@csweichel csweichel force-pushed the csweichel/revise-setting-workspace-4818 branch from 8974148 to 90fbe59 Compare July 15, 2021 15:33
@csweichel
Copy link
Contributor Author

/assign @geropl

@csweichel
Copy link
Contributor Author

csweichel commented Jul 15, 2021 •
edited by werft-gitpod-dev-com bot
Loading

/werft run no-preview

👍 started the job as gitpod-build-csweichel-revise-setting-workspace-4818.3

@geropl
Copy link
Member

geropl commented Jul 19, 2021

done

Awesome. Will start review now...

geropl
geropl requested changes Jul 19, 2021
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Looks good! IMO there two things missing:

  1. selecting stoppingTime when selecting sessions from the DB
  2. setting stoppedTime in bridge

I made the changes here, feel free to cherry-pick.

components/ws-manager-bridge/src/bridge.ts Show resolved Hide resolved
csweichel and others added 2 commits July 20, 2021 07:38
@csweichel
[bridge] Set stoppingTime when workspace is stopping
e5d3113 
there's no need to include the time our workspaces take to stop into account
when computing workspace runtime (which is accounting relevant).
@geropl @csweichel
[db, bridge] Review comments
0100bf5
@csweichel csweichel force-pushed the csweichel/revise-setting-workspace-4818 branch from 90fbe59 to 0100bf5 Compare July 20, 2021 07:39
@csweichel
Copy link
Contributor Author

Thank you @geropl - I've cherry-picked your changes.

@csweichel csweichel requested a review from geropl July 20, 2021 07:40
geropl
geropl reviewed Jul 20, 2021
View reviewed changes
components/server/src/workspace/workspace-starter.ts
@@ -222,6 +222,7 @@ export class WorkspaceStarter {
// We may have never actually started the workspace which means that ws-manager-bridge never set a workspace status.
// We have to set that status ourselves.
instance.status.phase = 'stopped';
instance.stoppingTime = new Date().toISOString();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a nit, but: guaranteed stoppingTime == stoppedTime is a nice thing to have for downstream usage: analytics, bug hunting, ad-hoc DB fixes, etc.

geropl
geropl reviewed Jul 20, 2021
View reviewed changes
components/ws-manager-bridge/src/bridge.ts
@@ -299,6 +311,7 @@ export class WorkspaceManagerBridge implements Disposable {

log.info({instanceId, workspaceId: instance.workspaceId}, "Database says the instance is starting for too long or running, but wsman does not know about it. Marking as stopped in database.", {installation});
instance.status.phase = "stopped";
instance.stoppingTime = new Date().toISOString();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a nit, but: guaranteed stoppingTime == stoppedTime is a nice thing to have for downstream usage: analytics, bug hunting, ad-hoc DB fixes, etc.

@geropl
Copy link
Member

geropl commented Jul 20, 2021 •
edited
Loading

I just tested and somehow ended up in a re-start loop 😕 Will investigate.

Ok, was missing #4867. Adjusted manually, works now.

geropl
geropl approved these changes Jul 20, 2021
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Works as advertised! Feels wholesome to have that column, finally :-D

Happy to fix the nits separately (if at all).

@roboquat roboquat added the lgtm label Jul 20, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 564c438d498ff7b96eaa7fceed0323b3463b244f

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csweichel, geropl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit be64af3 into main Jul 20, 2021
@roboquat roboquat deleted the csweichel/revise-setting-workspace-4818 branch July 20, 2021 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geropl geropl geropl approved these changes

@svenefftinge svenefftinge Awaiting requested review from svenefftinge

Assignees

@geropl geropl

Labels
approved size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revise setting workspace stop time: Don't let workspace stopping time eat up users' hours
3 participants
@csweichel @geropl @roboquat