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 Apr 21, 2024
1 parent aab2017 commit e9c286a
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 45 deletions.
2 changes: 1 addition & 1 deletion tests/cloner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1897,7 +1897,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())
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 @@ -282,24 +281,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 @@ -546,26 +536,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
8 changes: 4 additions & 4 deletions tests/import_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,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 @@ -1659,7 +1659,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, utils.UploadFileSize)).To(BeTrue())
}

if utils.DefaultStorageCSIRespectsFsGroup {
Expand Down Expand Up @@ -1722,7 +1722,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 @@ -1799,7 +1799,7 @@ var _ = Describe("Import populator", func() {
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())
Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath, utils.UploadFileSize)).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 @@ -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")
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit e9c286a

Please sign in to comment.