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 <archana.m.shinde@intel.com>

virtcontainers/cni.go

@@ -153,16 +153,20 @@ 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
 	}
 
 	if err := n.invokePluginsDelete(sandbox, networkNS); err != nil {
 		return err
 	}
 
-	return deleteNetNS(networkNS.NetNsPath, true)
+	if netNsCreated {
+		return deleteNetNS(networkNS.NetNsPath, true)
+	}
+
+	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
-// 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
 }

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
 }

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

@@ -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.