set virtio as default disk bus on Arm64

move setting default disk bus from virtwrap/converter to mutator, virtio is the default disk bus for Arm64 while SATA is the default disk bus for others. Signed-off-by: Howard Zhang <[email protected]>
KarstenB · Aug 18, 2022 · cb056d5 · cb056d5
1 parent 417fce4
commit cb056d5
Show file tree

Hide file tree

Showing 5 changed files with 124 additions and 49 deletions.
diff --git a/pkg/virt-api/webhooks/BUILD.bazel b/pkg/virt-api/webhooks/BUILD.bazel
@@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 go_library(
     name = "go_default_library",
     srcs = [
+        "amd64.go",
         "arm64.go",
         "hyperv.go",
         "utils.go",

diff --git a/pkg/virt-api/webhooks/amd64.go b/pkg/virt-api/webhooks/amd64.go
@@ -0,0 +1,52 @@
+/* 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 2021
+ *
+ */
+
+/*
+ * arm64 utilities are in the webhooks package because they are used both
+ * by validation and mutation webhooks.
+ */
+package webhooks
+
+import (
+	v1 "kubevirt.io/api/core/v1"
+)
+
+// setDefaultDisksBus set default Disks Bus
+func setAmd64DefaultDisksBus(vmi *v1.VirtualMachineInstance) {
+	// 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
+
+		if disk.Disk != nil && disk.Disk.Bus == "" {
+			disk.Disk.Bus = bus
+		}
+		if disk.CDRom != nil && disk.CDRom.Bus == "" {
+			disk.CDRom.Bus = bus
+		}
+		if disk.LUN != nil && disk.LUN.Bus == "" {
+			disk.LUN.Bus = bus
+		}
+	}
+}
+
+// SetVirtualMachineInstanceAmd64Defaults is mutating function for mutating-webhook
+func SetVirtualMachineInstanceAmd64Defaults(vmi *v1.VirtualMachineInstance) {
+	setAmd64DefaultDisksBus(vmi)
+}
diff --git a/pkg/virt-api/webhooks/arm64.go b/pkg/virt-api/webhooks/arm64.go
@@ -21,8 +21,6 @@
 package webhooks
 
 import (
-	"fmt"
-
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sfield "k8s.io/apimachinery/pkg/util/validation/field"
 
@@ -35,40 +33,75 @@ const (
 	defaultCPUModel = v1.CPUModeHostPassthrough
 )
 
-// verifyInvalidSetting verify if VMI spec contain unavailable setting for arm64, check following items:
+// ValidateVirtualMachineInstanceArm64Setting is validation function for validating-webhook
 // 1. if setting bios boot
 // 2. if use uefi secure boot
 // 3. if use host-model for cpu model
-func verifyInvalidSetting(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) (metav1.StatusCause, bool) {
+// 4. if not use 'scsi', 'virtio' as disk bus
+func ValidateVirtualMachineInstanceArm64Setting(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) []metav1.StatusCause {
+	var statusCauses []metav1.StatusCause
 	if spec.Domain.Firmware != nil && spec.Domain.Firmware.Bootloader != nil {
 		if spec.Domain.Firmware.Bootloader.BIOS != nil {
-			return metav1.StatusCause{
+			statusCauses = append(statusCauses, metav1.StatusCause{
 				Type:    metav1.CauseTypeFieldValueNotSupported,
 				Message: "Arm64 does not support bios boot, please change to uefi boot",
 				Field:   field.Child("domain", "firmware", "bootloader", "bios").String(),
-			}, false
+			})
 		}
 		if spec.Domain.Firmware.Bootloader.EFI != nil {
 			// When EFI is enable, secureboot is enabled by default, so here check two condition
 			// 1 is EFI is enabled without Secureboot setting
 			// 2 is both EFI and Secureboot enabled
 			if spec.Domain.Firmware.Bootloader.EFI.SecureBoot == nil || (spec.Domain.Firmware.Bootloader.EFI.SecureBoot != nil && *spec.Domain.Firmware.Bootloader.EFI.SecureBoot) {
-				return metav1.StatusCause{
+				statusCauses = append(statusCauses, metav1.StatusCause{
 					Type:    metav1.CauseTypeFieldValueNotSupported,
 					Message: "UEFI secure boot is currently not supported on aarch64 Arch",
 					Field:   field.Child("domain", "firmware", "bootloader", "efi", "secureboot").String(),
-				}, false
+				})
 			}
 		}
 	}
 	if spec.Domain.CPU != nil && (&spec.Domain.CPU.Model != nil) && spec.Domain.CPU.Model == "host-model" {
-		return metav1.StatusCause{
+		statusCauses = append(statusCauses, metav1.StatusCause{
 			Type:    metav1.CauseTypeFieldValueNotSupported,
 			Message: "Arm64 not support host model well",
 			Field:   field.Child("domain", "cpu", "model").String(),
-		}, false
+		})
+	}
+	if spec.Domain.Devices.Disks != nil {
+		// checkIfBusAvailable: if bus type is nil, virtio, scsi return true, otherwise, return false
+		checkIfBusAvailable := func(bus v1.DiskBus) bool {
+			if bus == "" || bus == v1.DiskBusVirtio || bus == v1.DiskBusSCSI {
+				return true
+			}
+			return false
+		}
+
+		for i, disk := range spec.Domain.Devices.Disks {
+			if disk.Disk != nil && !checkIfBusAvailable(disk.Disk.Bus) {
+				statusCauses = append(statusCauses, metav1.StatusCause{
+					Type:    metav1.CauseTypeFieldValueNotSupported,
+					Message: "Arm64 not support this disk bus type, please use virtio or scsi",
+					Field:   field.Child("domain", "devices", "disks").Index(i).Child("disk", "bus").String(),
+				})
+			}
+			if disk.CDRom != nil && !checkIfBusAvailable(disk.CDRom.Bus) {
+				statusCauses = append(statusCauses, metav1.StatusCause{
+					Type:    metav1.CauseTypeFieldValueNotSupported,
+					Message: "Arm64 not support this disk bus type, please use virtio or scsi",
+					Field:   field.Child("domain", "devices", "disks").Index(i).Child("cdrom", "bus").String(),
+				})
+			}
+			if disk.LUN != nil && !checkIfBusAvailable(disk.LUN.Bus) {
+				statusCauses = append(statusCauses, metav1.StatusCause{
+					Type:    metav1.CauseTypeFieldValueNotSupported,
+					Message: "Arm64 not support this disk bus type, please use virtio or scsi",
+					Field:   field.Child("domain", "devices", "disks").Index(i).Child("lun", "bus").String(),
+				})
+			}
+		}
 	}
-	return metav1.StatusCause{}, true
+	return statusCauses
 }
 
 // setDefaultCPUModel set default cpu model to host-passthrough
@@ -77,7 +110,9 @@ func setDefaultCPUModel(vmi *v1.VirtualMachineInstance) {
 		vmi.Spec.Domain.CPU = &v1.CPU{}
 	}
 
-	vmi.Spec.Domain.CPU.Model = defaultCPUModel
+	if vmi.Spec.Domain.CPU.Model == "" {
+		vmi.Spec.Domain.CPU.Model = defaultCPUModel
+	}
 }
 
 // setDefaultBootloader set default bootloader to uefi boot
@@ -94,23 +129,29 @@ func setDefaultBootloader(vmi *v1.VirtualMachineInstance) {
 	}
 }
 
-// ValidateVirtualMachineInstanceArm64Setting is validation function for validating-webhook
-func ValidateVirtualMachineInstanceArm64Setting(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) []metav1.StatusCause {
-	var causes []metav1.StatusCause
-	if cause, ok := verifyInvalidSetting(field, spec); !ok {
-		causes = append(causes, cause)
+// setDefaultDisksBus set default Disks Bus, because sata is not supported by qemu-kvm of Arm64
+func setDefaultDisksBus(vmi *v1.VirtualMachineInstance) {
+	bus := v1.DiskBusVirtio
+
+	for i := range vmi.Spec.Domain.Devices.Disks {
+		disk := &vmi.Spec.Domain.Devices.Disks[i].DiskDevice
+
+		if disk.Disk != nil && disk.Disk.Bus == "" {
+			disk.Disk.Bus = bus
+		}
+		if disk.CDRom != nil && disk.CDRom.Bus == "" {
+			disk.CDRom.Bus = bus
+		}
+		if disk.LUN != nil && disk.LUN.Bus == "" {
+			disk.LUN.Bus = bus
+		}
 	}
-	return causes
+
 }
 
 // SetVirtualMachineInstanceArm64Defaults is mutating function for mutating-webhook
-func SetVirtualMachineInstanceArm64Defaults(vmi *v1.VirtualMachineInstance) error {
-	path := k8sfield.NewPath("spec")
-	if cause, ok := verifyInvalidSetting(path, &vmi.Spec); ok {
-		setDefaultCPUModel(vmi)
-		setDefaultBootloader(vmi)
-	} else {
-		return fmt.Errorf("%s", cause.Message)
-	}
-	return nil
+func SetVirtualMachineInstanceArm64Defaults(vmi *v1.VirtualMachineInstance) {
+	setDefaultCPUModel(vmi)
+	setDefaultBootloader(vmi)
+	setDefaultDisksBus(vmi)
 }
diff --git a/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go b/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go
@@ -108,15 +108,12 @@ func (mutator *VMIsMutator) Mutate(ar *admissionv1.AdmissionReview) *admissionv1
 			log.Log.V(2).Infof("Failed to set HyperV dependencies: %s", err)
 		}
 
-		// Do some specific setting for Arm64 Arch. It should put before SetObjectDefaults_VirtualMachineInstance
+		// Do some CPU arch specific setting.
 		if webhooks.IsARM64() {
 			log.Log.V(4).Info("Apply Arm64 specific setting")
-			err = webhooks.SetVirtualMachineInstanceArm64Defaults(newVMI)
-			if err != nil {
-				// if SetVirtualMachineInstanceArm64Defaults fails, it's due to a validation error, which will get caught in the validation webhook after mutation finishes.
-				log.Log.V(2).Infof("Failed to setting for Arm64: %s", err)
-			}
+			webhooks.SetVirtualMachineInstanceArm64Defaults(newVMI)
 		} else {
+			webhooks.SetVirtualMachineInstanceAmd64Defaults(newVMI)
 			mutator.setDefaultCPUModel(newVMI)
 		}
 		if newVMI.IsRealtimeEnabled() {

diff --git a/staging/src/kubevirt.io/api/core/v1/defaults.go b/staging/src/kubevirt.io/api/core/v1/defaults.go
@@ -125,25 +125,9 @@ func SetDefaults_VirtualMachineInstance(obj *VirtualMachineInstance) {
 }
 
 func setDefaults_Disk(obj *VirtualMachineInstance) {
-	// 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 := DiskBusSATA
-
 	for i := range obj.Spec.Domain.Devices.Disks {
 		disk := &obj.Spec.Domain.Devices.Disks[i].DiskDevice
-
 		SetDefaults_DiskDevice(disk)
-
-		if disk.Disk != nil && disk.Disk.Bus == "" {
-			disk.Disk.Bus = bus
-		}
-		if disk.CDRom != nil && disk.CDRom.Bus == "" {
-			disk.CDRom.Bus = bus
-		}
-		if disk.LUN != nil && disk.LUN.Bus == "" {
-			disk.LUN.Bus = bus
-		}
 	}
 }