Reserve overhead when validating that a Filesystem has enough space #1319

maya-r · 2020-07-29T20:45:59Z

When validating disk space, reserve space for filesystem overhead

The amount of available space in a filesystem is not exactly
the advertise amount. Things like indirect blocks or metadata
may use up some of this space. Reserving it to avoid reaching
full capacity by default.

This value is configurable from the CDIConfig object spec,
both globally and per-storageclass.

The default value is 0.055, or "5.5% of the space is
reserved". This value was chosen because some filesystems
reserve 5% of the space as overhead for the root user and
this space doubles as reservation for the worst case
behaviour for unclear space usage. I've chosen a value
that is slightly higher.

This validation is only necessary because we use sparse
images instead of fallocated ones, which was done to have
reasonable alerts regarding space usage from various
storage providers.

Update CDIConfig filesystemOverhead status, validate, and
pass the final value to importer/upload pods.

Only the status values controlled by the config controller
are used, and it's filled out for all available storage
classes in the cluster.

Use this value in Validate calls to ensure that some of the
space is reserved for the filesystem overhead to guard from
accidents.

Caveats:

Doesn't use Default: to define the default of 0.055, instead
it is hard-coded in reconcile. It seems like we can't use a
default value.

Release note:

By default, reserve 5.5% of a filesystem for filesystem overhead.
This value is configurable from the CDIConfig object spec, both globally and per-storageclass.

kubectl edit cdiconfig config:
spec:
  filesystemOverhead:
    global: "override-0.055-for-all-values"
    storageClass:
      mystorageclass: "override-per-storageclass"

maya-r · 2020-07-29T20:46:40Z

Missing doc, unit tests, functional tests.
Code works, so I figured it's time to put it as a WIP pull request.

coveralls · 2020-07-29T21:07:27Z

Coverage decreased (-0.2%) to 74.611% when pulling 89a137d on maya-r:reserve-fs-overhead into 93fa687 on kubevirt:master.

awels · 2020-07-30T12:51:00Z

/retest

awels · 2020-07-30T12:56:19Z

pkg/controller/config-controller.go

@@ -211,6 +216,62 @@ func (r *CDIConfigReconciler) reconcileDefaultPodResourceRequirements(config *cd
 	return nil
 }

