From 7df3f45f8357f3c0d8efd3398bd03302f025e6f0 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Wed, 17 Jan 2018 13:46:26 -0800 Subject: [PATCH 1/5] scsi: Add support for scsi driver for hotplugging drives Introduce a configuration option for using either virtio-scsi or virtio-block for hotplugging drives with virtio-scsi being the default driver. Hotplug block devices based on this confuration. In order to hotplug SCSI drives, we need to coldplug a SCSI controller while starting up qemu. With virtio-scsi, use the index at which the drive is attached to come up with a SCSI id and LUN. This will be used by the agent to detect the drive name. Signed-off-by: Archana Shinde --- container.go | 11 ++++++----- hypervisor.go | 10 ++++++++++ mount.go | 27 +++++++++++++++++++++++++ mount_test.go | 50 +++++++++++++++++++++++++++++++++++++++++++++++ pod.go | 3 +++ qemu.go | 46 +++++++++++++++++++++++++++++++------------ qemu_arch_base.go | 21 ++++++++++++++++++++ 7 files changed, 150 insertions(+), 18 deletions(-) diff --git a/container.go b/container.go index 9d159ee0..a74c7266 100644 --- a/container.go +++ b/container.go @@ -679,12 +679,18 @@ func (c *Container) hotplugDrive() error { "fs-type": fsType, }).Info("Block device detected") + driveIndex, err := c.pod.getAndSetPodBlockIndex() + if err != nil { + return err + } + // Add drive with id as container id devID := makeNameID("drive", c.id) drive := Drive{ File: devicePath, Format: "raw", ID: devID, + Index: driveIndex, } if err := c.pod.hypervisor.hotplugAddDevice(drive, blockDev); err != nil { @@ -692,11 +698,6 @@ func (c *Container) hotplugDrive() error { } c.setStateHotpluggedDrive(true) - driveIndex, err := c.pod.getAndSetPodBlockIndex() - if err != nil { - return err - } - if err := c.setStateBlockIndex(driveIndex); err != nil { return err } diff --git a/hypervisor.go b/hypervisor.go index c90ba235..a39fcba8 100644 --- a/hypervisor.go +++ b/hypervisor.go @@ -47,6 +47,8 @@ const ( defaultMemSzMiB = 2048 defaultBridges = 1 + + defaultBlockDriver = VirtioSCSI ) // deviceType describes a virtualized device type. @@ -155,6 +157,10 @@ type HypervisorConfig struct { // DisableBlockDeviceUse disallows a block device from being used. DisableBlockDeviceUse bool + // BlockDeviceDriver specifies the driver to be used for block device + // either VirtioSCSI or VirtioBlock with the default driver being defaultBlockDriver + BlockDeviceDriver string + // KernelParams are additional guest kernel parameters. KernelParams []Param @@ -227,6 +233,10 @@ func (conf *HypervisorConfig) valid() (bool, error) { conf.DefaultBridges = defaultBridges } + if conf.BlockDeviceDriver == "" { + conf.BlockDeviceDriver = defaultBlockDriver + } + return true, nil } diff --git a/mount.go b/mount.go index 281e06ed..7bd5ab04 100644 --- a/mount.go +++ b/mount.go @@ -321,3 +321,30 @@ func bindUnmountAllRootfs(sharedDir string, pod Pod) { } } } + +const maxSCSIDevices = 65535 + +// getSCSIIdLun gets the SCSI id and lun, based on the index of the drive being inserted. +// qemu code suggests that scsi-id can take values from 0 to 255 inclusive, while lun can +// take values from 0 to 16383 inclusive. But lun values over 255 do not seem to follow +// consistent SCSI addressing. Hence we limit to 255. +func getSCSIIdLun(index int) (int, int, error) { + if index < 0 { + return -1, -1, fmt.Errorf("Index cannot be negative") + } + + if index > maxSCSIDevices { + return -1, -1, fmt.Errorf("Index cannot be greater than %d, maximum of %d devices are supported", maxSCSIDevices, maxSCSIDevices) + } + + return index / 256, index % 256, nil +} + +func getSCSIAddress(index int) (string, error) { + scsiID, lun, err := getSCSIIdLun(index) + if err != nil { + return "", err + } + + return fmt.Sprintf("%d:%d", scsiID, lun), nil +} diff --git a/mount_test.go b/mount_test.go index efb12624..337529ac 100644 --- a/mount_test.go +++ b/mount_test.go @@ -19,6 +19,7 @@ package virtcontainers import ( "bytes" "fmt" + "github.com/stretchr/testify/assert" "os" "os/exec" "path/filepath" @@ -282,3 +283,52 @@ func TestGetVirtDriveName(t *testing.T) { } } } + +func TestGetSCSIIdLun(t *testing.T) { + tests := []struct { + index int + expectedScsiID int + expectedLun int + }{ + {0, 0, 0}, + {1, 0, 1}, + {2, 0, 2}, + {255, 0, 255}, + {256, 1, 0}, + {257, 1, 1}, + {258, 1, 2}, + {512, 2, 0}, + {513, 2, 1}, + } + + for _, test := range tests { + scsiID, lun, err := getSCSIIdLun(test.index) + assert.Nil(t, err) + + if scsiID != test.expectedScsiID && lun != test.expectedLun { + t.Fatalf("Expecting scsi-id:lun %d:%d, Got %d:%d", test.expectedScsiID, test.expectedLun, scsiID, lun) + } + } + + _, _, err := getSCSIIdLun(maxSCSIDevices + 1) + assert.NotNil(t, err) +} + +func TestGetSCSIAddress(t *testing.T) { + tests := []struct { + index int + expectedSCSIAddress string + }{ + {0, "0:0"}, + {200, "0:200"}, + {255, "0:255"}, + {258, "1:2"}, + {512, "2:0"}, + } + + for _, test := range tests { + scsiAddr, err := getSCSIAddress(test.index) + assert.Nil(t, err) + assert.Equal(t, scsiAddr, test.expectedSCSIAddress) + } +} diff --git a/pod.go b/pod.go index 993c2a40..53c1d910 100644 --- a/pod.go +++ b/pod.go @@ -245,6 +245,9 @@ type Drive struct { // ID is used to identify this drive in the hypervisor options. ID string + + // Index assigned to the drive. In case of virtio-scsi, this is used as SCSI LUN index + Index int } // EnvVar is a key/value structure representing a command diff --git a/qemu.go b/qemu.go index 4bee5ae5..bd268e59 100644 --- a/qemu.go +++ b/qemu.go @@ -76,6 +76,10 @@ const ( removeDevice ) +const ( + scsiControllerID = "scsi0" +) + type qmpLogger struct { logger *logrus.Entry } @@ -328,7 +332,6 @@ func (q *qemu) createPod(podConfig PodConfig) error { devices = q.arch.append9PVolumes(devices, podConfig.Volumes) devices = q.arch.appendConsole(devices, q.getPodConsole(podConfig.ID)) - devices = q.arch.appendBridges(devices, q.state.Bridges) imagePath, err := q.config.ImageAssetPath() if err != nil { @@ -340,6 +343,15 @@ func (q *qemu) createPod(podConfig PodConfig) error { return err } + if q.config.BlockDeviceDriver == VirtioBlock { + devices = q.arch.appendBridges(devices, q.state.Bridges) + if err != nil { + return err + } + } else { + devices = q.arch.appendSCSIController(devices) + } + cpuModel := q.arch.cpuModel() firmwarePath, err := podConfig.HypervisorConfig.FirmwareAssetPath() @@ -578,22 +590,30 @@ func (q *qemu) hotplugBlockDevice(drive Drive, op operation) error { return err } - driver := "virtio-blk-pci" + if q.config.BlockDeviceDriver == VirtioBlock { + driver := "virtio-blk-pci" + addr, bus, err := q.addDeviceToBridge(drive.ID) - addr, bus, err := q.addDeviceToBridge(drive.ID) - if err != nil { - return err - } + if err = q.qmpMonitorCh.qmp.ExecutePCIDeviceAdd(q.qmpMonitorCh.ctx, drive.ID, devID, driver, addr, bus); err != nil { + return err + } + } else { + driver := "scsi-hd" - if err = q.qmpMonitorCh.qmp.ExecutePCIDeviceAdd(q.qmpMonitorCh.ctx, drive.ID, devID, driver, addr, bus); err != nil { - return err - } + // Bus exposed by the SCSI Controller + bus := scsiControllerID + ".0" - } else { - if err := q.removeDeviceFromBridge(drive.ID); err != nil { - return err - } + // Get SCSI-id and LUN based on the order of attaching drives. + scsiID, lun, err := getSCSIIdLun(drive.Index) + if err != nil { + return err + } + if err = q.qmpMonitorCh.qmp.ExecuteSCSIDeviceAdd(q.qmpMonitorCh.ctx, drive.ID, devID, driver, bus, scsiID, lun); err != nil { + return err + } + } + } else { if err := q.qmpMonitorCh.qmp.ExecuteDeviceDel(q.qmpMonitorCh.ctx, devID); err != nil { return err } diff --git a/qemu_arch_base.go b/qemu_arch_base.go index 1dd888db..02999b10 100644 --- a/qemu_arch_base.go +++ b/qemu_arch_base.go @@ -65,6 +65,9 @@ type qemuArch interface { // appendImage appends an image to devices appendImage(devices []govmmQemu.Device, path string) ([]govmmQemu.Device, error) + // appendSCSIController appens a SCSI controller to devices + appendSCSIController(devices []govmmQemu.Device) []govmmQemu.Device + // appendBridges appends bridges to devices appendBridges(devices []govmmQemu.Device, bridges []Bridge) []govmmQemu.Device @@ -107,6 +110,14 @@ const ( maxDevIDSize = 31 ) +const ( + // VirtioBlock means use virtio-blk for hotplugging drives + VirtioBlock = "virtio-blk" + + // VirtioSCSI means use virtio-scsi for hotplugging drives + VirtioSCSI = "virtio-scsi" +) + const ( // QemuPCLite is the QEMU pc-lite machine type for amd64 QemuPCLite = "pc-lite" @@ -277,6 +288,16 @@ func (q *qemuArchBase) appendImage(devices []govmmQemu.Device, path string) ([]g return q.appendBlockDevice(devices, drive), nil } +func (q *qemuArchBase) appendSCSIController(devices []govmmQemu.Device) []govmmQemu.Device { + scsiController := govmmQemu.SCSIController{ + ID: scsiControllerID, + } + + devices = append(devices, scsiController) + + return devices +} + // appendBridges appends to devices the given bridges func (q *qemuArchBase) appendBridges(devices []govmmQemu.Device, bridges []Bridge) []govmmQemu.Device { for idx, b := range bridges { From 80bdc509cccc367e8974de95f8b43532c8b98d39 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Tue, 20 Feb 2018 14:03:23 -0800 Subject: [PATCH 2/5] tests: Fix tests to take into account default driver for block devices Since virtio-scsi is used as default driver for block devices, modify tests to include this in the default config. Signed-off-by: Archana Shinde --- api_test.go | 26 ++++++++++++++------------ hypervisor_test.go | 13 +++++++------ qemu_test.go | 13 +++++++------ 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/api_test.go b/api_test.go index 7eeb0691..92a20f91 100644 --- a/api_test.go +++ b/api_test.go @@ -876,12 +876,13 @@ func TestStatusPodSuccessfulStateReady(t *testing.T) { config := newTestPodConfigNoop() hypervisorConfig := HypervisorConfig{ - KernelPath: filepath.Join(testDir, testKernel), - ImagePath: filepath.Join(testDir, testImage), - HypervisorPath: filepath.Join(testDir, testHypervisor), - DefaultVCPUs: defaultVCPUs, - DefaultMemSz: defaultMemSzMiB, - DefaultBridges: defaultBridges, + KernelPath: filepath.Join(testDir, testKernel), + ImagePath: filepath.Join(testDir, testImage), + HypervisorPath: filepath.Join(testDir, testHypervisor), + DefaultVCPUs: defaultVCPUs, + DefaultMemSz: defaultMemSzMiB, + DefaultBridges: defaultBridges, + BlockDeviceDriver: defaultBlockDriver, } expectedStatus := PodStatus{ @@ -930,12 +931,13 @@ func TestStatusPodSuccessfulStateRunning(t *testing.T) { config := newTestPodConfigNoop() hypervisorConfig := HypervisorConfig{ - KernelPath: filepath.Join(testDir, testKernel), - ImagePath: filepath.Join(testDir, testImage), - HypervisorPath: filepath.Join(testDir, testHypervisor), - DefaultVCPUs: defaultVCPUs, - DefaultMemSz: defaultMemSzMiB, - DefaultBridges: defaultBridges, + KernelPath: filepath.Join(testDir, testKernel), + ImagePath: filepath.Join(testDir, testImage), + HypervisorPath: filepath.Join(testDir, testHypervisor), + DefaultVCPUs: defaultVCPUs, + DefaultMemSz: defaultMemSzMiB, + DefaultBridges: defaultBridges, + BlockDeviceDriver: defaultBlockDriver, } expectedStatus := PodStatus{ diff --git a/hypervisor_test.go b/hypervisor_test.go index 10de21ce..c6619f9d 100644 --- a/hypervisor_test.go +++ b/hypervisor_test.go @@ -174,12 +174,13 @@ func TestHypervisorConfigDefaults(t *testing.T) { testHypervisorConfigValid(t, hypervisorConfig, true) hypervisorConfigDefaultsExpected := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: "", - DefaultVCPUs: defaultVCPUs, - DefaultMemSz: defaultMemSzMiB, - DefaultBridges: defaultBridges, + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: "", + DefaultVCPUs: defaultVCPUs, + DefaultMemSz: defaultMemSzMiB, + DefaultBridges: defaultBridges, + BlockDeviceDriver: defaultBlockDriver, } if reflect.DeepEqual(hypervisorConfig, hypervisorConfigDefaultsExpected) == false { t.Fatal() diff --git a/qemu_test.go b/qemu_test.go index 9cf7a0e3..ac907b4f 100644 --- a/qemu_test.go +++ b/qemu_test.go @@ -30,12 +30,13 @@ import ( func newQemuConfig() HypervisorConfig { return HypervisorConfig{ - KernelPath: testQemuKernelPath, - ImagePath: testQemuImagePath, - HypervisorPath: testQemuPath, - DefaultVCPUs: defaultVCPUs, - DefaultMemSz: defaultMemSzMiB, - DefaultBridges: defaultBridges, + KernelPath: testQemuKernelPath, + ImagePath: testQemuImagePath, + HypervisorPath: testQemuPath, + DefaultVCPUs: defaultVCPUs, + DefaultMemSz: defaultMemSzMiB, + DefaultBridges: defaultBridges, + BlockDeviceDriver: defaultBlockDriver, } } From 3c038f153a8324a4d93e920d43e1fa34e8dd5fe3 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Thu, 25 Jan 2018 17:07:53 -0800 Subject: [PATCH 3/5] scsi: Pass SCSI address to hyperstart agent In case of SCSI driver, pass the SCSI address in the format "SCSI-id:LUN" to hyperstart agent. This will be used by the agent to find the device within the VM. This avoids predicting the drive name on the host side in case of SCSI. Signed-off-by: Archana Shinde --- hyperstart_agent.go | 17 +++++++++++++---- pkg/hyperstart/types.go | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/hyperstart_agent.go b/hyperstart_agent.go index 4ac70894..0d831ff0 100644 --- a/hyperstart_agent.go +++ b/hyperstart_agent.go @@ -414,13 +414,22 @@ func (h *hyper) startOneContainer(pod Pod, c *Container) error { container.SystemMountsInfo.BindMountDev = c.systemMountsInfo.BindMountDev if c.state.Fstype != "" { - driveName, err := getVirtDriveName(c.state.BlockIndex) - if err != nil { - return err + // Pass a drive name only in case of block driver + if pod.config.HypervisorConfig.BlockDeviceDriver == VirtioBlock { + driveName, err := getVirtDriveName(c.state.BlockIndex) + if err != nil { + return err + } + container.Image = driveName + } else { + scsiAddr, err := getSCSIAddress(c.state.BlockIndex) + if err != nil { + return err + } + container.SCSIAddr = scsiAddr } container.Fstype = c.state.Fstype - container.Image = driveName } else { if err := bindMountContainerRootfs(defaultSharedDir, pod.id, c.id, c.rootFs, false); err != nil { diff --git a/pkg/hyperstart/types.go b/pkg/hyperstart/types.go index 877b2f97..367a4527 100644 --- a/pkg/hyperstart/types.go +++ b/pkg/hyperstart/types.go @@ -207,7 +207,7 @@ type Container struct { Rootfs string `json:"rootfs"` Fstype string `json:"fstype,omitempty"` Image string `json:"image"` - Addr string `json:"addr,omitempty"` + SCSIAddr string `json:"scsiAddr,omitempty"` Volumes []*VolumeDescriptor `json:"volumes,omitempty"` Fsmap []*FsmapDescriptor `json:"fsmap,omitempty"` Sysctl map[string]string `json:"sysctl,omitempty"` From a1a569a6f785508cf437035c35206acae75650e8 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Thu, 25 Jan 2018 17:11:42 -0800 Subject: [PATCH 4/5] scsi: Add SCSI support for volumes as well Volumes can now be passed using SCSI. Signed-off-by: Archana Shinde --- device.go | 31 +++++++++++++++++++++++-------- hyperstart_agent.go | 1 + pkg/hyperstart/types.go | 1 + 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/device.go b/device.go index 081aa0e4..7010f59b 100644 --- a/device.go +++ b/device.go @@ -340,7 +340,11 @@ type BlockDevice struct { DeviceType string DeviceInfo DeviceInfo - // Path at which the device appears inside the VM, outside of the container mount namespace. + // SCSI Address of the block device, in case the device is attached using SCSI driver + // SCSI address is in the format SCSI-Id:LUN + SCSIAddr string + + // Path at which the device appears inside the VM, outside of the container mount namespace VirtPath string } @@ -359,12 +363,6 @@ func (device *BlockDevice) attach(h hypervisor, c *Container) (err error) { device.DeviceInfo.ID = hex.EncodeToString(randBytes) - drive := Drive{ - File: device.DeviceInfo.HostPath, - Format: "raw", - ID: makeNameID("drive", device.DeviceInfo.ID), - } - // Increment the block index for the pod. 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. @@ -380,6 +378,13 @@ func (device *BlockDevice) attach(h hypervisor, c *Container) (err error) { return err } + drive := Drive{ + File: device.DeviceInfo.HostPath, + Format: "raw", + ID: makeNameID("drive", device.DeviceInfo.ID), + Index: index, + } + driveName, err := getVirtDriveName(index) if err != nil { return err @@ -393,7 +398,17 @@ func (device *BlockDevice) attach(h hypervisor, c *Container) (err error) { device.DeviceInfo.Hotplugged = true - device.VirtPath = filepath.Join("/dev", driveName) + if c.pod.config.HypervisorConfig.BlockDeviceDriver == VirtioBlock { + device.VirtPath = filepath.Join("/dev", driveName) + } else { + scsiAddr, err := getSCSIAddress(index) + if err != nil { + return err + } + + device.SCSIAddr = scsiAddr + } + return nil } diff --git a/hyperstart_agent.go b/hyperstart_agent.go index 0d831ff0..ae3f9084 100644 --- a/hyperstart_agent.go +++ b/hyperstart_agent.go @@ -459,6 +459,7 @@ func (h *hyper) startOneContainer(pod Pod, c *Container) error { Path: d.DeviceInfo.ContainerPath, AbsolutePath: true, DockerVolume: false, + SCSIAddr: d.SCSIAddr, } fsmap = append(fsmap, fsmapDesc) } diff --git a/pkg/hyperstart/types.go b/pkg/hyperstart/types.go index 367a4527..035f3d71 100644 --- a/pkg/hyperstart/types.go +++ b/pkg/hyperstart/types.go @@ -133,6 +133,7 @@ type FsmapDescriptor struct { ReadOnly bool `json:"readOnly"` DockerVolume bool `json:"dockerVolume"` AbsolutePath bool `json:"absolutePath"` + SCSIAddr string `json:"scsiAddr"` } // EnvironmentVar holds an environment variable and its value. From 07c2c883067f3decba4ca6f0e55822beeaa01900 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Thu, 25 Jan 2018 17:13:25 -0800 Subject: [PATCH 5/5] test: Modify block device test to test for SCSI as well Test for SCSI and block drivers while attaching block device. Signed-off-by: Archana Shinde --- device_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/device_test.go b/device_test.go index 4c4312dc..f41593f3 100644 --- a/device_test.go +++ b/device_test.go @@ -322,10 +322,19 @@ func TestAttachBlockDevice(t *testing.T) { fs := &filesystem{} hypervisor := &mockHypervisor{} + hConfig := HypervisorConfig{ + BlockDeviceDriver: VirtioBlock, + } + + config := &PodConfig{ + HypervisorConfig: hConfig, + } + pod := &Pod{ id: testPodID, storage: fs, hypervisor: hypervisor, + config: config, } contID := "100" @@ -376,4 +385,18 @@ func TestAttachBlockDevice(t *testing.T) { err = device.detach(hypervisor) assert.Nil(t, err) + + container.pod.config.HypervisorConfig.BlockDeviceDriver = VirtioSCSI + err = device.attach(hypervisor, &container) + assert.Nil(t, err) + + err = device.detach(hypervisor) + assert.Nil(t, err) + + container.state.State = StateReady + err = device.attach(hypervisor, &container) + assert.Nil(t, err) + + err = device.detach(hypervisor) + assert.Nil(t, err) }