From 8c52aa15f0e4927c0e570102efaa34dbe93d6156 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 26 Jul 2018 15:09:31 -0400 Subject: Fix handling of Linux network namespaces The CNI plugins upstream removed their network namespace creation code, making it a test package only. Copy it into our repository and slightly modify it for our use (most notably, use MNT_DETACH when unmounting namespaces). This new CNI code splits closing and unmounting network namespaces, which allows us to greatly reduce the number of occasions on which we call teardownNetwork() and make more errors in that function fatal instead of warnings. Instead, we can call Close() and just close the open file descriptor in cases where the namespace has already been cleaned up. Signed-off-by: Matthew Heon Closes: #1165 Approved by: baude --- libpod/container_linux.go | 14 ++++++++------ libpod/networking_linux.go | 46 +++++++++++++++++++++++++++++----------------- 2 files changed, 37 insertions(+), 23 deletions(-) (limited to 'libpod') diff --git a/libpod/container_linux.go b/libpod/container_linux.go index 2330f27a7..1b1b3a1d9 100644 --- a/libpod/container_linux.go +++ b/libpod/container_linux.go @@ -21,9 +21,10 @@ func (ctr *Container) setNamespace(netNSPath string, newState *containerState) e if ctr.state.NetNS != nil && netNSPath == ctr.state.NetNS.Path() { newState.NetNS = ctr.state.NetNS } else { - // Tear down the existing namespace - if err := ctr.runtime.teardownNetNS(ctr); err != nil { - logrus.Warnf(err.Error()) + // Close the existing namespace. + // Whoever removed it from the database already tore it down. + if err := ctr.runtime.closeNetNS(ctr); err != nil { + return err } // Open the new network namespace @@ -37,9 +38,10 @@ func (ctr *Container) setNamespace(netNSPath string, newState *containerState) e } } else { // The container no longer has a network namespace - // Tear down the old one - if err := ctr.runtime.teardownNetNS(ctr); err != nil { - logrus.Warnf(err.Error()) + // Close the old one, whoever removed it from the DB should have + // cleaned it up already. + if err := ctr.runtime.closeNetNS(ctr); err != nil { + return err } } return nil diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index d9eb87572..dbc68e04b 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -9,13 +9,13 @@ import ( "path/filepath" "strconv" "strings" - "syscall" cnitypes "github.com/containernetworking/cni/pkg/types/current" "github.com/containernetworking/plugins/pkg/ns" "github.com/cri-o/ocicni/pkg/ocicni" "github.com/pkg/errors" "github.com/projectatomic/libpod/pkg/inspect" + "github.com/projectatomic/libpod/pkg/netns" "github.com/projectatomic/libpod/utils" "github.com/sirupsen/logrus" "github.com/vishvananda/netlink" @@ -77,7 +77,7 @@ func (r *Runtime) configureNetNS(ctr *Container, ctrNS ns.NetNS) (err error) { // Create and configure a new network namespace for a container func (r *Runtime) createNetNS(ctr *Container) (err error) { - ctrNS, err := ns.NewNS() + ctrNS, err := netns.NewNS() if err != nil { return errors.Wrapf(err, "error creating network namespace for container %s", ctr.ID()) } @@ -148,7 +148,27 @@ func joinNetNS(path string) (ns.NetNS, error) { return ns, nil } -// Tear down a network namespace +// Close a network namespace. +// Differs from teardownNetNS() in that it will not attempt to undo the setup of +// the namespace, but will instead only close the open file descriptor +func (r *Runtime) closeNetNS(ctr *Container) error { + if ctr.state.NetNS == nil { + // The container has no network namespace, we're set + return nil + } + + if err := ctr.state.NetNS.Close(); err != nil { + return errors.Wrapf(err, "error closing network namespace for container %s", ctr.ID()) + } + + ctr.state.NetNS = nil + + return nil +} + +// Tear down a network namespace, undoing all state associated with it. +// The CNI firewall rules will be removed, the namespace will be unmounted, +// and the file descriptor associated with it closed. func (r *Runtime) teardownNetNS(ctr *Container) error { if ctr.state.NetNS == nil { // The container has no network namespace, we're set @@ -173,27 +193,19 @@ func (r *Runtime) teardownNetNS(ctr *Container) error { // The network may have already been torn down, so don't fail here, just log if err := r.netPlugin.TearDownPod(podNetwork); err != nil { - logrus.Errorf("Failed to tear down network namespace for container %s: %v", ctr.ID(), err) + return errors.Wrapf(err, "error tearing down CNI namespace configuration for container %s", ctr.ID()) } - nsPath := ctr.state.NetNS.Path() + // First unmount the namespace + if err := netns.UnmountNS(ctr.state.NetNS); err != nil { + return errors.Wrapf(err, "error unmounting network namespace for container %s", ctr.ID()) + } + // Now close the open file descriptor if err := ctr.state.NetNS.Close(); err != nil { return errors.Wrapf(err, "error closing network namespace for container %s", ctr.ID()) } - // We need to unconditionally try to unmount/remove the namespace - // because we may be in a separate process from the one that created the - // namespace, and Close() will only do that if it is the same process. - if err := unix.Unmount(nsPath, unix.MNT_DETACH); err != nil { - if err != syscall.EINVAL && err != syscall.ENOENT { - return errors.Wrapf(err, "error unmounting network namespace %s for container %s", nsPath, ctr.ID()) - } - } - if err := os.RemoveAll(nsPath); err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "error removing network namespace %s for container %s", nsPath, ctr.ID()) - } - ctr.state.NetNS = nil return nil -- cgit v1.2.3-54-g00ecf