Don't prevent boot on temp cleanup#2463
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2463 +/- ##
==========================================
- Coverage 49.29% 45.03% -4.26%
==========================================
Files 84 92 +8
Lines 7486 9425 +1939
==========================================
+ Hits 3690 4245 +555
- Misses 3116 4497 +1381
- Partials 680 683 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Log the error? It may be lost if the last unmount succeeds
Maybe move the log here instead ? I was going to suggest to return for other kinds of error, but I can only think of EPERM, ENOENT & EINVAL as possible other errors which we may want to ignore on boot too
There was a problem hiding this comment.
i'm debating this, I don't really want to put logging in the mount pkg but...
There was a problem hiding this comment.
We can either continue on any error, aggregate them and return; Or return for any error immediately. :) It doesn't seem good to silently skip unix.EBUSY.
There was a problem hiding this comment.
Yes, we can just keep on wrapping it otherwise :)
|
@crosbymichael Should we cherry-pick this? |
|
@Random-Liu probably in a 1.1.3 for next week or later. This is more of an issue on the ubuntu 16.04 kernels |
|
@crosbymichael Sure. And what will happen if we fail to umount the temp mount? Will it affect any containerd functionality? If not, should we just log the error in main.go? |
NVM, you are already doing so. |
crosbymichael
force-pushed
the
temp-clean
branch
from
209d71a to
2668366
Compare
|
Ok, what do you both think about this solution? |
crosbymichael
force-pushed
the
temp-clean
branch
from
2668366 to
cb82d4f
Compare
Fixes containerd#2462 Fixes containerd#2455 Signed-off-by: Michael Crosby <[email protected]>
crosbymichael
force-pushed
the
temp-clean
branch
from
cb82d4f to
0105959
Compare
| return errors.Wrap(err, "unmounting temp mounts") | ||
| warnings, err := mount.CleanupTempMounts(0) | ||
| if err != nil { | ||
| log.G(ctx).WithError(err).Error("unmounting temp mounts") |
There was a problem hiding this comment.
Do we still want err to return here?Looks like the only case where we would get an error is if getmntinfo returns an error
There was a problem hiding this comment.
ya, i think this is fine
|
Was this cherry-picked? |
|
@AkihiroSuda no |
6 participants
Fixes #2462
Fixes #2455
Signed-off-by: Michael Crosby [email protected]