Merge pull request #2833 from likebreath/fix_2832

clh: Add support to unplug block devices

virtcontainers/clh.go

@@ -89,6 +89,8 @@ type clhClient interface {
 	VmAddDevicePut(ctx context.Context, vmAddDevice chclient.VmAddDevice) (chclient.PciDeviceInfo, *http.Response, error)
 	// Add a new disk device to the VM
 	VmAddDiskPut(ctx context.Context, diskConfig chclient.DiskConfig) (chclient.PciDeviceInfo, *http.Response, error)
+	// Remove a device from the VM
+	VmRemoveDevicePut(ctx context.Context, vmRemoveDevice chclient.VmRemoveDevice) (*http.Response, error)
 }
 
 type CloudHypervisorVersion struct {
@@ -402,7 +404,11 @@ func (clh *cloudHypervisor) getThreadIDs() (vcpuThreadIDs, error) {
 	return vcpuInfo, nil
 }
 
-func (clh *cloudHypervisor) hotplugBlockDevice(drive *config.BlockDrive) error {
+func clhDriveIndexToID(i int) string {
+	return "clh_drive_" + strconv.Itoa(i)
+}
+
+func (clh *cloudHypervisor) hotplugAddBlockDevice(drive *config.BlockDrive) error {
 	if clh.config.BlockDeviceDriver != config.VirtioBlock {
 		return fmt.Errorf("incorrect hypervisor configuration on 'block_device_driver':"+
 			" using '%v' but only support '%v'", clh.config.BlockDeviceDriver, config.VirtioBlock)
@@ -417,6 +423,8 @@ func (clh *cloudHypervisor) hotplugBlockDevice(drive *config.BlockDrive) error {
 		return openAPIClientError(err)
 	}
 
+	driveID := clhDriveIndexToID(drive.Index)
+
 	//Explicitly set PCIAddr to NULL, so that VirtPath can be used
 	drive.PCIAddr = ""
 
@@ -427,6 +435,7 @@ func (clh *cloudHypervisor) hotplugBlockDevice(drive *config.BlockDrive) error {
 			Path:      drive.File,
 			Readonly:  drive.ReadOnly,
 			VhostUser: false,
+			Id:        driveID,
 		}
 		_, _, err = cl.VmAddDiskPut(ctx, blkDevice)
 	}
@@ -461,7 +470,7 @@ func (clh *cloudHypervisor) hotplugAddDevice(devInfo interface{}, devType device
 	switch devType {
 	case blockDev:
 		drive := devInfo.(*config.BlockDrive)
-		return nil, clh.hotplugBlockDevice(drive)
+		return nil, clh.hotplugAddBlockDevice(drive)
 	case vfioDev:
 		device := devInfo.(*config.VFIODev)
 		return nil, clh.hotPlugVFIODevice(*device)
@@ -471,9 +480,39 @@ func (clh *cloudHypervisor) hotplugAddDevice(devInfo interface{}, devType device
 
 }
 
+func (clh *cloudHypervisor) hotplugRemoveBlockDevice(drive *config.BlockDrive) error {
+	cl := clh.client()
+	ctx, cancel := context.WithTimeout(context.Background(), clhHotPlugAPITimeout*time.Second)
+	defer cancel()
+
+	driveID := clhDriveIndexToID(drive.Index)
+
+	if drive.Pmem {
+		return fmt.Errorf("pmem device hotplug remove not supported")
+	}
+
+	_, err := cl.VmRemoveDevicePut(ctx, chclient.VmRemoveDevice{Id: driveID})
+
+	if err != nil {
+		err = fmt.Errorf("failed to hotplug remove block device %+v %s", drive, openAPIClientError(err))
+	}
+
+	return err
+}
+
 func (clh *cloudHypervisor) hotplugRemoveDevice(devInfo interface{}, devType deviceType) (interface{}, error) {
-	clh.Logger().WithField("function", "hotplugRemoveDevice").Warn("hotplug remove device not supported")
-	return nil, nil
+	span, _ := clh.trace("hotplugRemoveDevice")
+	defer span.Finish()
+
+	switch devType {
+	case blockDev:
+		return nil, clh.hotplugRemoveBlockDevice(devInfo.(*config.BlockDrive))
+	default:
+		clh.Logger().WithFields(log.Fields{"devInfo": devInfo,
+			"deviceType": devType}).Error("hotplugRemoveDevice: unsupported device")
+		return nil, fmt.Errorf("Could not hot remove device: unsupported device: %v, type: %v",
+			devInfo, devType)
+	}
 }
 
 func (clh *cloudHypervisor) hypervisorConfig() HypervisorConfig {

virtcontainers/clh_test.go

@@ -104,6 +104,11 @@ func (c *clhClientMock) VmAddDiskPut(ctx context.Context, diskConfig chclient.Di
 	return chclient.PciDeviceInfo{}, nil, nil
 }
 
+//nolint:golint
+func (c *clhClientMock) VmRemoveDevicePut(ctx context.Context, vmRemoveDevice chclient.VmRemoveDevice) (*http.Response, error) {
+	return nil, nil
+}
+
 func TestCloudHypervisorAddVSock(t *testing.T) {
 	assert := assert.New(t)
 	clh := cloudHypervisor{}
@@ -363,7 +368,7 @@ func TestCheckVersion(t *testing.T) {
 	}
 }
 
-func TestCloudHypervisorHotplugBlockDevice(t *testing.T) {
+func TestCloudHypervisorHotplugAddBlockDevice(t *testing.T) {
 	assert := assert.New(t)
 
 	clhConfig, err := newClhConfig()
@@ -374,13 +379,31 @@ func TestCloudHypervisorHotplugBlockDevice(t *testing.T) {
 	clh.APIClient = &clhClientMock{}
 
 	clh.config.BlockDeviceDriver = config.VirtioBlock
-	err = clh.hotplugBlockDevice(&config.BlockDrive{Pmem: false})
+	err = clh.hotplugAddBlockDevice(&config.BlockDrive{Pmem: false})
 	assert.NoError(err, "Hotplug disk block device expected no error")
 
-	err = clh.hotplugBlockDevice(&config.BlockDrive{Pmem: true})
+	err = clh.hotplugAddBlockDevice(&config.BlockDrive{Pmem: true})
 	assert.Error(err, "Hotplug pmem block device expected error")
 
 	clh.config.BlockDeviceDriver = config.VirtioSCSI
-	err = clh.hotplugBlockDevice(&config.BlockDrive{Pmem: false})
+	err = clh.hotplugAddBlockDevice(&config.BlockDrive{Pmem: false})
 	assert.Error(err, "Hotplug block device not using 'virtio-blk' expected error")
 }
+
+func TestCloudHypervisorHotplugRemoveBlockDevice(t *testing.T) {
+	assert := assert.New(t)
+
+	clhConfig, err := newClhConfig()
+	assert.NoError(err)
+
+	clh := &cloudHypervisor{}
+	clh.config = clhConfig
+	clh.APIClient = &clhClientMock{}
+
+	clh.config.BlockDeviceDriver = config.VirtioBlock
+	err = clh.hotplugRemoveBlockDevice(&config.BlockDrive{Pmem: false})
+	assert.NoError(err, "Hotplug remove disk block device expected no error")
+
+	err = clh.hotplugRemoveBlockDevice(&config.BlockDrive{Pmem: true})
+	assert.Error(err, "Hotplug remove pmem block device expected error")
+}