diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 36328a78da..042013125d 100644 --- a/virtcontainers/container.go +++ b/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 } diff --git a/virtcontainers/device/drivers/block.go b/virtcontainers/device/drivers/block.go index b7d36a7fd5..38e5a4b64b 100644 --- a/virtcontainers/device/drivers/block.go +++ b/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 { diff --git a/virtcontainers/device/drivers/generic.go b/virtcontainers/device/drivers/generic.go index 50e65e0908..431d178439 100644 --- a/virtcontainers/device/drivers/generic.go +++ b/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 } diff --git a/virtcontainers/device/drivers/vfio.go b/virtcontainers/device/drivers/vfio.go index 72a3cedc89..a97270543e 100644 --- a/virtcontainers/device/drivers/vfio.go +++ b/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 } diff --git a/virtcontainers/device/drivers/vhost_user_blk.go b/virtcontainers/device/drivers/vhost_user_blk.go index fea18a36f5..6bb629461f 100644 --- a/virtcontainers/device/drivers/vhost_user_blk.go +++ b/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 } diff --git a/virtcontainers/device/drivers/vhost_user_net.go b/virtcontainers/device/drivers/vhost_user_net.go index 5402760516..6a731542ce 100644 --- a/virtcontainers/device/drivers/vhost_user_net.go +++ b/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 } diff --git a/virtcontainers/device/drivers/vhost_user_scsi.go b/virtcontainers/device/drivers/vhost_user_scsi.go index 690981b1c5..ce5f846cd5 100644 --- a/virtcontainers/device/drivers/vhost_user_scsi.go +++ b/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 } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 0e8ae6ca3b..bf265e0b60 100644 --- a/virtcontainers/sandbox.go +++ b/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: