devices: address some comments

Address some review comments:
* remove unnecessary rollback logics
* add vfio hot unplug handling.

Signed-off-by: Wei Zhang <zhangwei555@huawei.com>

virtcontainers/container.go

@@ -1113,6 +1113,9 @@ func (c *Container) removeDrive() (err error) {
 }
 
 func (c *Container) attachDevices() error {
+	// there's no need to do rollback when error happens,
+	// because if attachDevices fails, container creation will fail too,
+	// and rollbackFailingContainerCreation could do all the rollbacks
 	for _, dev := range c.devices {
 		if err := c.sandbox.devManager.AttachDevice(dev.ID, c.sandbox); err != nil {
 			if err == manager.ErrDeviceAttached {
@@ -1124,7 +1127,6 @@ func (c *Container) attachDevices() error {
 	}
 
 	if err := c.sandbox.storeSandboxDevices(); err != nil {
-		//TODO: roll back?
 		return err
 	}
 	return nil
@@ -1134,13 +1136,16 @@ func (c *Container) detachDevices() error {
 	for _, dev := range c.devices {
 		if err := c.sandbox.devManager.DetachDevice(dev.ID, c.sandbox); err != nil {
 			if err == manager.ErrDeviceNotAttached {
-				// skip if device is already attached before
+				// skip if device isn't attached
 				continue
 			}
 			return err
 		}
 	}
 
+	if err := c.sandbox.storeSandboxDevices(); err != nil {
+		return err
+	}
 	return nil
 }
 

virtcontainers/device/drivers/block.go

@@ -34,6 +34,10 @@ func NewBlockDevice(devInfo *config.DeviceInfo) *BlockDevice {
 // Attach is standard interface of api.Device, it's used to add device to some
 // DeviceReceiver
 func (device *BlockDevice) Attach(devReceiver api.DeviceReceiver) (err error) {
+	if device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	// Increment the block index for the sandbox. This is used to determine the name
 	// for the block device in the case where the block device is used as container
 	// rootfs and the predicted block device name needs to be provided to the agent.
@@ -87,6 +91,10 @@ func (device *BlockDevice) Attach(devReceiver api.DeviceReceiver) (err error) {
 // Detach is standard interface of api.Device, it's used to remove device from some
 // DeviceReceiver
 func (device *BlockDevice) Detach(devReceiver api.DeviceReceiver) error {
+	if !device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	deviceLogger().WithField("device", device.DeviceInfo.HostPath).Info("Unplugging block device")
 
 	if err := devReceiver.HotplugRemoveDevice(device, config.DeviceBlock); err != nil {

virtcontainers/device/drivers/generic.go

@@ -27,11 +27,19 @@ func NewGenericDevice(devInfo *config.DeviceInfo) *GenericDevice {
 
 // Attach is standard interface of api.Device
 func (device *GenericDevice) Attach(devReceiver api.DeviceReceiver) error {
+	if device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	return nil
 }
 
 // Detach is standard interface of api.Device
 func (device *GenericDevice) Detach(devReceiver api.DeviceReceiver) error {
+	if !device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	return nil
 }
 

virtcontainers/device/drivers/vfio.go

@@ -46,6 +46,10 @@ func NewVFIODevice(devInfo *config.DeviceInfo) *VFIODevice {
 // Attach is standard interface of api.Device, it's used to add device to some
 // DeviceReceiver
 func (device *VFIODevice) Attach(devReceiver api.DeviceReceiver) error {
+	if device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	vfioGroup := filepath.Base(device.DeviceInfo.HostPath)
 	iommuDevicesPath := filepath.Join(config.SysIOMMUPath, vfioGroup, "devices")
 
@@ -85,6 +89,20 @@ func (device *VFIODevice) Attach(devReceiver api.DeviceReceiver) error {
 // Detach is standard interface of api.Device, it's used to remove device from some
 // DeviceReceiver
 func (device *VFIODevice) Detach(devReceiver api.DeviceReceiver) error {
+	if !device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
+	// hotplug a VFIO device is actually hotplugging a group of iommu devices
+	if err := devReceiver.HotplugRemoveDevice(device, config.DeviceVFIO); err != nil {
+		deviceLogger().WithError(err).Error("Failed to remove device")
+		return err
+	}
+
+	deviceLogger().WithFields(logrus.Fields{
+		"device-group": device.DeviceInfo.HostPath,
+		"device-type":  "vfio-passthrough",
+	}).Info("Device group attached")
 	device.DeviceInfo.Hotplugged = false
 	return nil
 }

virtcontainers/device/drivers/vhost_user_blk.go

@@ -27,6 +27,10 @@ type VhostUserBlkDevice struct {
 // Attach is standard interface of api.Device, it's used to add device to some
 // DeviceReceiver
 func (device *VhostUserBlkDevice) Attach(devReceiver api.DeviceReceiver) (err error) {
+	if device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	// generate a unique ID to be used for hypervisor commandline fields
 	randBytes, err := utils.GenerateRandomBytes(8)
 	if err != nil {
@@ -48,6 +52,10 @@ func (device *VhostUserBlkDevice) Attach(devReceiver api.DeviceReceiver) (err er
 // Detach is standard interface of api.Device, it's used to remove device from some
 // DeviceReceiver
 func (device *VhostUserBlkDevice) Detach(devReceiver api.DeviceReceiver) error {
+	if !device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	device.DeviceInfo.Hotplugged = false
 	return nil
 }

virtcontainers/device/drivers/vhost_user_net.go

@@ -27,6 +27,10 @@ type VhostUserNetDevice struct {
 // Attach is standard interface of api.Device, it's used to add device to some
 // DeviceReceiver
 func (device *VhostUserNetDevice) Attach(devReceiver api.DeviceReceiver) (err error) {
+	if device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	// generate a unique ID to be used for hypervisor commandline fields
 	randBytes, err := utils.GenerateRandomBytes(8)
 	if err != nil {
@@ -48,6 +52,10 @@ func (device *VhostUserNetDevice) Attach(devReceiver api.DeviceReceiver) (err er
 // Detach is standard interface of api.Device, it's used to remove device from some
 // DeviceReceiver
 func (device *VhostUserNetDevice) Detach(devReceiver api.DeviceReceiver) error {
+	if !device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	device.DeviceInfo.Hotplugged = false
 	return nil
 }

virtcontainers/device/drivers/vhost_user_scsi.go

@@ -27,6 +27,10 @@ type VhostUserSCSIDevice struct {
 // Attach is standard interface of api.Device, it's used to add device to some
 // DeviceReceiver
 func (device *VhostUserSCSIDevice) Attach(devReceiver api.DeviceReceiver) (err error) {
+	if device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	// generate a unique ID to be used for hypervisor commandline fields
 	randBytes, err := utils.GenerateRandomBytes(8)
 	if err != nil {
@@ -48,6 +52,10 @@ func (device *VhostUserSCSIDevice) Attach(devReceiver api.DeviceReceiver) (err e
 // Detach is standard interface of api.Device, it's used to remove device from some
 // DeviceReceiver
 func (device *VhostUserSCSIDevice) Detach(devReceiver api.DeviceReceiver) error {
+	if !device.DeviceInfo.Hotplugged {
+		return nil
+	}
+
 	device.DeviceInfo.Hotplugged = false
 	return nil
 }

virtcontainers/sandbox.go

@@ -1459,28 +1459,10 @@ func (s *Sandbox) HotplugAddDevice(device api.Device, devType config.DeviceType)
 		if !ok {
 			return fmt.Errorf("device type mismatch, expect device type to be %s", devType)
 		}
-		addedDev := []*config.VFIODev{}
-		var err error
-		defer func() {
-			// if err happens,roll back and remove added device!
-			if err != nil {
-				for _, dev := range addedDev {
-					if _, rollbackErr := s.hypervisor.hotplugRemoveDevice(dev, vfioDev); rollbackErr != nil {
-						s.Logger().
-							WithFields(logrus.Fields{
-								"sandboxid":       s.id,
-								"vfio device ID":  dev.ID,
-								"vfio device BDF": dev.BDF,
-							}).WithError(rollbackErr).
-							Error("failed to remove vfio device for rolling back")
-					}
-				}
-			}
-		}()
 
 		// adding a group of VFIO devices
 		for _, dev := range vfioDevices {
-			if _, err = s.hypervisor.hotplugAddDevice(dev, vfioDev); err != nil {
+			if _, err := s.hypervisor.hotplugAddDevice(dev, vfioDev); err != nil {
 				s.Logger().
 					WithFields(logrus.Fields{
 						"sandboxid":       s.id,
@@ -1489,7 +1471,6 @@ func (s *Sandbox) HotplugAddDevice(device api.Device, devType config.DeviceType)
 					}).WithError(err).Error("failed to hotplug VFIO device")
 				return err
 			}
-			addedDev = append(addedDev, dev)
 		}
 		return nil
 	case config.DeviceBlock:
@@ -1515,28 +1496,10 @@ func (s *Sandbox) HotplugRemoveDevice(device api.Device, devType config.DeviceTy
 		if !ok {
 			return fmt.Errorf("device type mismatch, expect device type to be %s", devType)
 		}
-		removedDev := []*config.VFIODev{}
-		var err error
-		defer func() {
-			// if err happens,roll back and add the removed devices back!
-			if err != nil {
-				for _, dev := range removedDev {
-					if _, rollbackErr := s.hypervisor.hotplugAddDevice(dev, vfioDev); rollbackErr != nil {
-
-						s.Logger().WithError(rollbackErr).
-							WithFields(logrus.Fields{
-								"sandboxid":       s.id,
-								"vfio device ID":  dev.ID,
-								"vfio device BDF": dev.BDF,
-							}).Error("failed to add vfio device for rolling back")
-					}
-				}
-			}
-		}()
 
 		// remove a group of VFIO devices
 		for _, dev := range vfioDevices {
-			if _, err = s.hypervisor.hotplugRemoveDevice(dev, vfioDev); err != nil {
+			if _, err := s.hypervisor.hotplugRemoveDevice(dev, vfioDev); err != nil {
 				s.Logger().WithError(err).
 					WithFields(logrus.Fields{
 						"sandboxid":       s.id,
@@ -1545,7 +1508,6 @@ func (s *Sandbox) HotplugRemoveDevice(device api.Device, devType config.DeviceTy
 					}).Error("failed to hot unplug VFIO device")
 				return err
 			}
-			removedDev = append(removedDev, dev)
 		}
 		return nil
 	case config.DeviceBlock: