-
Notifications
You must be signed in to change notification settings - Fork 68
Add annotation for debugging workspace startup #425
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
Conversation
|
|
||
| // DevWorkspaceDebugStartAnnotation enables debugging workspace startup if set to "true". If a workspace with this annotation | ||
| // fails to start (i.e. enters the "Failed" phase), its deployment will not be scaled down in order to allow viewing logs, etc. | ||
| DevWorkspaceDebugStartAnnotation = "controller.devfile.io/debug-start" |
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.
Seems it's a good time(or too late) to agree on a convention on annotations/labels names we use.
We have - and _, like stopped-by debug-start but endpoint_name, devworkspace_name, devworkspace_id.
BTW we need to continue with one format even if leave another existing untouched.
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.
Ah yeah this is tricky -- stopped-by is used in Web Terminal, but endpoint_name is used by Theia, so it's hard to bring them in sync without requiring changes elsewhere. I think I'm responsible for our naming on both sides (_ and -) :D
I'm on the side of using dashes as a convention, since it's similar to common k8s labels such as app.kubernetes.io/part-of going forward (and also it's easier to type)
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.
+1 to use - for a new annotations and properties.
|
|
||
| // Stop failed workspaces | ||
| if workspace.Status.Phase == dw.DevWorkspaceStatusFailed && workspace.Spec.Started { | ||
| // If debug annotation is present, leave the deployment in place to let users |
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.
leaving the deployment in place is the result, but what actually it does - do not initialize stopping of failed/failing workspace.
Do I understand correctly, that with https://github.com/devfile/devworkspace-operator/pull/424/files users are supposed to manually stop workspace before starting it again if debug mode is activated?
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.
That would be the requirement, yes -- it leaves failed workspaces with spec.started=true so that you can debug issues (basically it treats workspaces with the annotation similar to how they were treated before #362).
The other way to implement this would be to leave the deployment in place for stopped workspaces when the debug annotation is present, which I think is more confusing.
This PR will need to be reworked slightly once #424 is merged.
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 but I haven't tested since it's going to be reworked after PR with failing state is merged
a5659d6 to
2b9fc36
Compare
Add annotation 'controller.devfile.io/debug-start' that disables the "stop workspace on failure" functionality. This is to make sure the workspace deployment stays on the cluster, allowing users to view logs and debug issues. Signed-off-by: Angel Misevski <[email protected]>
2b9fc36 to
2918287
Compare
|
Currently, the way the debug annotation works is:
I think this is an okay flow. Any suggestions for improvement? I considered trying to allow workspaces to still enter the |
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
Recently, I already had a case when I could suggest QE to use to investigate failure
|
/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, JPinkney, sleshchenko 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 |
I've had to temporarily cherry-pick this commit into multiple PR branches to figure out why my workspace wasn't starting :D |
What does this PR do?
Adds support for an annotation (
controller.devfile.io/debug-start: true) on devworkspaces that disables scale-to-zero when a workspace fails. This leaves the workspace deployment/pod on the cluster, allowing for debugging issues with startup.Notes
disable-scale-to-zeroI named the annotationcontroller.devfile.io/debug-start. This annotation could be used for enabling additional debug information in the future.What issues does this PR fix or reference?
Closes #418
Is it tested? How?