From 1a0092d6316a385ff8ccd9b2c581dd5733c45ed3 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Tue, 22 Aug 2023 07:42:59 +0000 Subject: [PATCH] runtime/qemu: fix image/initrd annotation handling Right now if we configure an image annotation and have a config file setting initrd, the initrd config would override the image annotation. Add a helper function ImageOrInitrdAssetPath to make sure annotations are preferred over config options in image and initrd path handling. Signed-off-by: Peng Tao --- src/runtime/virtcontainers/hypervisor.go | 41 ++++++++++++ src/runtime/virtcontainers/qemu.go | 77 ++++++++++------------- src/runtime/virtcontainers/types/asset.go | 2 + 3 files changed, 76 insertions(+), 44 deletions(-) diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 14d8b19373..7599d2794f 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -84,6 +84,7 @@ const ( var ( hvLogger = logrus.WithField("source", "virtcontainers/hypervisor") noGuestMemHotplugErr error = errors.New("guest memory hotplug not supported") + conflictingAssets error = errors.New("cannot set both image and initrd at the same time") ) // In some architectures the maximum number of vCPUs depends on the number of physical cores. @@ -698,6 +699,46 @@ func (conf *HypervisorConfig) AddCustomAsset(a *types.Asset) error { return nil } +// ImageOrInitrdAssetPath returns an image or an initrd path, along with the corresponding asset type +// Annotation path is preferred to config path. +func (conf *HypervisorConfig) ImageOrInitrdAssetPath() (string, types.AssetType, error) { + var image, initrd string + + checkAndReturn := func(image string, initrd string) (string, types.AssetType, error) { + if image != "" && initrd != "" { + return "", types.UnkownAsset, conflictingAssets + } + + if image != "" { + return image, types.ImageAsset, nil + } + + if initrd != "" { + return initrd, types.InitrdAsset, nil + } + + return "", types.UnkownAsset, fmt.Errorf("one of image and initrd must be set") + } + + if a, ok := conf.customAssets[types.ImageAsset]; ok { + image = a.Path() + } + + if a, ok := conf.customAssets[types.InitrdAsset]; ok { + initrd = a.Path() + } + + path, assetType, err := checkAndReturn(image, initrd) + if assetType != types.UnkownAsset { + return path, assetType, nil + } + if err == conflictingAssets { + return "", types.UnkownAsset, errors.Wrapf(err, "conflicting annotations") + } + + return checkAndReturn(conf.ImagePath, conf.InitrdPath) +} + func (conf *HypervisorConfig) assetPath(t types.AssetType) (string, error) { // Custom assets take precedence over the configured ones a, ok := conf.customAssets[t] diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 57fa036f61..b03b176b4a 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -347,22 +347,6 @@ func (q *qemu) getQemuMachine() (govmmQemu.Machine, error) { return machine, nil } -func (q *qemu) appendImage(ctx context.Context, devices []govmmQemu.Device) ([]govmmQemu.Device, error) { - imagePath, err := q.config.ImageAssetPath() - if err != nil { - return nil, err - } - - if imagePath != "" { - devices, err = q.arch.appendImage(ctx, devices, imagePath) - if err != nil { - return nil, err - } - } - - return devices, nil -} - func (q *qemu) createQmpSocket() ([]govmmQemu.QMPSocket, error) { monitorSockPath, err := q.qmpSocketPath(q.id) if err != nil { @@ -400,12 +384,16 @@ func (q *qemu) createQmpSocket() ([]govmmQemu.QMPSocket, error) { return sockets, nil } -func (q *qemu) buildDevices(ctx context.Context, initrdPath string) ([]govmmQemu.Device, *govmmQemu.IOThread, error) { +func (q *qemu) buildDevices(ctx context.Context, kernelPath string) ([]govmmQemu.Device, *govmmQemu.IOThread, *govmmQemu.Kernel, error) { var devices []govmmQemu.Device + kernel := &govmmQemu.Kernel{ + Path: kernelPath, + } + _, console, err := q.GetVMConsole(ctx, q.id) if err != nil { - return nil, nil, err + return nil, nil, nil, err } // Add bridges before any other devices. This way we make sure that @@ -414,20 +402,28 @@ func (q *qemu) buildDevices(ctx context.Context, initrdPath string) ([]govmmQemu devices, err = q.arch.appendConsole(ctx, devices, console) if err != nil { - return nil, nil, err + return nil, nil, nil, err } - if initrdPath == "" { - devices, err = q.appendImage(ctx, devices) + assetPath, assetType, err := q.config.ImageOrInitrdAssetPath() + if err != nil { + return nil, nil, nil, err + } + + if assetType == types.ImageAsset { + devices, err = q.arch.appendImage(ctx, devices, assetPath) if err != nil { - return nil, nil, err + return nil, nil, nil, err } + } else { + // InitrdAsset, need to set kernel initrd path + kernel.InitrdPath = assetPath } if q.config.IOMMU { devices, err = q.arch.appendIOMMU(devices) if err != nil { - return nil, nil, err + return nil, nil, nil, err } } @@ -438,10 +434,13 @@ func (q *qemu) buildDevices(ctx context.Context, initrdPath string) ([]govmmQemu var ioThread *govmmQemu.IOThread if q.config.BlockDeviceDriver == config.VirtioSCSI { - return q.arch.appendSCSIController(ctx, devices, q.config.EnableIOThreads) + devices, ioThread, err = q.arch.appendSCSIController(ctx, devices, q.config.EnableIOThreads) + if err != nil { + return nil, nil, nil, err + } } - return devices, ioThread, nil + return devices, ioThread, kernel, nil } func (q *qemu) setupTemplate(knobs *govmmQemu.Knobs, memory *govmmQemu.Memory) govmmQemu.Incoming { @@ -562,16 +561,6 @@ func (q *qemu) CreateVM(ctx context.Context, id string, network Network, hypervi IOMMUPlatform: q.config.IOMMUPlatform, } - kernelPath, err := q.config.KernelAssetPath() - if err != nil { - return err - } - - initrdPath, err := q.config.InitrdAssetPath() - if err != nil { - return err - } - incoming := q.setupTemplate(&knobs, &memory) // With the current implementations, VM templating will not work with file @@ -615,7 +604,12 @@ func (q *qemu) CreateVM(ctx context.Context, id string, network Network, hypervi return err } - devices, ioThread, err := q.buildDevices(ctx, initrdPath) + kernelPath, err := q.config.KernelAssetPath() + if err != nil { + return err + } + + devices, ioThread, kernel, err := q.buildDevices(ctx, kernelPath) if err != nil { return err } @@ -643,13 +637,8 @@ func (q *qemu) CreateVM(ctx context.Context, id string, network Network, hypervi return err } - // Breaks hypervisor abstraction has Kata Specific logic - kernel := govmmQemu.Kernel{ - Path: kernelPath, - InitrdPath: initrdPath, - // some devices configuration may also change kernel params, make sure this is called afterwards - Params: q.kernelParameters(), - } + // some devices configuration may also change kernel params, make sure this is called afterwards + kernel.Params = q.kernelParameters() q.checkBpfEnabled() qemuConfig := govmmQemu.Config{ @@ -666,7 +655,7 @@ func (q *qemu) CreateVM(ctx context.Context, id string, network Network, hypervi Devices: devices, CPUModel: cpuModel, SeccompSandbox: q.config.SeccompSandbox, - Kernel: kernel, + Kernel: *kernel, RTC: rtc, QMPSockets: qmpSockets, Knobs: knobs, diff --git a/src/runtime/virtcontainers/types/asset.go b/src/runtime/virtcontainers/types/asset.go index 3b00b5a206..6cad7dd335 100644 --- a/src/runtime/virtcontainers/types/asset.go +++ b/src/runtime/virtcontainers/types/asset.go @@ -41,6 +41,8 @@ const ( FirmwareAsset AssetType = "firmware" FirmwareVolumeAsset AssetType = "firmware_volume" + + UnkownAsset AssetType = "unknown" ) // AssetTypes returns a list of all known asset types.