Do not factor fs overhead into available space during validation (#2195)

* Create a test for an overhead bug

This image size and filesystem overhead combination was experimentally determined
to reproduce bz#2064936 in CI when using ceph/rbd with a Filesystem mode PV since
the filesystem capacity will be constrained by the PVC request size.

Below is the problem it tries to recreate:
When validating whether an image will fit into a PV we compare the
image's virtual size to the filesystem's reported available space to
guage whether it will fit.  The current calculation reduces the apparent
available space by the configured filesystem overhead value but the
overhead is already (mostly) factored into the result of Statfs.  This
causes the check to fail for PVCs that are just large enough to
accommodate an image plus overhead (ie. when using the DataVolume
Storage API with filesystem PVs with capacity constrained by the PVC
storage request size).

This was not caught in testing because HPP does not have capacity
constrained PVs and we are typically testing block volumes in the ceph
lanes.  It can be triggered in our CI by allocating a Filesystem PV on
ceph-rbd storage because these volumes are capacity constrained and
subject to filesystem overhead.

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Fix a target pvc validation bug

Corrects the validation logic for target volume.

Below description of the original problem:
When validating whether an image will fit into a PV we compare the
image's virtual size to the filesystem's reported available space to
guage whether it will fit.  The current calculation reduces the apparent
available space by the configured filesystem overhead value but the
overhead is already (mostly) factored into the result of Statfs.  This
causes the check to fail for PVCs that are just large enough to
accommodate an image plus overhead (ie. when using the DataVolume
Storage API with filesystem PVs with capacity constrained by the PVC
storage request size).

This was not caught in testing because HPP does not have capacity
constrained PVs and we are typically testing block volumes in the ceph
lanes.  It can be triggered in our CI by allocating a Filesystem PV on
ceph-rbd storage because these volumes are capacity constrained and
subject to filesystem overhead.

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Improve the warning message

Removed redundant and misleading part about pvc size and update the simplification

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Remove useless test

The test checks that the validation logic takes fs Overhead into account.
New validation logic does not check fs overhead. So test is no longer
relevant.

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>
This commit is contained in:
Bartosz Rybacki 2022-03-22 04:38:48 +01:00 committed by GitHub
parent 28f64436be
commit f89812c268
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 109 additions and 133 deletions

View File

@ -574,7 +574,7 @@ func setAnnotationsFromPodWithPrefix(anno map[string]string, pod *v1.Pod, prefix
}
func simplifyKnownMessage(msg string) string {
if strings.Contains(msg, "is larger than available size") ||
if strings.Contains(msg, "is larger than the reported available") ||
strings.Contains(msg, "no space left on device") ||
strings.Contains(msg, "file largest block is bigger than maxblock") {
return "DataVolume too small to contain image"

View File

@ -62,7 +62,7 @@ type QEMUOperations interface {
ConvertToRawStream(*url.URL, string, bool) error
Resize(string, resource.Quantity, bool) error
Info(url *url.URL) (*ImgInfo, error)
Validate(*url.URL, int64, float64) error
Validate(*url.URL, int64) error
CreateBlankImage(string, resource.Quantity, bool) error
Rebase(backingFile string, delta string) error
Commit(image string) error
@ -215,7 +215,7 @@ func isSupportedFormat(value string) bool {
}
}
func checkIfURLIsValid(info *ImgInfo, availableSize int64, filesystemOverhead float64, image string) error {
func checkIfURLIsValid(info *ImgInfo, availableSize int64, image string) error {
if !isSupportedFormat(info.Format) {
return errors.Errorf("Invalid format %s for image %s", info.Format, image)
}
@ -226,18 +226,18 @@ func checkIfURLIsValid(info *ImgInfo, availableSize int64, filesystemOverhead fl
}
}
if int64(float64(availableSize)*(1-filesystemOverhead)) < info.VirtualSize {
return errors.Errorf("Virtual image size %d is larger than available size %d (PVC size %d, reserved overhead %f%%). A larger PVC is required.", info.VirtualSize, int64((1-filesystemOverhead)*float64(availableSize)), info.VirtualSize, filesystemOverhead)
if availableSize < info.VirtualSize {
return errors.Errorf("Virtual image size %d is larger than the reported available storage %d. A larger PVC is required.", info.VirtualSize, availableSize)
}
return nil
}
func (o *qemuOperations) Validate(url *url.URL, availableSize int64, filesystemOverhead float64) error {
func (o *qemuOperations) Validate(url *url.URL, availableSize int64) error {
info, err := o.Info(url)
if err != nil {
return err
}
return checkIfURLIsValid(info, availableSize, filesystemOverhead, url.String())
return checkIfURLIsValid(info, availableSize, url.String())
}
// ConvertToRawStream converts an http accessible image to raw format without locally caching the image
@ -246,8 +246,8 @@ func ConvertToRawStream(url *url.URL, dest string, preallocate bool) error {
}
// Validate does basic validation of a qemu image
func Validate(url *url.URL, availableSize int64, filesystemOverhead float64) error {
return qemuIterface.Validate(url, availableSize, filesystemOverhead)
func Validate(url *url.URL, availableSize int64) error {
return qemuIterface.Validate(url, availableSize)
}
func reportProgress(line string) {

View File

@ -217,9 +217,9 @@ var _ = Describe("Resize", func() {
var _ = Describe("Validate", func() {
imageName, _ := url.Parse("myimage.qcow2")
table.DescribeTable("Validate should", func(execfunc execFunctionType, errString string, image *url.URL, overhead float64) {
table.DescribeTable("Validate should", func(execfunc execFunctionType, errString string, image *url.URL) {
replaceExecFunction(execfunc, func() {
err := Validate(image, 42949672960, overhead)
err := Validate(image, 42949672960)
if errString == "" {
Expect(err).NotTo(HaveOccurred())
@ -232,13 +232,12 @@ var _ = Describe("Validate", func() {
}
})
},
table.Entry("should return success", mockExecFunction(goodValidateJSON, "", expectedLimits, "info", "--output=json", imageName.String()), "", imageName, 0.0),
table.Entry("should return error", mockExecFunction("explosion", "exit 1", expectedLimits), "explosion, exit 1", imageName, 0.0),
table.Entry("should return error on bad json", mockExecFunction(badValidateJSON, "", expectedLimits), "unexpected end of JSON input", imageName, 0.0),
table.Entry("should return error on bad format", mockExecFunction(badFormatValidateJSON, "", expectedLimits), fmt.Sprintf("Invalid format raw2 for image %s", imageName), imageName, 0.0),
table.Entry("should return error on invalid backing file", mockExecFunction(backingFileValidateJSON, "", expectedLimits), fmt.Sprintf("Image %s is invalid because it has invalid backing file backing-file.qcow2", imageName), imageName, 0.0),
table.Entry("should return error when PVC is too small", mockExecFunction(hugeValidateJSON, "", expectedLimits), fmt.Sprintf("Virtual image size %d is larger than available size %d (PVC size %d, reserved overhead %f%%). A larger PVC is required.", 52949672960, 42949672960, 52949672960, 0.0), imageName, 0.0),
table.Entry("should return error when PVC is too small with overhead", mockExecFunction(hugeValidateJSON, "", expectedLimits), fmt.Sprintf("Virtual image size %d is larger than available size %d (PVC size %d, reserved overhead %f%%). A larger PVC is required.", 52949672960, 34359738368, 52949672960, 0.2), imageName, 0.2),
table.Entry("should return success", mockExecFunction(goodValidateJSON, "", expectedLimits, "info", "--output=json", imageName.String()), "", imageName),
table.Entry("should return error", mockExecFunction("explosion", "exit 1", expectedLimits), "explosion, exit 1", imageName),
table.Entry("should return error on bad json", mockExecFunction(badValidateJSON, "", expectedLimits), "unexpected end of JSON input", imageName),
table.Entry("should return error on bad format", mockExecFunction(badFormatValidateJSON, "", expectedLimits), fmt.Sprintf("Invalid format raw2 for image %s", imageName), imageName),
table.Entry("should return error on invalid backing file", mockExecFunction(backingFileValidateJSON, "", expectedLimits), fmt.Sprintf("Image %s is invalid because it has invalid backing file backing-file.qcow2", imageName), imageName),
table.Entry("should return error when PVC is too small", mockExecFunction(hugeValidateJSON, "", expectedLimits), fmt.Sprintf("Virtual image size %d is larger than the reported available storage %d. A larger PVC is required.", 52949672960, 42949672960), imageName),
)
})

View File

@ -286,7 +286,7 @@ func (dp *DataProcessor) ProcessDataWithPause() error {
func (dp *DataProcessor) validate(url *url.URL) error {
klog.V(1).Infoln("Validating image")
err := qemuOperations.Validate(url, dp.availableSpace, dp.filesystemOverhead)
err := qemuOperations.Validate(url, dp.availableSpace)
if err != nil {
return ValidationSizeError{err: err}
}

View File

@ -579,7 +579,7 @@ func (o *fakeQEMUOperations) ConvertToRawStream(*url.URL, string, bool) error {
return o.e2
}
func (o *fakeQEMUOperations) Validate(*url.URL, int64, float64) error {
func (o *fakeQEMUOperations) Validate(*url.URL, int64) error {
return o.e5
}

Binary file not shown.

View File

@ -330,90 +330,6 @@ var _ = Describe("[rfe_id:4784][crit:high] Importer respects node placement", fu
})
})
var _ = Describe("Importer CDI config manipulation tests", func() {
f := framework.NewFramework(namespacePrefix)
var (
config *cdiv1.CDIConfig
origSpec *cdiv1.CDIConfigSpec
err error
tinyCoreIsoURL = func() string { return fmt.Sprintf(utils.TinyCoreIsoURL, f.CdiInstallNs) }
)
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(), common.ConfigName, metav1.GetOptions{})
Expect(err).ToNot(HaveOccurred())
return reflect.DeepEqual(config.Spec, *origSpec)
}, 30*time.Second, time.Second).Should(BeTrue())
})
dvWithPvcSpec := func() *cdiv1.DataVolume {
return utils.NewDataVolumeWithHTTPImport("too-large-import", "50Mi", tinyCoreIsoURL())
}
dvWithStorageSpec := func() *cdiv1.DataVolume {
return utils.NewDataVolumeWithHTTPImportAndStorageSpec("too-large-import", "50Mi", tinyCoreIsoURL())
}
// value of .80 means 80% of filesystem is reserved for metadata,
// given 100Mi PVC, 80Mi is reserved, 20Mi available for image, we test using 50Mi PVC and around 20Mi image
highFsOverhead := "0.80"
DescribeTable("Filesystem overhead is honored with a RAW file", func(dvFunc func() *cdiv1.DataVolume, expectedSuccess bool, globalOverhead, scOverhead string) {
tests.SetFilesystemOverhead(f, globalOverhead, scOverhead)
dv := dvFunc()
By(dv.Name)
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(), "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", dvWithPvcSpec, true, "0.1", ""),
Entry("[posneg:negative] Fail with high global overhead", dvWithPvcSpec, false, highFsOverhead, ""),
Entry("Succeed with high global overhead when using spec.storage", dvWithStorageSpec, true, highFsOverhead, ""),
Entry("Succeed with low per-storageclass overhead (despite high global overhead)", dvWithPvcSpec, true, highFsOverhead, "0.1"),
Entry("[posneg:negative] Fail with high per-storageclass overhead (despite low global overhead)", dvWithPvcSpec, false, "0.1", highFsOverhead),
)
})
var _ = Describe("[rfe_id:1118][crit:high][vendor:cnv-qe@redhat.com][level:component]Importer Test Suite-prometheus", func() {
var prometheusURL string
var portForwardCmd *exec.Cmd

View File

@ -349,7 +349,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:compon
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") {
if strings.Contains(log, "is larger than the reported available") {
return true
}
if strings.Contains(log, "no space left on device") {
@ -821,33 +821,6 @@ var _ = Describe("CDIConfig manipulation upload tests", func() {
Expect(token).ToNot(BeEmpty())
})
DescribeTable("Async upload with filesystem overhead", func(expectedStatus int, globalOverhead, scOverhead string) {
tests.SetFilesystemOverhead(f, globalOverhead, scOverhead)
By("Creating PVC with upload target annotation")
pvc, err := f.CreateBoundPVCFromDefinition(utils.UploadPVCDefinition())
Expect(err).ToNot(HaveOccurred())
By("Verify PVC annotation says ready")
found, err := utils.WaitPVCPodStatusReady(f.K8sClient, pvc)
Expect(err).ToNot(HaveOccurred())
Expect(found).To(BeTrue())
By("Get an upload token")
token, err := utils.RequestUploadToken(f.CdiClient, pvc)
Expect(err).ToNot(HaveOccurred())
Expect(token).ToNot(BeEmpty())
By("Do upload")
Eventually(func() error {
return uploadImageAsync(uploadProxyURL, token, expectedStatus)
}, timeout, pollingInterval).Should(BeNil(), "uploadImageAsync should return nil, even if not ready")
},
Entry("[test_id:4548] Succeed with low global overhead", http.StatusOK, "0.1", ""),
Entry("[test_id:4672][posneg:negative] Fail with high global overhead", http.StatusBadRequest, "0.99", ""),
Entry("[test_id:4673] Succeed with low per-storageclass overhead (despite high global overhead)", http.StatusOK, "0.99", "0.1"),
Entry("[test_id:4714][posneg:negative] Fail with high per-storageclass overhead (despite low global overhead)", http.StatusBadRequest, "0.1", "0.99"),
)
})
var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:component] Upload tests", func() {
@ -891,6 +864,62 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:compon
Expect(deleted).To(BeTrue())
})
It("Upload an image exactly the same size as DV request (bz#2064936)", func() {
// This image size and filesystem overhead combination was experimentally determined
// to reproduce bz#2064936 in CI when using ceph/rbd with a Filesystem mode PV since
// the filesystem capacity will be constrained by the PVC request size.
size := "858993459"
fsOverhead := "0.055" // The default value
tests.SetFilesystemOverhead(f, fsOverhead, fsOverhead)
volumeMode := v1.PersistentVolumeMode(v1.PersistentVolumeFilesystem)
accessModes := []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}
dvName := "upload-dv"
By(fmt.Sprintf("Creating new datavolume %s", dvName))
dv := utils.NewDataVolumeForUploadWithStorageAPI(dvName, size)
dv.Spec.Storage.AccessModes = accessModes
dv.Spec.Storage.VolumeMode = &volumeMode
dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv)
pvc = utils.PersistentVolumeClaimFromDataVolume(dataVolume)
By("verifying pvc was created, force bind if needed")
pvc, err := utils.WaitForPVC(f.K8sClient, pvc.Namespace, pvc.Name)
Expect(err).ToNot(HaveOccurred())
f.ForceBindIfWaitForFirstConsumer(pvc)
phase := cdiv1.UploadReady
By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase)))
err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name)
if err != nil {
dv, dverr := f.CdiClient.CdiV1beta1().DataVolumes(f.Namespace.Name).Get(context.TODO(), dataVolume.Name, metav1.GetOptions{})
if dverr != nil {
Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase))
}
}
Expect(err).ToNot(HaveOccurred())
By("Get an upload token")
token, err := utils.RequestUploadToken(f.CdiClient, pvc)
Expect(err).ToNot(HaveOccurred())
Expect(token).ToNot(BeEmpty())
By("Do upload")
Eventually(func() error {
return uploadFileNameToPath(binaryRequestFunc, utils.FsOverheadFile, uploadProxyURL, syncUploadPath, token, http.StatusOK)
}, timeout, pollingInterval).Should(BeNil(), "Upload should eventually succeed, even if initially pod is not ready")
phase = cdiv1.Succeeded
By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase)))
err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name)
if err != nil {
dv, dverr := f.CdiClient.CdiV1beta1().DataVolumes(f.Namespace.Name).Get(context.TODO(), dataVolume.Name, metav1.GetOptions{})
if dverr != nil {
Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase))
}
}
Expect(err).ToNot(HaveOccurred())
})
It("[test_id:3993] Upload image to data volume and verify retry count", func() {
dvName := "upload-dv"
By(fmt.Sprintf("Creating new datavolume %s", dvName))

View File

@ -362,6 +362,29 @@ func NewDataVolumeForUpload(dataVolumeName string, size string) *cdiv1.DataVolum
}
}
// NewDataVolumeForUploadWithStorageAPI initializes a DataVolume struct with Upload annotations
// and using the Storage API instead of the PVC API
func NewDataVolumeForUploadWithStorageAPI(dataVolumeName string, size string) *cdiv1.DataVolume {
return &cdiv1.DataVolume{
ObjectMeta: metav1.ObjectMeta{
Name: dataVolumeName,
Annotations: map[string]string{},
},
Spec: cdiv1.DataVolumeSpec{
Source: &cdiv1.DataVolumeSource{
Upload: &cdiv1.DataVolumeSourceUpload{},
},
Storage: &cdiv1.StorageSpec{
Resources: k8sv1.ResourceRequirements{
Requests: k8sv1.ResourceList{
k8sv1.ResourceName(k8sv1.ResourceStorage): resource.MustParse(size),
},
},
},
},
}
}
// NewDataVolumeForBlankRawImage initializes a DataVolume struct for creating blank raw image
func NewDataVolumeForBlankRawImage(dataVolumeName, size string) *cdiv1.DataVolume {
return &cdiv1.DataVolume{
@ -624,6 +647,12 @@ func NewDataVolumeWithArchiveContent(dataVolumeName string, size string, httpURL
// PersistentVolumeClaimFromDataVolume creates a PersistentVolumeClaim definition so we can use PersistentVolumeClaim for various operations.
func PersistentVolumeClaimFromDataVolume(datavolume *cdiv1.DataVolume) *corev1.PersistentVolumeClaim {
// This function can work with DVs that use the 'Storage' field instead of the 'PVC' field
spec := corev1.PersistentVolumeClaimSpec{}
if datavolume.Spec.PVC != nil {
spec = *datavolume.Spec.PVC
}
return &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: datavolume.Name,
@ -636,7 +665,7 @@ func PersistentVolumeClaimFromDataVolume(datavolume *cdiv1.DataVolume) *corev1.P
}),
},
},
Spec: *datavolume.Spec.PVC,
Spec: spec,
}
}

View File

@ -22,6 +22,9 @@ const (
CirrosQCow2File = "/cirros-qcow2.img"
// UploadFile is the file to upload
UploadFile = imagesPath + TinyCoreFile
// FsOverheadFile a file with some arbitrary size to check the fsOverhead validation logic
FsOverheadFile = imagesPath + "/fs-overhead.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)