Merge pull request kubevirt#10058 from alicefr/configure-error-policy

Configure error policy for disks on IO errors

api/openapi-spec/swagger.json

@@ -16566,6 +16566,10 @@
       "description": "Attach a volume as a disk to the vmi.",
       "$ref": "#/definitions/v1.DiskTarget"
      },
+     "errorPolicy": {
+      "description": "If specified, it can change the default error policy (stop) for the disk",
+      "type": "string"
+     },
      "io": {
       "description": "IO specifies which QEMU disk IO mode should be used. Supported values are: native, default, threads.",
       "type": "string"

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

@@ -2493,6 +2493,15 @@ func validateDisks(field *k8sfield.Path, disks []v1.Disk) []metav1.StatusCause {
 			})
 		}
 
+		// Verify if error_policy is valid
+		if disk.ErrorPolicy != nil && *disk.ErrorPolicy != v1.DiskErrorPolicyStop && *disk.ErrorPolicy != v1.DiskErrorPolicyIgnore && *disk.ErrorPolicy != v1.DiskErrorPolicyReport && *disk.ErrorPolicy != v1.DiskErrorPolicyEnospace {
+			causes = append(causes, metav1.StatusCause{
+				Type:    metav1.CauseTypeFieldValueInvalid,
+				Message: fmt.Sprintf("%s has invalid value \"%s\"", field.Index(idx).Child("errorPolicy").String(), *disk.ErrorPolicy),
+				Field:   field.Index(idx).Child("errorPolicy").String(),
+			})
+		}
+
 		// Verify disk and volume name can be a valid container name since disk
 		// name can become a container name which will fail to schedule if invalid
 		errs := validation.IsDNS1123Label(disk.Name)

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go

@@ -45,6 +45,7 @@ import (
 	v1 "kubevirt.io/api/core/v1"
 
 	"kubevirt.io/kubevirt/pkg/hooks"
+	kubevirtpointer "kubevirt.io/kubevirt/pkg/pointer"
 	"kubevirt.io/kubevirt/pkg/testutils"
 	"kubevirt.io/kubevirt/pkg/virt-api/webhooks"
 	virtconfig "kubevirt.io/kubevirt/pkg/virt-config"
@@ -3165,6 +3166,37 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 			Entry("writeback", v1.CacheWriteBack),
 		)
 
