Log the error from umount when it fails in the background #8561
Conversation
There was a problem hiding this comment.
I found this logic a bit challenging to follow 😅 (After thinking about it, I see that we will only hit the default case if the context is done and we never read from the channel below, but it took a sec) Is there a way to make this more readable or could you add some comments?
|
I left a comment since I found any other way to write this code very clunky and repetitive. |
| // Since resultCh is unbuffered, this only happens when the outer | ||
| // function received from the channel and will return this err. | ||
| if err != nil { | ||
| log.CtxErrorf(ctx, "Failed to unmount %s: %s", f.mountPath, err) | ||
| } else { | ||
| log.CtxDebugf(ctx, "Unmounted %s", f.mountPath) | ||
| } |
There was a problem hiding this comment.
In this case, shouldn't the caller function be responsible for logging the error (i.e. this case should be empty right?) I think this means we're handling the error in two places
There was a problem hiding this comment.
Yeah, I guess what I missed is that firecracker.unmountAllVBDs skips logging on the first attempt. I'm not sure if this is helpful. If it fails because of a cancelled context, than maybe it makes sense to ignore logging, but if it fails because unmount failed, it seems useful to know what error was returned.
Previously, we logged
Unmounted %s in the background, even after the context was canceled, even if thef.server.Unmount()call failed.Also, there was a logging only race, where the context wouldn't be expired in the child goroutine, so it wouldn't log anything, and then it would be expired in the parent goroutine, which would indicate that we might unmount in the background.