Validate image fits in filesystem in a lot more cases. take filesystem overhead into account when resizing. (#1466)

* Validate images fit on a filesystem in more cases.

Background:
When the backing store is a filesystem, we store the images
as sparse files. So the file may eventually grow to be bigger
than the available storage. This will cause unfortunate
failures down the line.

Prior to this commit, we validated the size:
- In case the backing store implicitly did it for us (block volumes)
- On async upload
- When resizing (by the operation failing if the image cannot fit
in the available space).

The Resize phase is encountered quite commonly:
Transfer->Convert->Resize
TransferFile->Resize

Adding validation here for the non-resize case covers almost all
the cases.

The only exceptions that aren't validated now are:
- DataVolumeArchive via the HTTP datasource
- VDDK

Signed-off-by: Maya Rashish <mrashish@redhat.com>

* When resizing, take into account filesystem overhead.

Signed-off-by: Maya Rashish <mrashish@redhat.com>

* Add testing for too large upload/import

- Import/sync upload of too large physical size image (raw.xz, qcow2)
- Import/sync upload of too large virtual size image (raw.xz, qcow2)
- Import of a too large raw image file, if filesystem overhead is
taken into account

- Async upload of too large physical size qcow2.
The async upload cases do not mirror the sync upload ones because if
a block device is used as scratch space, it will hit a size limit
before the validation pause, and fail differently.
This scenario is identical to the sync upload case which was added.

Signed-off-by: Maya Rashish <mrashish@redhat.com>

* Refactor code in a way that requires less comments to explain.

We can just validate that the requested image size will fit in the
available space, and not rely on the fact we typically resize the
images to the full size.

Signed-off-by: Maya Rashish <mrashish@redhat.com>

* When calculating usable space, round down to a multiple of 512.

Our validation is indirectly:
image after resize to usable space <= usable space
For this to pass, we need to ensure that qemu-img's rounding
up to 512 doesn't change the size.

Signed-off-by: Maya Rashish <mrashish@redhat.com>

* Adjust qemu-img to the ones emitted by nbdkit:

- In some cases, we clearly don't rely on the qemu-img error,
so don't check for it.
- In one case, switch it to looking for the nbdkit equivalent
error message.

Signed-off-by: Maya Rashish <mrashish@redhat.com>

BUILD.bazel

@@ -82,6 +82,10 @@ filegroup(
         "//tests:images/archive.tar",
         "//tests:images/cirros-qcow2.img",
         "//tests:images/cirros.raw",
+	"//tests:images/cirros-large-virtual-size.raw.xz",
+	"//tests:images/cirros-large-virtual-size.qcow2",
+	"//tests:images/cirros-large-physical-size.raw.xz",
+	"//tests:images/cirros-large-physical-size.qcow2",
     ],
     visibility = ["//visibility:public"],
 )

pkg/importer/data-processor.go

