devmapper/Remove(): use Rmdir, ignore errors #36438
Conversation
Codecov Report
@@ Coverage Diff @@
## master #36438 +/- ##
==========================================
+ Coverage 34.65% 34.66% +0.01%
==========================================
Files 613 613
Lines 45375 45379 +4
==========================================
+ Hits 15725 15732 +7
Misses 27588 27588
+ Partials 2062 2059 -3 |
There was a problem hiding this comment.
nit: this (and logrus) should be in the same group;
diff --git a/daemon/graphdriver/devmapper/driver.go b/daemon/graphdriver/devmapper/driver.go
index e9d9e85aa..83a57f328 100644
--- a/daemon/graphdriver/devmapper/driver.go
+++ b/daemon/graphdriver/devmapper/driver.go
@@ -9,8 +9,6 @@ import (
"path"
"strconv"
- "github.com/sirupsen/logrus"
-
"github.com/docker/docker/daemon/graphdriver"
"github.com/docker/docker/pkg/containerfs"
"github.com/docker/docker/pkg/devicemapper"
@@ -18,7 +16,7 @@ import (
"github.com/docker/docker/pkg/locker"
"github.com/docker/docker/pkg/mount"
units "github.com/docker/go-units"
-
+ "github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
)There was a problem hiding this comment.
nit: s/as with an older kernel can/as an older kernel can/
There was a problem hiding this comment.
perhaps
logrus.Warnf("unable to remove mountpoint %q: %s", mp, err)("unable" or "failed" - used "unable" to make it look less as an important error)
There was a problem hiding this comment.
WithField("module", "graphdriver/devicemapper")
There was a problem hiding this comment.
WithField("module", "graphdriver/devicemapper")
devmapper uses aufs uses I am in flux about which of the three to use. I guess I'll use the one already used by devmapper -- let me know if you want me to change it. |
|
patch updated |
There was a problem hiding this comment.
if you remove the blank line and gofmt is should sort the imports correctly 😄
There was a problem hiding this comment.
In this case I wanted to separate "our" packages (ie github.com/docker/docker/*) from third-party ones. But maybe you're right, it makes little sense if the list is sorted, we clearly see docker/docker from others.
Blank line removed.
|
Should we have a tracking issue for standardizing those log properties among storage drivers, or not worth the effort? (could be a “beginners” issue for someone to work on) |
Makes total sense to me |
1. Replace EnsureRemoveAll() with Rmdir(), as here we are removing the container's mount point, which is already properly unmounted and is therefore an empty directory. 2. Ignore the Rmdir() error (but log it unless it's ENOENT). This is a mount point, currently unmounted (i.e. an empty directory), and an older kernel can return EBUSY if e.g. the mount was leaked to other mount namespaces. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It's a combination of factors that should not happen when using a recent moby/moby, although I guess if you would use an older kernel (like RHEL/CentOS 7.3) and "organize" a mount point leak, you might end up seeing this. It seems way too complicated to setup for me. |
|
Opened #36478 for tracking the "standardisation" of log-messages |
Replace EnsureRemoveAll() with Rmdir(), as here we are removing
the container's mount point, which is already properly unmounted
and is therefore an empty directory.
Ignore the Rmdir() error (but log it unless it's ENOENT). This
is a mount point, currently unmounted (i.e. an empty directory),
and an older kernel can return EBUSY if e.g. the mount was
leaked to other mount namespaces.