Skip to content

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.

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

Merged
merged 1 commit into from
Jun 24, 2021
Merged

Add annotation for debugging workspace startup #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions controllers/workspace/devworkspace_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,12 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct

// Stop failed workspaces
if workspace.Status.Phase == devworkspacePhaseFailing && workspace.Spec.Started {
// If debug annotation is present, leave the deployment in place to let users
Copy link
Member

@sleshchenko sleshchenko May 12, 2021

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?

Copy link
Collaborator Author

@amisevsk amisevsk May 13, 2021

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.

// view logs.
if workspace.Annotations[constants.DevWorkspaceDebugStartAnnotation] == "true" {
return reconcile.Result{}, nil
}

patch := []byte(`{"spec":{"started": false}}`)
err := r.Client.Patch(context.Background(), workspace, client.RawPatch(types.MergePatchType, patch))
if err != nil {
Expand Down
4 changes: 4 additions & 0 deletions pkg/constants/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ const (
// this annotation will be cleared
DevWorkspaceStopReasonAnnotation = "controller.devfile.io/stopped-by"

// 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"
Copy link
Member

@sleshchenko sleshchenko May 12, 2021

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.

Copy link
Collaborator Author

@amisevsk amisevsk May 13, 2021

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)

Copy link
Member

@sleshchenko sleshchenko May 24, 2021

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.


// WebhookRestartedAtAnnotation holds the the time (unixnano) of when the webhook server was forced to restart by controller
WebhookRestartedAtAnnotation = "controller.devfile.io/restarted-at"

Expand Down