From abb72482288b92da8518d06523a2faffe0d2a551 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Fri, 14 Jun 2024 21:26:36 -0400 Subject: [PATCH 1/3] kola/qemu: add support for simulating 512e disks It's come up a few times now that we need to dig into 512e-specific behaviours (most recently in OCPBUGS-35410). Let's add a knob for this in the qemu platform just like the one we have for 4Kn. --- docs/cosa/run.md | 1 + mantle/cmd/kola/options.go | 1 + mantle/cmd/kola/qemuexec.go | 19 +++++++---- mantle/platform/machine/qemu/cluster.go | 15 ++++++--- mantle/platform/machine/qemu/flight.go | 1 + mantle/platform/qemu.go | 44 +++++++++++++++---------- 6 files changed, 52 insertions(+), 29 deletions(-) diff --git a/docs/cosa/run.md b/docs/cosa/run.md index 729b8bc336..f3518f4054 100644 --- a/docs/cosa/run.md +++ b/docs/cosa/run.md @@ -87,6 +87,7 @@ Additional disks CLI arguments support optional flags using the `--add-disk - `mpath`: enables multipathing for the disk (see below for details). - `4k`: sets the disk as 4Kn (4096 physical sector size) +- `512e`: sets the disk as 512e (4096 physical sector size, 512 logical sector size) - `channel=CHANNEL`: set the channel type (e.g. `virtio`, `nvme`, `scsi`) - `serial=NAME`: sets the disk serial; this can then be used to customize the default `diskN` naming documented above (e.g. `serial=foobar` will make the diff --git a/mantle/cmd/kola/options.go b/mantle/cmd/kola/options.go index 82b61d4af9..154e15c1f6 100644 --- a/mantle/cmd/kola/options.go +++ b/mantle/cmd/kola/options.go @@ -157,6 +157,7 @@ func init() { bv(&kola.QEMUOptions.NbdDisk, "qemu-nbd-socket", false, "Present the disks over NBD socket to qemu") bv(&kola.QEMUOptions.MultiPathDisk, "qemu-multipath", false, "Enable multiple paths for the main disk") bv(&kola.QEMUOptions.Native4k, "qemu-native-4k", false, "Force 4k sectors for main disk") + bv(&kola.QEMUOptions.Disk512e, "qemu-512e", false, "Force 512e layout for main disk") bv(&kola.QEMUOptions.Nvme, "qemu-nvme", false, "Use NVMe for main disk") bv(&kola.QEMUOptions.Swtpm, "qemu-swtpm", true, "Create temporary software TPM") ssv(&kola.QEMUOptions.BindRO, "qemu-bind-ro", nil, "Inject a host directory; this does not automatically mount in the guest") diff --git a/mantle/cmd/kola/qemuexec.go b/mantle/cmd/kola/qemuexec.go index 93082545ef..49d088a98d 100644 --- a/mantle/cmd/kola/qemuexec.go +++ b/mantle/cmd/kola/qemuexec.go @@ -139,8 +139,12 @@ func buildDiskFromOptions() *platform.Disk { channel = "nvme" } sectorSize := 0 + logicalSectorSize := 0 if kola.QEMUOptions.Native4k { sectorSize = 4096 + } else if kola.QEMUOptions.Disk512e { + sectorSize = 4096 + logicalSectorSize = 512 } options := []string{} if kola.QEMUOptions.DriveOpts != "" { @@ -155,13 +159,14 @@ func buildDiskFromOptions() *platform.Disk { // Build the disk definition. Note that if kola.QEMUOptions.DiskImage is // "" we'll just end up with a blank disk image, which is what we want. disk := &platform.Disk{ - BackingFile: kola.QEMUOptions.DiskImage, - Channel: channel, - Size: size, - SectorSize: sectorSize, - DriveOpts: options, - MultiPathDisk: kola.QEMUOptions.MultiPathDisk, - NbdDisk: kola.QEMUOptions.NbdDisk, + BackingFile: kola.QEMUOptions.DiskImage, + Channel: channel, + Size: size, + SectorSize: sectorSize, + LogicalSectorSize: logicalSectorSize, + DriveOpts: options, + MultiPathDisk: kola.QEMUOptions.MultiPathDisk, + NbdDisk: kola.QEMUOptions.NbdDisk, } return disk } diff --git a/mantle/platform/machine/qemu/cluster.go b/mantle/platform/machine/qemu/cluster.go index 4ff119ccc4..ffbec20470 100644 --- a/mantle/platform/machine/qemu/cluster.go +++ b/mantle/platform/machine/qemu/cluster.go @@ -145,8 +145,12 @@ func (qc *Cluster) NewMachineWithQemuOptions(userdata *conf.UserData, options pl channel = "nvme" } sectorSize := 0 + logicalSectorSize := 0 if qc.flight.opts.Native4k { sectorSize = 4096 + } else if qc.flight.opts.Disk512e { + sectorSize = 4096 + logicalSectorSize = 512 } multiPathDisk := options.MultiPathDisk || qc.flight.opts.MultiPathDisk var diskSize string @@ -156,11 +160,12 @@ func (qc *Cluster) NewMachineWithQemuOptions(userdata *conf.UserData, options pl diskSize = qc.flight.opts.DiskSize } primaryDisk := platform.Disk{ - BackingFile: qc.flight.opts.DiskImage, - Channel: channel, - Size: diskSize, - SectorSize: sectorSize, - MultiPathDisk: multiPathDisk, + BackingFile: qc.flight.opts.DiskImage, + Channel: channel, + Size: diskSize, + SectorSize: sectorSize, + LogicalSectorSize: logicalSectorSize, + MultiPathDisk: multiPathDisk, } if options.OverrideBackingFile != "" { diff --git a/mantle/platform/machine/qemu/flight.go b/mantle/platform/machine/qemu/flight.go index 8abc4db8e6..e54f7cbdcc 100644 --- a/mantle/platform/machine/qemu/flight.go +++ b/mantle/platform/machine/qemu/flight.go @@ -41,6 +41,7 @@ type Options struct { NbdDisk bool MultiPathDisk bool Native4k bool + Disk512e bool Nvme bool //Option to create a temporary software TPM - true by default diff --git a/mantle/platform/qemu.go b/mantle/platform/qemu.go index 9eed1fa049..751bde03e6 100644 --- a/mantle/platform/qemu.go +++ b/mantle/platform/qemu.go @@ -89,16 +89,17 @@ type QEMUMachine interface { // Disk holds the details of a virtual disk. type Disk struct { - Size string // disk image size in bytes, optional suffixes "K", "M", "G", "T" allowed. - BackingFile string // raw disk image to use. - BackingFormat string // qcow2, raw, etc. If unspecified will be autodetected. - Channel string // virtio (default), nvme, scsi - DeviceOpts []string // extra options to pass to qemu -device. "serial=XXXX" makes disks show up as /dev/disk/by-id/virtio- - DriveOpts []string // extra options to pass to -drive - SectorSize int // if not 0, override disk sector size - NbdDisk bool // if true, the disks should be presented over nbd:unix socket - MultiPathDisk bool // if true, present multiple paths - Wwn uint64 // Optional World wide name for the SCSI disk. If not set or set to 0, a random one will be generated. Used only with "channel=scsi". Must be an integer + Size string // disk image size in bytes, optional suffixes "K", "M", "G", "T" allowed. + BackingFile string // raw disk image to use. + BackingFormat string // qcow2, raw, etc. If unspecified will be autodetected. + Channel string // virtio (default), nvme, scsi + DeviceOpts []string // extra options to pass to qemu -device. "serial=XXXX" makes disks show up as /dev/disk/by-id/virtio- + DriveOpts []string // extra options to pass to -drive + SectorSize int // if not 0, override disk sector size + LogicalSectorSize int // if not 0, override disk sector size + NbdDisk bool // if true, the disks should be presented over nbd:unix socket + MultiPathDisk bool // if true, present multiple paths + Wwn uint64 // Optional World wide name for the SCSI disk. If not set or set to 0, a random one will be generated. Used only with "channel=scsi". Must be an integer attachEndPoint string // qemuPath to attach to dstFileName string // the prepared file @@ -108,6 +109,7 @@ type Disk struct { func ParseDisk(spec string) (*Disk, error) { var channel string sectorSize := 0 + logicalSectorSize := 0 serialOpt := []string{} multipathed := false var wwn uint64 @@ -123,6 +125,9 @@ func ParseDisk(spec string) (*Disk, error) { channel = value case "4k": sectorSize = 4096 + case "512e": + sectorSize = 4096 + logicalSectorSize = 512 case "mpath": multipathed = true case "serial": @@ -139,12 +144,13 @@ func ParseDisk(spec string) (*Disk, error) { } return &Disk{ - Size: fmt.Sprintf("%dG", size), - Channel: channel, - DeviceOpts: serialOpt, - SectorSize: sectorSize, - MultiPathDisk: multipathed, - Wwn: wwn, + Size: fmt.Sprintf("%dG", size), + Channel: channel, + DeviceOpts: serialOpt, + SectorSize: sectorSize, + LogicalSectorSize: logicalSectorSize, + MultiPathDisk: multipathed, + Wwn: wwn, }, nil } @@ -1171,7 +1177,11 @@ func (builder *QemuBuilder) addDiskImpl(disk *Disk, primary bool) error { channel = "virtio" } if disk.SectorSize != 0 { - diskOpts = append(diskOpts, fmt.Sprintf("physical_block_size=%[1]d,logical_block_size=%[1]d", disk.SectorSize)) + logical := disk.LogicalSectorSize + if logical == 0 { + logical = disk.SectorSize + } + diskOpts = append(diskOpts, fmt.Sprintf("physical_block_size=%d,logical_block_size=%d", disk.SectorSize, logical)) } // Primary disk gets bootindex 1, all other disks have unspecified // bootindex, which means lower priority. From a8b236fcc6e6da74b44099e5f00b0da59c1565de Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Fri, 14 Jun 2024 21:26:37 -0400 Subject: [PATCH 2/3] kola: add support for `primaryDisk` key Just like we have `additionalDisks`, add a new `primaryDisk` key which takes the same "diskspec" format. This immediately unlocks all the same knobs available to additional disks to the primary disk itself from external tests. E.g. with this, we can now migrate multipath tests to external tests that use `primaryDisk: 20G:mpath`. My immediate motivation for this however is to be able to use the new `512e` disk option from an external test as part of fixing OCPBUGS-35410. --- docs/kola/external-tests.md | 4 ++- mantle/kola/harness.go | 3 ++ mantle/kola/register/register.go | 9 ++++-- mantle/platform/machine/qemu/cluster.go | 41 ++++++++++++------------- mantle/platform/platform.go | 1 + 5 files changed, 34 insertions(+), 24 deletions(-) diff --git a/docs/kola/external-tests.md b/docs/kola/external-tests.md index 9c60f9e3cc..ac61dba8aa 100644 --- a/docs/kola/external-tests.md +++ b/docs/kola/external-tests.md @@ -191,6 +191,7 @@ Here's an example `kola.json`: "platforms": "qemu", "tags": "sometagname needs-internet skip-base-checks othertag", "requiredTag": "special", + "primaryDisk": ["20G:mpath"], "additionalDisks": [ "5G" ], "minMemory": 4096, "minDisk": 15, @@ -229,7 +230,8 @@ If a test has a `requiredTag`, it is run only if the required tag is specified. In the example above, the test would only run if `--tag special` was provided. The `additionalDisks` key has the same semantics as the `--add-disk` argument -to `qemuexec`. It is currently only supported on `qemu`. +to `qemuexec`. It is currently only supported on `qemu`. The `primaryDisk` key +also supports the same syntax and controls the primary boot disk. The `injectContainer` boolean if set will cause the framework to inject the ostree base image container into the target system; the path can be diff --git a/mantle/kola/harness.go b/mantle/kola/harness.go index 5d4671a60c..6ecd84b81d 100644 --- a/mantle/kola/harness.go +++ b/mantle/kola/harness.go @@ -1020,6 +1020,7 @@ type externalTestMeta struct { Tags string `json:"tags,omitempty" yaml:"tags,omitempty"` RequiredTag string `json:"requiredTag,omitempty" yaml:"requiredTag,omitempty"` AdditionalDisks []string `json:"additionalDisks,omitempty" yaml:"additionalDisks,omitempty"` + PrimaryDisk string `json:"primaryDisk,omitempty" yaml:"primaryDisk,omitempty"` InjectContainer bool `json:"injectContainer,omitempty" yaml:"injectContainer,omitempty"` MinMemory int `json:"minMemory,omitempty" yaml:"minMemory,omitempty"` MinDiskSize int `json:"minDisk,omitempty" yaml:"minDisk,omitempty"` @@ -1233,6 +1234,7 @@ ExecStart=%s Tags: []string{"external"}, AdditionalDisks: targetMeta.AdditionalDisks, + PrimaryDisk: targetMeta.PrimaryDisk, InjectContainer: targetMeta.InjectContainer, MinMemory: targetMeta.MinMemory, MinDiskSize: targetMeta.MinDiskSize, @@ -1752,6 +1754,7 @@ func runTest(h *harness.H, t *register.Test, pltfrm string, flight platform.Flig options := platform.MachineOptions{ MultiPathDisk: t.MultiPathDisk, + PrimaryDisk: t.PrimaryDisk, AdditionalDisks: t.AdditionalDisks, MinMemory: t.MinMemory, MinDiskSize: t.MinDiskSize, diff --git a/mantle/kola/register/register.go b/mantle/kola/register/register.go index 65bedb205c..a2ede2e674 100644 --- a/mantle/kola/register/register.go +++ b/mantle/kola/register/register.go @@ -70,7 +70,7 @@ type Test struct { RequiredTag string // if specified, test is filtered by default unless tag is provided -- defaults to none Description string // test description - // Whether the primary disk is multipathed. + // Whether the primary disk is multipathed. Deprecated in favour of PrimaryDisk. MultiPathDisk bool // Sizes of additional empty disks to attach to the node, followed by @@ -78,13 +78,18 @@ type Test struct { // "5G:mpath,foo,bar"]) -- defaults to none. AdditionalDisks []string + // Size of primary disk to attach to the node, followed by + // comma-separated list of optional options (e.g. "20G:mpath"]). + PrimaryDisk string + // InjectContainer will cause the ostree base image to be injected into the target InjectContainer bool // Minimum amount of memory in MB required for test. MinMemory int - // Minimum amount of primary disk in GB required for test. + // Minimum amount of primary disk in GB required for test. Deprecated in favour + // of PrimaryDisk. MinDiskSize int // Additional amount of NICs required for test. diff --git a/mantle/platform/machine/qemu/cluster.go b/mantle/platform/machine/qemu/cluster.go index ffbec20470..114daaf054 100644 --- a/mantle/platform/machine/qemu/cluster.go +++ b/mantle/platform/machine/qemu/cluster.go @@ -140,34 +140,33 @@ func (qc *Cluster) NewMachineWithQemuOptions(userdata *conf.UserData, options pl builder.MemoryMiB = 4096 // SE needs at least 4GB } - channel := "virtio" + var primaryDisk platform.Disk + if options.PrimaryDisk != "" { + var diskp *platform.Disk + if diskp, err = platform.ParseDisk(options.PrimaryDisk); err != nil { + return nil, errors.Wrapf(err, "parsing primary disk spec '%s'", options.PrimaryDisk) + } + primaryDisk = *diskp + } + if qc.flight.opts.Nvme || options.Nvme { - channel = "nvme" + primaryDisk.Channel = "nvme" } - sectorSize := 0 - logicalSectorSize := 0 if qc.flight.opts.Native4k { - sectorSize = 4096 + primaryDisk.SectorSize = 4096 } else if qc.flight.opts.Disk512e { - sectorSize = 4096 - logicalSectorSize = 512 + primaryDisk.SectorSize = 4096 + primaryDisk.LogicalSectorSize = 512 } - multiPathDisk := options.MultiPathDisk || qc.flight.opts.MultiPathDisk - var diskSize string - if options.MinDiskSize > 0 { - diskSize = fmt.Sprintf("%dG", options.MinDiskSize) - } else { - diskSize = qc.flight.opts.DiskSize + if options.MultiPathDisk || qc.flight.opts.MultiPathDisk { + primaryDisk.MultiPathDisk = true } - primaryDisk := platform.Disk{ - BackingFile: qc.flight.opts.DiskImage, - Channel: channel, - Size: diskSize, - SectorSize: sectorSize, - LogicalSectorSize: logicalSectorSize, - MultiPathDisk: multiPathDisk, + if options.MinDiskSize > 0 { + primaryDisk.Size = fmt.Sprintf("%dG", options.MinDiskSize) + } else if qc.flight.opts.DiskSize != "" { + primaryDisk.Size = qc.flight.opts.DiskSize } - + primaryDisk.BackingFile = qc.flight.opts.DiskImage if options.OverrideBackingFile != "" { primaryDisk.BackingFile = options.OverrideBackingFile } diff --git a/mantle/platform/platform.go b/mantle/platform/platform.go index ddcaf8ebb8..4ceb10a667 100644 --- a/mantle/platform/platform.go +++ b/mantle/platform/platform.go @@ -155,6 +155,7 @@ type Flight interface { type MachineOptions struct { MultiPathDisk bool + PrimaryDisk string AdditionalDisks []string MinMemory int MinDiskSize int From d90f3f0c5ddc7fceef93c1bb46a5a00f6e9b8786 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Fri, 14 Jun 2024 21:26:38 -0400 Subject: [PATCH 3/3] kola: allow missing size in diskspec for `primaryDisk` When using the new `primaryDisk` key, allow the diskspec to omit the size component, in which case we don't resize the boot disk. --- docs/kola/external-tests.md | 4 +++- mantle/platform/api/gcloud/compute.go | 2 +- mantle/platform/machine/qemu/cluster.go | 2 +- mantle/platform/qemu.go | 12 ++++++++---- mantle/util/common.go | 20 ++++++++++++++------ 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/docs/kola/external-tests.md b/docs/kola/external-tests.md index ac61dba8aa..8d2b82d250 100644 --- a/docs/kola/external-tests.md +++ b/docs/kola/external-tests.md @@ -231,7 +231,9 @@ In the example above, the test would only run if `--tag special` was provided. The `additionalDisks` key has the same semantics as the `--add-disk` argument to `qemuexec`. It is currently only supported on `qemu`. The `primaryDisk` key -also supports the same syntax and controls the primary boot disk. +also supports the same syntax and controls the primary boot disk. Only for the +`primaryDisk` key, the size can be omitted (e.g. `:mpath`), in which case the +qcow2 will not be resized. The `injectContainer` boolean if set will cause the framework to inject the ostree base image container into the target system; the path can be diff --git a/mantle/platform/api/gcloud/compute.go b/mantle/platform/api/gcloud/compute.go index 68a3b5b705..9e8e16434f 100644 --- a/mantle/platform/api/gcloud/compute.go +++ b/mantle/platform/api/gcloud/compute.go @@ -38,7 +38,7 @@ func (a *API) vmname() string { func ParseDisk(spec string, zone string) (*compute.AttachedDisk, error) { var diskInterface string - size, diskmap, err := util.ParseDiskSpec(spec) + size, diskmap, err := util.ParseDiskSpec(spec, false) if err != nil { return nil, fmt.Errorf("failed to parse disk spec %q: %w", spec, err) } diff --git a/mantle/platform/machine/qemu/cluster.go b/mantle/platform/machine/qemu/cluster.go index 114daaf054..a16275a968 100644 --- a/mantle/platform/machine/qemu/cluster.go +++ b/mantle/platform/machine/qemu/cluster.go @@ -143,7 +143,7 @@ func (qc *Cluster) NewMachineWithQemuOptions(userdata *conf.UserData, options pl var primaryDisk platform.Disk if options.PrimaryDisk != "" { var diskp *platform.Disk - if diskp, err = platform.ParseDisk(options.PrimaryDisk); err != nil { + if diskp, err = platform.ParseDisk(options.PrimaryDisk, true); err != nil { return nil, errors.Wrapf(err, "parsing primary disk spec '%s'", options.PrimaryDisk) } primaryDisk = *diskp diff --git a/mantle/platform/qemu.go b/mantle/platform/qemu.go index 751bde03e6..6fe76da9fc 100644 --- a/mantle/platform/qemu.go +++ b/mantle/platform/qemu.go @@ -106,7 +106,7 @@ type Disk struct { nbdServCmd exec.Cmd // command to serve the disk } -func ParseDisk(spec string) (*Disk, error) { +func ParseDisk(spec string, allowNoSize bool) (*Disk, error) { var channel string sectorSize := 0 logicalSectorSize := 0 @@ -114,7 +114,7 @@ func ParseDisk(spec string) (*Disk, error) { multipathed := false var wwn uint64 - size, diskmap, err := util.ParseDiskSpec(spec) + size, diskmap, err := util.ParseDiskSpec(spec, allowNoSize) if err != nil { return nil, fmt.Errorf("failed to parse disk spec %q: %w", spec, err) } @@ -143,8 +143,12 @@ func ParseDisk(spec string) (*Disk, error) { } } + sizeStr := "" + if size > 0 { + sizeStr = fmt.Sprintf("%dG", size) + } return &Disk{ - Size: fmt.Sprintf("%dG", size), + Size: sizeStr, Channel: channel, DeviceOpts: serialOpt, SectorSize: sectorSize, @@ -1302,7 +1306,7 @@ func (builder *QemuBuilder) AddDisk(disk *Disk) error { // AddDisksFromSpecs adds multiple secondary disks from their specs. func (builder *QemuBuilder) AddDisksFromSpecs(specs []string) error { for _, spec := range specs { - if disk, err := ParseDisk(spec); err != nil { + if disk, err := ParseDisk(spec, false); err != nil { return errors.Wrapf(err, "parsing additional disk spec '%s'", spec) } else if err = builder.AddDisk(disk); err != nil { return errors.Wrapf(err, "adding additional disk '%s'", spec) diff --git a/mantle/util/common.go b/mantle/util/common.go index 94acfac6b9..f5d7ea1477 100644 --- a/mantle/util/common.go +++ b/mantle/util/common.go @@ -125,10 +125,14 @@ func RunCmdTimeout(timeout time.Duration, cmd string, args ...string) error { // ParseDiskSpec converts a disk specification into a Disk. The format is: // [:,,...], like ["5G:channel=nvme"] -func ParseDiskSpec(spec string) (int64, map[string]string, error) { +func ParseDiskSpec(spec string, allowNoSize bool) (int64, map[string]string, error) { diskmap := map[string]string{} split := strings.Split(spec, ":") - if split[0] == "" || (!strings.HasSuffix(split[0], "G")) { + if split[0] == "" { + if !allowNoSize { + return 0, nil, fmt.Errorf("no size provided in '%s'", spec) + } + } else if !strings.HasSuffix(split[0], "G") { return 0, nil, fmt.Errorf("invalid size opt %s", spec) } var disksize string @@ -149,10 +153,14 @@ func ParseDiskSpec(spec string) (int64, map[string]string, error) { } else { return 0, nil, fmt.Errorf("invalid disk spec %s", spec) } - disksize = strings.TrimSuffix(disksize, "G") - size, err := strconv.ParseInt(disksize, 10, 32) - if err != nil { - return 0, nil, fmt.Errorf("failed to convert %q to int64: %w", disksize, err) + var size int64 = 0 + if disksize != "" { + disksize = strings.TrimSuffix(disksize, "G") + var err error + size, err = strconv.ParseInt(disksize, 10, 32) + if err != nil { + return 0, nil, fmt.Errorf("failed to convert %q to int64: %w", disksize, err) + } } return size, diskmap, nil }