Skip to content
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

Log warning instead of tracing error span #11830

Merged
merged 1 commit into from
Aug 4, 2022
Merged

Log warning instead of tracing error span #11830

merged 1 commit into from
Aug 4, 2022

Conversation

jenting
Copy link
Contributor

@jenting jenting commented Aug 3, 2022

Description

Log warning instead of tracing error span if the error message contains cannot find workspace.

Related Issue(s)

Fixes #11710
Fixes #11713

How to test

None

Release Notes

None

Documentation

None

Werft options:

  • /werft with-preview

@jenting jenting marked this pull request as ready for review August 3, 2022 07:47
@jenting jenting requested a review from a team August 3, 2022 07:47
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Aug 3, 2022
@jenting jenting marked this pull request as draft August 3, 2022 07:55
@jenting
Copy link
Contributor Author

jenting commented Aug 3, 2022

I'm thinking about whether we should handle this kind of error within ws-manager, rather than ws-daemon.

We made a similar approach for WaitForInit function, special handling the gRPC not found error.

if st, ok := grpc_status.FromError(err); ok && st.Code() == codes.NotFound {
// Looks like we have missed the CREATING phase in which we'd otherwise start the workspace content initialization.
// Let's see if we're initializing already. If so, there's something very wrong because ws-daemon does not know about
// this workspace yet. In that case we'll run another desperate attempt to initialize the workspace.
m.initializerMapLock.Lock()
if _, alreadyInitializing := m.initializerMap[pod.Name]; alreadyInitializing {
// we're already initializing but wsdaemon does not know about this workspace. That's very bad.
log.WithFields(wsk8s.GetOWIFromObject(&pod.ObjectMeta)).Error("we were already initializing but wsdaemon does not know about this workspace (bug in ws-daemon?). Trying again!")
delete(m.initializerMap, pod.Name)
}
m.initializerMapLock.Unlock()
// It's ok - maybe we were restarting in that time. Instead of waiting for things to finish, we'll just start the
// initialization now.
err = m.initializeWorkspaceContent(ctx, pod)

WDYT? @csweichel @aledbf @kylos101

@csweichel
Copy link
Contributor

I'm thinking about whether we should handle this kind of error within ws-manager, rather than ws-daemon.

We made a similar approach for WaitForInit function, special handling the gRPC not found error.

if st, ok := grpc_status.FromError(err); ok && st.Code() == codes.NotFound {
// Looks like we have missed the CREATING phase in which we'd otherwise start the workspace content initialization.
// Let's see if we're initializing already. If so, there's something very wrong because ws-daemon does not know about
// this workspace yet. In that case we'll run another desperate attempt to initialize the workspace.
m.initializerMapLock.Lock()
if _, alreadyInitializing := m.initializerMap[pod.Name]; alreadyInitializing {
// we're already initializing but wsdaemon does not know about this workspace. That's very bad.
log.WithFields(wsk8s.GetOWIFromObject(&pod.ObjectMeta)).Error("we were already initializing but wsdaemon does not know about this workspace (bug in ws-daemon?). Trying again!")
delete(m.initializerMap, pod.Name)
}
m.initializerMapLock.Unlock()
// It's ok - maybe we were restarting in that time. Instead of waiting for things to finish, we'll just start the
// initialization now.
err = m.initializeWorkspaceContent(ctx, pod)

WDYT? @csweichel @aledbf @kylos101

Agreed - handling this in ws-manager is the better way. For one, from ws-daemon's perspective the lifecycle of the workspace has actually ended, and keeping info about that workspace beyond this point doesn't feel "natural". From ws-manaer's perspective it's easier to keep the Git status in memory because the workspace still exists.

@roboquat roboquat added size/XS and removed size/S labels Aug 3, 2022
@jenting jenting changed the title Log warning instead of return error Check annotation gitpod.io/disposalStatus Aug 3, 2022
@roboquat roboquat added size/S and removed size/XS labels Aug 4, 2022
@jenting jenting force-pushed the jenting/11710 branch 2 times, most recently from 5c19591 to 3f8ecb3 Compare August 4, 2022 01:48
@jenting jenting changed the title Check annotation gitpod.io/disposalStatus Log warning instead of tracing error span Aug 4, 2022
@jenting jenting force-pushed the jenting/11710 branch 3 times, most recently from d829105 to 195ab3a Compare August 4, 2022 03:28
@jenting jenting marked this pull request as ready for review August 4, 2022 03:44
Special handling the error message for tracing when we can't find
workspace in memory.

Signed-off-by: JenTing Hsiao <hsiaoairplane@gmail.com>
@roboquat roboquat merged commit d5c9bd7 into main Aug 4, 2022
@roboquat roboquat deleted the jenting/11710 branch August 4, 2022 09:46
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note-none size/M team: workspace Issue belongs to the Workspace team
Projects
None yet
4 participants