cache: Clean up temporary mount pool on restart #2652
Conversation
There was a problem hiding this comment.
Looks good, one comment about a corner case.
Also, if it's feasible it would be nice to have a test of this behavior. Unfortunately I don't think the client integ tests would support something like sending SIGKILL to buildkitd and then restarting it, but even something in the unit tests would be helpful.
cache/manager.go
Outdated
| dir := filepath.Join(opt.MountPoolRoot, file.Name()) | ||
| logrus.Debugf("cleaning up existing temporary mount %q", dir) | ||
| if err := mount.Unmount(dir, 0); err != nil { | ||
| logrus.WithError(err).Warnf("failed to unmount existing temporary mount %q", dir) |
There was a problem hiding this comment.
I guess if the host experienced a sudden power off then when buildkitd starts up again you could end up with dirs left here but no mounts. In this case Unmount would always fail but since we continue the dir would never be removed and this log would be printed every startup forever. Maybe it's better to check the err and try removing the dir if the error indicates there wasn't a mount there.
Also, nit about using bklog for these statements.
There was a problem hiding this comment.
Thank you for the review.
I guess if the host experienced a sudden power off then when buildkitd starts up again you could end up with dirs left here but no mounts. In this case Unmount would always fail but since we continue the dir would never be removed and this log would be printed every startup forever.
containerd's mount.Unmount ignores EINVAL which can is returned when the dir isn't a mount point. So I think it won't be occur.
Also, nit about using bklog for these statements.
Fixed to use bklog.
Also, if it's feasible it would be nice to have a test of this behavior. Unfortunately I don't think the client integ tests would support something like sending SIGKILL to buildkitd and then restarting it, but even something in the unit tests would be helpful.
Thank you for the suggestion.
Added an unit test.
There was a problem hiding this comment.
containerd's mount.Unmount ignores EINVAL which can is returned when the dir isn't a mount point. So I think it won't be occur.
Ah nice, good to know. Thanks for updating.
ktock
force-pushed
the
sharemounts-cleanup
branch
2 times, most recently
from
5ae5d93
to
856bed2
Compare
cache/refs.go
Outdated
| for _, file := range files { | ||
| if file.IsDir() { | ||
| dir := filepath.Join(tmpdirRoot, file.Name()) | ||
| logrus.Debugf("cleaning up existing temporary mount %q", dir) |
There was a problem hiding this comment.
Thank you for the review. fixed this.
ktock
force-pushed
the
sharemounts-cleanup
branch
from
856bed2
to
b251f33
Compare
|
@ktock Needs rebase |
Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
ktock
force-pushed
the
sharemounts-cleanup
branch
from
b251f33
to
3b74229
Compare
|
Thank you. |
Following-up #2637 (comment)
Currently, if buildkitd crashes before unmounting temporary (writable) overlayfs mounts, it could leak these mounts.
Then if buildkitd started up again, same issue of duplicating overlay mounts can happen because the old mounts remain on the host.
This commit fixes this issue by introducing a directory
cachemountsunder/var/lib/buildkit/for storing all temporary overlay mounts.When buildkitd starts, it cleans up all existing (old) mounts under that directory so we can avoid the issue of duplicating overlay mounts.