Skip to content

[ws-daemon] Fix workspace location for log backup #18401

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 2 commits into from
Aug 2, 2023
Merged

[ws-daemon] Fix workspace location for log backup #18401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c9fa34
[ws-daemon] Fix workspace location for log backup
WVerlaek Aug 1, 2023
dc86de5
Update integration test to check for uploaded logs
WVerlaek Aug 2, 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
7 changes: 3 additions & 4 deletions components/ws-daemon/pkg/controller/workspace_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,10 +293,9 @@ func (wsc *WorkspaceController) handleWorkspaceStop(ctx context.Context, ws *wor
WorkspaceID: ws.Spec.Ownership.WorkspaceID,
InstanceID: ws.Name,
},
WorkspaceLocation: ws.Spec.WorkspaceLocation,
SnapshotName: snapshotName,
BackupLogs: ws.Spec.Type == workspacev1.WorkspaceTypePrebuild,
UpdateGitStatus: ws.Spec.Type == workspacev1.WorkspaceTypeRegular,
SnapshotName: snapshotName,
BackupLogs: ws.Spec.Type == workspacev1.WorkspaceTypePrebuild,
UpdateGitStatus: ws.Spec.Type == workspacev1.WorkspaceTypeRegular,
})

err = retry.RetryOnConflict(retryParams, func() error {
Expand Down
15 changes: 7 additions & 8 deletions components/ws-daemon/pkg/controller/workspace_operations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,10 @@ type InitOptions struct {
}

type BackupOptions struct {
Meta WorkspaceMeta
WorkspaceLocation string
BackupLogs bool
UpdateGitStatus bool
SnapshotName string
Meta WorkspaceMeta
BackupLogs bool
UpdateGitStatus bool
SnapshotName string
}

func NewWorkspaceOperations(config content.Config, provider *WorkspaceProvider, reg prometheus.Registerer) (WorkspaceOperations, error) {
Expand Down Expand Up @@ -231,7 +230,7 @@ func (wso *DefaultWorkspaceOperations) BackupWorkspace(ctx context.Context, opts
}

if opts.BackupLogs {
err := wso.uploadWorkspaceLogs(ctx, opts)
err := wso.uploadWorkspaceLogs(ctx, opts, ws.Location)
if err != nil {
// we do not fail the workspace yet because we still might succeed with its content!
glog.WithError(err).WithFields(ws.OWI()).Error("log backup failed")
Expand Down Expand Up @@ -345,9 +344,9 @@ func ensureCleanSlate(location string) error {
return nil
}

func (wso *DefaultWorkspaceOperations) uploadWorkspaceLogs(ctx context.Context, opts BackupOptions) (err error) {
func (wso *DefaultWorkspaceOperations) uploadWorkspaceLogs(ctx context.Context, opts BackupOptions, location string) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Any unit tests we need to add/update as part of this to ensure the regression doesn't happen again in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated an existing integration test to check for uploaded logs. There's no unit tests nor is it easy to add one now that would catch this regression

// currently we're only uploading prebuild log files
logFiles, err := logs.ListPrebuildLogFiles(ctx, opts.WorkspaceLocation)
logFiles, err := logs.ListPrebuildLogFiles(ctx, location)
if err != nil {
return err
}
Expand Down
7 changes: 5 additions & 2 deletions test/pkg/integration/apis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,6 +1106,7 @@ func (c *ComponentAPI) ClearImageBuilderClientCache() {
type ContentService interface {
csapi.ContentServiceClient
csapi.WorkspaceServiceClient
csapi.HeadlessLogServiceClient
}

func (c *ComponentAPI) ContentService() (ContentService, error) {
Expand Down Expand Up @@ -1141,11 +1142,13 @@ func (c *ComponentAPI) ContentService() (ContentService, error) {
type cs struct {
csapi.ContentServiceClient
csapi.WorkspaceServiceClient
csapi.HeadlessLogServiceClient
}

c.contentServiceStatus.ContentService = cs{
ContentServiceClient: csapi.NewContentServiceClient(conn),
WorkspaceServiceClient: csapi.NewWorkspaceServiceClient(conn),
ContentServiceClient: csapi.NewContentServiceClient(conn),
WorkspaceServiceClient: csapi.NewWorkspaceServiceClient(conn),
HeadlessLogServiceClient: csapi.NewHeadlessLogServiceClient(conn),
}

return c.contentServiceStatus.ContentService, nil
Expand Down
22 changes: 22 additions & 0 deletions test/tests/components/ws-manager/prebuild_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,9 @@ func TestOpenWorkspaceFromPrebuild(t *testing.T) {
}

t.Logf("prebuild snapshot: %s", prebuildSnapshot)

// check the prebuild logs have been uploaded
checkPrebuildLogUploaded(t, ctx, api, ws)
}()

// launch the workspace from prebuild
Expand Down Expand Up @@ -692,6 +695,25 @@ func checkGitFolderPermission(t *testing.T, rsa *integration.RpcClient, workspac
}
}

func checkPrebuildLogUploaded(t *testing.T, ctx context.Context, api *integration.ComponentAPI, ws *integration.LaunchWorkspaceDirectlyResult) {
cs, err := api.ContentService()
if err != nil {
t.Fatal(err)
}
resp, err := cs.ListLogs(ctx, &csapi.ListLogsRequest{
WorkspaceId: ws.LastStatus.Metadata.MetaId,
InstanceId: ws.Req.Id,
OwnerId: ws.LastStatus.Metadata.Owner,
})
if err != nil {
t.Fatal(err)
}
if len(resp.TaskId) == 0 {
t.Fatal("no logs found")
}
t.Logf("found logs (task ids: %v)", resp.TaskId)
}

func findSnapshotFromStoppedWs(t *testing.T, ctx context.Context, lastStatus *wsmanapi.WorkspaceStatus) (string, *wsmanapi.VolumeSnapshotInfo, error) {
if lastStatus == nil {
return "", nil, fmt.Errorf("did not get last workspace status")
Expand Down