Skip to content

[content-service] Return ErrNotFound if bucket object does not exists #3773

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

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Apr 6, 2021

fixes #3772

@aledbf aledbf requested review from csweichel and geropl April 6, 2021 13:45
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM

Thx! 🚀

@aledbf
Copy link
Member Author

aledbf commented Apr 6, 2021

/werft run

👍 started the job as gitpod-build-aledbf-contenterr.1

@geropl
Copy link
Member

geropl commented Apr 6, 2021

@aledbf I just checked the client code: It seems we do not properly check for ErrNotFound. What about going fixing that as well and move on without this today? :-)

@aledbf
Copy link
Member Author

aledbf commented Apr 6, 2021

I just checked the client code

Where exactly? (I read WorkspaceService and the code checks the error)

@geropl
Copy link
Member

geropl commented Apr 7, 2021

@aledbf You're absolutely right, I got lost in the WithError noise here:

if req.IncludeSnapshots {
prefix := cs.s.BackupObject(req.WorkspaceId, "")
if !strings.HasSuffix(prefix, "/") {
prefix = prefix + "/"
}
err = cs.s.DeleteObject(ctx, cs.s.Bucket(req.OwnerId), &storage.DeleteObjectQuery{Prefix: prefix})
if err != nil {
if err == storage.ErrNotFound {
log.WithError(err).Error("deleting workspace backup: NotFound")
return &api.DeleteWorkspaceResponse{}, nil
}
log.WithError(err).Error("error deleting workspace backup")
return nil, status.Error(codes.Unknown, err.Error())
}
return &api.DeleteWorkspaceResponse{}, nil
}
blobName := cs.s.BackupObject(req.WorkspaceId, storage.DefaultBackup)
err = cs.s.DeleteObject(ctx, cs.s.Bucket(req.OwnerId), &storage.DeleteObjectQuery{Name: blobName})
if err != nil {
if err == storage.ErrNotFound {
log.WithError(err).Error("deleting workspace backup: NotFound, ", blobName)
return &api.DeleteWorkspaceResponse{}, nil
}
log.WithError(err).Error("error deleting workspace backup: ", blobName)
return nil, status.Error(codes.Unknown, err.Error())
}
trailPrefix := cs.s.BackupObject(req.WorkspaceId, "trail-")
err = cs.s.DeleteObject(ctx, cs.s.Bucket(req.OwnerId), &storage.DeleteObjectQuery{Prefix: trailPrefix})
if err != nil {
if err == storage.ErrNotFound {
log.WithError(err).Error("deleting workspace backup: NotFound, ", trailPrefix)
return &api.DeleteWorkspaceResponse{}, nil
}
log.WithError(err).Error("error deleting workspace backup: ", trailPrefix)
return nil, status.Error(codes.Unknown, err.Error())
}

In case of ErrNotFound we should default to Debug() and not Error().

@aledbf aledbf force-pushed the aledbf/contenterr branch from 74fc107 to 796d762 Compare April 7, 2021 09:50
@aledbf
Copy link
Member Author

aledbf commented Apr 7, 2021

In case of ErrNotFound we should default to Debug() and not Error().

Done

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! 🙏

@aledbf aledbf mentioned this pull request Apr 7, 2021
24 tasks
@aledbf aledbf merged commit 7d1844b into main Apr 7, 2021
@aledbf aledbf deleted the aledbf/contenterr branch April 7, 2021 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server/Content Service fail when deleting non-existing workspace backup (which is a regular case)
2 participants