diff --git a/src/runtime/virtcontainers/acrn.go b/src/runtime/virtcontainers/acrn.go index f6da05ec58..1c3ebc1476 100644 --- a/src/runtime/virtcontainers/acrn.go +++ b/src/runtime/virtcontainers/acrn.go @@ -348,10 +348,6 @@ func (a *Acrn) createDummyVirtioBlkDev(ctx context.Context, devices []Device) ([ } func (a *Acrn) setConfig(config *HypervisorConfig) error { - if err := config.Valid(); err != nil { - return err - } - a.config = *config return nil diff --git a/src/runtime/virtcontainers/clh.go b/src/runtime/virtcontainers/clh.go index 6984b73be5..85ec05f15a 100644 --- a/src/runtime/virtcontainers/clh.go +++ b/src/runtime/virtcontainers/clh.go @@ -268,11 +268,6 @@ var clhDebugKernelParams = []Param{ //########################################################### func (clh *cloudHypervisor) setConfig(config *HypervisorConfig) error { - err := config.Valid() - if err != nil { - return err - } - clh.config = *config return nil diff --git a/src/runtime/virtcontainers/fc.go b/src/runtime/virtcontainers/fc.go index 25012d3f3d..434e320417 100644 --- a/src/runtime/virtcontainers/fc.go +++ b/src/runtime/virtcontainers/fc.go @@ -189,11 +189,6 @@ func (fc *firecracker) truncateID(id string) string { } func (fc *firecracker) setConfig(config *HypervisorConfig) error { - err := config.Valid() - if err != nil { - return err - } - fc.config = *config return nil diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 119c4667c3..18ccd7ae26 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -568,61 +568,6 @@ func (conf *HypervisorConfig) CheckTemplateConfig() error { return nil } -func (conf *HypervisorConfig) Valid() error { - // Kata specific checks. Should be done outside the hypervisor - if conf.KernelPath == "" { - return fmt.Errorf("Missing kernel path") - } - - if conf.ConfidentialGuest && conf.HypervisorMachineType == QemuCCWVirtio { - if conf.ImagePath != "" || conf.InitrdPath != "" { - fmt.Println("yes, failing") - return fmt.Errorf("Neither the image or initrd path may be set for Secure Execution") - } - } else if conf.ImagePath == "" && conf.InitrdPath == "" { - return fmt.Errorf("Missing image and initrd path") - } else if conf.ImagePath != "" && conf.InitrdPath != "" { - return fmt.Errorf("Image and initrd path cannot be both set") - } - - if err := conf.CheckTemplateConfig(); err != nil { - return err - } - - if conf.NumVCPUs == 0 { - conf.NumVCPUs = defaultVCPUs - } - - if conf.MemorySize == 0 { - conf.MemorySize = defaultMemSzMiB - } - - if conf.DefaultBridges == 0 { - conf.DefaultBridges = defaultBridges - } - - if conf.BlockDeviceDriver == "" { - conf.BlockDeviceDriver = defaultBlockDriver - } else if conf.BlockDeviceDriver == config.VirtioBlock && conf.HypervisorMachineType == QemuCCWVirtio { - conf.BlockDeviceDriver = config.VirtioBlockCCW - } - - if conf.DefaultMaxVCPUs == 0 || conf.DefaultMaxVCPUs > defaultMaxVCPUs { - conf.DefaultMaxVCPUs = defaultMaxVCPUs - } - - if conf.ConfidentialGuest && conf.NumVCPUs != conf.DefaultMaxVCPUs { - hvLogger.Warnf("Confidential guests do not support hotplugging of vCPUs. Setting DefaultMaxVCPUs to NumVCPUs (%d)", conf.NumVCPUs) - conf.DefaultMaxVCPUs = conf.NumVCPUs - } - - if conf.Msize9p == 0 && conf.SharedFS != config.VirtioFS { - conf.Msize9p = defaultMsize9p - } - - return nil -} - // AddKernelParam allows the addition of new kernel parameters to an existing // hypervisor configuration. func (conf *HypervisorConfig) AddKernelParam(p Param) error { diff --git a/src/runtime/virtcontainers/hypervisor_test.go b/src/runtime/virtcontainers/hypervisor_test.go index ec475e86eb..31452200d3 100644 --- a/src/runtime/virtcontainers/hypervisor_test.go +++ b/src/runtime/virtcontainers/hypervisor_test.go @@ -86,124 +86,6 @@ func TestNewHypervisorFromUnknownHypervisorType(t *testing.T) { assert.Nil(hy) } -func testHypervisorConfigValid(t *testing.T, hypervisorConfig *HypervisorConfig, success bool) { - err := hypervisorConfig.Valid() - assert := assert.New(t) - assert.False(success && err != nil) - assert.False(!success && err == nil) -} - -func TestHypervisorConfigNoKernelPath(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: "", - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - } - - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigNoImagePath(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: "", - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - } - - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigNoHypervisorPath(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: "", - } - - testHypervisorConfigValid(t, hypervisorConfig, true) -} - -func TestHypervisorConfigIsValid(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - } - - testHypervisorConfigValid(t, hypervisorConfig, true) -} - -func TestHypervisorConfigBothInitrdAndImage(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - InitrdPath: fmt.Sprintf("%s/%s", testDir, testInitrd), - HypervisorPath: "", - } - - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigSecureExecution(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - InitrdPath: fmt.Sprintf("%s/%s", testDir, testInitrd), - ConfidentialGuest: true, - HypervisorMachineType: QemuCCWVirtio, - } - - // Secure Execution should only specify a kernel (encrypted image contains all components) - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigValidTemplateConfig(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - BootToBeTemplate: true, - BootFromTemplate: true, - } - testHypervisorConfigValid(t, hypervisorConfig, false) - - hypervisorConfig.BootToBeTemplate = false - testHypervisorConfigValid(t, hypervisorConfig, false) - hypervisorConfig.MemoryPath = "foobar" - testHypervisorConfigValid(t, hypervisorConfig, false) - hypervisorConfig.DevicesStatePath = "foobar" - testHypervisorConfigValid(t, hypervisorConfig, true) - - hypervisorConfig.BootFromTemplate = false - hypervisorConfig.BootToBeTemplate = true - testHypervisorConfigValid(t, hypervisorConfig, true) - hypervisorConfig.MemoryPath = "" - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigDefaults(t *testing.T) { - assert := assert.New(t) - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: "", - } - testHypervisorConfigValid(t, hypervisorConfig, true) - - hypervisorConfigDefaultsExpected := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: "", - NumVCPUs: defaultVCPUs, - MemorySize: defaultMemSzMiB, - DefaultBridges: defaultBridges, - BlockDeviceDriver: defaultBlockDriver, - DefaultMaxVCPUs: defaultMaxVCPUs, - Msize9p: defaultMsize9p, - } - - assert.Exactly(hypervisorConfig, hypervisorConfigDefaultsExpected) -} - func TestAppendParams(t *testing.T) { assert := assert.New(t) paramList := []Param{ diff --git a/src/runtime/virtcontainers/mock_hypervisor.go b/src/runtime/virtcontainers/mock_hypervisor.go index 83e776d289..f4a0b934ec 100644 --- a/src/runtime/virtcontainers/mock_hypervisor.go +++ b/src/runtime/virtcontainers/mock_hypervisor.go @@ -31,10 +31,6 @@ func (m *mockHypervisor) HypervisorConfig() HypervisorConfig { } func (m *mockHypervisor) setConfig(config *HypervisorConfig) error { - if err := config.Valid(); err != nil { - return err - } - return nil } diff --git a/src/runtime/virtcontainers/mock_hypervisor_test.go b/src/runtime/virtcontainers/mock_hypervisor_test.go index 5e89ae8a69..0159a993dd 100644 --- a/src/runtime/virtcontainers/mock_hypervisor_test.go +++ b/src/runtime/virtcontainers/mock_hypervisor_test.go @@ -21,9 +21,9 @@ func TestMockHypervisorCreateVM(t *testing.T) { config: &SandboxConfig{ ID: "mock_sandbox", HypervisorConfig: HypervisorConfig{ - KernelPath: "", - ImagePath: "", - HypervisorPath: "", + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), }, }, } @@ -33,16 +33,6 @@ func TestMockHypervisorCreateVM(t *testing.T) { ctx := context.Background() - // wrong config - err = m.CreateVM(ctx, sandbox.config.ID, network, &sandbox.config.HypervisorConfig) - assert.Error(err) - - sandbox.config.HypervisorConfig = HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - } - err = m.CreateVM(ctx, sandbox.config.ID, network, &sandbox.config.HypervisorConfig) assert.NoError(err) } diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index a91505d285..7440c82587 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -461,11 +461,6 @@ func (q *qemu) setupFileBackedMem(knobs *govmmQemu.Knobs, memory *govmmQemu.Memo } func (q *qemu) setConfig(config *HypervisorConfig) error { - err := config.Valid() - if err != nil { - return err - } - q.config = *config return nil diff --git a/src/runtime/virtcontainers/sandbox.go b/src/runtime/virtcontainers/sandbox.go index a995f1f77b..550362afdd 100644 --- a/src/runtime/virtcontainers/sandbox.go +++ b/src/runtime/virtcontainers/sandbox.go @@ -593,6 +593,10 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor s.Logger().WithError(err).Debug("restore sandbox failed") } + if err := validateHypervisorConfig(&sandboxConfig.HypervisorConfig); err != nil { + return nil, err + } + // store doesn't require hypervisor to be stored immediately if err = s.hypervisor.CreateVM(ctx, s.id, s.network, &sandboxConfig.HypervisorConfig); err != nil { return nil, err @@ -605,6 +609,62 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor return s, nil } +func validateHypervisorConfig(conf *HypervisorConfig) error { + if conf.KernelPath == "" { + return fmt.Errorf("Missing kernel path") + } + + QemuCCWVirtio := "" + if conf.ConfidentialGuest && conf.HypervisorMachineType == QemuCCWVirtio { + if conf.ImagePath != "" || conf.InitrdPath != "" { + fmt.Println("yes, failing") + return fmt.Errorf("Neither the image or initrd path may be set for Secure Execution") + } + } else if conf.ImagePath == "" && conf.InitrdPath == "" { + return fmt.Errorf("Missing image and initrd path") + } else if conf.ImagePath != "" && conf.InitrdPath != "" { + return fmt.Errorf("Image and initrd path cannot be both set") + } + + if err := conf.CheckTemplateConfig(); err != nil { + return err + } + + if conf.NumVCPUs == 0 { + conf.NumVCPUs = defaultVCPUs + } + + if conf.MemorySize == 0 { + conf.MemorySize = defaultMemSzMiB + } + + if conf.DefaultBridges == 0 { + conf.DefaultBridges = defaultBridges + } + + if conf.BlockDeviceDriver == "" { + conf.BlockDeviceDriver = defaultBlockDriver + } else if conf.BlockDeviceDriver == config.VirtioBlock && conf.HypervisorMachineType == QemuCCWVirtio { + conf.BlockDeviceDriver = config.VirtioBlockCCW + } + + if conf.DefaultMaxVCPUs == 0 || conf.DefaultMaxVCPUs > defaultMaxVCPUs { + conf.DefaultMaxVCPUs = defaultMaxVCPUs + } + + if conf.ConfidentialGuest && conf.NumVCPUs != conf.DefaultMaxVCPUs { + hvLogger.Warnf("Confidential guests do not support hotplugging of vCPUs. Setting DefaultMaxVCPUs to NumVCPUs (%d)", conf.NumVCPUs) + conf.DefaultMaxVCPUs = conf.NumVCPUs + } + + if conf.Msize9p == 0 && conf.SharedFS != config.VirtioFS { + conf.Msize9p = defaultMsize9p + } + + return nil + +} + func (s *Sandbox) createResourceController() error { var err error cgroupPath := "" diff --git a/src/runtime/virtcontainers/vm.go b/src/runtime/virtcontainers/vm.go index a9db8efc06..a96661d434 100644 --- a/src/runtime/virtcontainers/vm.go +++ b/src/runtime/virtcontainers/vm.go @@ -42,9 +42,8 @@ type VMConfig struct { HypervisorConfig HypervisorConfig } -// Valid Check VMConfig validity. func (c *VMConfig) Valid() error { - return c.HypervisorConfig.Valid() + return validateHypervisorConfig(&c.HypervisorConfig) } // ToGrpc convert VMConfig struct to grpc format pb.GrpcVMConfig.