From 8a1620d711a16aa450a3d7fdec0cb671110a5d45 Mon Sep 17 00:00:00 2001
From: Lee Yarwood <lyarwood@redhat.com>
Date: Mon, 30 Jan 2023 19:17:52 +0000
Subject: [PATCH 1/3] vms-admitter: Add test for bug #9102

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
---
 .../admitters/vms-admitter_test.go            | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go
index 055af40d1d01..77dfeb2a1f4c 100644
--- a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go
+++ b/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go
@@ -171,6 +171,32 @@ var _ = Describe("Validating VM Admitter", func() {
 		Expect(resp.Allowed).To(BeTrue())
 	})
 
+	It("should accept VM requesting hugepages but missing spec.template.spec.domain.resources.requests.memory - bug #9102", func() {
+		vmi := api.NewMinimalVMI("testvmi")
+		vmi.Spec.Domain.Resources = v1.ResourceRequirements{}
+		guestMemory := resource.MustParse("1Gi")
+		vmi.Spec.Domain.Memory = &v1.Memory{
+			Guest: &guestMemory,
+			Hugepages: &v1.Hugepages{
+				PageSize: "2Mi",
+			},
+		}
+		vm := &v1.VirtualMachine{
+			Spec: v1.VirtualMachineSpec{
+				Running: &notRunning,
+				Template: &v1.VirtualMachineInstanceTemplateSpec{
+					Spec: vmi.Spec,
+				},
+			},
+		}
+
+		resp := admitVm(vmsAdmitter, vm)
+		// FIXME - This is bug #9102, the request should be accepted with guest visible memory taken into account
+		Expect(resp.Allowed).To(BeFalse())
+		Expect(resp.Result.Details.Causes).To(HaveLen(1))
+		Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.template.spec.domain.resources.requests.memory"))
+	})
+
 	DescribeTable("should reject VolumeRequests on a migrating vm", func(requests []v1.VirtualMachineVolumeRequest) {
 		now := metav1.Now()
 		vmi := api.NewMinimalVMI("testvmi")

From 69976c0b3ef8da7abab53788dce7a56b3adc291a Mon Sep 17 00:00:00 2001
From: Lee Yarwood <lyarwood@redhat.com>
Date: Mon, 30 Jan 2023 18:28:47 +0000
Subject: [PATCH 2/3] virt-api: Refactor and share VM and VMI defaulting code
 between webhooks

This change moves the code responsible for defaulting the
VirtualMachineInstance and VirtualMachineInstanceSpec into a shared
location ahead of use by the VirtualMachine validation webhook. As a
result the code is also refactored to operate on the shared
VirtualMachineInstanceSpec. Helpers are also provided ensuring the
`defaulter-gen` generated code for each type is also called.

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
---
 pkg/util/util.go                              |  13 +-
 pkg/virt-api/webhooks/BUILD.bazel             |   5 +
 pkg/virt-api/webhooks/amd64.go                |  13 +-
 pkg/virt-api/webhooks/arm64.go                |  42 ++--
 pkg/virt-api/webhooks/defaults.go             | 211 ++++++++++++++++++
 pkg/virt-api/webhooks/hyperv.go               |  26 +--
 .../mutating-webhook/mutators/BUILD.bazel     |   1 -
 .../mutating-webhook/mutators/vmi-mutator.go  | 157 +------------
 .../mutators/vmi-mutator_test.go              |  12 +-
 pkg/virt-config/virt-config.go                |  14 +-
 pkg/virt-controller/watch/vm.go               |   4 +-
 .../virtwrap/converter/converter_test.go      |   6 +-
 12 files changed, 282 insertions(+), 222 deletions(-)
 create mode 100644 pkg/virt-api/webhooks/defaults.go

diff --git a/pkg/util/util.go b/pkg/util/util.go
index b3e02c1ea098..fffa85e51ec0 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -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,
 				},
diff --git a/pkg/virt-api/webhooks/BUILD.bazel b/pkg/virt-api/webhooks/BUILD.bazel
index b7ebdc8cb437..abe3641901aa 100644
--- a/pkg/virt-api/webhooks/BUILD.bazel
+++ b/pkg/virt-api/webhooks/BUILD.bazel
@@ -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",
diff --git a/pkg/virt-api/webhooks/amd64.go b/pkg/virt-api/webhooks/amd64.go
index d66b7edde2a4..257709eef0fb 100644
--- a/pkg/virt-api/webhooks/amd64.go
+++ b/pkg/virt-api/webhooks/amd64.go
@@ -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
@@ -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)
 }
diff --git a/pkg/virt-api/webhooks/arm64.go b/pkg/virt-api/webhooks/arm64.go
index fd1b76432694..e536da836d00 100644
--- a/pkg/virt-api/webhooks/arm64.go
+++ b/pkg/virt-api/webhooks/arm64.go
@@ -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
@@ -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)
 }
diff --git a/pkg/virt-api/webhooks/defaults.go b/pkg/virt-api/webhooks/defaults.go
new file mode 100644
index 000000000000..9bad3703e495
--- /dev/null
+++ b/pkg/virt-api/webhooks/defaults.go
@@ -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
+		}
+	}
+}
diff --git a/pkg/virt-api/webhooks/hyperv.go b/pkg/virt-api/webhooks/hyperv.go
index a4773b460877..5629c56adbe8 100644
--- a/pkg/virt-api/webhooks/hyperv.go
+++ b/pkg/virt-api/webhooks/hyperv.go
@@ -199,10 +199,10 @@ func ValidateVirtualMachineInstanceHypervFeatureDependencies(field *k8sfield.Pat
 	return causes
 }
 
-func SetVirtualMachineInstanceHypervFeatureDependencies(vmi *v1.VirtualMachineInstance) error {
+func SetHypervFeatureDependencies(spec *v1.VirtualMachineInstanceSpec) error {
 	path := k8sfield.NewPath("spec")
 
-	if features := getHypervFeatureDependencies(path, &vmi.Spec); features != nil {
+	if features := getHypervFeatureDependencies(path, spec); features != nil {
 		for _, feat := range features {
 			if err := feat.TryToSetRequirement(); err != nil {
 				return err
@@ -211,15 +211,15 @@ func SetVirtualMachineInstanceHypervFeatureDependencies(vmi *v1.VirtualMachineIn
 	}
 
 	//Check if vmi has EVMCS feature enabled. If yes, we have to add vmx cpu feature
-	if vmi.Spec.Domain.Features != nil && vmi.Spec.Domain.Features.Hyperv != nil && vmi.Spec.Domain.Features.Hyperv.EVMCS != nil &&
-		(vmi.Spec.Domain.Features.Hyperv.EVMCS.Enabled == nil || (*vmi.Spec.Domain.Features.Hyperv.EVMCS.Enabled) == true) {
-		setEVMCSDependency(vmi)
+	if spec.Domain.Features != nil && spec.Domain.Features.Hyperv != nil && spec.Domain.Features.Hyperv.EVMCS != nil &&
+		(spec.Domain.Features.Hyperv.EVMCS.Enabled == nil || (*spec.Domain.Features.Hyperv.EVMCS.Enabled) == true) {
+		setEVMCSDependency(spec)
 	}
 
 	return nil
 }
 
-func setEVMCSDependency(vmi *v1.VirtualMachineInstance) {
+func setEVMCSDependency(spec *v1.VirtualMachineInstanceSpec) {
 	vmxFeature := v1.CPUFeature{
 		Name:   nodelabellerutil.VmxFeature,
 		Policy: nodelabellerutil.RequirePolicy,
@@ -229,33 +229,33 @@ func setEVMCSDependency(vmi *v1.VirtualMachineInstance) {
 		vmxFeature,
 	}
 
-	if vmi.Spec.Domain.CPU == nil {
-		vmi.Spec.Domain.CPU = &v1.CPU{
+	if spec.Domain.CPU == nil {
+		spec.Domain.CPU = &v1.CPU{
 			Features: cpuFeatures,
 		}
 		return
 	}
 
-	if len(vmi.Spec.Domain.CPU.Features) == 0 {
-		vmi.Spec.Domain.CPU.Features = cpuFeatures
+	if len(spec.Domain.CPU.Features) == 0 {
+		spec.Domain.CPU.Features = cpuFeatures
 		return
 	}
 
 	for _, requiredFeature := range cpuFeatures {
 		featureFound := false
 
-		for i, existingFeature := range vmi.Spec.Domain.CPU.Features {
+		for i, existingFeature := range spec.Domain.CPU.Features {
 			if existingFeature.Name == requiredFeature.Name {
 				featureFound = true
 				if existingFeature.Policy != requiredFeature.Policy {
-					vmi.Spec.Domain.CPU.Features[i].Policy = requiredFeature.Policy
+					spec.Domain.CPU.Features[i].Policy = requiredFeature.Policy
 				}
 				break
 			}
 		}
 
 		if !featureFound {
-			vmi.Spec.Domain.CPU.Features = append(vmi.Spec.Domain.CPU.Features, requiredFeature)
+			spec.Domain.CPU.Features = append(spec.Domain.CPU.Features, requiredFeature)
 		}
 	}
 
diff --git a/pkg/virt-api/webhooks/mutating-webhook/mutators/BUILD.bazel b/pkg/virt-api/webhooks/mutating-webhook/mutators/BUILD.bazel
index d7acc8003faa..e956bf21ef59 100644
--- a/pkg/virt-api/webhooks/mutating-webhook/mutators/BUILD.bazel
+++ b/pkg/virt-api/webhooks/mutating-webhook/mutators/BUILD.bazel
@@ -28,7 +28,6 @@ go_library(
         "//vendor/k8s.io/api/admission/v1:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/equality: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/labels:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library",
diff --git a/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go b/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go
index 02124a18d470..df206f9eb116 100644
--- a/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go
+++ b/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go
@@ -27,9 +27,7 @@ import (
 	"time"
 
 	admissionv1 "k8s.io/api/admission/v1"
-	k8sv1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
-	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/cache"
 
@@ -80,40 +78,12 @@ func (mutator *VMIsMutator) Mutate(ar *admissionv1.AdmissionReview) *admissionv1
 			}
 		}
 
-		// Set VMI defaults
+		// Set VirtualMachineInstance defaults
 		log.Log.Object(newVMI).V(4).Info("Apply defaults")
-		mutator.setDefaultMachineType(newVMI)
-		mutator.setDefaultResourceRequests(newVMI)
-		mutator.setDefaultGuestCPUTopology(newVMI)
-		mutator.setDefaultPullPoliciesOnContainerDisks(newVMI)
-		err = mutator.ClusterConfig.SetVMIDefaultNetworkInterface(newVMI)
-		if err != nil {
+		if err = webhooks.SetDefaultVirtualMachineInstance(mutator.ClusterConfig, newVMI); err != nil {
 			return webhookutils.ToAdmissionResponseError(err)
 		}
-		util.SetDefaultVolumeDisk(newVMI)
-		v1.SetObjectDefaults_VirtualMachineInstance(newVMI)
-
-		// 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")
-		err = webhooks.SetVirtualMachineInstanceHypervFeatureDependencies(newVMI)
-		if 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)
-		}
 
-		// Do some CPU arch specific setting.
-		if webhooks.IsARM64() {
-			log.Log.V(4).Info("Apply Arm64 specific setting")
-			webhooks.SetVirtualMachineInstanceArm64Defaults(newVMI)
-		} else {
-			webhooks.SetVirtualMachineInstanceAmd64Defaults(newVMI)
-			mutator.setDefaultCPUModel(newVMI)
-		}
 		if newVMI.IsRealtimeEnabled() {
 			log.Log.V(4).Info("Add realtime node label selector")
 			addNodeSelector(newVMI, v1.RealtimeLabel)
@@ -203,129 +173,6 @@ func (mutator *VMIsMutator) Mutate(ar *admissionv1.AdmissionReview) *admissionv1
 	}
 }
 
-func (mutator *VMIsMutator) setDefaultCPUModel(vmi *v1.VirtualMachineInstance) {
-	// create cpu topology struct
-	if vmi.Spec.Domain.CPU == nil {
-		vmi.Spec.Domain.CPU = &v1.CPU{}
-	}
-
-	// if vmi doesn't have cpu model set
-	if vmi.Spec.Domain.CPU.Model == "" {
-		if clusterConfigCPUModel := mutator.ClusterConfig.GetCPUModel(); clusterConfigCPUModel != "" {
-			//set is as vmi cpu model
-			vmi.Spec.Domain.CPU.Model = clusterConfigCPUModel
-		} else {
-			vmi.Spec.Domain.CPU.Model = v1.DefaultCPUModel
-		}
-	}
-}
-
-func (mutator *VMIsMutator) setDefaultGuestCPUTopology(vmi *v1.VirtualMachineInstance) {
-	cores := uint32(1)
-	threads := uint32(1)
-	sockets := uint32(1)
-	vmiCPU := vmi.Spec.Domain.CPU
-	if vmiCPU == nil || (vmiCPU.Cores == 0 && vmiCPU.Sockets == 0 && vmiCPU.Threads == 0) {
-		// create cpu topology struct
-		if vmi.Spec.Domain.CPU == nil {
-			vmi.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 := vmi.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())
-		}
-
-		vmi.Spec.Domain.CPU.Sockets = sockets
-		vmi.Spec.Domain.CPU.Cores = cores
-		vmi.Spec.Domain.CPU.Threads = threads
-	}
-}
-
-func (mutator *VMIsMutator) setDefaultMachineType(vmi *v1.VirtualMachineInstance) {
-	machineType := mutator.ClusterConfig.GetMachineType()
-
-	if machine := vmi.Spec.Domain.Machine; machine != nil {
-		if machine.Type == "" {
-			machine.Type = machineType
-		}
-	} else {
-		vmi.Spec.Domain.Machine = &v1.Machine{Type: machineType}
-	}
-}
-
-func (mutator *VMIsMutator) setDefaultPullPoliciesOnContainerDisks(vmi *v1.VirtualMachineInstance) {
-	for _, volume := range vmi.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 (mutator *VMIsMutator) setDefaultResourceRequests(vmi *v1.VirtualMachineInstance) {
-
-	resources := &vmi.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 vmi.Spec.Domain.Memory != nil && vmi.Spec.Domain.Memory.Guest != nil {
-			memory = vmi.Spec.Domain.Memory.Guest
-		}
-		if memory == nil && vmi.Spec.Domain.Memory != nil && vmi.Spec.Domain.Memory.Hugepages != nil {
-			if hugepagesSize, err := resource.ParseQuantity(vmi.Spec.Domain.Memory.Hugepages.PageSize); err == nil {
-				memory = &hugepagesSize
-			}
-		}
-		if memory != nil && memory.Value() > 0 {
-			if resources.Requests == nil {
-				resources.Requests = k8sv1.ResourceList{}
-			}
-			overcommit := mutator.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.Object(vmi).V(4).Infof("Set memory-request to %s as a result of memory-overcommit = %v%%", memoryRequest.String(), overcommit)
-		}
-	}
-	if cpuRequest := mutator.ClusterConfig.GetCPURequest(); !cpuRequest.Equal(resource.MustParse(virtconfig.DefaultCPURequest)) {
-		if _, exists := resources.Requests[k8sv1.ResourceCPU]; !exists {
-			if vmi.Spec.Domain.CPU != nil && vmi.Spec.Domain.CPU.DedicatedCPUPlacement {
-				return
-			}
-			if resources.Requests == nil {
-				resources.Requests = k8sv1.ResourceList{}
-			}
-			resources.Requests[k8sv1.ResourceCPU] = *cpuRequest
-		}
-	}
-
-}
-
 func addNodeSelector(vmi *v1.VirtualMachineInstance, label string) {
 	if vmi.Spec.NodeSelector == nil {
 		vmi.Spec.NodeSelector = map[string]string{}
diff --git a/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator_test.go b/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator_test.go
index 6b0cba105422..025bf368ad81 100644
--- a/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator_test.go
+++ b/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator_test.go
@@ -619,7 +619,7 @@ var _ = Describe("VirtualMachineInstance Mutator", func() {
 	It("Should not mutate VMIs without HyperV configuration", func() {
 		vmi := api.NewMinimalVMI("testvmi")
 		Expect(vmi.Spec.Domain.Features).To(BeNil())
-		err := webhooks.SetVirtualMachineInstanceHypervFeatureDependencies(vmi)
+		err := webhooks.SetHypervFeatureDependencies(&vmi.Spec)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(vmi.Spec.Domain.Features).To(BeNil())
 	})
@@ -629,7 +629,7 @@ var _ = Describe("VirtualMachineInstance Mutator", func() {
 		vmi.Spec.Domain.Features = &v1.Features{
 			Hyperv: &v1.FeatureHyperv{},
 		}
-		err := webhooks.SetVirtualMachineInstanceHypervFeatureDependencies(vmi)
+		err := webhooks.SetHypervFeatureDependencies(&vmi.Spec)
 		Expect(err).ToNot(HaveOccurred())
 		hyperv := v1.FeatureHyperv{}
 		ok := equality.Semantic.DeepEqual(*vmi.Spec.Domain.Features.Hyperv, hyperv)
@@ -656,7 +656,7 @@ var _ = Describe("VirtualMachineInstance Mutator", func() {
 				},
 			},
 		}
-		err := webhooks.SetVirtualMachineInstanceHypervFeatureDependencies(vmi)
+		err := webhooks.SetHypervFeatureDependencies(&vmi.Spec)
 		Expect(err).ToNot(HaveOccurred())
 
 		hyperv := v1.FeatureHyperv{
@@ -692,7 +692,7 @@ var _ = Describe("VirtualMachineInstance Mutator", func() {
 				},
 			},
 		}
-		err := webhooks.SetVirtualMachineInstanceHypervFeatureDependencies(vmi)
+		err := webhooks.SetHypervFeatureDependencies(&vmi.Spec)
 		Expect(err).ToNot(HaveOccurred())
 
 		hyperv := v1.FeatureHyperv{
@@ -735,7 +735,7 @@ var _ = Describe("VirtualMachineInstance Mutator", func() {
 				},
 			},
 		}
-		webhooks.SetVirtualMachineInstanceHypervFeatureDependencies(vmi)
+		webhooks.SetHypervFeatureDependencies(&vmi.Spec)
 
 		// we MUST report the error in mutation, but production code is
 		// supposed to ignore it to fulfill the design semantics, see
@@ -879,7 +879,7 @@ var _ = Describe("VirtualMachineInstance Mutator", func() {
 		vmi.Spec.Domain.Features = &v1.Features{
 			Hyperv: hyperv,
 		}
-		err := webhooks.SetVirtualMachineInstanceHypervFeatureDependencies(vmi)
+		err := webhooks.SetHypervFeatureDependencies(&vmi.Spec)
 		Expect(err).ToNot(HaveOccurred(), "it should not fail")
 		if resultCPUTopology == nil {
 			Expect(vmi.Spec.Domain.CPU).To(BeNil(), "cpu topology should not be updated")
diff --git a/pkg/virt-config/virt-config.go b/pkg/virt-config/virt-config.go
index 16de6cea367e..20514cf1454e 100644
--- a/pkg/virt-config/virt-config.go
+++ b/pkg/virt-config/virt-config.go
@@ -169,32 +169,32 @@ func (c *ClusterConfig) GetDefaultNetworkInterface() string {
 	return c.GetConfig().NetworkConfiguration.NetworkInterface
 }
 
-func (c *ClusterConfig) SetVMIDefaultNetworkInterface(vmi *v1.VirtualMachineInstance) error {
-	autoAttach := vmi.Spec.Domain.Devices.AutoattachPodInterface
+func (c *ClusterConfig) SetVMISpecDefaultNetworkInterface(spec *v1.VirtualMachineInstanceSpec) error {
+	autoAttach := spec.Domain.Devices.AutoattachPodInterface
 	if autoAttach != nil && *autoAttach == false {
 		return nil
 	}
 
 	// Override only when nothing is specified
-	if len(vmi.Spec.Networks) == 0 && len(vmi.Spec.Domain.Devices.Interfaces) == 0 {
+	if len(spec.Networks) == 0 && len(spec.Domain.Devices.Interfaces) == 0 {
 		iface := v1.NetworkInterfaceType(c.GetDefaultNetworkInterface())
 		switch iface {
 		case v1.BridgeInterface:
 			if !c.IsBridgeInterfaceOnPodNetworkEnabled() {
 				return fmt.Errorf("Bridge interface is not enabled in kubevirt-config")
 			}
-			vmi.Spec.Domain.Devices.Interfaces = []v1.Interface{*v1.DefaultBridgeNetworkInterface()}
+			spec.Domain.Devices.Interfaces = []v1.Interface{*v1.DefaultBridgeNetworkInterface()}
 		case v1.MasqueradeInterface:
-			vmi.Spec.Domain.Devices.Interfaces = []v1.Interface{*v1.DefaultMasqueradeNetworkInterface()}
+			spec.Domain.Devices.Interfaces = []v1.Interface{*v1.DefaultMasqueradeNetworkInterface()}
 		case v1.SlirpInterface:
 			if !c.IsSlirpInterfaceEnabled() {
 				return fmt.Errorf("Slirp interface is not enabled in kubevirt-config")
 			}
 			defaultIface := v1.DefaultSlirpNetworkInterface()
-			vmi.Spec.Domain.Devices.Interfaces = []v1.Interface{*defaultIface}
+			spec.Domain.Devices.Interfaces = []v1.Interface{*defaultIface}
 		}
 
-		vmi.Spec.Networks = []v1.Network{*v1.DefaultPodNetwork()}
+		spec.Networks = []v1.Network{*v1.DefaultPodNetwork()}
 	}
 	return nil
 }
diff --git a/pkg/virt-controller/watch/vm.go b/pkg/virt-controller/watch/vm.go
index b9b078c000fa..302f954d4445 100644
--- a/pkg/virt-controller/watch/vm.go
+++ b/pkg/virt-controller/watch/vm.go
@@ -901,11 +901,11 @@ func (c *VMController) startVMI(vm *virtv1.VirtualMachine) error {
 		return err
 	}
 
-	util.SetDefaultVolumeDisk(vmi)
+	util.SetDefaultVolumeDisk(&vmi.Spec)
 
 	autoAttachInputDevice(vmi)
 
-	err = c.clusterConfig.SetVMIDefaultNetworkInterface(vmi)
+	err = c.clusterConfig.SetVMISpecDefaultNetworkInterface(&vmi.Spec)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/virt-launcher/virtwrap/converter/converter_test.go b/pkg/virt-launcher/virtwrap/converter/converter_test.go
index 59d16cb385e7..a78d76589a0b 100644
--- a/pkg/virt-launcher/virtwrap/converter/converter_test.go
+++ b/pkg/virt-launcher/virtwrap/converter/converter_test.go
@@ -3505,14 +3505,14 @@ func False() *bool {
 // it needs to run the mutate function before verifying converter
 func vmiArchMutate(arch string, vmi *v1.VirtualMachineInstance, c *ConverterContext) {
 	if arch == "arm64" {
-		webhooks.SetVirtualMachineInstanceArm64Defaults(vmi)
-		// bootloader has been initialized in webhooks.SetVirtualMachineInstanceArm64Defaults,
+		webhooks.SetArm64Defaults(&vmi.Spec)
+		// bootloader has been initialized in webhooks.SetArm64Defaults,
 		// c.EFIConfiguration.SecureLoader is needed in the converter.Convert_v1_VirtualMachineInstance_To_api_Domain.
 		c.EFIConfiguration = &EFIConfiguration{
 			SecureLoader: false,
 		}
 
 	} else {
-		webhooks.SetVirtualMachineInstanceAmd64Defaults(vmi)
+		webhooks.SetAmd64Defaults(&vmi.Spec)
 	}
 }

From e7898af39b54298043adef02ae578d825743120e Mon Sep 17 00:00:00 2001
From: Lee Yarwood <lyarwood@redhat.com>
Date: Mon, 30 Jan 2023 19:10:46 +0000
Subject: [PATCH 3/3] vms-admitter: Set defaults on copy of VirtualMachine
 before validation

With the defaulting code now shared between the webhooks we can apply
these defaults in the VirtualMachine validation webhook before
validating the VirtualMachineInstanceSpec.

This resolves bug #9102 as any requests for guest visible memory are
translated into an appropriate resource request allowing the huge page
validation to succeed.

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
---
 .../webhooks/validating-webhook/admitters/vms-admitter.go    | 5 +++++
 .../validating-webhook/admitters/vms-admitter_test.go        | 5 +----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go
index 9f8ff1b76ac6..abf999b45d6c 100644
--- a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go
+++ b/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go
@@ -108,6 +108,11 @@ func (admitter *VMsAdmitter) Admit(ar *admissionv1.AdmissionReview) *admissionv1
 		return webhookutils.ToAdmissionResponse(causes)
 	}
 
+	// Set VirtualMachine defaults on the copy before validating
+	if err = webhooks.SetDefaultVirtualMachine(admitter.ClusterConfig, vmCopy); err != nil {
+		return webhookutils.ToAdmissionResponseError(err)
+	}
+
 	causes = ValidateVirtualMachineSpec(k8sfield.NewPath("spec"), &vmCopy.Spec, admitter.ClusterConfig, accountName)
 	if len(causes) > 0 {
 		return webhookutils.ToAdmissionResponse(causes)
diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go
index 77dfeb2a1f4c..b023f4172ca7 100644
--- a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go
+++ b/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go
@@ -191,10 +191,7 @@ var _ = Describe("Validating VM Admitter", func() {
 		}
 
 		resp := admitVm(vmsAdmitter, vm)
-		// FIXME - This is bug #9102, the request should be accepted with guest visible memory taken into account
-		Expect(resp.Allowed).To(BeFalse())
-		Expect(resp.Result.Details.Causes).To(HaveLen(1))
-		Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.template.spec.domain.resources.requests.memory"))
+		Expect(resp.Allowed).To(BeTrue())
 	})
 
 	DescribeTable("should reject VolumeRequests on a migrating vm", func(requests []v1.VirtualMachineVolumeRequest) {