Skip to content

local cli: better logging for ws stop #19031

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 4 commits into from
Nov 8, 2023
Merged

local cli: better logging for ws stop #19031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2662c48
local cli: better logging for `ws stop`
filiptronicek Nov 7, 2023
d0fc9e5
Fix `ws start` status logging
filiptronicek Nov 7, 2023
a24f72d
Fix hangs
filiptronicek Nov 7, 2023
ffa3606
Defer stream closure for status observance
filiptronicek Nov 7, 2023
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
44 changes: 27 additions & 17 deletions components/local-app/cmd/workspace-stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,54 +34,64 @@ var workspaceStopCommand = &cobra.Command{
return err
}

slog.Info("stopping workspace...")
slog.Debug("stopping workspace...")
Copy link
Member Author

Choose a reason for hiding this comment

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

By default, we only output the waiting for workspace to stop... which can be found below. We essentially had 2 very similar messages, which are more useful for debugging than actually using the CLI.

wsInfo, err := gitpod.Workspaces.StopWorkspace(ctx, connect.NewRequest(&v1.StopWorkspaceRequest{WorkspaceId: workspaceID}))
if err != nil {
return err
}

currentPhase := wsInfo.Msg.GetResult().Status.Instance.Status.Phase

if currentPhase == v1.WorkspaceInstanceStatus_PHASE_STOPPED {
wsPhase := wsInfo.Msg.GetResult().Status.Instance.Status.Phase
switch wsPhase {
case v1.WorkspaceInstanceStatus_PHASE_STOPPED:
slog.Info("workspace is already stopped")
return nil
}
if currentPhase == v1.WorkspaceInstanceStatus_PHASE_STOPPING {
case v1.WorkspaceInstanceStatus_PHASE_STOPPING:
slog.Info("workspace is already stopping")
return nil
}

if stopDontWait {
slog.Info("workspace stopping")
return nil
}

stream, err := gitpod.Workspaces.StreamWorkspaceStatus(ctx, connect.NewRequest(&v1.StreamWorkspaceStatusRequest{WorkspaceId: workspaceID}))

if err != nil {
return err
}
Copy link
Contributor

@mustard-mh mustard-mh Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In Golang we used to defer close / release something once it was created successfully (maybe there will have some special cases but it's a good habit to do so)


defer stream.Close()

slog.Info("waiting for workspace to stop...")
slog.Info("workspace " + prettyprint.FormatWorkspacePhase(currentPhase))

previousStatus := ""

for stream.Receive() {
msg := stream.Msg()
if msg == nil {
slog.Debug("no message received")
continue
}

if msg.GetResult().Instance.Status.Phase == v1.WorkspaceInstanceStatus_PHASE_STOPPED {
slog.Info("workspace stopped")
wsPhase = msg.GetResult().Instance.Status.Phase
switch wsPhase {
case v1.WorkspaceInstanceStatus_PHASE_STOPPED:
{
slog.Info("workspace stopped")
return nil
}
case v1.WorkspaceInstanceStatus_PHASE_RUNNING:
// Skip reporting the "running" status as it is often the initial state and seems confusing to the user.
// There is some delay between requesting a workspace to stop and it actually stopping, so we don't want
// to report "running" in the meantime.
break
}

currentStatus := prettyprint.FormatWorkspacePhase(msg.GetResult().Instance.Status.Phase)
if currentStatus != previousStatus {
slog.Info("workspace " + currentStatus)
previousStatus = currentStatus
default:
{
currentStatus := prettyprint.FormatWorkspacePhase(wsPhase)
if currentStatus != previousStatus {
slog.Info("workspace status: " + currentStatus)
previousStatus = currentStatus
}
}
}
}

Expand Down
19 changes: 11 additions & 8 deletions components/local-app/pkg/helper/workspace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,11 @@ func ObserveWorkspaceUntilStarted(ctx context.Context, clnt *client.Gitpod, work
return ws.Status, nil
}

var wsStatus string
slog.Info("waiting for workspace to start...", "workspaceID", workspaceID)
if HasInstanceStatus(wsInfo.Msg.Result) {
slog.Info("workspace " + prettyprint.FormatWorkspacePhase(wsInfo.Msg.Result.Status.Instance.Status.Phase))
slog.Info("workspace status: " + prettyprint.FormatWorkspacePhase(wsInfo.Msg.Result.Status.Instance.Status.Phase))
wsStatus = prettyprint.FormatWorkspacePhase(wsInfo.Msg.Result.Status.Instance.Status.Phase)
}

var (
Expand All @@ -186,7 +188,8 @@ func ObserveWorkspaceUntilStarted(ctx context.Context, clnt *client.Gitpod, work
continue
}

previousStatus := ""
defer stream.Close()

for stream.Receive() {
msg := stream.Msg()
if msg == nil {
Expand All @@ -200,13 +203,13 @@ func ObserveWorkspaceUntilStarted(ctx context.Context, clnt *client.Gitpod, work
return ws, nil
}

var currentStatus string
if HasInstanceStatus(wsInfo.Msg.Result) {
currentStatus = prettyprint.FormatWorkspacePhase(wsInfo.Msg.Result.Status.Instance.Status.Phase)
Copy link
Member Author

Choose a reason for hiding this comment

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

This actually made it so that the status never got updates (should be ws instead of wsInfo). Here we fix it as well

}
if currentStatus != previousStatus {
slog.Info("workspace " + currentStatus)
previousStatus = currentStatus
newWsStatus := prettyprint.FormatWorkspacePhase(ws.Instance.Status.Phase)
// De-duplicate status messages
if wsStatus != newWsStatus {
slog.Info("workspace status: " + newWsStatus)
wsStatus = newWsStatus
}
}
}
if err := stream.Err(); err != nil {
Expand Down