Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use statfs Bfree when calculating FS overhead #2194

Closed
wants to merge 1 commit into from

Conversation

maya-r
Copy link
Contributor

@maya-r maya-r commented Mar 21, 2022

Using Bavail takes into account root reservation, so we ended up
failing if 5% + 5.5% is not available.

This resulted in importing to just barely big enough DVs failed on
filesystem volumes that reserve some space for root.

Addresses bz#2059057

What this PR does / why we need it:
Significantly smaller change than #2193 - easier to backport.

The choice of 5.5% was meant to be larger than the root reservation typically chosen.
Choosing different behaviour for FS overhead will require us to change the numbers chosen, which is too involved.

Why not #2193: complicated, and removes the space validation.
Why not remove the space reservation: kubevirt runs unprivileged, and might bump into the limits for unprivileged processes.
Also it seems other storage providers for kubernetes don't do a space reservation for root.

No tests - I'm not sure we have a storage provider for CI which has root reservation, so I'm having trouble reproducing the scenario for failure.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
https://bugzilla.redhat.com/show_bug.cgi?id=2059057

Release note:

When validating space, ignore the root reservation.

Using Bavail takes into account root reservation, so we ended up
failing if 5% + 5.5% is not available.

This resulted in importing to just barely big enough DVs failed on
filesystem volumes that reserve some space for root.

Addresses bz#2059057

Signed-off-by: Maya Rashish <mrashish@redhat.com>
@kubevirt-bot 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 Mar 21, 2022
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign aglitke after the PR has been reviewed.
You can assign the PR to them by writing /assign @aglitke in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@maya-r
Copy link
Contributor Author

maya-r commented Mar 21, 2022

closing in favour of #2195.

Rationale: most users who want just barely big enough DVs are using the storage API (it's mostly vm-import-operator), which already pads their size requests to be big enough.
Let's avoid trying to coordinate two different checks of FS overhead, and limit ourselves to storage profiles creating a bigger PVC to account for the FS overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants