virtcontainers: refactor hotplug qmp functions

Refactor these functions so differernt types of endpoints can use a unified function to hotplug nics. Fixes #731 Signed-off-by: Ruidong Cao <[email protected]>
kata-containers · Oct 22, 2018 · f8f2962 · f8f2962
1 parent daf52de
commit f8f2962
Show file tree

Hide file tree

Showing 12 changed files with 111 additions and 56 deletions.
diff --git a/virtcontainers/bridgedmacvlan_endpoint.go b/virtcontainers/bridgedmacvlan_endpoint.go
@@ -71,6 +71,11 @@ func (endpoint *BridgedMacvlanEndpoint) PciAddr() string {
 	return endpoint.PCIAddr
 }
 
+// SetPciAddr sets the PCI address of the endpoint.
+func (endpoint *BridgedMacvlanEndpoint) SetPciAddr(pciAddr string) {
+	endpoint.PCIAddr = pciAddr
+}
+
 // NetworkPair returns the network pair of the endpoint.
 func (endpoint *BridgedMacvlanEndpoint) NetworkPair() *NetworkInterfacePair {
 	return &endpoint.NetPair

diff --git a/virtcontainers/bridgedmacvlan_endpoint_test.go b/virtcontainers/bridgedmacvlan_endpoint_test.go
@@ -16,15 +16,17 @@ func TestCreateBridgedMacvlanEndpoint(t *testing.T) {
 
 	expected := &BridgedMacvlanEndpoint{
 		NetPair: NetworkInterfacePair{
-			ID:   "uniqueTestID-4",
-			Name: "br4_kata",
+			TapInterface: TapInterface{
+				ID:   "uniqueTestID-4",
+				Name: "br4_kata",
+				TAPIface: NetworkInterface{
+					Name: "tap4_kata",
+				},
+			},
 			VirtIface: NetworkInterface{
 				Name:     "eth4",
 				HardAddr: macAddr.String(),
 			},
-			TAPIface: NetworkInterface{
-				Name: "tap4_kata",
-			},
 			NetInterworkingModel: DefaultNetInterworkingModel,
 		},
 		EndpointType: BridgedMacvlanEndpointType,

diff --git a/virtcontainers/endpoint.go b/virtcontainers/endpoint.go
@@ -19,6 +19,7 @@ type Endpoint interface {
 	NetworkPair() *NetworkInterfacePair
 
 	SetProperties(NetworkInfo)
+	SetPciAddr(string)
 	Attach(hypervisor) error
 	Detach(netNsCreated bool, netNsPath string) error
 	HotAttach(h hypervisor) error

diff --git a/virtcontainers/macvtap_endpoint.go b/virtcontainers/macvtap_endpoint.go
@@ -93,6 +93,11 @@ func (endpoint *MacvtapEndpoint) PciAddr() string {
 	return endpoint.PCIAddr
 }
 
+// SetPciAddr sets the PCI address of the endpoint.
+func (endpoint *MacvtapEndpoint) SetPciAddr(pciAddr string) {
+	endpoint.PCIAddr = pciAddr
+}
+
 // NetworkPair returns the network pair of the endpoint.
 func (endpoint *MacvtapEndpoint) NetworkPair() *NetworkInterfacePair {
 	return nil

diff --git a/virtcontainers/network.go b/virtcontainers/network.go
@@ -122,15 +122,20 @@ type NetworkInterface struct {
 	Addrs    []netlink.Addr
 }
 
+// TapInterface defines a tap nic.
+type TapInterface struct {
+	ID       string
+	Name     string
+	TAPIface NetworkInterface
+	VMFds    []*os.File
+	VhostFds []*os.File
+}
+
 // NetworkInterfacePair defines a pair between VM and virtual network interfaces.
 type NetworkInterfacePair struct {
-	ID        string
-	Name      string
+	TapInterface
 	VirtIface NetworkInterface
-	TAPIface  NetworkInterface
 	NetInterworkingModel
-	VMFds    []*os.File
-	VhostFds []*os.File
 }
 
 // NetworkConfig is the network configuration related to a network.
@@ -962,15 +967,17 @@ func createNetworkInterfacePair(idx int, ifName string, interworkingModel NetInt
 	}
 
 	netPair := NetworkInterfacePair{
-		ID:   uniqueID,
-		Name: fmt.Sprintf("br%d_kata", idx),
+		TapInterface: TapInterface{
+			ID:   uniqueID,
+			Name: fmt.Sprintf("br%d_kata", idx),
+			TAPIface: NetworkInterface{
+				Name: fmt.Sprintf("tap%d_kata", idx),
+			},
+		},
 		VirtIface: NetworkInterface{
 			Name:     fmt.Sprintf("eth%d", idx),
 			HardAddr: randomMacAddr,
 		},
-		TAPIface: NetworkInterface{
-			Name: fmt.Sprintf("tap%d_kata", idx),
-		},
 		NetInterworkingModel: interworkingModel,
 	}
 

diff --git a/virtcontainers/physical_endpoint.go b/virtcontainers/physical_endpoint.go
@@ -53,6 +53,11 @@ func (endpoint *PhysicalEndpoint) PciAddr() string {
 	return endpoint.PCIAddr
 }
 
+// SetPciAddr sets the PCI address of the endpoint.
+func (endpoint *PhysicalEndpoint) SetPciAddr(pciAddr string) {
+	endpoint.PCIAddr = pciAddr
+}
+
 // SetProperties sets the properties of the physical endpoint.
 func (endpoint *PhysicalEndpoint) SetProperties(properties NetworkInfo) {
 	endpoint.EndpointProperties = properties

diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go
@@ -838,66 +838,77 @@ func (q *qemu) hotplugVFIODevice(device *config.VFIODev, op operation) error {
 	return nil
 }
 
-func (q *qemu) hotplugMacvtap(drive *VethEndpoint) error {
+func (q *qemu) hotAddNetDevice(name, hardAddr string, VMFds, VhostFds []*os.File) error {
 	var (
 		VMFdNames    []string
 		VhostFdNames []string
 	)
-	for i, VMFd := range drive.NetPair.VMFds {
+	for i, VMFd := range VMFds {
 		fdName := fmt.Sprintf("fd%d", i)
-		err := q.qmpMonitorCh.qmp.ExecuteGetFD(q.qmpMonitorCh.ctx, fdName, VMFd)
-		if err != nil {
+		if err := q.qmpMonitorCh.qmp.ExecuteGetFD(q.qmpMonitorCh.ctx, fdName, VMFd); err != nil {
 			return err
 		}
 		VMFdNames = append(VMFdNames, fdName)
 	}
-	for i, VhostFd := range drive.NetPair.VhostFds {
+	for i, VhostFd := range VhostFds {
 		fdName := fmt.Sprintf("vhostfd%d", i)
-		err := q.qmpMonitorCh.qmp.ExecuteGetFD(q.qmpMonitorCh.ctx, fdName, VhostFd)
-		if err != nil {
+		if err := q.qmpMonitorCh.qmp.ExecuteGetFD(q.qmpMonitorCh.ctx, fdName, VhostFd); err != nil {
 			return err
 		}
 		VhostFdNames = append(VhostFdNames, fdName)
 	}
-	return q.qmpMonitorCh.qmp.ExecuteNetdevAddByFds(q.qmpMonitorCh.ctx, "tap", drive.NetPair.Name, VMFdNames, VhostFdNames)
+	return q.qmpMonitorCh.qmp.ExecuteNetdevAddByFds(q.qmpMonitorCh.ctx, "tap", name, VMFdNames, VhostFdNames)
 }
 
-func (q *qemu) hotplugNetDevice(drive *VethEndpoint, op operation) error {
+func (q *qemu) hotplugNetDevice(endpoint Endpoint, op operation) error {
 	err := q.qmpSetup()
 	if err != nil {
 		return err
 	}
-	devID := "virtio-" + drive.NetPair.ID
+	var tap TapInterface
 
 	if op == addDevice {
-		switch drive.NetPair.NetInterworkingModel {
-		case NetXConnectBridgedModel:
-			if err := q.qmpMonitorCh.qmp.ExecuteNetdevAdd(q.qmpMonitorCh.ctx, "tap", drive.NetPair.Name, drive.NetPair.TAPIface.Name, "no", "no", int(q.config.NumVCPUs)); err != nil {
-				return err
-			}
-		case NetXConnectMacVtapModel:
-			if err := q.hotplugMacvtap(drive); err != nil {
-				return err
-			}
+		switch endpoint.Type() {
+		case VethEndpointType:
+			drive := endpoint.(*VethEndpoint)
+			tap = drive.NetPair.TapInterface
 		default:
-			return fmt.Errorf("this net interworking model is not supported")
+			return fmt.Errorf("this endpoint is not supported")
 		}
-		addr, bridge, err := q.addDeviceToBridge(drive.NetPair.ID)
+
+		if err = q.hotAddNetDevice(tap.Name, endpoint.HardwareAddr(), tap.VMFds, tap.VhostFds); err != nil {
+			return err
+		}
+
+		addr, bridge, err := q.addDeviceToBridge(tap.ID)
 		if err != nil {
 			return err
 		}
-		drive.PCIAddr = fmt.Sprintf("%02x/%s", bridge.Addr, addr)
-		if err = q.qmpMonitorCh.qmp.ExecuteNetPCIDeviceAdd(q.qmpMonitorCh.ctx, drive.NetPair.Name, devID, drive.NetPair.TAPIface.HardAddr, addr, bridge.ID, romFile, int(q.config.NumVCPUs)); err != nil {
+		pciAddr := fmt.Sprintf("%02x/%s", bridge.Addr, addr)
+		endpoint.SetPciAddr(pciAddr)
+
+		devID := "virtio-" + tap.ID
+		if err = q.qmpMonitorCh.qmp.ExecuteNetPCIDeviceAdd(q.qmpMonitorCh.ctx, tap.Name, devID, endpoint.HardwareAddr(), addr, bridge.ID, romFile, int(q.config.NumVCPUs)); err != nil {
 			return err
 		}
 	} else {
-		if err := q.removeDeviceFromBridge(drive.NetPair.ID); err != nil {
+		switch endpoint.Type() {
+		case VethEndpointType:
+			drive := endpoint.(*VethEndpoint)
+			tap = drive.NetPair.TapInterface
+		default:
+			return fmt.Errorf("this endpoint is not supported")
+		}
+
+		if err := q.removeDeviceFromBridge(tap.ID); err != nil {
 			return err
 		}
+
+		devID := "virtio-" + tap.ID
 		if err := q.qmpMonitorCh.qmp.ExecuteDeviceDel(q.qmpMonitorCh.ctx, devID); err != nil {
 			return err
 		}
-		if err := q.qmpMonitorCh.qmp.ExecuteNetdevDel(q.qmpMonitorCh.ctx, drive.NetPair.Name); err != nil {
+		if err := q.qmpMonitorCh.qmp.ExecuteNetdevDel(q.qmpMonitorCh.ctx, tap.Name); err != nil {
 			return err
 		}
 	}
@@ -919,7 +930,7 @@ func (q *qemu) hotplugDevice(devInfo interface{}, devType deviceType, op operati
 		memdev := devInfo.(*memoryDevice)
 		return q.hotplugMemory(memdev, op)
 	case netDev:
-		device := devInfo.(*VethEndpoint)
+		device := devInfo.(Endpoint)
 		return nil, q.hotplugNetDevice(device, op)
 	default:
 		return nil, fmt.Errorf("cannot hotplug device: unsupported device type '%v'", devType)

diff --git a/virtcontainers/qemu_arch_base_test.go b/virtcontainers/qemu_arch_base_test.go
@@ -443,15 +443,17 @@ func TestQemuArchBaseAppendNetwork(t *testing.T) {
 
 	macvlanEp := &BridgedMacvlanEndpoint{
 		NetPair: NetworkInterfacePair{
-			ID:   "uniqueTestID-4",
-			Name: "br4_kata",
+			TapInterface: TapInterface{
+				ID:   "uniqueTestID-4",
+				Name: "br4_kata",
+				TAPIface: NetworkInterface{
+					Name: "tap4_kata",
+				},
+			},
 			VirtIface: NetworkInterface{
 				Name:     "eth4",
 				HardAddr: macAddr.String(),
 			},
-			TAPIface: NetworkInterface{
-				Name: "tap4_kata",
-			},
 			NetInterworkingModel: DefaultNetInterworkingModel,
 		},
 		EndpointType: BridgedMacvlanEndpointType,

diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go
@@ -1142,6 +1142,9 @@ func (s *Sandbox) RemoveInterface(inf *grpc.Interface) (*grpc.Interface, error)
 				return inf, err
 			}
 			s.networkNS.Endpoints = append(s.networkNS.Endpoints[:i], s.networkNS.Endpoints[i+1:]...)
+			if err := s.storage.storeSandboxNetwork(s.id, s.networkNS); err != nil {
+				return inf, err
+			}
 			break
 		}
 	}

diff --git a/virtcontainers/veth_endpoint.go b/virtcontainers/veth_endpoint.go
@@ -70,6 +70,11 @@ func (endpoint *VethEndpoint) PciAddr() string {
 	return endpoint.PCIAddr
 }
 
+// SetPciAddr sets the PCI address of the endpoint.
+func (endpoint *VethEndpoint) SetPciAddr(pciAddr string) {
+	endpoint.PCIAddr = pciAddr
+}
+
 // NetworkPair returns the network pair of the endpoint.
 func (endpoint *VethEndpoint) NetworkPair() *NetworkInterfacePair {
 	return &endpoint.NetPair

diff --git a/virtcontainers/veth_endpoint_test.go b/virtcontainers/veth_endpoint_test.go
@@ -16,15 +16,17 @@ func TestCreateVethNetworkEndpoint(t *testing.T) {
 
 	expected := &VethEndpoint{
 		NetPair: NetworkInterfacePair{
-			ID:   "uniqueTestID-4",
-			Name: "br4_kata",
+			TapInterface: TapInterface{
+				ID:   "uniqueTestID-4",
+				Name: "br4_kata",
+				TAPIface: NetworkInterface{
+					Name: "tap4_kata",
+				},
+			},
 			VirtIface: NetworkInterface{
 				Name:     "eth4",
 				HardAddr: macAddr.String(),
 			},
-			TAPIface: NetworkInterface{
-				Name: "tap4_kata",
-			},
 			NetInterworkingModel: DefaultNetInterworkingModel,
 		},
 		EndpointType: VethEndpointType,
@@ -51,15 +53,17 @@ func TestCreateVethNetworkEndpointChooseIfaceName(t *testing.T) {
 
 	expected := &VethEndpoint{
 		NetPair: NetworkInterfacePair{
-			ID:   "uniqueTestID-4",
-			Name: "br4_kata",
+			TapInterface: TapInterface{
+				ID:   "uniqueTestID-4",
+				Name: "br4_kata",
+				TAPIface: NetworkInterface{
+					Name: "tap4_kata",
+				},
+			},
 			VirtIface: NetworkInterface{
 				Name:     "eth1",
 				HardAddr: macAddr.String(),
 			},
-			TAPIface: NetworkInterface{
-				Name: "tap4_kata",
-			},
 			NetInterworkingModel: DefaultNetInterworkingModel,
 		},
 		EndpointType: VethEndpointType,

diff --git a/virtcontainers/vhostuser_endpoint.go b/virtcontainers/vhostuser_endpoint.go
@@ -62,6 +62,11 @@ func (endpoint *VhostUserEndpoint) PciAddr() string {
 	return endpoint.PCIAddr
 }
 
+// SetPciAddr sets the PCI address of the endpoint.
+func (endpoint *VhostUserEndpoint) SetPciAddr(pciAddr string) {
+	endpoint.PCIAddr = pciAddr
+}
+
 // NetworkPair returns the network pair of the endpoint.
 func (endpoint *VhostUserEndpoint) NetworkPair() *NetworkInterfacePair {
 	return nil