@@ -269,18 +269,29 @@ func (dp *DataProcessor) convert(url *url.URL) (ProcessingPhase, error) {
 }
 
 func (dp *DataProcessor) resize() (ProcessingPhase, error) {
-	// Resize only if we have a resize request, and if the image is on a file system pvc.
 	size, _ := getAvailableSpaceBlockFunc(dp.dataFile)
 	klog.V(3).Infof("Available space in dataFile: %d", size)
+	isBlockDev := size >= int64(0)
 	shouldPreallocate := false
-	if dp.requestImageSize != "" && size < int64(0) {
+	if !isBlockDev {
+		if dp.requestImageSize != "" {
 			var err error
 			klog.V(3).Infoln("Resizing image")
-		shouldPreallocate, err = ResizeImage(dp.dataFile, dp.requestImageSize, dp.availableSpace)
+			shouldPreallocate, err = ResizeImage(dp.dataFile, dp.requestImageSize, dp.getUsableSpace())
 			if err != nil {
 				return ProcessingPhaseError, errors.Wrap(err, "Resize of image failed")
 			}
 		}
+		// Validate that a sparse file will fit even as it fills out.
+		dataFileURL, err := url.Parse(dp.dataFile)
+		if err != nil {
+			return ProcessingPhaseError, err
+		}
+		err = dp.validate(dataFileURL)
+		if err != nil {
+			return ProcessingPhaseError, err
+		}
+	}
 	if dp.dataFile != "" {
 		// Change permissions to 0660
 		err := os.Chmod(dp.dataFile, 0660)
@@ -380,3 +391,12 @@ func (dp *DataProcessor) calculateTargetSize() int64 {
 func (dp *DataProcessor) PreallocationApplied() common.PreallocationStatus {
 	return dp.preallocationApplied
 }
+
+func (dp *DataProcessor) getUsableSpace() int64 {
+	blockSize := int64(512)
+	spaceWithOverhead := int64((1 - dp.filesystemOverhead) * float64(dp.availableSpace))
+	// qemu-img will round up, making us use more than the usable space.
+	// This later conflicts with image size validation.
+	qemuImgCorrection := util.RoundDown(spaceWithOverhead, blockSize)
+	return qemuImgCorrection
+}

pkg/importer/data-processor_test.go

@@ -244,7 +244,7 @@ var _ = Describe("Data Processor", func() {
 		}
 		dp := NewDataProcessor(mdp, "dest", "dataDir", tmpDir, "1G", 0.055, false)
 		dp.availableSpace = int64(1500)
-		qemuOperations := NewFakeQEMUOperations(nil, nil, fakeInfoRet, nil, nil, resource.NewScaledQuantity(int64(1500), 0))
+		qemuOperations := NewFakeQEMUOperations(nil, nil, fakeInfoRet, nil, nil, resource.NewScaledQuantity(dp.getUsableSpace(), 0))
 		replaceQEMUOperations(qemuOperations, func() {
 			err = dp.ProcessData()
 			Expect(err).ToNot(HaveOccurred())
@@ -311,10 +311,13 @@ var _ = Describe("Resize", func() {
 			url: url,
 		}
 		dp := NewDataProcessor(mdp, "dest", "dataDir", "scratchDataDir", "", 0.055, false)
+		qemuOperations := NewFakeQEMUOperations(nil, nil, fakeInfoOpRetVal{&fakeZeroImageInfo, nil}, nil, nil, nil)
+		replaceQEMUOperations(qemuOperations, func() {
 			nextPhase, err := dp.resize()
 			Expect(err).ToNot(HaveOccurred())
 			Expect(ProcessingPhaseComplete).To(Equal(nextPhase))
 		})
+	})
 
 	It("Should not resize and return complete, when requestedSize is valid, but datadir doesn't exist (block device)", func() {
 		replaceAvailableSpaceBlockFunc(func(dataDir string) (int64, error) {
@@ -327,11 +330,14 @@ var _ = Describe("Resize", func() {
 				url: url,
 			}
 			dp := NewDataProcessor(mdp, "dest", "dataDir", "scratchDataDir", "1G", 0.055, false)
+			qemuOperations := NewFakeQEMUOperations(nil, nil, fakeInfoOpRetVal{&fakeZeroImageInfo, nil}, nil, nil, nil)
+			replaceQEMUOperations(qemuOperations, func() {
 				nextPhase, err := dp.resize()
 				Expect(err).ToNot(HaveOccurred())
 				Expect(ProcessingPhaseComplete).To(Equal(nextPhase))
 			})
 		})
+	})
 
 	It("Should resize and return complete, when requestedSize is valid, and datadir exists", func() {
 		tmpDir, err := ioutil.TempDir("", "data")

pkg/util/util.go

@@ -273,3 +273,8 @@ func CopyDir(source string, dest string) (err error) {
 	}
 	return
 }
+
+// RoundDown returns the number rounded down to the nearest multiple.
+func RoundDown(number, multiple int64) int64 {
+	return number / multiple * multiple
+}

pkg/util/util_test.go

@@ -29,6 +29,19 @@ var _ = Describe("Util", func() {
 		Expect(regexp.MustCompile(pattern).Match([]byte(got))).To(BeTrue())
 	})
 
+	table.DescribeTable("Round down", func(input, multiple, expectedResult int64) {
+		result := RoundDown(input, multiple)
+		Expect(result).To(Equal(expectedResult))
+	},
+		table.Entry("Round down 513 to nearest multiple of 512", int64(513), int64(512), int64(512)),
+		table.Entry("Round down 512 to nearest multiple of 512", int64(512), int64(512), int64(512)),
+		table.Entry("Round down 510 to nearest multiple of 512", int64(510), int64(512), int64(0)),
+		table.Entry("Round down 0 to nearest multiple of 512", int64(0), int64(512), int64(0)),
+		table.Entry("Round down 513 to nearest multiple of 2", int64(513), int64(2), int64(512)),
+		table.Entry("Round down 512 to nearest multiple of 2", int64(512), int64(2), int64(512)),
+		table.Entry("Round down 510 to nearest multiple of 2", int64(510), int64(2), int64(510)),
+	)
+
 	table.DescribeTable("Find Namespace", func(inputFile, expectedResult string) {
 		result := getNamespace(inputFile)
 		Expect(result).To(Equal(expectedResult))

tests/BUILD.bazel

@@ -89,6 +89,10 @@ exports_files(
         "images/archive.tar",
         "images/cirros-qcow2.img",
         "images/cirros.raw",
+        "images/cirros-large-virtual-size.raw.xz",
+        "images/cirros-large-virtual-size.qcow2",
+        "images/cirros-large-physical-size.raw.xz",
+        "images/cirros-large-physical-size.qcow2",
         "images/invalid_qcow_images/invalid-qcow-backing-file.img",
         "images/invalid_qcow_images/invalid-qcow-large-json.img",
         "images/invalid_qcow_images/invalid-qcow-large-memory.img",

tests/import_test.go

@@ -50,6 +50,45 @@ var _ = Describe("[rfe_id:1115][crit:high][vendor:cnv-qe@redhat.com][level:compo
 		ns = f.Namespace.Name
 	})
 
+	DescribeTable("[test_id:2329] Should fail to import images that require too much space", func(uploadURL string) {
+		imageURL := fmt.Sprintf(uploadURL, f.CdiInstallNs)
+
+		By(imageURL)
+		dv := utils.NewDataVolumeWithHTTPImport("too-large-import", "500Mi", imageURL)
+		dv, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv)
+		Expect(err).ToNot(HaveOccurred())
+
+		pvc, err := utils.WaitForPVC(f.K8sClient, dv.Namespace, dv.Name)
+		Expect(err).ToNot(HaveOccurred())
+		f.ForceBindIfWaitForFirstConsumer(pvc)
+
+		importer, err := utils.FindPodByPrefix(f.K8sClient, f.Namespace.Name, common.ImporterPodName, common.CDILabelSelector)
+		Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Unable to get importer pod"))
+
+		By(fmt.Sprintf("logs for pod -n %s %s", importer.Name, importer.Namespace))
+		By("Verify size error in logs")
+		Eventually(func() bool {
+			log, _ := tests.RunKubectlCommand(f, "logs", importer.Name, "-n", importer.Namespace)
+			if strings.Contains(log, "is larger than available size") {
+				return true
+			}
+			if strings.Contains(log, "no space left on device") {
+				return true
+			}
+			if strings.Contains(log, "file largest block is bigger than maxblock") {
+				return true
+			}
+			By("Failed to find error messages about a too large image in log:")
+			By(log)
+			return false
+		}, controllerSkipPVCCompleteTimeout, assertionPollInterval).Should(BeTrue())
+	},
+		Entry("fail given a large virtual size RAW XZ file", utils.LargeVirtualDiskXz),
+		Entry("fail given a large virtual size QCOW2 file", utils.LargeVirtualDiskQcow),
+		Entry("fail given a large physical size RAW XZ file", utils.LargePhysicalDiskXz),
+		Entry("fail given a large physical size QCOW2 file", utils.LargePhysicalDiskQcow),
+	)
+
 	It("[test_id:4967]Should not perform CDI operations on PVC without annotations", func() {
 		// Make sure the PVC name is unique, we have no guarantee on order and we are not
 		// deleting the PVC at the end of the test, so if another runs first we will fail.
@@ -163,7 +202,99 @@ var _ = Describe("[rfe_id:4784][crit:high] Importer respects node placement", fu
 		match := tests.PodSpecHasTestNodePlacementValues(f, importer.Spec)
 		Expect(match).To(BeTrue(), fmt.Sprintf("node placement in pod spec\n%v\n differs from node placement values in CDI CR\n%v\n", importer.Spec, cr.Spec.Workloads))
 	})
+})
 
+var _ = Describe("Importer CDI config manipulation tests", func() {
+	var config *cdiv1.CDIConfig
+	var origSpec *cdiv1.CDIConfigSpec
+	var err error
+	f := framework.NewFramework(namespacePrefix)
+
+	BeforeEach(func() {
+		config, err = f.CdiClient.CdiV1beta1().CDIConfigs().Get(context.TODO(), common.ConfigName, metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+
+		origSpec = config.Spec.DeepCopy()
+	})
+
+	AfterEach(func() {
+		By("Restoring CDIConfig to original state")
+		err := utils.UpdateCDIConfig(f.CrClient, func(config *cdiv1.CDIConfigSpec) {
+			origSpec.DeepCopyInto(config)
+		})
+
+		Eventually(func() bool {
+			config, err = f.CdiClient.CdiV1beta1().CDIConfigs().Get(context.TODO(), "cdi", metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			return reflect.DeepEqual(config.Spec, origSpec)
+		}, 30*time.Second, time.Second)
+	})
+
+	DescribeTable("Filesystem overhead is honored with a RAW file", func(expectedSuccess bool, globalOverhead, scOverhead string) {
+		defaultSCName := utils.DefaultStorageClass.GetName()
+		testedFilesystemOverhead := &cdiv1.FilesystemOverhead{}
+		if globalOverhead != "" {
+			testedFilesystemOverhead.Global = cdiv1.Percent(globalOverhead)
+		}
+		if scOverhead != "" {
+			testedFilesystemOverhead.StorageClass = map[string]cdiv1.Percent{defaultSCName: cdiv1.Percent(scOverhead)}
+		}
+		config.Spec.FilesystemOverhead = testedFilesystemOverhead.DeepCopy()
+		By(fmt.Sprintf("Updating CDIConfig filesystem overhead to %v", config.Spec.FilesystemOverhead))
+		err := utils.UpdateCDIConfig(f.CrClient, func(config *cdiv1.CDIConfigSpec) {
+			config.FilesystemOverhead = testedFilesystemOverhead.DeepCopy()
+		})
+		Expect(err).ToNot(HaveOccurred())
+		By(fmt.Sprintf("Waiting for filsystem overhead status to be set to %v", testedFilesystemOverhead))
+		Eventually(func() bool {
+			config, err := f.CdiClient.CdiV1beta1().CDIConfigs().Get(context.TODO(), common.ConfigName, metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			if scOverhead != "" {
+				return config.Status.FilesystemOverhead.StorageClass[defaultSCName] == cdiv1.Percent(scOverhead)
+			}
+			return config.Status.FilesystemOverhead.StorageClass[defaultSCName] == cdiv1.Percent(globalOverhead)
+		}, timeout, pollingInterval).Should(BeTrue(), "CDIConfig filesystem overhead wasn't set")
+
+		imageURL := fmt.Sprintf(utils.TinyCoreIsoURL, f.CdiInstallNs)
+
+		By(imageURL)
+		dv := utils.NewDataVolumeWithHTTPImport("too-large-import", "500Mi", imageURL)
+		dv, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv)
+		Expect(err).ToNot(HaveOccurred())
+
+		pvc, err := utils.WaitForPVC(f.K8sClient, dv.Namespace, dv.Name)
+		Expect(err).ToNot(HaveOccurred())
+		f.ForceBindIfWaitForFirstConsumer(pvc)
+
+		importer, err := utils.FindPodByPrefix(f.K8sClient, f.Namespace.Name, common.ImporterPodName, common.CDILabelSelector)
+		Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Unable to get importer pod"))
+
+		if expectedSuccess {
+			By("Waiting for import to be completed")
+			err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, cdiv1.Succeeded, dv.Name)
+			Expect(err).ToNot(HaveOccurred(), "Datavolume not in phase succeeded in time")
+		} else {
+			By(fmt.Sprintf("logs for pod -n %s %s", importer.Name, importer.Namespace))
+			By("Verify size error in logs")
+			Eventually(func() bool {
+				log, _ := tests.RunKubectlCommand(f, "logs", importer.Name, "-n", importer.Namespace)
+				if strings.Contains(log, "is larger than available size") {
+					return true
+				}
+				if strings.Contains(log, "no space left on device") {
+					return true
+				}
+				By("Failed to find error messages about a too large image in log:")
+				By(log)
+				return false
+			}, controllerSkipPVCCompleteTimeout, assertionPollInterval).Should(BeTrue())
+		}
+	},
+		Entry("Succeed with low global overhead", true, "0.1", ""),
+		Entry("[posneg:negative] Fail with high global overhead", false, "0.99", ""),
+		Entry("Succeed with low per-storageclass overhead (despite high global overhead)", true, "0.99", "0.1"),
+		Entry("[posneg:negative] Fail with high per-storageclass overhead (despite low global overhead)", false, "0.1", "0.99"),
+	)
 })
 
 var _ = Describe("[rfe_id:1118][crit:high][vendor:cnv-qe@redhat.com][level:component]Importer Test Suite-prometheus", func() {

tests/upload_test.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"os/exec"
 	"strconv"
+	"strings"
 	"time"
 
 	. "github.com/onsi/ginkgo"
@@ -199,7 +200,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:compon
 
 	})
 
-	It("[test_id:4989]Verify validation error message on async upload if virtual size > pvc size", func() {
+	DescribeTable("Verify validation error message on async upload if virtual size > pvc size", func(filename string) {
 		By("Verify PVC annotation says ready")
 		found, err := utils.WaitPVCPodStatusReady(f.K8sClient, pvc)
 		Expect(err).ToNot(HaveOccurred())
@@ -213,9 +214,54 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:compon
 
 		By("Do upload")
 		Eventually(func() error {
-			return uploadFileNameToPath(binaryRequestFunc, utils.UploadFileLargeVirtualDisk, uploadProxyURL, asyncUploadPath, token, http.StatusBadRequest)
+			return uploadFileNameToPath(binaryRequestFunc, filename, uploadProxyURL, asyncUploadPath, token, http.StatusBadRequest)
 		}, timeout, pollingInterval).Should(BeNil(), "Upload should eventually succeed, even if initially pod is not ready")
-	})
+	},
+		Entry("[test_id:4989]fail given a large virtual size QCOW2 file", utils.UploadFileLargeVirtualDiskQcow),
+		Entry("fail given a large physical size QCOW2 file", utils.UploadFileLargePhysicalDiskQcow),
+	)
+
+	DescribeTable("[posneg:negative][test_id:2330]Verify failure on sync upload if virtual size > pvc size", func(filename string) {
+		By("Verify PVC annotation says ready")
+		found, err := utils.WaitPVCPodStatusReady(f.K8sClient, pvc)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(found).To(BeTrue())
+
+		var token string
+		By("Get an upload token")
+		token, err = utils.RequestUploadToken(f.CdiClient, pvc)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(token).ToNot(BeEmpty())
+
+		By("Do upload")
+		err = uploadFileNameToPath(binaryRequestFunc, filename, uploadProxyURL, syncUploadPath, token, http.StatusOK)
+		Expect(err).To(HaveOccurred())
+
+		uploadPod, err := utils.FindPodByPrefix(f.K8sClient, f.Namespace.Name, utils.UploadPodName(pvc), common.CDILabelSelector)
+		Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Unable to get uploader pod %q", f.Namespace.Name+"/"+utils.UploadPodName(pvc)))
+
+		By("Verify size error in logs")
+		Eventually(func() bool {
+			log, _ := tests.RunKubectlCommand(f, "logs", uploadPod.Name, "-n", uploadPod.Namespace)
+			if strings.Contains(log, "is larger than available size") {
+				return true
+			}
+			if strings.Contains(log, "no space left on device") {
+				return true
+			}
+			if strings.Contains(log, "qemu-img execution failed") {
+				return true
+			}
+			By("Failed to find error messages about a too large image in log:")
+			By(log)
+			return false
+		}, controllerSkipPVCCompleteTimeout, assertionPollInterval).Should(BeTrue())
+	},
+		Entry("fail given a large virtual size RAW XZ file", utils.UploadFileLargeVirtualDiskXz),
+		Entry("fail given a large virtual size QCOW2 file", utils.UploadFileLargeVirtualDiskQcow),
+		Entry("fail given a large physical size RAW XZ file", utils.UploadFileLargePhysicalDiskXz),
+		Entry("fail given a large physical size QCOW2 file", utils.UploadFileLargePhysicalDiskQcow),
+	)
 })
 
 var TestFakeError = errors.New("TestFakeError")

tests/utils/datavolume.go

@@ -41,6 +41,14 @@ const (
 	TinyCoreQcow2URLRateLimit = "http://cdi-file-host.%s:82/tinyCore.qcow2"
 	// InvalidQcowImagesURL provides a test url for invalid qcow images
 	InvalidQcowImagesURL = "http://cdi-file-host.%s/invalid_qcow_images/"
+	// LargeVirtualDiskQcow provides a test url for a cirros image with a large virtual size, in qcow2 format
+	LargeVirtualDiskQcow = "http://cdi-file-host.%s/cirros-large-virtual-size.qcow2"
+	// LargeVirtualDiskXz provides a test url for a cirros image with a large virtual size, in RAW format, XZ-compressed
+	LargeVirtualDiskXz = "http://cdi-file-host.%s/cirros-large-virtual-size.raw.xz"
+	// LargePhysicalDiskQcow provides a test url for a cirros image with a large physical size, in qcow2 format
+	LargePhysicalDiskQcow = "http://cdi-file-host.%s/cirros-large-physical-size.qcow2"
+	// LargePhysicalDiskXz provides a test url for a cirros image with a large physical size, in RAW format, XZ-compressed
+	LargePhysicalDiskXz = "http://cdi-file-host.%s/cirros-large-physical-size.raw.xz"
 	// TarArchiveURL provides a test url for a tar achive file
 	TarArchiveURL = "http://cdi-file-host.%s/archive.tar"
 	// CirrosURL provides the standard cirros image qcow image

tests/utils/upload.go

@@ -15,8 +15,14 @@ import (
 const (
 	// UploadFile is the file to upload
 	UploadFile = "./images/tinyCore.iso"
-	// UploadFileLargeVirtualDisk is the file to upload
-	UploadFileLargeVirtualDisk = "./images/cirros-large-vdisk.qcow2"
+	// UploadFileLargeVirtualDiskQcow is the file to upload (QCOW2)
+	UploadFileLargeVirtualDiskQcow = "./images/cirros-large-virtual-size.qcow2"
+	// UploadFileLargeVirtualDiskXz is the file to upload (XZ-compressed RAW file)
+	UploadFileLargeVirtualDiskXz = "./images/cirros-large-virtual-size.raw.xz"
+	// UploadFileLargePhysicalDiskQcow is the file to upload (QCOW2)
+	UploadFileLargePhysicalDiskQcow = "./images/cirros-large-physical-size.qcow2"
+	// UploadFileLargePhysicalDiskXz is the file to upload (XZ-compressed RAW file)
+	UploadFileLargePhysicalDiskXz = "./images/cirros-large-physical-size.raw.xz"
 
 	// UploadFileSize is the size of UploadFile
 	UploadFileSize = 18874368