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>
This commit is contained in:
Wei Zhang 2018-07-10 23:26:58 +08:00
parent 44c37bf774
commit b7464899ec
8 changed files with 67 additions and 42 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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