From f2d9632bc0d42b40d266f02dd24a72e91214dc3c Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Mon, 11 Jun 2018 12:46:08 -0700 Subject: [PATCH 1/3] network: Always bind back physical interfaces In case of physical network interfaces, we explicitly pass through them to the VM. We need to bind them back to the host driver when the sandbox is stopped, irrespective if the network namespace has been created by virtcontainers or not. Fixes #384 Signed-off-by: Archana Shinde --- virtcontainers/cni.go | 14 ++++++++---- virtcontainers/cnm.go | 14 ++++++++---- virtcontainers/network.go | 42 ++++++++++++++++++++++------------ virtcontainers/noop_network.go | 2 +- virtcontainers/sandbox.go | 6 +---- 5 files changed, 48 insertions(+), 30 deletions(-) diff --git a/virtcontainers/cni.go b/virtcontainers/cni.go index 96074b23b6..3406e59cb3 100644 --- a/virtcontainers/cni.go +++ b/virtcontainers/cni.go @@ -153,10 +153,10 @@ func (n *cni) add(sandbox *Sandbox, config NetworkConfig, netNsPath string, netN return networkNS, nil } -// remove unbridges and deletes TAP interfaces. It also removes virtual network -// interfaces and deletes the network namespace for the CNI network. -func (n *cni) remove(sandbox *Sandbox, networkNS NetworkNamespace) error { - if err := removeNetworkCommon(networkNS); err != nil { +// remove network endpoints in the network namespace. It also deletes the network +// namespace in case the namespace has been created by us. +func (n *cni) remove(sandbox *Sandbox, networkNS NetworkNamespace, netNsCreated bool) error { + if err := removeNetworkCommon(networkNS, netNsCreated); err != nil { return err } @@ -164,5 +164,9 @@ func (n *cni) remove(sandbox *Sandbox, networkNS NetworkNamespace) error { return err } - return deleteNetNS(networkNS.NetNsPath, true) + if netNsCreated { + return deleteNetNS(networkNS.NetNsPath, true) + } + + return nil } diff --git a/virtcontainers/cnm.go b/virtcontainers/cnm.go index 98c7c467fc..c508b0938c 100644 --- a/virtcontainers/cnm.go +++ b/virtcontainers/cnm.go @@ -47,12 +47,16 @@ func (n *cnm) add(sandbox *Sandbox, config NetworkConfig, netNsPath string, netN return networkNS, nil } -// remove unbridges and deletes TAP interfaces. It also removes virtual network -// interfaces and deletes the network namespace for the CNM network. -func (n *cnm) remove(sandbox *Sandbox, networkNS NetworkNamespace) error { - if err := removeNetworkCommon(networkNS); err != nil { +// remove network endpoints in the network namespace. It also deletes the network +// namespace in case the namespace has been created by us. +func (n *cnm) remove(sandbox *Sandbox, networkNS NetworkNamespace, netNsCreated bool) error { + if err := removeNetworkCommon(networkNS, netNsCreated); err != nil { return err } - return deleteNetNS(networkNS.NetNsPath, true) + if netNsCreated { + return deleteNetNS(networkNS.NetNsPath, true) + } + + return nil } diff --git a/virtcontainers/network.go b/virtcontainers/network.go index fb6f109aab..197f1324f9 100644 --- a/virtcontainers/network.go +++ b/virtcontainers/network.go @@ -153,7 +153,7 @@ type Endpoint interface { SetProperties(NetworkInfo) Attach(hypervisor) error - Detach() error + Detach(netNsCreated bool, netNsPath string) error } // VirtualEndpoint gathers a network pair and its properties. @@ -230,9 +230,18 @@ func (endpoint *VirtualEndpoint) Attach(h hypervisor) error { // Detach for the virtual endpoint tears down the tap and bridge // created for the veth interface. -func (endpoint *VirtualEndpoint) Detach() error { +func (endpoint *VirtualEndpoint) Detach(netNsCreated bool, netNsPath string) error { + // The network namespace would have been deleted at this point + // if it has not been created by virtcontainers. + if !netNsCreated { + return nil + } + networkLogger().Info("Detaching virtual endpoint") - return xconnectVMNetwork(&(endpoint.NetPair), false) + + return doNetNS(netNsPath, func(_ ns.NetNS) error { + return xconnectVMNetwork(&(endpoint.NetPair), false) + }) } // Properties returns the properties of the interface. @@ -281,7 +290,7 @@ func (endpoint *VhostUserEndpoint) Attach(h hypervisor) error { } // Detach for vhostuser endpoint -func (endpoint *VhostUserEndpoint) Detach() error { +func (endpoint *VhostUserEndpoint) Detach(netNsCreated bool, netNsPath string) error { networkLogger().Info("Detaching vhostuser based endpoint") return nil } @@ -343,9 +352,14 @@ func (endpoint *PhysicalEndpoint) Attach(h hypervisor) error { // Detach for physical endpoint unbinds the physical network interface from vfio-pci // and binds it back to the saved host driver. -func (endpoint *PhysicalEndpoint) Detach() error { +func (endpoint *PhysicalEndpoint) Detach(netNsCreated bool, netNsPath string) error { // Bind back the physical network interface to host. + // We need to do this even if a new network namespace has not + // been created by virtcontainers. networkLogger().Info("Detaching physical endpoint") + + // We do not need to enter the network namespace to bind back the + // physical interface to host driver. return bindNICToHost(endpoint) } @@ -606,16 +620,16 @@ func addNetworkCommon(sandbox *Sandbox, networkNS *NetworkNamespace) error { return err } -func removeNetworkCommon(networkNS NetworkNamespace) error { - return doNetNS(networkNS.NetNsPath, func(_ ns.NetNS) error { - for _, endpoint := range networkNS.Endpoints { - if err := endpoint.Detach(); err != nil { - return err - } +func removeNetworkCommon(networkNS NetworkNamespace, netNsCreated bool) error { + for _, endpoint := range networkNS.Endpoints { + // Detach for an endpoint should enter the network namespace + // if required. + if err := endpoint.Detach(netNsCreated, networkNS.NetNsPath); err != nil { + return err } + } - return nil - }) + return nil } func createLink(netHandle *netlink.Handle, name string, expectedLink netlink.Link) (netlink.Link, []*os.File, error) { @@ -1410,5 +1424,5 @@ type network interface { // remove unbridges and deletes TAP interfaces. It also removes virtual network // interfaces and deletes the network namespace. - remove(sandbox *Sandbox, networkNS NetworkNamespace) error + remove(sandbox *Sandbox, networkNS NetworkNamespace, netNsCreated bool) error } diff --git a/virtcontainers/noop_network.go b/virtcontainers/noop_network.go index 7d37476866..e4ef408753 100644 --- a/virtcontainers/noop_network.go +++ b/virtcontainers/noop_network.go @@ -32,6 +32,6 @@ func (n *noopNetwork) add(sandbox *Sandbox, config NetworkConfig, netNsPath stri // remove unbridges and deletes TAP interfaces. It also removes virtual network // interfaces and deletes the network namespace for the Noop network. // It does nothing. -func (n *noopNetwork) remove(sandbox *Sandbox, networkNS NetworkNamespace) error { +func (n *noopNetwork) remove(sandbox *Sandbox, networkNS NetworkNamespace, netNsCreated bool) error { return nil } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 7b7459e5ac..5f48066f59 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -918,11 +918,7 @@ func (s *Sandbox) createNetwork() error { } func (s *Sandbox) removeNetwork() error { - if s.networkNS.NetNsCreated { - return s.network.remove(s, s.networkNS) - } - - return nil + return s.network.remove(s, s.networkNS, s.networkNS.NetNsCreated) } // startVM starts the VM. From 0806dcc19c8b3814484bc49a50dc4d1728a28932 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Fri, 15 Jun 2018 13:32:18 -0700 Subject: [PATCH 2/3] network: Drop mounted parameter in call to deleteNetNS All calls to deleteNetNS were passing the "mounted" parameter as true. So drop this parameter. Signed-off-by: Archana Shinde --- virtcontainers/cni.go | 2 +- virtcontainers/cnm.go | 2 +- virtcontainers/network.go | 16 ++++++---------- virtcontainers/network_test.go | 2 +- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/virtcontainers/cni.go b/virtcontainers/cni.go index 3406e59cb3..41e0e71e34 100644 --- a/virtcontainers/cni.go +++ b/virtcontainers/cni.go @@ -165,7 +165,7 @@ func (n *cni) remove(sandbox *Sandbox, networkNS NetworkNamespace, netNsCreated } if netNsCreated { - return deleteNetNS(networkNS.NetNsPath, true) + return deleteNetNS(networkNS.NetNsPath) } return nil diff --git a/virtcontainers/cnm.go b/virtcontainers/cnm.go index c508b0938c..e565ce0e83 100644 --- a/virtcontainers/cnm.go +++ b/virtcontainers/cnm.go @@ -55,7 +55,7 @@ func (n *cnm) remove(sandbox *Sandbox, networkNS NetworkNamespace, netNsCreated } if netNsCreated { - return deleteNetNS(networkNS.NetNsPath, true) + return deleteNetNS(networkNS.NetNsPath) } return nil diff --git a/virtcontainers/network.go b/virtcontainers/network.go index 197f1324f9..53e188374b 100644 --- a/virtcontainers/network.go +++ b/virtcontainers/network.go @@ -1112,7 +1112,7 @@ func doNetNS(netNSPath string, cb func(ns.NetNS) error) error { return cb(targetNS) } -func deleteNetNS(netNSPath string, mounted bool) error { +func deleteNetNS(netNSPath string) error { n, err := ns.GetNS(netNSPath) if err != nil { return err @@ -1123,15 +1123,11 @@ func deleteNetNS(netNSPath string, mounted bool) error { return err } - // This unmount part is supposed to be done in the cni/ns package, but the "mounted" - // flag is not updated when retrieving NetNs handler from GetNS(). - if mounted { - if err = unix.Unmount(netNSPath, unix.MNT_DETACH); err != nil { - return fmt.Errorf("Failed to unmount namespace %s: %v", netNSPath, err) - } - if err := os.RemoveAll(netNSPath); err != nil { - return fmt.Errorf("Failed to clean up namespace %s: %v", netNSPath, err) - } + if err = unix.Unmount(netNSPath, unix.MNT_DETACH); err != nil { + return fmt.Errorf("Failed to unmount namespace %s: %v", netNSPath, err) + } + if err := os.RemoveAll(netNSPath); err != nil { + return fmt.Errorf("Failed to clean up namespace %s: %v", netNSPath, err) } return nil diff --git a/virtcontainers/network_test.go b/virtcontainers/network_test.go index addd5e759e..21e999587f 100644 --- a/virtcontainers/network_test.go +++ b/virtcontainers/network_test.go @@ -123,7 +123,7 @@ func TestCreateDeleteNetNS(t *testing.T) { t.Fatal(err) } - err = deleteNetNS(netNSPath, true) + err = deleteNetNS(netNSPath) if err != nil { t.Fatal(err) } From a31dd496ebe8c41588c9acdaf7db73a90f3f72fa Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Sat, 16 Jun 2018 21:41:14 -0700 Subject: [PATCH 3/3] cni: Use the vendored version of CNI plugins to install binaries Out CI is failing because of a recent change introduced in the CNI plugins repo(github.com/containernetworking/plugins) that vendors in CNI v0.7.0-alpha0. Refer to commit #e4fdb6cd1883b7b. However, it looks like the the plugins themselves have not been updated yet, causing failures in CI. This was verified by vendoring in the latest CNI and CNI plugins in our repo. Till the plugin binaries our fixed, use older version of CNI plugins for testing virtcontainers. See this: https://github.com/containernetworking/plugins/commit/68b4efb4056c In any case we should keep this version in sync with what we vendor in, in our runtime and not use the latest commit. Signed-off-by: Archana Shinde --- virtcontainers/utils/virtcontainers-setup.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/virtcontainers/utils/virtcontainers-setup.sh b/virtcontainers/utils/virtcontainers-setup.sh index d241ad8cd3..ef56c220a7 100755 --- a/virtcontainers/utils/virtcontainers-setup.sh +++ b/virtcontainers/utils/virtcontainers-setup.sh @@ -60,12 +60,13 @@ echo -e "Move to ${tmpdir}/${virtcontainers_build_dir}" pushd ${tmpdir}/${virtcontainers_build_dir} echo "Clone cni" git clone https://github.com/containernetworking/plugins.git +pushd plugins +git checkout 7f98c94613021d8b57acfa1a2f0c8d0f6fd7ae5a echo "Copy CNI config files" cp $GOPATH/src/github.com/kata-containers/runtime/virtcontainers/test/cni/10-mynet.conf ${ETCDIR}/cni/net.d/ cp $GOPATH/src/github.com/kata-containers/runtime/virtcontainers/test/cni/99-loopback.conf ${ETCDIR}/cni/net.d/ -pushd plugins ./build.sh cp ./bin/bridge ${TMPDIR}/cni/bin/cni-bridge cp ./bin/loopback ${TMPDIR}/cni/bin/loopback