From 7fbe61db7b02e2e30e2c5d39b708fc61a96115e1 Mon Sep 17 00:00:00 2001
From: Maya Rashish <mrashish@redhat.com>
Date: Sun, 9 Jan 2022 16:55:20 +0200
Subject: [PATCH 1/3] Limit ourselves to expanding to
 min(spec.request,status.capacity)

With storage classes like NFS, HPP and local storage, the listed
status.capacity is the full underlying file system.
Using all of it for a single PVC is unexpected.

Don't expand beyond what we requested for the PVC in this scenario.

Signed-off-by: Maya Rashish <mrashish@redhat.com>
---
 api/openapi-spec/swagger.json                 |  9 ++++-
 pkg/virt-controller/watch/vmi.go              |  1 +
 pkg/virt-launcher/virtwrap/api/BUILD.bazel    |  1 -
 .../virtwrap/api/deepcopy_generated.go        |  4 +-
 pkg/virt-launcher/virtwrap/api/schema.go      | 37 +++++++++---------
 .../virtwrap/converter/converter.go           | 38 +++++++++++++++++--
 .../virtwrap/converter/converter_test.go      | 30 +++++++++++++++
 pkg/virt-launcher/virtwrap/manager.go         | 14 ++-----
 pkg/virt-launcher/virtwrap/manager_test.go    | 18 +++------
 .../components/validations_generated.go       | 12 +++++-
 .../api/core/v1/deepcopy_generated.go         |  7 ++++
 staging/src/kubevirt.io/api/core/v1/types.go  |  6 ++-
 .../api/core/v1/types_swagger_generated.go    |  3 +-
 .../client-go/api/openapi_generated.go        | 16 +++++++-
 14 files changed, 143 insertions(+), 53 deletions(-)

diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
index 059056118..cd9070cda 100644
--- a/api/openapi-spec/swagger.json
+++ b/api/openapi-spec/swagger.json
@@ -14448,7 +14448,7 @@
       "x-kubernetes-list-type": "atomic"
      },
      "capacity": {
-      "description": "Capacity represents the capacity set on the corresponding PVC spec",
+      "description": "Capacity represents the capacity set on the corresponding PVC status",
       "type": "object",
       "additionalProperties": {
        "$ref": "#/definitions/k8s.io.apimachinery.pkg.api.resource.Quantity"
@@ -14462,6 +14462,13 @@
       "description": "Preallocated indicates if the PVC's storage is preallocated or not",
       "type": "boolean"
      },
+     "requests": {
+      "description": "Requests represents the resources requested by the corresponding PVC spec",
+      "type": "object",
+      "additionalProperties": {
+       "$ref": "#/definitions/k8s.io.apimachinery.pkg.api.resource.Quantity"
+      }
+     },
      "volumeMode": {
       "description": "VolumeMode defines what type of volume is required by the claim. Value of Filesystem is implied when not included in claim spec.",
       "type": "string"
diff --git a/pkg/virt-controller/watch/vmi.go b/pkg/virt-controller/watch/vmi.go
index ba7b72a0c..a3fca9ab5 100644
--- a/pkg/virt-controller/watch/vmi.go
+++ b/pkg/virt-controller/watch/vmi.go
@@ -2056,6 +2056,7 @@ func (c *VMIController) updateVolumeStatus(vmi *virtv1.VirtualMachineInstance, v
 					AccessModes:  pvc.Spec.AccessModes,
 					VolumeMode:   pvc.Spec.VolumeMode,
 					Capacity:     pvc.Status.Capacity,
+					Requests:     pvc.Spec.Resources.Requests,
 					Preallocated: kubevirttypes.IsPreallocated(pvc.ObjectMeta.Annotations),
 				}
 				filesystemOverhead, err := c.getFilesystemOverhead(pvc)
diff --git a/pkg/virt-launcher/virtwrap/api/BUILD.bazel b/pkg/virt-launcher/virtwrap/api/BUILD.bazel
index 97d593a38..f95c4dbe9 100644
--- a/pkg/virt-launcher/virtwrap/api/BUILD.bazel
+++ b/pkg/virt-launcher/virtwrap/api/BUILD.bazel
@@ -15,7 +15,6 @@ go_library(
         "//staging/src/kubevirt.io/client-go/precond:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/meta: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/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
diff --git a/pkg/virt-launcher/virtwrap/api/deepcopy_generated.go b/pkg/virt-launcher/virtwrap/api/deepcopy_generated.go
index 877cb9c14..f4f5a0092 100644
--- a/pkg/virt-launcher/virtwrap/api/deepcopy_generated.go
+++ b/pkg/virt-launcher/virtwrap/api/deepcopy_generated.go
@@ -803,8 +803,8 @@ func (in *Disk) DeepCopyInto(out *Disk) {
 	}
 	if in.Capacity != nil {
 		in, out := &in.Capacity, &out.Capacity
-		x := (*in).DeepCopy()
-		*out = &x
+		*out = new(int64)
+		**out = **in
 	}
 	return
 }
diff --git a/pkg/virt-launcher/virtwrap/api/schema.go b/pkg/virt-launcher/virtwrap/api/schema.go
index 34e29a954..539e78c5c 100644
--- a/pkg/virt-launcher/virtwrap/api/schema.go
+++ b/pkg/virt-launcher/virtwrap/api/schema.go
@@ -27,7 +27,6 @@ import (
 
 	kubev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
-	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
@@ -568,24 +567,24 @@ type ControllerDriver struct {
 // BEGIN Disk -----------------------------
 
 type Disk struct {
-	Device             string             `xml:"device,attr"`
-	Snapshot           string             `xml:"snapshot,attr,omitempty"`
-	Type               string             `xml:"type,attr"`
-	Source             DiskSource         `xml:"source"`
-	Target             DiskTarget         `xml:"target"`
-	Serial             string             `xml:"serial,omitempty"`
-	Driver             *DiskDriver        `xml:"driver,omitempty"`
-	ReadOnly           *ReadOnly          `xml:"readonly,omitempty"`
-	Auth               *DiskAuth          `xml:"auth,omitempty"`
-	Alias              *Alias             `xml:"alias,omitempty"`
-	BackingStore       *BackingStore      `xml:"backingStore,omitempty"`
-	BootOrder          *BootOrder         `xml:"boot,omitempty"`
-	Address            *Address           `xml:"address,omitempty"`
-	Model              string             `xml:"model,attr,omitempty"`
-	BlockIO            *BlockIO           `xml:"blockio,omitempty"`
-	FilesystemOverhead *cdiv1.Percent     `xml:"filesystemOverhead,omitempty"`
-	Capacity           *resource.Quantity `xml:"capacity,omitempty"`
-	ExpandDisksEnabled bool               `xml:"expandDisksEnabled,omitempty"`
+	Device             string         `xml:"device,attr"`
+	Snapshot           string         `xml:"snapshot,attr,omitempty"`
+	Type               string         `xml:"type,attr"`
+	Source             DiskSource     `xml:"source"`
+	Target             DiskTarget     `xml:"target"`
+	Serial             string         `xml:"serial,omitempty"`
+	Driver             *DiskDriver    `xml:"driver,omitempty"`
+	ReadOnly           *ReadOnly      `xml:"readonly,omitempty"`
+	Auth               *DiskAuth      `xml:"auth,omitempty"`
+	Alias              *Alias         `xml:"alias,omitempty"`
+	BackingStore       *BackingStore  `xml:"backingStore,omitempty"`
+	BootOrder          *BootOrder     `xml:"boot,omitempty"`
+	Address            *Address       `xml:"address,omitempty"`
+	Model              string         `xml:"model,attr,omitempty"`
+	BlockIO            *BlockIO       `xml:"blockio,omitempty"`
+	FilesystemOverhead *cdiv1.Percent `xml:"filesystemOverhead,omitempty"`
+	Capacity           *int64         `xml:"capacity,omitempty"`
+	ExpandDisksEnabled bool           `xml:"expandDisksEnabled,omitempty"`
 }
 
 type DiskAuth struct {
diff --git a/pkg/virt-launcher/virtwrap/converter/converter.go b/pkg/virt-launcher/virtwrap/converter/converter.go
index 9330364f4..2e6ea10d4 100644
--- a/pkg/virt-launcher/virtwrap/converter/converter.go
+++ b/pkg/virt-launcher/virtwrap/converter/converter.go
@@ -224,10 +224,7 @@ func Convert_v1_Disk_To_api_Disk(c *ConverterContext, diskDevice *v1.Disk, disk
 		volumeStatus, ok := volumeStatusMap[diskDevice.Name]
 		if ok && volumeStatus.PersistentVolumeClaimInfo != nil {
 			disk.FilesystemOverhead = volumeStatus.PersistentVolumeClaimInfo.FilesystemOverhead
-			capacity, ok := volumeStatus.PersistentVolumeClaimInfo.Capacity[k8sv1.ResourceStorage]
-			if ok {
-				disk.Capacity = &capacity
-			}
+			disk.Capacity = getDiskCapacity(volumeStatus.PersistentVolumeClaimInfo)
 		}
 		disk.ExpandDisksEnabled = c.ExpandDisksEnabled
 	}
@@ -245,6 +242,39 @@ func Convert_v1_Disk_To_api_Disk(c *ConverterContext, diskDevice *v1.Disk, disk
 	return nil
 }
 
+// Get expected disk capacity - a minimum between the request and the PVC capacity.
+// Returns nil when we have insufficient data to calculate this minimum.
+func getDiskCapacity(pvcInfo *v1.PersistentVolumeClaimInfo) *int64 {
+	logger := log.DefaultLogger()
+	storageCapacityResource, ok := pvcInfo.Capacity[k8sv1.ResourceStorage]
+	if !ok {
+		return nil
+	}
+	storageCapacity, ok := storageCapacityResource.AsInt64()
+	if !ok {
+		logger.Infof("Failed to convert storage capacity %+v to int64", storageCapacityResource)
+		return nil
+	}
+	storageRequestResource, ok := pvcInfo.Requests[k8sv1.ResourceStorage]
+	if !ok {
+		return nil
+	}
+	storageRequest, ok := storageRequestResource.AsInt64()
+	if !ok {
+		logger.Infof("Failed to convert storage request %+v to int64", storageRequestResource)
+		return nil
+	}
+	preferredSize := min(storageRequest, storageCapacity)
+	return &preferredSize
+}
+
+func min(one, two int64) int64 {
+	if one < two {
+		return one
+	}
+	return two
+}
+
 type DirectIOChecker interface {
 	CheckBlockDevice(path string) (bool, error)
 	CheckFile(path string) (bool, error)
diff --git a/pkg/virt-launcher/virtwrap/converter/converter_test.go b/pkg/virt-launcher/virtwrap/converter/converter_test.go
index f1c9e52e0..181cfc789 100644
--- a/pkg/virt-launcher/virtwrap/converter/converter_test.go
+++ b/pkg/virt-launcher/virtwrap/converter/converter_test.go
@@ -103,6 +103,36 @@ var _ = Describe("Converter", func() {
 	})
 
 	Context("with v1.Disk", func() {
+		table.DescribeTable("Should define disk capacity as the minimum of capacity and request", func(requests, capacity int64) {
+			context := &ConverterContext{}
+			v1Disk := v1.Disk{
+				Name: "myvolume",
+				DiskDevice: v1.DiskDevice{
+					Disk: &v1.DiskTarget{Bus: "virtio"},
+				},
+			}
+			apiDisk := api.Disk{}
+			devicePerBus := map[string]deviceNamer{}
+			numQueues := uint(2)
+			volumeStatusMap := make(map[string]v1.VolumeStatus)
+			volumeStatusMap["myvolume"] = v1.VolumeStatus{
+				PersistentVolumeClaimInfo: &v1.PersistentVolumeClaimInfo{
+					Capacity: k8sv1.ResourceList{
+						k8sv1.ResourceStorage: *resource.NewQuantity(capacity, resource.DecimalSI),
+					},
+					Requests: k8sv1.ResourceList{
+						k8sv1.ResourceStorage: *resource.NewQuantity(requests, resource.DecimalSI),
+					},
+				},
+			}
+			Convert_v1_Disk_To_api_Disk(context, &v1Disk, &apiDisk, devicePerBus, &numQueues, volumeStatusMap)
+			Expect(apiDisk.Capacity).ToNot(BeNil())
+			Expect(*apiDisk.Capacity).To(Equal(min(capacity, requests)))
+		},
+			table.Entry("Higher request than capacity", int64(9999), int64(1111)),
+			table.Entry("Lower request than capacity", int64(1111), int64(9999)),
+		)
+
 		It("Should add boot order when provided", func() {
 			order := uint(1)
 			kubevirtDisk := &v1.Disk{
diff --git a/pkg/virt-launcher/virtwrap/manager.go b/pkg/virt-launcher/virtwrap/manager.go
index 2dc8ab0cb..b81bc6c84 100644
--- a/pkg/virt-launcher/virtwrap/manager.go
+++ b/pkg/virt-launcher/virtwrap/manager.go
@@ -618,17 +618,11 @@ func expandDiskImageOffline(imagePath string, size int64) error {
 }
 
 func possibleGuestSize(disk api.Disk) (int64, bool) {
-	var err error
-	capacityResource := disk.Capacity
-	if capacityResource == nil {
-		log.DefaultLogger().Error("Failed to get storage capacity")
-		return 0, false
-	}
-	capacity, ok := capacityResource.AsInt64()
-	if !ok {
-		log.DefaultLogger().Error("Failed to convert capacity to int64")
+	if disk.Capacity == nil {
+		log.DefaultLogger().Error("No disk capacity")
 		return 0, false
 	}
+	preferredSize := *disk.Capacity
 	if disk.FilesystemOverhead == nil {
 		log.DefaultLogger().Errorf("No filesystem overhead found for disk %v", disk)
 		return 0, false
@@ -638,7 +632,7 @@ func possibleGuestSize(disk api.Disk) (int64, bool) {
 		log.DefaultLogger().Reason(err).Error("Failed to parse filesystem overhead as float")
 		return 0, false
 	}
-	return int64((1 - filesystemOverhead) * float64(capacity)), true
+	return int64((1 - filesystemOverhead) * float64(preferredSize)), true
 }
 
 func shouldExpandOffline(disk api.Disk) bool {
diff --git a/pkg/virt-launcher/virtwrap/manager_test.go b/pkg/virt-launcher/virtwrap/manager_test.go
index 79cfcd09f..e8097b901 100644
--- a/pkg/virt-launcher/virtwrap/manager_test.go
+++ b/pkg/virt-launcher/virtwrap/manager_test.go
@@ -2250,20 +2250,21 @@ var _ = Describe("Manager helper functions", func() {
 		BeforeEach(func() {
 			fakePercentFloat = 0.7648
 			fakePercent := cdiv1beta1.Percent(fmt.Sprint(fakePercentFloat))
+			fakeCapacity := int64(123000)
 
 			properDisk = api.Disk{
 				FilesystemOverhead: &fakePercent,
-				Capacity:           resource.NewScaledQuantity(123, 4),
+				Capacity:           &fakeCapacity,
 			}
 		})
 
 		It("should return correct value", func() {
 			size, ok := possibleGuestSize(properDisk)
 			Expect(ok).To(BeTrue())
-			capacity, ok := properDisk.Capacity.AsInt64()
-			Expect(ok).To(BeTrue())
+			capacity := properDisk.Capacity
+			Expect(capacity).ToNot(Equal(nil))
 
-			expectedSize := int64((1 - fakePercentFloat) * float64(capacity))
+			expectedSize := int64((1 - fakePercentFloat) * float64(*capacity))
 
 			Expect(size).To(Equal(expectedSize))
 		})
@@ -2276,19 +2277,12 @@ var _ = Describe("Manager helper functions", func() {
 				disk.Capacity = nil
 				return disk
 			}),
-			table.Entry("disk capacity non-int", func() api.Disk {
-				disk := properDisk
-				nonIntQuantity, ok := resource.ParseQuantity("0.456546456")
-				Expect(ok).To(BeTrue())
-				disk.Capacity = &nonIntQuantity
-				return disk
-			}),
 			table.Entry("filesystem overhead is nil", func() api.Disk {
 				disk := properDisk
 				disk.FilesystemOverhead = nil
 				return disk
 			}),
-			table.Entry("filesystem is non-float", func() api.Disk {
+			table.Entry("filesystem overhead is non-float", func() api.Disk {
 				disk := properDisk
 				fakePercent := cdiv1beta1.Percent(fmt.Sprint("abcdefg"))
 				disk.FilesystemOverhead = &fakePercent
diff --git a/pkg/virt-operator/resource/generate/components/validations_generated.go b/pkg/virt-operator/resource/generate/components/validations_generated.go
index 0461a06e5..86c21f019 100644
--- a/pkg/virt-operator/resource/generate/components/validations_generated.go
+++ b/pkg/virt-operator/resource/generate/components/validations_generated.go
@@ -8842,7 +8842,7 @@ var CRDsValidation map[string]string = map[string]string{
                       pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
                       x-kubernetes-int-or-string: true
                     description: Capacity represents the capacity set on the corresponding
-                      PVC spec
+                      PVC status
                     type: object
                   filesystemOverhead:
                     description: Percentage of filesystem's size to be reserved when
@@ -8853,6 +8853,16 @@ var CRDsValidation map[string]string = map[string]string{
                     description: Preallocated indicates if the PVC's storage is preallocated
                       or not
                     type: boolean
+                  requests:
+                    additionalProperties:
+                      anyOf:
+                      - type: integer
+                      - type: string
+                      pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
+                      x-kubernetes-int-or-string: true
+                    description: Requests represents the resources requested by the
+                      corresponding PVC spec
+                    type: object
                   volumeMode:
                     description: VolumeMode defines what type of volume is required
                       by the claim. Value of Filesystem is implied when not included
diff --git a/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go b/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go
index 7da3beaa2..62be5003e 100644
--- a/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go
+++ b/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go
@@ -2865,6 +2865,13 @@ func (in *PersistentVolumeClaimInfo) DeepCopyInto(out *PersistentVolumeClaimInfo
 			(*out)[key] = val.DeepCopy()
 		}
 	}
+	if in.Requests != nil {
+		in, out := &in.Requests, &out.Requests
+		*out = make(corev1.ResourceList, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val.DeepCopy()
+		}
+	}
 	if in.FilesystemOverhead != nil {
 		in, out := &in.FilesystemOverhead, &out.FilesystemOverhead
 		*out = new(v1beta1.Percent)
diff --git a/staging/src/kubevirt.io/api/core/v1/types.go b/staging/src/kubevirt.io/api/core/v1/types.go
index 58383d064..8bb22a6b8 100644
--- a/staging/src/kubevirt.io/api/core/v1/types.go
+++ b/staging/src/kubevirt.io/api/core/v1/types.go
@@ -244,10 +244,14 @@ type PersistentVolumeClaimInfo struct {
 	// +optional
 	VolumeMode *k8sv1.PersistentVolumeMode `json:"volumeMode,omitempty"`
 
-	// Capacity represents the capacity set on the corresponding PVC spec
+	// Capacity represents the capacity set on the corresponding PVC status
 	// +optional
 	Capacity k8sv1.ResourceList `json:"capacity,omitempty"`
 
+	// Requests represents the resources requested by the corresponding PVC spec
+	// +optional
+	Requests k8sv1.ResourceList `json:"requests,omitempty"`
+
 	// Preallocated indicates if the PVC's storage is preallocated or not
 	// +optional
 	Preallocated bool `json:"preallocated,omitempty"`
diff --git a/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go b/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go
index 0f9e81214..b83583a79 100644
--- a/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go
+++ b/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go
@@ -81,7 +81,8 @@ func (PersistentVolumeClaimInfo) SwaggerDoc() map[string]string {
 		"":                   "PersistentVolumeClaimInfo contains the relavant information virt-handler needs cached about a PVC",
 		"accessModes":        "AccessModes contains the desired access modes the volume should have.\nMore info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#access-modes-1\n+listType=atomic\n+optional",
 		"volumeMode":         "VolumeMode defines what type of volume is required by the claim.\nValue of Filesystem is implied when not included in claim spec.\n+optional",
-		"capacity":           "Capacity represents the capacity set on the corresponding PVC spec\n+optional",
+		"capacity":           "Capacity represents the capacity set on the corresponding PVC status\n+optional",
+		"requests":           "Requests represents the resources requested by the corresponding PVC spec\n+optional",
 		"preallocated":       "Preallocated indicates if the PVC's storage is preallocated or not\n+optional",
 		"filesystemOverhead": "Percentage of filesystem's size to be reserved when resizing the PVC\n+optional",
 	}
diff --git a/staging/src/kubevirt.io/client-go/api/openapi_generated.go b/staging/src/kubevirt.io/client-go/api/openapi_generated.go
index 8a941827f..4f86ca0dd 100644
--- a/staging/src/kubevirt.io/client-go/api/openapi_generated.go
+++ b/staging/src/kubevirt.io/client-go/api/openapi_generated.go
@@ -18298,7 +18298,21 @@ func schema_kubevirtio_api_core_v1_PersistentVolumeClaimInfo(ref common.Referenc
 					},
 					"capacity": {
 						SchemaProps: spec.SchemaProps{
-							Description: "Capacity represents the capacity set on the corresponding PVC spec",
+							Description: "Capacity represents the capacity set on the corresponding PVC status",
+							Type:        []string{"object"},
+							AdditionalProperties: &spec.SchemaOrBool{
+								Allows: true,
+								Schema: &spec.Schema{
+									SchemaProps: spec.SchemaProps{
+										Ref: ref("k8s.io/apimachinery/pkg/api/resource.Quantity"),
+									},
+								},
+							},
+						},
+					},
+					"requests": {
+						SchemaProps: spec.SchemaProps{
+							Description: "Requests represents the resources requested by the corresponding PVC spec",
 							Type:        []string{"object"},
 							AdditionalProperties: &spec.SchemaOrBool{
 								Allows: true,
-- 
2.36.1


From e818c70fdd0d5757ec6b7d8a91e2fadfd8004ae3 Mon Sep 17 00:00:00 2001
From: Jed Lejosne <jed@redhat.com>
Date: Mon, 8 Nov 2021 09:04:57 -0500
Subject: [PATCH 2/3] Only create 4k-aligned disk images

Signed-off-by: Jed Lejosne <jed@redhat.com>
---
 pkg/emptydisk/BUILD.bazel                  |  1 +
 pkg/emptydisk/emptydisk.go                 | 10 ++++++++--
 pkg/host-disk/host-disk.go                 |  5 ++++-
 pkg/util/util.go                           | 14 ++++++++++++++
 pkg/virt-launcher/virtwrap/manager.go      |  9 +++++++--
 pkg/virt-launcher/virtwrap/manager_test.go |  2 ++
 tests/storage/storage.go                   |  8 ++++----
 tests/vmi_configuration_test.go            |  4 ++--
 8 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/pkg/emptydisk/BUILD.bazel b/pkg/emptydisk/BUILD.bazel
index 5dc6af303..c47a4e9f9 100644
--- a/pkg/emptydisk/BUILD.bazel
+++ b/pkg/emptydisk/BUILD.bazel
@@ -9,6 +9,7 @@ go_library(
         "//pkg/ephemeral-disk-utils:go_default_library",
         "//pkg/util:go_default_library",
         "//staging/src/kubevirt.io/api/core/v1:go_default_library",
+        "//staging/src/kubevirt.io/client-go/log:go_default_library",
     ],
 )
 
diff --git a/pkg/emptydisk/emptydisk.go b/pkg/emptydisk/emptydisk.go
index d8dfe47f5..7a0ba2f02 100644
--- a/pkg/emptydisk/emptydisk.go
+++ b/pkg/emptydisk/emptydisk.go
@@ -7,6 +7,7 @@ import (
 	"strconv"
 
 	v1 "kubevirt.io/api/core/v1"
+	"kubevirt.io/client-go/log"
 	ephemeraldiskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils"
 	"kubevirt.io/kubevirt/pkg/util"
 )
@@ -19,11 +20,16 @@ type emptyDiskCreator struct {
 }
 
 func (c *emptyDiskCreator) CreateTemporaryDisks(vmi *v1.VirtualMachineInstance) error {
-	for _, volume := range vmi.Spec.Volumes {
+	logger := log.Log.Object(vmi)
 
+	for _, volume := range vmi.Spec.Volumes {
 		if volume.EmptyDisk != nil {
 			// qemu-img takes the size in bytes or in Kibibytes/Mebibytes/...; lets take bytes
-			size := strconv.FormatInt(volume.EmptyDisk.Capacity.ToDec().ScaledValue(0), 10)
+			intSize := volume.EmptyDisk.Capacity.ToDec().ScaledValue(0)
+			// round down the size to the nearest 4KiB multiple
+			intSize = util.AlignImageSizeTo4k(intSize, logger.With("volume", volume.Name))
+			// convert the size to string for qemu-img
+			size := strconv.FormatInt(intSize, 10)
 			file := filePathForVolumeName(c.emptyDiskBaseDir, volume.Name)
 			if err := util.MkdirAllWithNosec(c.emptyDiskBaseDir); err != nil {
 				return err
diff --git a/pkg/host-disk/host-disk.go b/pkg/host-disk/host-disk.go
index 852e71567..2b5adb73e 100644
--- a/pkg/host-disk/host-disk.go
+++ b/pkg/host-disk/host-disk.go
@@ -99,10 +99,13 @@ func replaceForHostDisk(volumeSource *v1.VolumeSource, volumeName string, pvcVol
 	volumeStatus := pvcVolume[volumeName]
 	isShared := types.HasSharedAccessMode(volumeStatus.PersistentVolumeClaimInfo.AccessModes)
 	file := getPVCDiskImgPath(volumeName, "disk.img")
+	capacity := volumeStatus.PersistentVolumeClaimInfo.Capacity[k8sv1.ResourceStorage]
+	// The host-disk must be 4k-aligned. If the volume specifies a misaligned size, shrink it down to the nearest multiple of 4096
+	capacity.Set(util.AlignImageSizeTo4k(capacity.Value(), log.Log.V(2)))
 	volumeSource.HostDisk = &v1.HostDisk{
 		Path:     file,
 		Type:     v1.HostDiskExistsOrCreate,
-		Capacity: volumeStatus.PersistentVolumeClaimInfo.Capacity[k8sv1.ResourceStorage],
+		Capacity: capacity,
 		Shared:   &isShared,
 	}
 }
diff --git a/pkg/util/util.go b/pkg/util/util.go
index 60f6262d7..b2984a176 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	v1 "kubevirt.io/api/core/v1"
+	"kubevirt.io/client-go/log"
 )
 
 const (
@@ -144,3 +145,16 @@ func IsReadOnlyDisk(disk *v1.Disk) bool {
 
 	return isReadOnlyCDRom
 }
+
+func AlignImageSizeTo4k(size int64, logger *log.FilteredLogger) int64 {
+	remainder := size % 4096
+	if remainder == 0 {
+		return size
+	} else {
+		newSize := size - remainder
+		if logger != nil {
+			logger.Warningf("new size is not 4k-aligned. Adjusting from %d down to %d.", size, newSize)
+		}
+		return newSize
+	}
+}
diff --git a/pkg/virt-launcher/virtwrap/manager.go b/pkg/virt-launcher/virtwrap/manager.go
index b81bc6c84..e8e7234a5 100644
--- a/pkg/virt-launcher/virtwrap/manager.go
+++ b/pkg/virt-launcher/virtwrap/manager.go
@@ -38,6 +38,8 @@ import (
 	"sync"
 	"time"
 
+	util2 "kubevirt.io/kubevirt/pkg/util"
+
 	"kubevirt.io/kubevirt/pkg/virt-launcher/virtwrap/device/hostdevice/generic"
 	"kubevirt.io/kubevirt/pkg/virt-launcher/virtwrap/device/hostdevice/gpu"
 
@@ -609,10 +611,11 @@ func expandDiskImageOffline(imagePath string, size int64) error {
 	} else {
 		preallocateFlag = "--preallocation=off"
 	}
+	size = util2.AlignImageSizeTo4k(size, log.Log.With("image", imagePath))
 	cmd := exec.Command("/usr/bin/qemu-img", "resize", preallocateFlag, imagePath, strconv.FormatInt(size, 10))
 	out, err := cmd.CombinedOutput()
 	if err != nil {
-		return fmt.Errorf("Expanding image failed with error: %v, output: %s", err, out)
+		return fmt.Errorf("expanding image failed with error: %v, output: %s", err, out)
 	}
 	return nil
 }
@@ -632,7 +635,9 @@ func possibleGuestSize(disk api.Disk) (int64, bool) {
 		log.DefaultLogger().Reason(err).Error("Failed to parse filesystem overhead as float")
 		return 0, false
 	}
-	return int64((1 - filesystemOverhead) * float64(preferredSize)), true
+	size := int64((1 - filesystemOverhead) * float64(preferredSize))
+	size = util2.AlignImageSizeTo4k(size, log.DefaultLogger())
+	return size, true
 }
 
 func shouldExpandOffline(disk api.Disk) bool {
diff --git a/pkg/virt-launcher/virtwrap/manager_test.go b/pkg/virt-launcher/virtwrap/manager_test.go
index e8097b901..75f74a772 100644
--- a/pkg/virt-launcher/virtwrap/manager_test.go
+++ b/pkg/virt-launcher/virtwrap/manager_test.go
@@ -2265,6 +2265,8 @@ var _ = Describe("Manager helper functions", func() {
 			Expect(capacity).ToNot(Equal(nil))
 
 			expectedSize := int64((1 - fakePercentFloat) * float64(*capacity))
+			// The size is expected to be 4k-aligned
+			expectedSize = expectedSize - expectedSize%4096
 
 			Expect(size).To(Equal(expectedSize))
 		})
diff --git a/tests/storage/storage.go b/tests/storage/storage.go
index a5ebb9b08..7ba436ba8 100644
--- a/tests/storage/storage.go
+++ b/tests/storage/storage.go
@@ -320,7 +320,7 @@ var _ = SIGDescribe("Storage", func() {
 					Name: "emptydisk1",
 					VolumeSource: virtv1.VolumeSource{
 						EmptyDisk: &virtv1.EmptyDiskSource{
-							Capacity: resource.MustParse("2Gi"),
+							Capacity: resource.MustParse("1G"),
 						},
 					},
 				})
@@ -328,10 +328,10 @@ var _ = SIGDescribe("Storage", func() {
 
 				Expect(libnet.WithIPv6(console.LoginToCirros)(vmi)).To(Succeed())
 
-				By("Checking that /dev/vdc has a capacity of 2Gi")
+				By("Checking that /dev/vdc has a capacity of 1G, aligned to 4k")
 				Expect(console.SafeExpectBatch(vmi, []expect.Batcher{
 					&expect.BSnd{S: "sudo blockdev --getsize64 /dev/vdc\n"},
-					&expect.BExp{R: "2147483648"}, // 2Gi in bytes
+					&expect.BExp{R: "999997440"}, // 1G in bytes rounded down to nearest 4k boundary
 				}, 10)).To(Succeed())
 
 				By("Checking if we can write to /dev/vdc")
@@ -645,7 +645,7 @@ var _ = SIGDescribe("Storage", func() {
 
 						Expect(console.SafeExpectBatch(vmi, []expect.Batcher{
 							&expect.BSnd{S: "blockdev --getsize64 /dev/vdb\n"},
-							&expect.BExp{R: "1014686208"},
+							&expect.BExp{R: "1014685696"},
 						}, 200)).To(Succeed())
 					}
 
diff --git a/tests/vmi_configuration_test.go b/tests/vmi_configuration_test.go
index 64b62ec6d..05802f37f 100644
--- a/tests/vmi_configuration_test.go
+++ b/tests/vmi_configuration_test.go
@@ -2011,10 +2011,10 @@ var _ = Describe("[sig-compute]Configurations", func() {
 			// PVC is mounted as tmpfs on kind, which does not support direct I/O.
 			// As such, it behaves as plugging in a hostDisk - check disks[6].
 			if tests.IsRunningOnKindInfra() {
-				// The chache mode is set to cacheWritethrough
+				// The cache mode is set to cacheWritethrough
 				Expect(string(disks[2].Driver.IO)).To(Equal(ioNone))
 			} else {
-				// The chache mode is set to cacheNone
+				// The cache mode is set to cacheNone
 				Expect(disks[2].Driver.IO).To(Equal(ioNative))
 			}
 
-- 
2.36.1


From 0bc593aad3f1fe8ca9fd39f62e45e6511f023282 Mon Sep 17 00:00:00 2001
From: Jed Lejosne <jed@redhat.com>
Date: Tue, 15 Feb 2022 15:05:16 -0500
Subject: [PATCH 3/3] Align disks on 1MiB instead of 4KiB

Signed-off-by: Jed Lejosne <jed@redhat.com>
---
 pkg/emptydisk/emptydisk.go                 |  8 ++++++--
 pkg/host-disk/host-disk.go                 | 22 +++++++++++++++++-----
 pkg/host-disk/host-disk_test.go            |  3 +++
 pkg/util/util.go                           | 13 ++++++++++---
 pkg/virt-launcher/virtwrap/manager.go      | 10 ++++++++--
 pkg/virt-launcher/virtwrap/manager_test.go |  6 +++---
 tests/storage/storage.go                   |  4 ++--
 7 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/pkg/emptydisk/emptydisk.go b/pkg/emptydisk/emptydisk.go
index 7a0ba2f02..ad72a7004 100644
--- a/pkg/emptydisk/emptydisk.go
+++ b/pkg/emptydisk/emptydisk.go
@@ -1,6 +1,7 @@
 package emptydisk
 
 import (
+	"fmt"
 	"os"
 	"os/exec"
 	"path"
@@ -26,8 +27,11 @@ func (c *emptyDiskCreator) CreateTemporaryDisks(vmi *v1.VirtualMachineInstance)
 		if volume.EmptyDisk != nil {
 			// qemu-img takes the size in bytes or in Kibibytes/Mebibytes/...; lets take bytes
 			intSize := volume.EmptyDisk.Capacity.ToDec().ScaledValue(0)
-			// round down the size to the nearest 4KiB multiple
-			intSize = util.AlignImageSizeTo4k(intSize, logger.With("volume", volume.Name))
+			// round down the size to the nearest 1MiB multiple
+			intSize = util.AlignImageSizeTo1MiB(intSize, logger.With("volume", volume.Name))
+			if intSize == 0 {
+				return fmt.Errorf("the size for volume %s is too low", volume.Name)
+			}
 			// convert the size to string for qemu-img
 			size := strconv.FormatInt(intSize, 10)
 			file := filePathForVolumeName(c.emptyDiskBaseDir, volume.Name)
diff --git a/pkg/host-disk/host-disk.go b/pkg/host-disk/host-disk.go
index 2b5adb73e..a44231794 100644
--- a/pkg/host-disk/host-disk.go
+++ b/pkg/host-disk/host-disk.go
@@ -78,7 +78,10 @@ func ReplacePVCByHostDisk(vmi *v1.VirtualMachineInstance) error {
 				continue
 			}
 
-			replaceForHostDisk(volumeSource, volume.Name, pvcVolume)
+			err := replaceForHostDisk(volumeSource, volume.Name, pvcVolume)
+			if err != nil {
+				return err
+			}
 			// PersistenVolumeClaim is replaced by HostDisk
 			volumeSource.PersistentVolumeClaim = nil
 		}
@@ -87,7 +90,10 @@ func ReplacePVCByHostDisk(vmi *v1.VirtualMachineInstance) error {
 				continue
 			}
 
-			replaceForHostDisk(volumeSource, volume.Name, pvcVolume)
+			err := replaceForHostDisk(volumeSource, volume.Name, pvcVolume)
+			if err != nil {
+				return err
+			}
 			// PersistenVolumeClaim is replaced by HostDisk
 			volumeSource.DataVolume = nil
 		}
@@ -95,19 +101,25 @@ func ReplacePVCByHostDisk(vmi *v1.VirtualMachineInstance) error {
 	return nil
 }
 
-func replaceForHostDisk(volumeSource *v1.VolumeSource, volumeName string, pvcVolume map[string]v1.VolumeStatus) {
+func replaceForHostDisk(volumeSource *v1.VolumeSource, volumeName string, pvcVolume map[string]v1.VolumeStatus) error {
 	volumeStatus := pvcVolume[volumeName]
 	isShared := types.HasSharedAccessMode(volumeStatus.PersistentVolumeClaimInfo.AccessModes)
 	file := getPVCDiskImgPath(volumeName, "disk.img")
 	capacity := volumeStatus.PersistentVolumeClaimInfo.Capacity[k8sv1.ResourceStorage]
-	// The host-disk must be 4k-aligned. If the volume specifies a misaligned size, shrink it down to the nearest multiple of 4096
-	capacity.Set(util.AlignImageSizeTo4k(capacity.Value(), log.Log.V(2)))
+	// The host-disk must be 1MiB-aligned. If the volume specifies a misaligned size, shrink it down to the nearest multiple of 1MiB
+	size := util.AlignImageSizeTo1MiB(capacity.Value(), log.Log.V(2))
+	if size == 0 {
+		return fmt.Errorf("the size for volume %s is too low, must be at least 1MiB", volumeName)
+	}
+	capacity.Set(size)
 	volumeSource.HostDisk = &v1.HostDisk{
 		Path:     file,
 		Type:     v1.HostDiskExistsOrCreate,
 		Capacity: capacity,
 		Shared:   &isShared,
 	}
+
+	return nil
 }
 
 func shouldSkipVolumeSource(passthoughFSVolumes map[string]struct{}, hotplugVolumes map[string]bool, pvcVolume map[string]v1.VolumeStatus, volumeName string) bool {
diff --git a/pkg/host-disk/host-disk_test.go b/pkg/host-disk/host-disk_test.go
index 9fccd9be8..68f3fe8aa 100644
--- a/pkg/host-disk/host-disk_test.go
+++ b/pkg/host-disk/host-disk_test.go
@@ -399,6 +399,9 @@ var _ = Describe("HostDisk", func() {
 					Name: volumeName,
 					PersistentVolumeClaimInfo: &v1.PersistentVolumeClaimInfo{
 						VolumeMode: &mode,
+						Capacity: map[k8sv1.ResourceName]resource.Quantity{
+							k8sv1.ResourceStorage: *resource.NewQuantity(1024*1024, resource.BinarySI),
+						},
 					},
 				},
 			}
diff --git a/pkg/util/util.go b/pkg/util/util.go
index b2984a176..3ba7c8386 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -146,14 +146,21 @@ func IsReadOnlyDisk(disk *v1.Disk) bool {
 	return isReadOnlyCDRom
 }
 
-func AlignImageSizeTo4k(size int64, logger *log.FilteredLogger) int64 {
-	remainder := size % 4096
+// AlignImageSizeTo1MiB rounds down the size to the nearest multiple of 1MiB
+// A warning or an error may get logged
+// The caller is responsible for ensuring the rounded-down size is not 0
+func AlignImageSizeTo1MiB(size int64, logger *log.FilteredLogger) int64 {
+	remainder := size % (1024 * 1024)
 	if remainder == 0 {
 		return size
 	} else {
 		newSize := size - remainder
 		if logger != nil {
-			logger.Warningf("new size is not 4k-aligned. Adjusting from %d down to %d.", size, newSize)
+			if newSize == 0 {
+				logger.Errorf("disks must be at least 1MiB, %d bytes is too small", size)
+			} else {
+				logger.Warningf("disk size is not 1MiB-aligned. Adjusting from %d down to %d.", size, newSize)
+			}
 		}
 		return newSize
 	}
diff --git a/pkg/virt-launcher/virtwrap/manager.go b/pkg/virt-launcher/virtwrap/manager.go
index e8e7234a5..2276f1ea9 100644
--- a/pkg/virt-launcher/virtwrap/manager.go
+++ b/pkg/virt-launcher/virtwrap/manager.go
@@ -611,7 +611,10 @@ func expandDiskImageOffline(imagePath string, size int64) error {
 	} else {
 		preallocateFlag = "--preallocation=off"
 	}
-	size = util2.AlignImageSizeTo4k(size, log.Log.With("image", imagePath))
+	size = util2.AlignImageSizeTo1MiB(size, log.Log.With("image", imagePath))
+	if size == 0 {
+		return fmt.Errorf("%s must be at least 1MiB", imagePath)
+	}
 	cmd := exec.Command("/usr/bin/qemu-img", "resize", preallocateFlag, imagePath, strconv.FormatInt(size, 10))
 	out, err := cmd.CombinedOutput()
 	if err != nil {
@@ -636,7 +639,10 @@ func possibleGuestSize(disk api.Disk) (int64, bool) {
 		return 0, false
 	}
 	size := int64((1 - filesystemOverhead) * float64(preferredSize))
-	size = util2.AlignImageSizeTo4k(size, log.DefaultLogger())
+	size = util2.AlignImageSizeTo1MiB(size, log.DefaultLogger())
+	if size == 0 {
+		return 0, false
+	}
 	return size, true
 }
 
diff --git a/pkg/virt-launcher/virtwrap/manager_test.go b/pkg/virt-launcher/virtwrap/manager_test.go
index 75f74a772..96956ba5d 100644
--- a/pkg/virt-launcher/virtwrap/manager_test.go
+++ b/pkg/virt-launcher/virtwrap/manager_test.go
@@ -2250,7 +2250,7 @@ var _ = Describe("Manager helper functions", func() {
 		BeforeEach(func() {
 			fakePercentFloat = 0.7648
 			fakePercent := cdiv1beta1.Percent(fmt.Sprint(fakePercentFloat))
-			fakeCapacity := int64(123000)
+			fakeCapacity := int64(2345 * 3456) // We need (1-0.7648)*fakeCapacity to be > 1MiB and misaligned
 
 			properDisk = api.Disk{
 				FilesystemOverhead: &fakePercent,
@@ -2265,8 +2265,8 @@ var _ = Describe("Manager helper functions", func() {
 			Expect(capacity).ToNot(Equal(nil))
 
 			expectedSize := int64((1 - fakePercentFloat) * float64(*capacity))
-			// The size is expected to be 4k-aligned
-			expectedSize = expectedSize - expectedSize%4096
+			// The size is expected to be 1MiB-aligned
+			expectedSize = expectedSize - expectedSize%(1024*1024)
 
 			Expect(size).To(Equal(expectedSize))
 		})
diff --git a/tests/storage/storage.go b/tests/storage/storage.go
index 7ba436ba8..5dce49ee9 100644
--- a/tests/storage/storage.go
+++ b/tests/storage/storage.go
@@ -331,7 +331,7 @@ var _ = SIGDescribe("Storage", func() {
 				By("Checking that /dev/vdc has a capacity of 1G, aligned to 4k")
 				Expect(console.SafeExpectBatch(vmi, []expect.Batcher{
 					&expect.BSnd{S: "sudo blockdev --getsize64 /dev/vdc\n"},
-					&expect.BExp{R: "999997440"}, // 1G in bytes rounded down to nearest 4k boundary
+					&expect.BExp{R: "999292928"}, // 1G in bytes rounded down to nearest 1MiB boundary
 				}, 10)).To(Succeed())
 
 				By("Checking if we can write to /dev/vdc")
@@ -645,7 +645,7 @@ var _ = SIGDescribe("Storage", func() {
 
 						Expect(console.SafeExpectBatch(vmi, []expect.Batcher{
 							&expect.BSnd{S: "blockdev --getsize64 /dev/vdb\n"},
-							&expect.BExp{R: "1014685696"},
+							&expect.BExp{R: "1013972992"},
 						}, 200)).To(Succeed())
 					}
 
-- 
2.36.1

