-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[server] maintain workspace instance state #12080
Conversation
get closer to workspace instance state reflects actual state in cluster.
started the job as gitpod-build-sefftinge-server-improve-maintenance-12067.1 because the annotations in the pull request description changed |
/werft run 👍 started the job as gitpod-build-sefftinge-server-improve-maintenance-12067.2 |
@@ -346,6 +346,15 @@ export class WorkspaceManagerBridge implements Disposable { | |||
} | |||
|
|||
instance.status.phase = "running"; | |||
// let's check if the state is inconsistent and be loud if it is. | |||
if (instance.stoppedTime || instance.stoppingTime) { |
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 didn't find any places where either stoppedTime
or stoppingTime
would be set without the status being set accordingly.
OTOH there is a handling of wsman events where the RUNNING phase is being persisted blindly
instance.status.phase = "running"; |
That said, I'm fine this adding this check here, but it seems, it should be done before the RUNNING phase is persisted bogusly.
cc. @geropl because he reimplemented the status mapping for prebuilds, which seems to be a better pattern for the whole update cascade of states.
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! That change to workspace starter makes great sense 🥇
I left a comment on the change to the bridge – it seems to that it reverts a bogus status update in error cases, which in itself sounds ok. Actually we need to avoid having persisted the bogus state beforehand, not least because these status updates are used as base of prebuild status updates and client notifications via messagebus. That's definitely tracking as a separate issue, and perhaps solve it like it's done for the prebuild status updates.
Description
get closer to workspace instance state reflects actual state in cluster.
Related Issue(s)
Fixes #12067
How to test
Release Notes
Documentation
Werft options: