Merge pull request #385 from amshinde/always-bind-back-physical-interfaces

network: Always bind back physical interfaces

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
+// remove network endpoints in the network namespace. It also deletes the network
-// interfaces and deletes the network namespace for the CNI network.
+// namespace in case the namespace has been created by us.
-func (n *cni) remove(sandbox *Sandbox, networkNS NetworkNamespace) error {
+func (n *cni) remove(sandbox *Sandbox, networkNS NetworkNamespace, netNsCreated bool) error {
-	if err := removeNetworkCommon(networkNS); err != nil {
+	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)
+	}
+
+	return nil
 }

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
+// remove network endpoints in the network namespace. It also deletes the network
-// interfaces and deletes the network namespace for the CNM network.
+// namespace in case the namespace has been created by us.
-func (n *cnm) remove(sandbox *Sandbox, networkNS NetworkNamespace) error {
+func (n *cnm) remove(sandbox *Sandbox, networkNS NetworkNamespace, netNsCreated bool) error {
-	if err := removeNetworkCommon(networkNS); err != nil {
+	if err := removeNetworkCommon(networkNS, netNsCreated); err != nil {
 		return err
 	}
 
-	return deleteNetNS(networkNS.NetNsPath, true)
+	if netNsCreated {
+		return deleteNetNS(networkNS.NetNsPath)
+	}
+
+	return nil
 }

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 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 {
+func removeNetworkCommon(networkNS NetworkNamespace, netNsCreated bool) error {
-	return doNetNS(networkNS.NetNsPath, func(_ ns.NetNS) error {
 	for _, endpoint := range networkNS.Endpoints {
-			if err := endpoint.Detach(); err != nil {
+		// Detach for an endpoint should enter the network namespace
+		// if required.
+		if err := endpoint.Detach(netNsCreated, networkNS.NetNsPath); err != nil {
 			return err
 		}
 	}
 
 	return nil
-	})
 }
 
 func createLink(netHandle *netlink.Handle, name string, expectedLink netlink.Link) (netlink.Link, []*os.File, error) {
@@ -1098,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
@@ -1109,16 +1123,12 @@ 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)
 	}
-	}
 
 	return nil
 }
@@ -1410,5 +1420,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
 }

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)
 	}

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
 }

virtcontainers/sandbox.go

@@ -923,11 +923,7 @@ func (s *Sandbox) createNetwork() error {
 }
 
 func (s *Sandbox) removeNetwork() error {
-	if s.networkNS.NetNsCreated {
+	return s.network.remove(s, s.networkNS, s.networkNS.NetNsCreated)
-		return s.network.remove(s, s.networkNS)
-	}
-
-	return nil
 }
 
 // startVM starts the VM.

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