+func (r *CDIConfigReconciler) reconcileFilesystemOverhead(config *cdiv1.CDIConfig) error {
+	var globalOverhead cdiv1.Percent = "0.055"


Can you make "0.055" a const name defaultGlobalOverhead or something?

pkg/controller/config-controller.go

awels · 2020-07-30T13:10:24Z

pkg/controller/import-controller.go

@@ -412,6 +413,10 @@ func (r *ImportReconciler) createImportEnvVar(pvc *corev1.PersistentVolumeClaim)
 		if err != nil {
 			return nil, err
 		}
+		podEnvVar.filesystemOverhead, err = GetFilesystemOverhead(r.uncachedClient, pvc)


Any reason you are using the uncachedClient instead of the client here?

pkg/controller/util.go

awels · 2020-07-30T13:21:35Z

pkg/image/qemu_test.go

@@ -226,7 +226,7 @@ var _ = Describe("Validate", func() {
 		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 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 available size %d. A larger PVC is required.", 52949672960, 42949672960), imageName),
+		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),


Can we add a non 0 overhead test as well?

awels · 2020-07-30T13:26:20Z

pkg/operator/resources/cluster/cdiconfig.go

+									Type:        "object",
+									Properties: map[string]extv1beta1.JSONSchemaProps{
+										"global": {
+											Description: "How much space of a Filesystem volume should be reserved for safety. This value is the global one to be used unless a per-storageClass value is chosen.",


You can add "Pattern", here with a regular expression, this will stop invalid values from reaching our controllers.

pkg/apis/core/v1beta1/types.go

awels · 2020-07-30T14:24:50Z

pkg/operator/resources/cluster/cdiconfig.go

+											Description: "How much space of a Filesystem volume should be reserved for safety. This value is the global one to be used unless a per-storageClass value is chosen.",
+											Type:        "string",
+										},
+										"storageClass": {


We need to specify more here about what goes in, in particular we want the pattern on value as well. It should look something like this:

"storageClass": { Items: &extv1beta1.JSONSchemaPropsOrArray{ Schema: &extv1beta1.JSONSchemaProps{ Description: "Some description explaining what the storage class map does, same as in types.go", Type: "array", Properties: map[string]extv1beta1.JSONSchemaProps{ "storageClass": { Description: "Name of the storage class", Type: "object", }, "overhead": { Description: "The overhead as a decimal", Type: "object", Pattern: "regex for pattern", }, },

Not entirely sure this will work since you used a map. But I think the validation is useful

mhenriks · 2020-07-31T17:23:04Z

pkg/apis/core/v1beta1/types.go

+// XXX: validation doesn't actually seem to do anything.
+// Actually relies on the reconcile not to use bogus values.
+// +kubebuilder:validation:Pattern=`^(0\.[0-9]+|0)$`
+type Percent string


Why is a string better than using a float?

May be nice to have a NewPercent function that validates and returns a Percent.

An actual float is problematic for portability reasons, and so the tooling discourages it.

mhenriks · 2020-07-31T17:25:08Z

pkg/controller/config-controller.go

+		storageClassNameOverhead, found := perStorageConfig[storageClassName]
+
+		if found {
+			valid, err := validOverhead(storageClassNameOverhead)


can validation be enforced by CRD schema?

There is a regexp on the field now, to enforce 0 <= overhead <= 1

btw: I only managed to add it to the global overhead one, not to the map[storageclass]overhead.

pkg/apis/core/v1alpha1/types.go

maya-r · 2020-09-09T15:33:21Z

I think I'm going to have to add validation to a lot of other data sources, but that's probably a separate PR

maya-r · 2020-09-10T12:06:24Z

The tests are fine if run independently, I just introduced some flakiness

maya-r · 2020-09-23T21:32:32Z

The NFS tests still fail locally as being flaky. Simply adding a wait between the tests helps, but I don't know what else I could be doing to un-flake them.

The amount of available space in a filesystem is not exactly the advertise amount. Things like indirect blocks or metadata may use up some of this space. Reserving it to avoid reaching full capacity by default. This value is configurable from the CDIConfig object spec, both globally and per-storageclass. The default value is 0.055, or "5.5% of the space is reserved". This value was chosen because some filesystems reserve 5% of the space as overhead for the root user and this space doubles as reservation for the worst case behaviour for unclear space usage. I've chosen a value that is slightly higher. This validation is only necessary because we use sparse images instead of fallocated ones, which was done to have reasonable alerts regarding space usage from various storage providers. --- Update CDIConfig filesystemOverhead status, validate, and pass the final value to importer/upload pods. Only the status values controlled by the config controller are used, and it's filled out for all available storage classes in the cluster. Use this value in Validate calls to ensure that some of the space is reserved for the filesystem overhead to guard from accidents. Caveats: Doesn't use Default: to define the default of 0.055, instead it is hard-coded in reconcile. It seems like we can't use a default value. Validates the per-storageClass values in reconcile, and doesn't reject bad values. Signed-off-by: Maya Rashish <mrashish@redhat.com>

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

kubevirt-bot · 2020-09-24T22:20:05Z

@maya-r: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-containerized-data-importer-e2e-os-3.11.0-crio	`d53bec6`	link	`/test pull-containerized-data-importer-e2e-os-3.11.0-crio`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

maya-r · 2020-09-30T06:59:43Z

/test pull-containerized-data-importer-e2e-k8s-1.17-ceph

awels

Looking pretty good, a few comments and one want. I would like to see a functional test in cdiconfig_test.go that verifies that if you add an invalid value for the percent field, that the update fails. So things like -0.04 or 1.00 or 0.9999 which don't match the regex.

pkg/controller/util.go

awels · 2020-09-30T17:26:22Z

tests/framework/nfs-utils.go

+		if err := utils.DeletePV(client, pv); err != nil {
+			return err
+		}
+		if err := utils.WaitTimeoutForPVDeleted(client, pv, timeout); err != nil {


Optimization here, right now you are doing the following:

Delete PV

Wait For PV to actually be gone

Go to 1
So you are sequentially waiting for the PV to be deleted before starting a new delete. How about you do

for i := 1; i <= pvCount; i++ { pv := nfsPVDef(strconv.Itoa(i), utils.NfsService.Spec.ClusterIP) if err := utils.DeletePV(client, pv); err != nil { return err } } // kicked off deletion of all PVs I care about for i := 1; i <= pvCount; i++ { if err := utils.WaitTimeoutForPVDeleted(client, pv, timeout); err != nil { return err } }

In this order you start the deletion of the PVs before waiting for any of them to really be deleted. That way the deletion is in parallell while the waiting is sequential. And hopefully the wait will be less because when you get to the wait for the later PVs, they will be gone already.

Good idea. done

So the way it is now, it looks like you are waiting for just the last PV (since the PV variable is set to the last at the end of the first loop). But there is no guarantee in the order in which the PVs are deleted, it is possible at this point that some of the earlier ones aren't gone yet. I would either just re-create the pv defs in the second loop, or store them in a slice and loop over the slice in the second loop to ensure all the PVs are gone at the end.

awels · 2020-09-30T17:32:23Z

pkg/controller/config-controller.go

+		storageClassNameOverhead, found := perStorageConfig[storageClassName]
+
+		if found {
+			valid, err := validOverhead(storageClassNameOverhead)


There is a regexp on the field now, to enforce 0 <= overhead <= 1

awels · 2020-09-30T17:33:15Z

pkg/controller/import-controller.go

@@ -440,6 +442,11 @@ func (r *ImportReconciler) createImportEnvVar(pvc *corev1.PersistentVolumeClaim)
 		if err != nil {
 			return nil, err
 		}
+		fsOverhead, err := GetFilesystemOverhead(r.uncachedClient, pvc)


Any particular reason to use the uncached client here? I see the cached client being used in the uploadServer.

Nope, missed it. thanks

Intended to help with flakes, but didn't make a difference. Probably still worth doing. Signed-off-by: Maya Rashish <mrashish@redhat.com>

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

Note that this change isn't expected to make a difference, as we check if the targetStorageClass is nil later on and have the same behaviour, but this is probably more correct API usage. Signed-off-by: Maya Rashish <mrashish@redhat.com>

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

awels

I think there is a small logic error in the nfs deletion logic. Otherwise I am happy.

awels · 2020-10-01T12:28:07Z

tests/framework/nfs-utils.go

+		if err := utils.DeletePV(client, pv); err != nil {
+			return err
+		}
+		if err := utils.WaitTimeoutForPVDeleted(client, pv, timeout); err != nil {


So the way it is now, it looks like you are waiting for just the last PV (since the PV variable is set to the last at the end of the first loop). But there is no guarantee in the order in which the PVs are deleted, it is possible at this point that some of the earlier ones aren't gone yet. I would either just re-create the pv defs in the second loop, or store them in a slice and loop over the slice in the second loop to ensure all the PVs are gone at the end.

Wait for all of them, not just the last one. Signed-off-by: Maya Rashish <mrashish@redhat.com>

awels

/lgtm
/approve

kubevirt-bot · 2020-10-01T12:51:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [awels]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 29, 2020

maya-r marked this pull request as draft July 29, 2020 20:46

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL labels Jul 29, 2020

kubevirt-bot requested review from awels and mhenriks July 29, 2020 20:46

awels requested changes Jul 30, 2020

View reviewed changes

kubevirt-bot assigned awels Jul 30, 2020

mhenriks reviewed Jul 31, 2020

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2020

maya-r force-pushed the reserve-fs-overhead branch from d53bec6 to 9b34faa Compare September 9, 2020 07:21

awels reviewed Sep 9, 2020

View reviewed changes

pkg/apis/core/v1alpha1/types.go Outdated Show resolved Hide resolved

maya-r force-pushed the reserve-fs-overhead branch from 9b34faa to 3f1d0ee Compare September 9, 2020 15:14

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2020

maya-r force-pushed the reserve-fs-overhead branch from 3f1d0ee to 85f4101 Compare September 9, 2020 15:15

maya-r marked this pull request as ready for review September 9, 2020 15:33

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2020

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2020

maya-r force-pushed the reserve-fs-overhead branch from fd76730 to cb60be7 Compare September 23, 2020 21:31

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2020

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2020

maya-r force-pushed the reserve-fs-overhead branch from cb60be7 to 9acaa33 Compare September 24, 2020 14:46

kubevirt-bot added size/XXL and removed size/XL labels Sep 24, 2020

maya-r force-pushed the reserve-fs-overhead branch from 9acaa33 to 1c48937 Compare September 24, 2020 14:56

kubevirt-bot added size/XL and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL labels Sep 24, 2020

maya-r added 3 commits September 24, 2020 22:02

Use util GetStorageClassByName

e644de8

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

Test filesystem overhead validation against async upload endpoint

eca28b1

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

maya-r force-pushed the reserve-fs-overhead branch from 1c48937 to e477664 Compare September 24, 2020 19:03

awels reviewed Sep 30, 2020

View reviewed changes

maya-r added 4 commits October 1, 2020 02:09

wait for NFS PVs to be deleted before continuing

ac7efc3

Intended to help with flakes, but didn't make a difference. Probably still worth doing. Signed-off-by: Maya Rashish <mrashish@redhat.com>

Avoid using the uncached client unnecessarily

838a5d3

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

Add testing for the validation of filesystem overhead values

cec1791

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

maya-r force-pushed the reserve-fs-overhead branch from e477664 to cec1791 Compare October 1, 2020 01:19

awels reviewed Oct 1, 2020

View reviewed changes

Fix logical error in waiting for NFS PVs.

89a137d

Wait for all of them, not just the last one. Signed-off-by: Maya Rashish <mrashish@redhat.com>

awels approved these changes Oct 1, 2020

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2020

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2020

kubevirt-bot merged commit b91887e into kubevirt:master Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reserve overhead when validating that a Filesystem has enough space #1319

Reserve overhead when validating that a Filesystem has enough space #1319

maya-r commented Jul 29, 2020

maya-r commented Jul 29, 2020

coveralls commented Jul 29, 2020 •

edited

awels commented Jul 30, 2020

awels Jul 30, 2020

awels Jul 30, 2020

awels Jul 30, 2020

awels Jul 30, 2020

awels Jul 30, 2020

mhenriks Jul 31, 2020

maya-r Sep 6, 2020

mhenriks Jul 31, 2020

awels Sep 30, 2020

maya-r Sep 30, 2020

maya-r commented Sep 9, 2020

maya-r commented Sep 10, 2020

maya-r commented Sep 23, 2020

kubevirt-bot commented Sep 24, 2020 •

edited

maya-r commented Sep 30, 2020

awels left a comment

awels Sep 30, 2020

maya-r Oct 1, 2020

awels Oct 1, 2020

awels Sep 30, 2020

awels Sep 30, 2020

maya-r Oct 1, 2020

awels left a comment

awels Oct 1, 2020

awels left a comment

kubevirt-bot commented Oct 1, 2020

Reserve overhead when validating that a Filesystem has enough space #1319

Reserve overhead when validating that a Filesystem has enough space #1319

Conversation

maya-r commented Jul 29, 2020

maya-r commented Jul 29, 2020

coveralls commented Jul 29, 2020 • edited

awels commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maya-r commented Sep 9, 2020

maya-r commented Sep 10, 2020

maya-r commented Sep 23, 2020

kubevirt-bot commented Sep 24, 2020 • edited

maya-r commented Sep 30, 2020

awels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Oct 1, 2020

coveralls commented Jul 29, 2020 •

edited

kubevirt-bot commented Sep 24, 2020 •

edited