Correct wrong check of resulting image sparsiness

The first issue is that we look at content size since 2168,
which is not beneficial in the sparseness check as opposed to disk usage.

The second issue is that the check we have in tests for sparsiness is not honest because of the "equal to" part.
The virtual size has to be strictly greater than the content
(as the content is displayed by tools that understand sparseness).

The third issue is that we almost always resize which results in significantly larger virtual size.
What we really care about is comparing against the original virtual size of the image.

Signed-off-by: Alex Kalenyuk <[email protected]>

Author: akalenyu

tests/cloner_test.go

@@ -1899,7 +1899,7 @@ var _ = Describe("all clone tests", func() {
 			Expect(err).ToNot(HaveOccurred())
 
 			By("Verifying the image is sparse")
-			Expect(f.VerifySparse(f.Namespace, utils.PersistentVolumeClaimFromDataVolume(targetDv), utils.DefaultImagePath)).To(BeTrue())
+			Expect(f.VerifySparse(f.Namespace, utils.PersistentVolumeClaimFromDataVolume(targetDv), utils.DefaultImagePath, utils.UploadFileSize)).To(BeTrue())
 			By("Deleting verifier pod")
 			err = utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name)
 			Expect(err).ToNot(HaveOccurred())

tests/framework/pvc.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"strconv"
 	"strings"
 	"time"
 
@@ -284,24 +283,15 @@ func (f *Framework) VerifyBlankDisk(namespace *k8sv1.Namespace, pvc *k8sv1.Persi
 }
 
 // VerifySparse checks a disk image being sparse after creation/resize.
-func (f *Framework) VerifySparse(namespace *k8sv1.Namespace, pvc *k8sv1.PersistentVolumeClaim, imagePath string) (bool, error) {
+func (f *Framework) VerifySparse(namespace *k8sv1.Namespace, pvc *k8sv1.PersistentVolumeClaim, imagePath string, originalVirtualSize int64) (bool, error) {
 	var info image.ImgInfo
-	var imageContentSize int64
 	err := f.GetImageInfo(namespace, pvc, imagePath, &info)
 	if err != nil {
 		return false, err
 	}
-	// qemu-img info gives us ActualSize but that is size on disk
-	// which isn't important to us in this comparison; we compare content size
-	err = f.GetImageContentSize(namespace, pvc, imagePath, &imageContentSize)
-	if err != nil {
-		return false, err
-	}
-	if info.ActualSize-imageContentSize >= units.MiB {
-		return false, fmt.Errorf("Diff between content size %d and size on disk %d is significant, something's not right", imageContentSize, info.ActualSize)
-	}
-	fmt.Fprintf(ginkgo.GinkgoWriter, "INFO: VerifySparse comparison: Virtual: %d vs Content: %d\n", info.VirtualSize, imageContentSize)
-	return info.VirtualSize >= imageContentSize, nil
+	fmt.Fprintf(ginkgo.GinkgoWriter, "INFO: VerifySparse comparison: OriginalVirtual: %d vs SizeOnDisk: %d\n", originalVirtualSize, info.ActualSize)
+	// The size on disk of a sparse image is significantly lower than the image's virtual size
+	return originalVirtualSize-info.ActualSize >= units.MB, nil
 }
 
 // VerifyFSOverhead checks whether virtual size is smaller than actual size. That means FS Overhead has been accounted for.
@@ -548,26 +538,6 @@ func (f *Framework) GetImageInfo(namespace *k8sv1.Namespace, pvc *k8sv1.Persiste
 	return err
 }
 
-// GetImageContentSize returns the content size (as opposed to size on disk) of an image
-func (f *Framework) GetImageContentSize(namespace *k8sv1.Namespace, pvc *k8sv1.PersistentVolumeClaim, imagePath string, imageSize *int64) error {
-	cmd := fmt.Sprintf("du -s --apparent-size -B 1 %s | cut -f 1", imagePath)
-
-	_, err := f.verifyInPod(namespace, pvc, cmd, func(output, stderr string) (bool, error) {
-		fmt.Fprintf(ginkgo.GinkgoWriter, "CMD (%s) output %s\n", cmd, output)
-
-		size, err := strconv.ParseInt(output, 10, 64)
-		if err != nil {
-			klog.Errorf("Invalid image content size:\n%s\n", output)
-			return false, err
-		}
-		*imageSize = size
-
-		return true, nil
-	})
-
-	return err
-}
-
 func (f *Framework) startVerifierPod(namespace *k8sv1.Namespace, pvc *k8sv1.PersistentVolumeClaim) (*k8sv1.Pod, error) {
 	var executorPod *k8sv1.Pod
 	var err error

tests/import_test.go

@@ -17,6 +17,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
+	"github.com/docker/go-units"
 	"github.com/google/uuid"
 
 	v1 "k8s.io/api/core/v1"
@@ -126,7 +127,7 @@ var _ = Describe("[rfe_id:1115][crit:high][vendor:[email protected]][level:compo
 		By("Verify the image contents")
 		Expect(f.VerifyBlankDisk(f.Namespace, pvc)).To(BeTrue())
 		By("Verifying the image is sparse")
-		Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath)).To(BeTrue())
+		Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath, utils.UploadFileSize)).To(BeTrue())
 		By("Verifying permissions are 660")
 		Expect(f.VerifyPermissions(f.Namespace, pvc)).To(BeTrue(), "Permissions on disk image are not 660")
 		if utils.DefaultStorageCSIRespectsFsGroup {
@@ -1626,7 +1627,7 @@ var _ = Describe("Import populator", func() {
 		tests.DisableWebhookPvcRendering(f.CrClient)
 	})
 
-	DescribeTable("should import fileSystem PVC", func(expectedMD5 string, volumeImportSourceFunc func(cdiv1.DataVolumeContentType, bool) error, preallocation, webhookRendering bool) {
+	DescribeTable("should import fileSystem PVC", func(expectedMD5 string, originalVirtualSize int, volumeImportSourceFunc func(cdiv1.DataVolumeContentType, bool) error, preallocation, webhookRendering bool) {
 		pvc = importPopulationPVCDefinition()
 
 		if webhookRendering {
@@ -1660,7 +1661,7 @@ var _ = Describe("Import populator", func() {
 			Expect(ok).To(BeTrue())
 		} else {
 			By("Verifying the image is sparse")
-			Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath)).To(BeTrue())
+			Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath, int64(originalVirtualSize))).To(BeTrue())
 		}
 
 		if utils.DefaultStorageCSIRespectsFsGroup {
@@ -1685,17 +1686,17 @@ var _ = Describe("Import populator", func() {
 			return err != nil && k8serrors.IsNotFound(err)
 		}, timeout, pollingInterval).Should(BeTrue())
 	},
-		Entry("[test_id:11001]with HTTP image and preallocation", utils.TinyCoreMD5, createHTTPImportPopulatorCR, true, false),
-		Entry("[test_id:11002]with HTTP image without preallocation", utils.TinyCoreMD5, createHTTPImportPopulatorCR, false, false),
-		Entry("[rfe_id:10985][crit:high][test_id:11003]with HTTP image and preallocation, with incomplete PVC webhook rendering", Serial, utils.TinyCoreMD5, createHTTPImportPopulatorCR, true, true),
-		Entry("[test_id:11004]with Registry image and preallocation", utils.TinyCoreMD5, createRegistryImportPopulatorCR, true, false),
-		Entry("[test_id:11005]with Registry image without preallocation", utils.TinyCoreMD5, createRegistryImportPopulatorCR, false, false),
-		Entry("[test_id:11006]with ImageIO image with preallocation", Serial, utils.ImageioMD5, createImageIOImportPopulatorCR, true, false),
-		Entry("[test_id:11007]with ImageIO image without preallocation", Serial, utils.ImageioMD5, createImageIOImportPopulatorCR, false, false),
-		Entry("[test_id:11008]with VDDK image with preallocation", utils.VcenterMD5, createVDDKImportPopulatorCR, true, false),
-		Entry("[test_id:11009]with VDDK image without preallocation", utils.VcenterMD5, createVDDKImportPopulatorCR, false, false),
-		Entry("[test_id:11010]with Blank image with preallocation", utils.BlankMD5, createBlankImportPopulatorCR, true, false),
-		Entry("[test_id:11011]with Blank image without preallocation", utils.BlankMD5, createBlankImportPopulatorCR, false, false),
+		Entry("[test_id:11001]with HTTP image and preallocation", utils.TinyCoreMD5, utils.UploadFileSize, createHTTPImportPopulatorCR, true, false),
+		Entry("[test_id:11002]with HTTP image without preallocation", utils.TinyCoreMD5, utils.UploadFileSize, createHTTPImportPopulatorCR, false, false),
+		Entry("[rfe_id:10985][crit:high][test_id:11003]with HTTP image and preallocation, with incomplete PVC webhook rendering", Serial, utils.TinyCoreMD5, utils.UploadFileSize, createHTTPImportPopulatorCR, true, true),
+		Entry("[test_id:11004]with Registry image and preallocation", utils.TinyCoreMD5, utils.UploadFileSize, createRegistryImportPopulatorCR, true, false),
+		Entry("[test_id:11005]with Registry image without preallocation", utils.TinyCoreMD5, utils.UploadFileSize, createRegistryImportPopulatorCR, false, false),
+		Entry("[test_id:11006]with ImageIO image with preallocation", Serial, utils.ImageioMD5, utils.CirrosRawFileSize, createImageIOImportPopulatorCR, true, false),
+		Entry("[test_id:11007]with ImageIO image without preallocation", Serial, utils.ImageioMD5, utils.CirrosRawFileSize, createImageIOImportPopulatorCR, false, false),
+		Entry("[test_id:11008]with VDDK image with preallocation", utils.VcenterMD5, utils.CirrosRawFileSize, createVDDKImportPopulatorCR, true, false),
+		Entry("[test_id:11009]with VDDK image without preallocation", utils.VcenterMD5, utils.CirrosRawFileSize, createVDDKImportPopulatorCR, false, false),
+		Entry("[test_id:11010]with Blank image with preallocation", utils.BlankMD5, units.MiB, createBlankImportPopulatorCR, true, false),
+		Entry("[test_id:11011]with Blank image without preallocation", utils.BlankMD5, units.MiB, createBlankImportPopulatorCR, false, false),
 	)
 
 	DescribeTable("should import Block PVC", func(expectedMD5 string, volumeImportSourceFunc func(cdiv1.DataVolumeContentType, bool) error) {
@@ -1723,7 +1724,7 @@ var _ = Describe("Import populator", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(md5).To(Equal(expectedMD5))
 		By("Verifying the image is sparse")
-		Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultPvcMountPath)).To(BeTrue())
+		Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultPvcMountPath, utils.UploadFileSize)).To(BeTrue())
 
 		By("Verify 100.0% annotation")
 		progress, ok, err := utils.WaitForPVCAnnotation(f.K8sClient, f.Namespace.Name, pvc, controller.AnnPopulatorProgress)
@@ -1799,8 +1800,6 @@ var _ = Describe("Import populator", func() {
 		md5, err := f.GetMD5(f.Namespace, pvc, utils.DefaultImagePath, utils.MD5PrefixSize)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(md5).To(Equal(utils.TinyCoreMD5))
-		By("Verifying the image is sparse")
-		Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath)).To(BeTrue())
 		sourceMD5 := md5
 
 		By("Retaining PV")

tests/transport_test.go

@@ -123,7 +123,7 @@ var _ = Describe("Transport Tests", func() {
 				}
 			}
 			By("Verifying the image is sparse")
-			Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath)).To(BeTrue())
+			Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath, utils.UploadFileSize)).To(BeTrue())
 		} else {
 			By("Verify PVC status annotation says failed")
 			found, err := utils.WaitPVCPodStatusRunning(f.K8sClient, pvc)

tests/upload_test.go

@@ -195,7 +195,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:[email protected]][level:compon
 				Expect(err).ToNot(HaveOccurred())
 				Expect(same).To(BeTrue())
 				By("Verifying the image is sparse")
-				Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath)).To(BeTrue())
+				Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath, utils.UploadFileSize)).To(BeTrue())
 				if utils.DefaultStorageCSIRespectsFsGroup {
 					// CSI storage class, it should respect fsGroup
 					By("Checking that disk image group is qemu")
@@ -411,7 +411,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:[email protected]][level:compon
 					Expect(err).ToNot(HaveOccurred())
 					Expect(same).To(BeTrue())
 					By("Verifying the image is sparse")
-					Expect(f.VerifySparse(f.Namespace, archivePVC, pathInPvc)).To(BeTrue())
+					Expect(f.VerifySparse(f.Namespace, archivePVC, pathInPvc, utils.UploadFileSize)).To(BeTrue())
 				}
 			} else {
 				checkFailureNoValidToken(archivePVC)
@@ -525,7 +525,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:[email protected]][level:compon
 						Expect(err).ToNot(HaveOccurred())
 						Expect(same).To(BeTrue())
 						By("Verifying the image is sparse")
-						Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath)).To(BeTrue())
+						Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath, utils.UploadFileSize)).To(BeTrue())
 						if utils.DefaultStorageCSIRespectsFsGroup {
 							// CSI storage class, it should respect fsGroup
 							By("Checking that disk image group is qemu")
@@ -599,7 +599,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:[email protected]][level:compon
 				Expect(err).ToNot(HaveOccurred())
 				Expect(same).To(BeTrue())
 				By("Verifying the image is sparse")
-				Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath)).To(BeTrue())
+				Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath, utils.UploadFileSize)).To(BeTrue())
 				if utils.DefaultStorageCSIRespectsFsGroup {
 					// CSI storage class, it should respect fsGroup
 					By("Checking that disk image group is qemu")
@@ -730,7 +730,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:[email protected]][level:compon
 						Expect(err).ToNot(HaveOccurred())
 						Expect(same).To(BeTrue())
 						By("Verifying the image is sparse")
-						Expect(f.VerifySparse(f.Namespace, pvc, pathInPvc)).To(BeTrue())
+						Expect(f.VerifySparse(f.Namespace, pvc, pathInPvc, utils.UploadFileSize)).To(BeTrue())
 					}
 				} else {
 					checkFailureNoValidToken(pvcPrime)

tests/utils/upload.go

@@ -41,6 +41,8 @@ const (
 
 	// UploadFileSize is the size of UploadFile
 	UploadFileSize = 18874368
+	// CirrosRawFileSize is the size of cirros.raw
+	CirrosRawFileSize = 46137344
 
 	// UploadFileMD5 is the expected MD5 of the uploaded file
 	UploadFileMD5 = "2a7a52285c846314d1dbd79e9818270d"