From efa19c41ebede954ae9c2ead97dd970691632f91 Mon Sep 17 00:00:00 2001 From: zhanghj Date: Mon, 14 Mar 2022 09:17:04 +0800 Subject: [PATCH] device: use const strings for block-driver option instead of hard coding Currently, the block driver option is specifed by hard coding, maybe it is better to use const string variables instead of hard coded strings. Another modification is to remove duplicate consts for virtio driver in manager.go. Fixes: #3321 Signed-off-by: Jason Zhang --- src/runtime/virtcontainers/container_test.go | 4 +-- .../virtcontainers/device/config/config.go | 10 ++++-- .../virtcontainers/device/config/pmem.go | 4 +-- .../virtcontainers/device/drivers/block.go | 16 ++++----- .../device/drivers/vhost_user_blk.go | 10 +++--- .../virtcontainers/device/manager/manager.go | 35 ++++++------------- .../device/manager/manager_linux_test.go | 2 +- .../device/manager/manager_test.go | 12 +++---- .../documentation/api/1.0/api.md | 4 +-- src/runtime/virtcontainers/kata_agent_test.go | 4 +-- .../virtcontainers/persist/api/device.go | 2 +- src/runtime/virtcontainers/persist_test.go | 6 ++-- src/runtime/virtcontainers/sandbox_test.go | 4 +-- 13 files changed, 53 insertions(+), 60 deletions(-) diff --git a/src/runtime/virtcontainers/container_test.go b/src/runtime/virtcontainers/container_test.go index b41fcc108..eddf8ed70 100644 --- a/src/runtime/virtcontainers/container_test.go +++ b/src/runtime/virtcontainers/container_test.go @@ -86,7 +86,7 @@ func TestContainerRemoveDrive(t *testing.T) { sandbox := &Sandbox{ ctx: context.Background(), id: "sandbox", - devManager: manager.NewDeviceManager(manager.VirtioSCSI, false, "", nil), + devManager: manager.NewDeviceManager(config.VirtioSCSI, false, "", nil), config: &SandboxConfig{}, } @@ -320,7 +320,7 @@ func TestContainerAddDriveDir(t *testing.T) { sandbox := &Sandbox{ ctx: context.Background(), id: testSandboxID, - devManager: manager.NewDeviceManager(manager.VirtioSCSI, false, "", nil), + devManager: manager.NewDeviceManager(config.VirtioSCSI, false, "", nil), hypervisor: &mockHypervisor{}, agent: &mockAgent{}, config: &SandboxConfig{ diff --git a/src/runtime/virtcontainers/device/config/config.go b/src/runtime/virtcontainers/device/config/config.go index 69be4f583..48280092d 100644 --- a/src/runtime/virtcontainers/device/config/config.go +++ b/src/runtime/virtcontainers/device/config/config.go @@ -51,7 +51,7 @@ const ( // VirtioBlock means use virtio-blk for hotplugging drives VirtioBlock = "virtio-blk" - // VirtioBlockCCW means use virtio-blk for hotplugging drives + // VirtioBlockCCW means use virtio-blk-ccw for hotplugging drives VirtioBlockCCW = "virtio-blk-ccw" // VirtioSCSI means use virtio-scsi for hotplugging drives @@ -72,6 +72,12 @@ const ( VirtioFSNydus = "virtio-fs-nydus" ) +const ( + // Define the string key for DriverOptions in DeviceInfo struct + FsTypeOpt = "fstype" + BlockDriverOpt = "block-driver" +) + const ( // The OCI spec requires the major-minor number to be provided for a // device. We have chosen the below major numbers to represent @@ -97,7 +103,7 @@ var getSysDevPath = getSysDevPathImpl // DeviceInfo is an embedded type that contains device data common to all types of devices. type DeviceInfo struct { // DriverOptions is specific options for each device driver - // for example, for BlockDevice, we can set DriverOptions["blockDriver"]="virtio-blk" + // for example, for BlockDevice, we can set DriverOptions["block-driver"]="virtio-blk" DriverOptions map[string]string // Hostpath is device path on host diff --git a/src/runtime/virtcontainers/device/config/pmem.go b/src/runtime/virtcontainers/device/config/pmem.go index 81d1da9b5..44fd32187 100644 --- a/src/runtime/virtcontainers/device/config/pmem.go +++ b/src/runtime/virtcontainers/device/config/pmem.go @@ -81,8 +81,8 @@ func PmemDeviceInfo(source, destination string) (*DeviceInfo, error) { fstype = "ext4" } - pmemLog.WithField("fstype", fstype).Debug("filesystem for mount point") - device.DriverOptions["fstype"] = fstype + pmemLog.WithField(FsTypeOpt, fstype).Debug("filesystem for mount point") + device.DriverOptions[FsTypeOpt] = fstype return device, nil } diff --git a/src/runtime/virtcontainers/device/drivers/block.go b/src/runtime/virtcontainers/device/drivers/block.go index ce7eaceda..faaadf32c 100644 --- a/src/runtime/virtcontainers/device/drivers/block.go +++ b/src/runtime/virtcontainers/device/drivers/block.go @@ -70,13 +70,13 @@ func (device *BlockDevice) Attach(ctx context.Context, devReceiver api.DeviceRec ReadOnly: device.DeviceInfo.ReadOnly, } - if fs, ok := device.DeviceInfo.DriverOptions["fstype"]; ok { + if fs, ok := device.DeviceInfo.DriverOptions[config.FsTypeOpt]; ok { drive.Format = fs } customOptions := device.DeviceInfo.DriverOptions if customOptions == nil || - customOptions["block-driver"] == "virtio-scsi" { + customOptions[config.BlockDriverOpt] == config.VirtioSCSI { // User has not chosen a specific block device type // Default to SCSI scsiAddr, err := utils.GetSCSIAddress(index) @@ -85,15 +85,15 @@ func (device *BlockDevice) Attach(ctx context.Context, devReceiver api.DeviceRec } drive.SCSIAddr = scsiAddr - } else if customOptions["block-driver"] != "nvdimm" { + } else if customOptions[config.BlockDriverOpt] != config.Nvdimm { var globalIdx int - switch customOptions["block-driver"] { - case "virtio-blk": + switch customOptions[config.BlockDriverOpt] { + case config.VirtioBlock: globalIdx = index - case "virtio-blk-ccw": + case config.VirtioBlockCCW: globalIdx = index - case "virtio-mmio": + case config.VirtioMmio: //With firecracker the rootfs for the VM itself //sits at /dev/vda and consumes the first index. //Longer term block based VM rootfs should be added @@ -111,7 +111,7 @@ func (device *BlockDevice) Attach(ctx context.Context, devReceiver api.DeviceRec drive.VirtPath = filepath.Join("/dev", driveName) } - deviceLogger().WithField("device", device.DeviceInfo.HostPath).WithField("VirtPath", drive.VirtPath).Infof("Attaching %s device", customOptions["block-driver"]) + deviceLogger().WithField("device", device.DeviceInfo.HostPath).WithField("VirtPath", drive.VirtPath).Infof("Attaching %s device", customOptions[config.BlockDriverOpt]) device.BlockDrive = drive if err = devReceiver.HotplugAddDevice(ctx, device, config.DeviceBlock); err != nil { return err diff --git a/src/runtime/virtcontainers/device/drivers/vhost_user_blk.go b/src/runtime/virtcontainers/device/drivers/vhost_user_blk.go index 39dd2bd23..4a495c9d5 100644 --- a/src/runtime/virtcontainers/device/drivers/vhost_user_blk.go +++ b/src/runtime/virtcontainers/device/drivers/vhost_user_blk.go @@ -100,14 +100,14 @@ func isVirtioBlkBlockDriver(customOptions map[string]string) bool { if customOptions == nil { // User has not chosen a specific block device type // Default to SCSI - blockDriverOption = "virtio-scsi" + blockDriverOption = config.VirtioSCSI } else { - blockDriverOption = customOptions["block-driver"] + blockDriverOption = customOptions[config.BlockDriverOpt] } - if blockDriverOption == "virtio-blk" || - blockDriverOption == "virtio-blk-ccw" || - blockDriverOption == "virtio-mmio" { + if blockDriverOption == config.VirtioBlock || + blockDriverOption == config.VirtioBlockCCW || + blockDriverOption == config.VirtioMmio { return true } diff --git a/src/runtime/virtcontainers/device/manager/manager.go b/src/runtime/virtcontainers/device/manager/manager.go index 195187ba8..9a5bba260 100644 --- a/src/runtime/virtcontainers/device/manager/manager.go +++ b/src/runtime/virtcontainers/device/manager/manager.go @@ -21,19 +21,6 @@ import ( "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" ) -const ( - // VirtioMmio indicates block driver is virtio-mmio based - VirtioMmio string = "virtio-mmio" - // VirtioBlock indicates block driver is virtio-blk based - VirtioBlock string = "virtio-blk" - // VirtioBlockCCW indicates block driver is virtio-blk-ccw based - VirtioBlockCCW string = "virtio-blk-ccw" - // VirtioSCSI indicates block driver is virtio-scsi based - VirtioSCSI string = "virtio-scsi" - // Nvdimm indicates block driver is nvdimm based - Nvdimm string = "nvdimm" -) - var ( // ErrIDExhausted represents that devices are too many // and no more IDs can be generated @@ -69,16 +56,16 @@ func NewDeviceManager(blockDriver string, vhostUserStoreEnabled bool, vhostUserS vhostUserStorePath: vhostUserStorePath, devices: make(map[string]api.Device), } - if blockDriver == VirtioMmio { - dm.blockDriver = VirtioMmio - } else if blockDriver == VirtioBlock { - dm.blockDriver = VirtioBlock - } else if blockDriver == Nvdimm { - dm.blockDriver = Nvdimm - } else if blockDriver == VirtioBlockCCW { - dm.blockDriver = VirtioBlockCCW + if blockDriver == config.VirtioMmio { + dm.blockDriver = config.VirtioMmio + } else if blockDriver == config.VirtioBlock { + dm.blockDriver = config.VirtioBlock + } else if blockDriver == config.Nvdimm { + dm.blockDriver = config.Nvdimm + } else if blockDriver == config.VirtioBlockCCW { + dm.blockDriver = config.VirtioBlockCCW } else { - dm.blockDriver = VirtioSCSI + dm.blockDriver = config.VirtioSCSI } drivers.AllPCIeDevs = make(map[string]bool) @@ -132,13 +119,13 @@ func (dm *deviceManager) createDevice(devInfo config.DeviceInfo) (dev api.Device if devInfo.DriverOptions == nil { devInfo.DriverOptions = make(map[string]string) } - devInfo.DriverOptions["block-driver"] = dm.blockDriver + devInfo.DriverOptions[config.BlockDriverOpt] = dm.blockDriver return drivers.NewVhostUserBlkDevice(&devInfo), nil } else if isBlock(devInfo) { if devInfo.DriverOptions == nil { devInfo.DriverOptions = make(map[string]string) } - devInfo.DriverOptions["block-driver"] = dm.blockDriver + devInfo.DriverOptions[config.BlockDriverOpt] = dm.blockDriver return drivers.NewBlockDevice(&devInfo), nil } else { deviceLogger().WithField("device", devInfo.HostPath).Info("Device has not been passed to the container") diff --git a/src/runtime/virtcontainers/device/manager/manager_linux_test.go b/src/runtime/virtcontainers/device/manager/manager_linux_test.go index 78773fc5c..b47a38c0a 100644 --- a/src/runtime/virtcontainers/device/manager/manager_linux_test.go +++ b/src/runtime/virtcontainers/device/manager/manager_linux_test.go @@ -31,7 +31,7 @@ func TestAttachVhostUserBlkDevice(t *testing.T) { tmpDir, err := os.MkdirTemp("", "") dm := &deviceManager{ - blockDriver: VirtioBlock, + blockDriver: config.VirtioBlock, devices: make(map[string]api.Device), vhostUserStoreEnabled: true, vhostUserStorePath: tmpDir, diff --git a/src/runtime/virtcontainers/device/manager/manager_test.go b/src/runtime/virtcontainers/device/manager/manager_test.go index f0d7ef974..8e1b8ec4b 100644 --- a/src/runtime/virtcontainers/device/manager/manager_test.go +++ b/src/runtime/virtcontainers/device/manager/manager_test.go @@ -26,7 +26,7 @@ const dirMode = os.FileMode(0750) | os.ModeDir func TestNewDevice(t *testing.T) { dm := &deviceManager{ - blockDriver: VirtioBlock, + blockDriver: config.VirtioBlock, devices: make(map[string]api.Device), } savedSysDevPrefix := config.SysDevPrefix @@ -96,7 +96,7 @@ func TestNewDevice(t *testing.T) { func TestAttachVFIODevice(t *testing.T) { dm := &deviceManager{ - blockDriver: VirtioBlock, + blockDriver: config.VirtioBlock, devices: make(map[string]api.Device), } tmpDir, err := os.MkdirTemp("", "") @@ -155,7 +155,7 @@ func TestAttachVFIODevice(t *testing.T) { func TestAttachGenericDevice(t *testing.T) { dm := &deviceManager{ - blockDriver: VirtioBlock, + blockDriver: config.VirtioBlock, devices: make(map[string]api.Device), } path := "/dev/tty2" @@ -180,7 +180,7 @@ func TestAttachGenericDevice(t *testing.T) { func TestAttachBlockDevice(t *testing.T) { dm := &deviceManager{ - blockDriver: VirtioBlock, + blockDriver: config.VirtioBlock, devices: make(map[string]api.Device), } path := "/dev/hda" @@ -203,7 +203,7 @@ func TestAttachBlockDevice(t *testing.T) { assert.Nil(t, err) // test virtio SCSI driver - dm.blockDriver = VirtioSCSI + dm.blockDriver = config.VirtioSCSI device, err = dm.NewDevice(deviceInfo) assert.Nil(t, err) err = device.Attach(context.Background(), devReceiver) @@ -214,7 +214,7 @@ func TestAttachBlockDevice(t *testing.T) { } func TestAttachDetachDevice(t *testing.T) { - dm := NewDeviceManager(VirtioSCSI, false, "", nil) + dm := NewDeviceManager(config.VirtioSCSI, false, "", nil) path := "/dev/hda" deviceInfo := config.DeviceInfo{ diff --git a/src/runtime/virtcontainers/documentation/api/1.0/api.md b/src/runtime/virtcontainers/documentation/api/1.0/api.md index a2e1a55ff..6455e19d0 100644 --- a/src/runtime/virtcontainers/documentation/api/1.0/api.md +++ b/src/runtime/virtcontainers/documentation/api/1.0/api.md @@ -547,7 +547,7 @@ type DeviceInfo struct { ID string // DriverOptions is specific options for each device driver - // for example, for BlockDevice, we can set DriverOptions["blockDriver"]="virtio-blk" + // for example, for BlockDevice, we can set DriverOptions["block-driver"]="virtio-blk" DriverOptions map[string]string } ``` @@ -835,7 +835,7 @@ type DeviceInfo struct { ID string // DriverOptions is specific options for each device driver - // for example, for BlockDevice, we can set DriverOptions["blockDriver"]="virtio-blk" + // for example, for BlockDevice, we can set DriverOptions["block-driver"]="virtio-blk" DriverOptions map[string]string } ``` diff --git a/src/runtime/virtcontainers/kata_agent_test.go b/src/runtime/virtcontainers/kata_agent_test.go index f494626c6..bcca754ba 100644 --- a/src/runtime/virtcontainers/kata_agent_test.go +++ b/src/runtime/virtcontainers/kata_agent_test.go @@ -390,10 +390,10 @@ func TestHandleBlockVolume(t *testing.T) { mounts = append(mounts, vMount, bMount, dMount) tmpDir := "/vhost/user/dir" - dm := manager.NewDeviceManager(manager.VirtioBlock, true, tmpDir, devices) + dm := manager.NewDeviceManager(config.VirtioBlock, true, tmpDir, devices) sConfig := SandboxConfig{} - sConfig.HypervisorConfig.BlockDeviceDriver = manager.VirtioBlock + sConfig.HypervisorConfig.BlockDeviceDriver = config.VirtioBlock sandbox := Sandbox{ id: "100", containers: containers, diff --git a/src/runtime/virtcontainers/persist/api/device.go b/src/runtime/virtcontainers/persist/api/device.go index acb05fc67..dd61efbfc 100644 --- a/src/runtime/virtcontainers/persist/api/device.go +++ b/src/runtime/virtcontainers/persist/api/device.go @@ -86,7 +86,7 @@ type VhostUserDeviceAttrs struct { // Refs: virtcontainers/device/drivers/generic.go:GenericDevice type DeviceState struct { // DriverOptions is specific options for each device driver - // for example, for BlockDevice, we can set DriverOptions["blockDriver"]="virtio-blk" + // for example, for BlockDevice, we can set DriverOptions["block-driver"]="virtio-blk" DriverOptions map[string]string // VhostUserDeviceAttrs is specific for vhost-user device driver diff --git a/src/runtime/virtcontainers/persist_test.go b/src/runtime/virtcontainers/persist_test.go index 228638a96..b6c096a5d 100644 --- a/src/runtime/virtcontainers/persist_test.go +++ b/src/runtime/virtcontainers/persist_test.go @@ -10,11 +10,11 @@ import ( "os" "testing" - "github.com/stretchr/testify/assert" - + "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/manager" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types" + "github.com/stretchr/testify/assert" ) func TestSandboxRestore(t *testing.T) { @@ -32,7 +32,7 @@ func TestSandboxRestore(t *testing.T) { sandbox := Sandbox{ id: "test-exp", containers: container, - devManager: manager.NewDeviceManager(manager.VirtioSCSI, false, "", nil), + devManager: manager.NewDeviceManager(config.VirtioSCSI, false, "", nil), hypervisor: &mockHypervisor{}, network: network, ctx: context.Background(), diff --git a/src/runtime/virtcontainers/sandbox_test.go b/src/runtime/virtcontainers/sandbox_test.go index 344551e3b..b1556ec15 100644 --- a/src/runtime/virtcontainers/sandbox_test.go +++ b/src/runtime/virtcontainers/sandbox_test.go @@ -548,7 +548,7 @@ func TestSandboxAttachDevicesVFIO(t *testing.T) { config.SysIOMMUPath = savedIOMMUPath }() - dm := manager.NewDeviceManager(manager.VirtioSCSI, false, "", nil) + dm := manager.NewDeviceManager(config.VirtioSCSI, false, "", nil) path := filepath.Join(vfioPath, testFDIOGroup) deviceInfo := config.DeviceInfo{ HostPath: path, @@ -599,7 +599,7 @@ func TestSandboxAttachDevicesVhostUserBlk(t *testing.T) { tmpDir, err := os.MkdirTemp("", "") assert.Nil(t, err) os.RemoveAll(tmpDir) - dm := manager.NewDeviceManager(manager.VirtioSCSI, true, tmpDir, nil) + dm := manager.NewDeviceManager(config.VirtioSCSI, true, tmpDir, nil) vhostUserDevNodePath := filepath.Join(tmpDir, "/block/devices/") vhostUserSockPath := filepath.Join(tmpDir, "/block/sockets/")