+		DescribeTable("should reject disk with invalid errorPolicy", func(policy string) {
+			vmi := api.NewMinimalVMI("testvmi")
+			vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{
+				Name: "testdisk", ErrorPolicy: kubevirtpointer.P(v1.DiskErrorPolicy(policy)), DiskDevice: v1.DiskDevice{
+					Disk: &v1.DiskTarget{}}})
+
+			causes := validateDisks(k8sfield.NewPath("fake"), vmi.Spec.Domain.Devices.Disks)
+			Expect(causes).To(HaveLen(1))
+			Expect(string(causes[0].Type)).To(Equal("FieldValueInvalid"))
+			Expect(causes[0].Field).To(Equal("fake[0].errorPolicy"))
+			Expect(causes[0].Message).To(Equal(fmt.Sprintf("fake[0].errorPolicy has invalid value \"%s\"", policy)))
+		},
+			Entry("with arbitrary string", "unsupported"),
+			Entry("with empty string", ""),
+		)
+
+		DescribeTable("It should accept a disk with a valid errorPolicy", func(mode v1.DiskErrorPolicy) {
+			vmi := api.NewMinimalVMI("testvmi")
+			vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{
+				Name: "testdisk", ErrorPolicy: kubevirtpointer.P(mode), DiskDevice: v1.DiskDevice{
+					Disk: &v1.DiskTarget{}}})
+
+			causes := validateDisks(k8sfield.NewPath("fake"), vmi.Spec.Domain.Devices.Disks)
+			Expect(causes).To(BeEmpty())
+		},
+			Entry("stop", v1.DiskErrorPolicyStop),
+			Entry("report", v1.DiskErrorPolicyReport),
+			Entry("ignore", v1.DiskErrorPolicyIgnore),
+			Entry("enospace", v1.DiskErrorPolicyEnospace),
+		)
+
 		It("should reject invalid SN characters", func() {
 			vmi := api.NewMinimalVMI("testvmi")
 			order := uint(1)

pkg/virt-launcher/virtwrap/api/schema.go

@@ -657,15 +657,15 @@ type DiskTarget struct {
 }
 
 type DiskDriver struct {
-	Cache       string      `xml:"cache,attr,omitempty"`
-	ErrorPolicy string      `xml:"error_policy,attr,omitempty"`
-	IO          v1.DriverIO `xml:"io,attr,omitempty"`
-	Name        string      `xml:"name,attr"`
-	Type        string      `xml:"type,attr"`
-	IOThread    *uint       `xml:"iothread,attr,omitempty"`
-	Queues      *uint       `xml:"queues,attr,omitempty"`
-	Discard     string      `xml:"discard,attr,omitempty"`
-	IOMMU       string      `xml:"iommu,attr,omitempty"`
+	Cache       string             `xml:"cache,attr,omitempty"`
+	ErrorPolicy v1.DiskErrorPolicy `xml:"error_policy,attr,omitempty"`
+	IO          v1.DriverIO        `xml:"io,attr,omitempty"`
+	Name        string             `xml:"name,attr"`
+	Type        string             `xml:"type,attr"`
+	IOThread    *uint              `xml:"iothread,attr,omitempty"`
+	Queues      *uint              `xml:"queues,attr,omitempty"`
+	Discard     string             `xml:"discard,attr,omitempty"`
+	IOMMU       string             `xml:"iommu,attr,omitempty"`
 }
 
 type DiskSourceHost struct {

pkg/virt-launcher/virtwrap/converter/BUILD.bazel

@@ -50,6 +50,7 @@ go_test(
     deps = [
         "//pkg/ephemeral-disk/fake:go_default_library",
         "//pkg/handler-launcher-com/cmd/v1:go_default_library",
+        "//pkg/pointer:go_default_library",
         "//pkg/testutils:go_default_library",
         "//pkg/virt-api/webhooks:go_default_library",
         "//pkg/virt-controller/services:go_default_library",

pkg/virt-launcher/virtwrap/converter/converter.go

@@ -209,10 +209,9 @@ func Convert_v1_Disk_To_api_Disk(c *ConverterContext, diskDevice *v1.Disk, disk
 		}
 	}
 	disk.Driver = &api.DiskDriver{
-		Name:        "qemu",
-		Cache:       string(diskDevice.Cache),
-		IO:          diskDevice.IO,
-		ErrorPolicy: "stop",
+		Name:  "qemu",
+		Cache: string(diskDevice.Cache),
+		IO:    diskDevice.IO,
 	}
 	if diskDevice.Disk != nil || diskDevice.LUN != nil {
 		if !contains(c.VolumesDiscardIgnore, diskDevice.Name) {
@@ -283,6 +282,20 @@ func setReservation(disk *api.Disk) {
 	}
 }
 
+func setErrorPolicy(diskDevice *v1.Disk, disk *api.Disk) error {
+	if diskDevice.ErrorPolicy == nil {
+		disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
+		return nil
+	}
+	switch *diskDevice.ErrorPolicy {
+	case v1.DiskErrorPolicyStop, v1.DiskErrorPolicyIgnore, v1.DiskErrorPolicyReport, v1.DiskErrorPolicyEnospace:
+		disk.Driver.ErrorPolicy = *diskDevice.ErrorPolicy
+	default:
+		return fmt.Errorf("error policy %s not recognized", *diskDevice.ErrorPolicy)
+	}
+	return nil
+}
+
 type DirectIOChecker interface {
 	CheckBlockDevice(path string) (bool, error)
 	CheckFile(path string) (bool, error)
@@ -661,7 +674,7 @@ func Convert_v1_Hotplug_Volume_To_api_Disk(source *v1.Volume, disk *api.Disk, c
 func Convert_v1_Config_To_api_Disk(volumeName string, disk *api.Disk, configType config.Type) error {
 	disk.Type = "file"
 	disk.Driver.Type = "raw"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	switch configType {
 	case config.ConfigMap:
 		disk.Source.File = config.GetConfigMapDiskPath(volumeName)
@@ -734,7 +747,7 @@ func Convert_v1_Hotplug_DataVolume_To_api_Disk(name string, disk *api.Disk, c *C
 func Convert_v1_FilesystemVolumeSource_To_api_Disk(volumeName string, disk *api.Disk, volumesDiscardIgnore []string) error {
 	disk.Type = "file"
 	disk.Driver.Type = "raw"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	disk.Source.File = GetFilesystemVolumePath(volumeName)
 	if !contains(volumesDiscardIgnore, volumeName) {
 		disk.Driver.Discard = "unmap"
@@ -746,7 +759,7 @@ func Convert_v1_FilesystemVolumeSource_To_api_Disk(volumeName string, disk *api.
 func Convert_v1_Hotplug_FilesystemVolumeSource_To_api_Disk(volumeName string, disk *api.Disk, volumesDiscardIgnore []string) error {
 	disk.Type = "file"
 	disk.Driver.Type = "raw"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	if !contains(volumesDiscardIgnore, volumeName) {
 		disk.Driver.Discard = "unmap"
 	}
@@ -757,7 +770,7 @@ func Convert_v1_Hotplug_FilesystemVolumeSource_To_api_Disk(volumeName string, di
 func Convert_v1_BlockVolumeSource_To_api_Disk(volumeName string, disk *api.Disk, volumesDiscardIgnore []string) error {
 	disk.Type = "block"
 	disk.Driver.Type = "raw"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	if !contains(volumesDiscardIgnore, volumeName) {
 		disk.Driver.Discard = "unmap"
 	}
@@ -770,7 +783,7 @@ func Convert_v1_BlockVolumeSource_To_api_Disk(volumeName string, disk *api.Disk,
 func Convert_v1_Hotplug_BlockVolumeSource_To_api_Disk(volumeName string, disk *api.Disk, volumesDiscardIgnore []string) error {
 	disk.Type = "block"
 	disk.Driver.Type = "raw"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	if !contains(volumesDiscardIgnore, volumeName) {
 		disk.Driver.Discard = "unmap"
 	}
@@ -781,7 +794,7 @@ func Convert_v1_Hotplug_BlockVolumeSource_To_api_Disk(volumeName string, disk *a
 func Convert_v1_HostDisk_To_api_Disk(volumeName string, path string, disk *api.Disk) error {
 	disk.Type = "file"
 	disk.Driver.Type = "raw"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	disk.Source.File = hostdisk.GetMountedHostDiskPath(volumeName, path)
 	return nil
 }
@@ -814,7 +827,7 @@ func Convert_v1_CloudInitSource_To_api_Disk(source v1.VolumeSource, disk *api.Di
 	disk.Source.File = cloudinit.GetIsoFilePath(dataSource, c.VirtualMachine.Name, c.VirtualMachine.Namespace)
 	disk.Type = "file"
 	disk.Driver.Type = "raw"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	return nil
 }
 
@@ -842,7 +855,7 @@ func Convert_v1_EmptyDiskSource_To_api_Disk(volumeName string, _ *v1.EmptyDiskSo
 	disk.Driver.Type = "qcow2"
 	disk.Driver.Discard = "unmap"
 	disk.Source.File = emptydisk.NewEmptyDiskCreator().FilePathForVolumeName(volumeName)
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 
 	return nil
 }
@@ -853,7 +866,7 @@ func Convert_v1_ContainerDiskSource_To_api_Disk(volumeName string, _ *v1.Contain
 	}
 	disk.Type = "file"
 	disk.Driver.Type = "qcow2"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	disk.Driver.Discard = "unmap"
 	disk.Source.File = c.EphemeraldiskCreator.GetFilePath(volumeName)
 	disk.BackingStore = &api.BackingStore{
@@ -876,7 +889,7 @@ func Convert_v1_ContainerDiskSource_To_api_Disk(volumeName string, _ *v1.Contain
 func Convert_v1_EphemeralVolumeSource_To_api_Disk(volumeName string, disk *api.Disk, c *ConverterContext) error {
 	disk.Type = "file"
 	disk.Driver.Type = "qcow2"
-	disk.Driver.ErrorPolicy = "stop"
+	disk.Driver.ErrorPolicy = v1.DiskErrorPolicyStop
 	disk.Driver.Discard = "unmap"
 	disk.Source.File = c.EphemeraldiskCreator.GetFilePath(volumeName)
 	disk.BackingStore = &api.BackingStore{
@@ -1572,6 +1585,9 @@ func Convert_v1_VirtualMachineInstance_To_api_Domain(vmi *v1.VirtualMachineInsta
 		if _, ok := c.PermanentVolumes[disk.Name]; ok || len(c.PermanentVolumes) == 0 || (hpOk && (hpStatus.Phase == v1.HotplugVolumeMounted || hpStatus.Phase == v1.VolumeReady)) {
 			domain.Spec.Devices.Disks = append(domain.Spec.Devices.Disks, newDisk)
 		}
+		if err := setErrorPolicy(&disk, &newDisk); err != nil {
+			return err
+		}
 	}
 	// Handle virtioFS
 	domain.Spec.Devices.Filesystems = append(domain.Spec.Devices.Filesystems, convertFileSystems(vmi.Spec.Domain.Devices.Filesystems)...)

pkg/virt-launcher/virtwrap/converter/converter_test.go

@@ -52,6 +52,7 @@ import (
 	kvapi "kubevirt.io/client-go/api"
 
 	cmdv1 "kubevirt.io/kubevirt/pkg/handler-launcher-com/cmd/v1"
+	kubevirtpointer "kubevirt.io/kubevirt/pkg/pointer"
 	sev "kubevirt.io/kubevirt/pkg/virt-launcher/virtwrap/launchsecurity"
 )
 
@@ -152,7 +153,7 @@ var _ = Describe("Converter", func() {
 			var convertedDisk = `<Disk device="disk" type="" model="virtio-non-transitional">
   <source></source>
   <target bus="virtio" dev="vda"></target>
-  <driver error_policy="stop" name="qemu" type="" discard="unmap"></driver>
+  <driver name="qemu" type="" discard="unmap"></driver>
   <alias name="ua-mydisk"></alias>
   <boot order="1"></boot>
 </Disk>`
@@ -168,7 +169,7 @@ var _ = Describe("Converter", func() {
 			expectedXML := `<Disk device="" type="">
   <source></source>
   <target></target>
-  <driver error_policy="stop" io="native" name="qemu" type=""></driver>
+  <driver io="native" name="qemu" type=""></driver>
   <alias name="ua-"></alias>
 </Disk>`
 			Expect(xml).To(Equal(expectedXML))
@@ -180,7 +181,7 @@ var _ = Describe("Converter", func() {
 			expectedXML := `<Disk device="" type="">
   <source></source>
   <target></target>
-  <driver error_policy="stop" name="qemu" type=""></driver>
+  <driver name="qemu" type=""></driver>
   <alias name="ua-"></alias>
 </Disk>`
 			Expect(xml).To(Equal(expectedXML))
@@ -198,7 +199,7 @@ var _ = Describe("Converter", func() {
 			var convertedDisk = `<Disk device="disk" type="" model="virtio-non-transitional">
   <source></source>
   <target bus="virtio" dev="vda"></target>
-  <driver error_policy="stop" name="qemu" type="" discard="unmap"></driver>
+  <driver name="qemu" type="" discard="unmap"></driver>
   <alias name="ua-mydisk"></alias>
 </Disk>`
 			xml := diskToDiskXML(kubevirtDisk)
@@ -239,7 +240,7 @@ var _ = Describe("Converter", func() {
 			var expectedXML = `<Disk device="disk" type="" model="virtio-non-transitional">
   <source></source>
   <target bus="virtio" dev="vda"></target>
-  <driver cache="none" error_policy="stop" name="qemu" type="" discard="unmap"></driver>
+  <driver cache="none" name="qemu" type="" discard="unmap"></driver>
   <alias name="ua-mydisk"></alias>
   <shareable></shareable>
 </Disk>`
@@ -1360,6 +1361,7 @@ var _ = Describe("Converter", func() {
 		isBlockPVCMap["pvc_block_test"] = true
 		isBlockDVMap := make(map[string]bool)
 		isBlockDVMap["dv_block_test"] = true
+
 		BeforeEach(func() {
 			c = &ConverterContext{
 				VirtualMachine: vmi,
@@ -2024,6 +2026,35 @@ var _ = Describe("Converter", func() {
 			Entry("use virtio transitional", true),
 			Entry("use virtio non-transitional", false),
 		)
+		DescribeTable("Should set the error policy", func(epolicy *v1.DiskErrorPolicy, expected string) {
+			vmi.Spec.Domain.Devices.Disks[0] = v1.Disk{
+				Name: "mydisk",
+				DiskDevice: v1.DiskDevice{
+					Disk: &v1.DiskTarget{
+						Bus: v1.VirtIO,
+					},
+				},
+				ErrorPolicy: epolicy,
+			}
+			vmi.Spec.Volumes[0] = v1.Volume{
+				Name: "mydisk",
+				VolumeSource: v1.VolumeSource{
+					Ephemeral: &v1.EphemeralVolumeSource{
+						PersistentVolumeClaim: &k8sv1.PersistentVolumeClaimVolumeSource{
+							ClaimName: "testclaim",
+						},
+					},
+				},
+			}
+			domainSpec := vmiToDomainXMLToDomainSpec(vmi, c)
+			Expect(string(domainSpec.Devices.Disks[0].Driver.ErrorPolicy)).To(Equal(expected))
+		},
+			Entry("ErrorPolicy not specified", nil, "stop"),
+			Entry("ErrorPolicy equal to stop", kubevirtpointer.P(v1.DiskErrorPolicyStop), "stop"),
+			Entry("ErrorPolicy equal to ignore", kubevirtpointer.P(v1.DiskErrorPolicyIgnore), "ignore"),
+			Entry("ErrorPolicy equal to report", kubevirtpointer.P(v1.DiskErrorPolicyReport), "report"),
+			Entry("ErrorPolicy equal to enospace", kubevirtpointer.P(v1.DiskErrorPolicyEnospace), "enospace"),
+		)
 
 	})
 	Context("Network convert", func() {