Skip to content

Commit

Permalink
Correct wrong check of resulting image sparsiness
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@akalenyu
akalenyu committed Jul 29, 2024
1 parent 368ad80 commit 647e247
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 57 deletions.
1 change: 1 addition & 0 deletions tests/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ go_test(
"//tests/framework:go_default_library",
"//tests/reporter:go_default_library",
"//tests/utils:go_default_library",
"//vendor/github.com/docker/go-units:go_default_library",
"//vendor/github.com/ghodss/yaml:go_default_library",
"//vendor/github.com/google/uuid:go_default_library",
"//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1:go_default_library",
Expand Down
38 changes: 4 additions & 34 deletions tests/framework/pvc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"encoding/json"
"fmt"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
33 changes: 16 additions & 17 deletions tests/import_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -128,7 +129,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 {
Expand Down Expand Up @@ -1636,7 +1637,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 {
Expand Down Expand Up @@ -1670,7 +1671,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 {
Expand All @@ -1695,17 +1696,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) {
Expand Down Expand Up @@ -1733,7 +1734,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)
Expand Down Expand Up @@ -1809,8 +1810,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")
Expand Down
2 changes: 1 addition & 1 deletion tests/transport_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 5 additions & 5 deletions tests/upload_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,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")
Expand Down Expand Up @@ -414,7 +414,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)
Expand Down Expand Up @@ -529,7 +529,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")
Expand Down Expand Up @@ -603,7 +603,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")
Expand Down Expand Up @@ -734,7 +734,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)
Expand Down
2 changes: 2 additions & 0 deletions tests/utils/upload.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 647e247

Please sign in to comment.