Skip to content

Commit

Permalink
Merge pull request kubevirt#9103 from lyarwood/bug-9102
Browse files Browse the repository at this point in the history
virt-api: Refactor, share and use `VirtualMachineInstanceSpec` defaulting code from VM validation webhook
  • Loading branch information
kubevirt-bot authored Feb 3, 2023
2 parents e26b8df + e7898af commit d8c3456
Show file tree
Hide file tree
Showing 14 changed files with 310 additions and 222 deletions.
13 changes: 6 additions & 7 deletions pkg/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,22 +216,21 @@ func MarkAsNonroot(vmi *v1.VirtualMachineInstance) {
vmi.Status.RuntimeUser = 107
}

func SetDefaultVolumeDisk(obj *v1.VirtualMachineInstance) {

func SetDefaultVolumeDisk(spec *v1.VirtualMachineInstanceSpec) {
diskAndFilesystemNames := make(map[string]struct{})

for _, disk := range obj.Spec.Domain.Devices.Disks {
for _, disk := range spec.Domain.Devices.Disks {
diskAndFilesystemNames[disk.Name] = struct{}{}
}

for _, fs := range obj.Spec.Domain.Devices.Filesystems {
for _, fs := range spec.Domain.Devices.Filesystems {
diskAndFilesystemNames[fs.Name] = struct{}{}
}

for _, volume := range obj.Spec.Volumes {
for _, volume := range spec.Volumes {
if _, foundDisk := diskAndFilesystemNames[volume.Name]; !foundDisk {
obj.Spec.Domain.Devices.Disks = append(
obj.Spec.Domain.Devices.Disks,
spec.Domain.Devices.Disks = append(
spec.Domain.Devices.Disks,
v1.Disk{
Name: volume.Name,
},
Expand Down
5 changes: 5 additions & 0 deletions pkg/virt-api/webhooks/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,23 @@ go_library(
srcs = [
"amd64.go",
"arm64.go",
"defaults.go",
"hyperv.go",
"utils.go",
],
importpath = "kubevirt.io/kubevirt/pkg/virt-api/webhooks",
visibility = ["//visibility:public"],
deps = [
"//pkg/util:go_default_library",
"//pkg/virt-config:go_default_library",
"//pkg/virt-handler/node-labeller/util:go_default_library",
"//pkg/virt-operator/resource/generate/rbac:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
"//staging/src/kubevirt.io/api/pool/v1alpha1:go_default_library",
"//staging/src/kubevirt.io/client-go/log:go_default_library",
"//staging/src/kubevirt.io/client-go/util:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
"//vendor/k8s.io/client-go/tools/cache:go_default_library",
Expand Down
13 changes: 6 additions & 7 deletions pkg/virt-api/webhooks/amd64.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,14 @@ import (
v1 "kubevirt.io/api/core/v1"
)

// setDefaultDisksBus set default Disks Bus
func setAmd64DefaultDisksBus(vmi *v1.VirtualMachineInstance) {
func setDefaultAmd64DisksBus(spec *v1.VirtualMachineInstanceSpec) {
// Setting SATA as the default bus since it is typically supported out of the box by
// guest operating systems (we support only q35 and therefore IDE is not supported)
// TODO: consider making this OS-specific (VIRTIO for linux, SATA for others)
bus := v1.DiskBusSATA

for i := range vmi.Spec.Domain.Devices.Disks {
disk := &vmi.Spec.Domain.Devices.Disks[i].DiskDevice
for i := range spec.Domain.Devices.Disks {
disk := &spec.Domain.Devices.Disks[i].DiskDevice

if disk.Disk != nil && disk.Disk.Bus == "" {
disk.Disk.Bus = bus
Expand All @@ -46,7 +45,7 @@ func setAmd64DefaultDisksBus(vmi *v1.VirtualMachineInstance) {
}
}

// SetVirtualMachineInstanceAmd64Defaults is mutating function for mutating-webhook
func SetVirtualMachineInstanceAmd64Defaults(vmi *v1.VirtualMachineInstance) {
setAmd64DefaultDisksBus(vmi)
// SetAmd64Defaults is mutating function for mutating-webhook
func SetAmd64Defaults(spec *v1.VirtualMachineInstanceSpec) {
setDefaultAmd64DisksBus(spec)
}
42 changes: 21 additions & 21 deletions pkg/virt-api/webhooks/arm64.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,36 +105,36 @@ func ValidateVirtualMachineInstanceArm64Setting(field *k8sfield.Path, spec *v1.V
}

// setDefaultCPUModel set default cpu model to host-passthrough
func setDefaultCPUModel(vmi *v1.VirtualMachineInstance) {
if vmi.Spec.Domain.CPU == nil {
vmi.Spec.Domain.CPU = &v1.CPU{}
func setDefaultArm64CPUModel(spec *v1.VirtualMachineInstanceSpec) {
if spec.Domain.CPU == nil {
spec.Domain.CPU = &v1.CPU{}
}

if vmi.Spec.Domain.CPU.Model == "" {
vmi.Spec.Domain.CPU.Model = defaultCPUModel
if spec.Domain.CPU.Model == "" {
spec.Domain.CPU.Model = defaultCPUModel
}
}

// setDefaultBootloader set default bootloader to uefi boot
func setDefaultBootloader(vmi *v1.VirtualMachineInstance) {
if vmi.Spec.Domain.Firmware == nil || vmi.Spec.Domain.Firmware.Bootloader == nil {
if vmi.Spec.Domain.Firmware == nil {
vmi.Spec.Domain.Firmware = &v1.Firmware{}
func setDefaultArm64Bootloader(spec *v1.VirtualMachineInstanceSpec) {
if spec.Domain.Firmware == nil || spec.Domain.Firmware.Bootloader == nil {
if spec.Domain.Firmware == nil {
spec.Domain.Firmware = &v1.Firmware{}
}
if vmi.Spec.Domain.Firmware.Bootloader == nil {
vmi.Spec.Domain.Firmware.Bootloader = &v1.Bootloader{}
if spec.Domain.Firmware.Bootloader == nil {
spec.Domain.Firmware.Bootloader = &v1.Bootloader{}
}
vmi.Spec.Domain.Firmware.Bootloader.EFI = &v1.EFI{}
vmi.Spec.Domain.Firmware.Bootloader.EFI.SecureBoot = &_false
spec.Domain.Firmware.Bootloader.EFI = &v1.EFI{}
spec.Domain.Firmware.Bootloader.EFI.SecureBoot = &_false
}
}

// setDefaultDisksBus set default Disks Bus, because sata is not supported by qemu-kvm of Arm64
func setDefaultDisksBus(vmi *v1.VirtualMachineInstance) {
func setDefaultArm64DisksBus(spec *v1.VirtualMachineInstanceSpec) {
bus := v1.DiskBusVirtio

for i := range vmi.Spec.Domain.Devices.Disks {
disk := &vmi.Spec.Domain.Devices.Disks[i].DiskDevice
for i := range spec.Domain.Devices.Disks {
disk := &spec.Domain.Devices.Disks[i].DiskDevice

if disk.Disk != nil && disk.Disk.Bus == "" {
disk.Disk.Bus = bus
Expand All @@ -149,9 +149,9 @@ func setDefaultDisksBus(vmi *v1.VirtualMachineInstance) {

}

// SetVirtualMachineInstanceArm64Defaults is mutating function for mutating-webhook
func SetVirtualMachineInstanceArm64Defaults(vmi *v1.VirtualMachineInstance) {
setDefaultCPUModel(vmi)
setDefaultBootloader(vmi)
setDefaultDisksBus(vmi)
// SetArm64Defaults is mutating function for mutating-webhook
func SetArm64Defaults(spec *v1.VirtualMachineInstanceSpec) {
setDefaultArm64CPUModel(spec)
setDefaultArm64Bootloader(spec)
setDefaultArm64DisksBus(spec)
}
211 changes: 211 additions & 0 deletions pkg/virt-api/webhooks/defaults.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
/*
* This file is part of the KubeVirt project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* Copyright 2023 Red Hat, Inc.
*
*/

package webhooks

import (
"strings"

k8sv1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"

v1 "kubevirt.io/api/core/v1"
"kubevirt.io/client-go/log"

"kubevirt.io/kubevirt/pkg/util"
virtconfig "kubevirt.io/kubevirt/pkg/virt-config"
)

func SetDefaultVirtualMachine(clusterConfig *virtconfig.ClusterConfig, vm *v1.VirtualMachine) error {
if err := setDefaultVirtualMachineInstanceSpec(clusterConfig, &vm.Spec.Template.Spec); err != nil {
return err
}
v1.SetObjectDefaults_VirtualMachine(vm)
setDefaultHypervFeatureDependencies(&vm.Spec.Template.Spec)
setDefaultCPUArch(clusterConfig, &vm.Spec.Template.Spec)
return nil
}

func SetDefaultVirtualMachineInstance(clusterConfig *virtconfig.ClusterConfig, vmi *v1.VirtualMachineInstance) error {
if err := setDefaultVirtualMachineInstanceSpec(clusterConfig, &vmi.Spec); err != nil {
return err
}
v1.SetObjectDefaults_VirtualMachineInstance(vmi)
setDefaultHypervFeatureDependencies(&vmi.Spec)
setDefaultCPUArch(clusterConfig, &vmi.Spec)
return nil
}

func setDefaultCPUArch(clusterConfig *virtconfig.ClusterConfig, spec *v1.VirtualMachineInstanceSpec) {
// Do some CPU arch specific setting.
if IsARM64() {
log.Log.V(4).Info("Apply Arm64 specific setting")
SetArm64Defaults(spec)
} else {
SetAmd64Defaults(spec)
setDefaultCPUModel(clusterConfig, spec)
}
}

func setDefaultHypervFeatureDependencies(spec *v1.VirtualMachineInstanceSpec) {
// In a future, yet undecided, release either libvirt or QEMU are going to check the hyperv dependencies, so we can get rid of this code.
// Until that time, we need to handle the hyperv deps to avoid obscure rejections from QEMU later on
log.Log.V(4).Info("Set HyperV dependencies")
if err := SetHypervFeatureDependencies(spec); err != nil {
// HyperV is a special case. If our best-effort attempt fails, we should leave
// rejection to be performed later on in the validating webhook, and continue here.
// Please note this means that partial changes may have been performed.
// This is OK since each dependency must be atomic and independent (in ACID sense),
// so the VMI configuration is still legal.
log.Log.V(2).Infof("Failed to set HyperV dependencies: %s", err)
}
}

func setDefaultVirtualMachineInstanceSpec(clusterConfig *virtconfig.ClusterConfig, spec *v1.VirtualMachineInstanceSpec) error {
setDefaultMachineType(clusterConfig, spec)
setDefaultResourceRequests(clusterConfig, spec)
setDefaultGuestCPUTopology(clusterConfig, spec)
setDefaultPullPoliciesOnContainerDisks(clusterConfig, spec)
if err := clusterConfig.SetVMISpecDefaultNetworkInterface(spec); err != nil {
return err
}
util.SetDefaultVolumeDisk(spec)
return nil
}

func setDefaultMachineType(clusterConfig *virtconfig.ClusterConfig, spec *v1.VirtualMachineInstanceSpec) {
machineType := clusterConfig.GetMachineType()

if machine := spec.Domain.Machine; machine != nil {
if machine.Type == "" {
machine.Type = machineType
}
} else {
spec.Domain.Machine = &v1.Machine{Type: machineType}
}
}

func setDefaultPullPoliciesOnContainerDisks(clusterConfig *virtconfig.ClusterConfig, spec *v1.VirtualMachineInstanceSpec) {
for _, volume := range spec.Volumes {
if volume.ContainerDisk != nil && volume.ContainerDisk.ImagePullPolicy == "" {
if strings.HasSuffix(volume.ContainerDisk.Image, ":latest") || !strings.ContainsAny(volume.ContainerDisk.Image, ":@") {
volume.ContainerDisk.ImagePullPolicy = k8sv1.PullAlways
} else {
volume.ContainerDisk.ImagePullPolicy = k8sv1.PullIfNotPresent
}
}
}
}

func setDefaultResourceRequests(clusterConfig *virtconfig.ClusterConfig, spec *v1.VirtualMachineInstanceSpec) {
resources := &spec.Domain.Resources

if !resources.Limits.Cpu().IsZero() && resources.Requests.Cpu().IsZero() {
if resources.Requests == nil {
resources.Requests = k8sv1.ResourceList{}
}
resources.Requests[k8sv1.ResourceCPU] = resources.Limits[k8sv1.ResourceCPU]
}

if !resources.Limits.Memory().IsZero() && resources.Requests.Memory().IsZero() {
if resources.Requests == nil {
resources.Requests = k8sv1.ResourceList{}
}
resources.Requests[k8sv1.ResourceMemory] = resources.Limits[k8sv1.ResourceMemory]
}

if _, exists := resources.Requests[k8sv1.ResourceMemory]; !exists {
var memory *resource.Quantity
if spec.Domain.Memory != nil && spec.Domain.Memory.Guest != nil {
memory = spec.Domain.Memory.Guest
}
if memory == nil && spec.Domain.Memory != nil && spec.Domain.Memory.Hugepages != nil {
if hugepagesSize, err := resource.ParseQuantity(spec.Domain.Memory.Hugepages.PageSize); err == nil {
memory = &hugepagesSize
}
}
if memory != nil && memory.Value() > 0 {
if resources.Requests == nil {
resources.Requests = k8sv1.ResourceList{}
}
overcommit := clusterConfig.GetMemoryOvercommit()
if overcommit == 100 {
resources.Requests[k8sv1.ResourceMemory] = *memory
} else {
value := (memory.Value() * int64(100)) / int64(overcommit)
resources.Requests[k8sv1.ResourceMemory] = *resource.NewQuantity(value, memory.Format)
}
memoryRequest := resources.Requests[k8sv1.ResourceMemory]
log.Log.V(4).Infof("Set memory-request to %s as a result of memory-overcommit = %v%%", memoryRequest.String(), overcommit)
}
}
if cpuRequest := clusterConfig.GetCPURequest(); !cpuRequest.Equal(resource.MustParse(virtconfig.DefaultCPURequest)) {
if _, exists := resources.Requests[k8sv1.ResourceCPU]; !exists {
if spec.Domain.CPU != nil && spec.Domain.CPU.DedicatedCPUPlacement {
return
}
if resources.Requests == nil {
resources.Requests = k8sv1.ResourceList{}
}
resources.Requests[k8sv1.ResourceCPU] = *cpuRequest
}
}
}

func setDefaultGuestCPUTopology(clusterConfig *virtconfig.ClusterConfig, spec *v1.VirtualMachineInstanceSpec) {
cores := uint32(1)
threads := uint32(1)
sockets := uint32(1)
vmiCPU := spec.Domain.CPU
if vmiCPU == nil || (vmiCPU.Cores == 0 && vmiCPU.Sockets == 0 && vmiCPU.Threads == 0) {
// create cpu topology struct
if spec.Domain.CPU == nil {
spec.Domain.CPU = &v1.CPU{}
}
//if cores, sockets, threads are not set, take value from domain resources request or limits and
//set value into sockets, which have best performance (https://bugzilla.redhat.com/show_bug.cgi?id=1653453)
resources := spec.Domain.Resources
if cpuLimit, ok := resources.Limits[k8sv1.ResourceCPU]; ok {
sockets = uint32(cpuLimit.Value())
} else if cpuRequests, ok := resources.Requests[k8sv1.ResourceCPU]; ok {
sockets = uint32(cpuRequests.Value())
}

spec.Domain.CPU.Sockets = sockets
spec.Domain.CPU.Cores = cores
spec.Domain.CPU.Threads = threads
}
}

func setDefaultCPUModel(clusterConfig *virtconfig.ClusterConfig, spec *v1.VirtualMachineInstanceSpec) {
// create cpu topology struct
if spec.Domain.CPU == nil {
spec.Domain.CPU = &v1.CPU{}
}

// if vmi doesn't have cpu model set
if spec.Domain.CPU.Model == "" {
if clusterConfigCPUModel := clusterConfig.GetCPUModel(); clusterConfigCPUModel != "" {
//set is as vmi cpu model
spec.Domain.CPU.Model = clusterConfigCPUModel
} else {
spec.Domain.CPU.Model = v1.DefaultCPUModel
}
}
}
Loading

0 comments on commit d8c3456

Please sign in to comment.