Increase default filesystem overhead to 6%#3812
Conversation
|
Skipping CI for Draft Pull Request. |
kubevirt-bot
added
do-not-merge/work-in-progress
The previous default of 5.5% was originally chosen to account for filesystem overhead. Due to inaccuracies in our overhead inflation calculations, the actual effective overhead was closer to 5.8%. Now that the calculation functions have been fixed, we're increasing the default to 6% to ensure sufficient inflation for typical filesystem overhead and avoid under-provisioning. Signed-off-by: Alvaro Romero <[email protected]>
alromeros
force-pushed
the
update-default-fsoverhead
branch
from
18d738b to
6e8a8fb
Compare
kubevirt-bot
removed
the
needs-rebase
|
/test pull-containerized-data-importer-e2e-ceph |
alromeros
changed the title
kubevirt-bot
removed
the
do-not-merge/work-in-progress
|
how is upgrade handled? If the user never specified their own overhead will the new default be applied? |
The controller reconciles the overhead by setting the default value if none is specified in the spec https://github.com/kubevirt/containerized-data-importer/blob/main/pkg/controller/config-controller.go#L504 so yeah, if the user didn't set any overhead the new default should be applied on upgrade. |
|
/retest |
|
/approve |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
akalenyu
left a comment
akalenyu
left a comment
There was a problem hiding this comment.
looks good, really minor comment
| size := "2147483648" | ||
| fsOverhead := "0.055" // The default value | ||
| fsOverhead := "0.06" // The default value | ||
| tests.SetFilesystemOverhead(f, fsOverhead, fsOverhead) |
There was a problem hiding this comment.
I'm not sure why this test sets the default? should already be there
There was a problem hiding this comment.
I assumed we wanted to make sure we are using the default here in case we chose a different value in cdiConfig, but just might be some leftover from legacy behavior. I can remove it if you want.
There was a problem hiding this comment.
It seems off to me, if other tests change it they probably:
- label themselves Serial
- Revert back in AfterEach
Regarding the "input cluster" having a different overhead, I'd say we want to test it as is, but I'm not aware of such testing setup existing
|
|
||
| It("Filesystem overhead is honored with blank volume", Serial, func() { | ||
| tests.SetFilesystemOverhead(f, "0.055", "0.055") | ||
| tests.SetFilesystemOverhead(f, "0.06", "0.06") |
There was a problem hiding this comment.
Same point about "setting" it to the default
Signed-off-by: Alvaro Romero <[email protected]>
Signed-off-by: Alvaro Romero <[email protected]>
alromeros
force-pushed
the
update-default-fsoverhead
branch
from
6e8a8fb to
ecc6314
Compare
|
/cherrypick release-v1.63 |
|
@alromeros: new pull request created: #3857 DetailsIn response to this:
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-sigs/prow repository. |
6 participants
What this PR does / why we need it:
The previous default of 5.5% was originally chosen to account for filesystem overhead. Due to inaccuracies in our overhead inflation calculations, the actual effective overhead was closer to 5.8%.
Now that the calculation functions have been fixed in #3779, we're increasing the default to 6% to ensure sufficient inflation for typical filesystem overhead and avoid under-provisioning.
Feel free to propose a different value. 7% was considered in the past (I think around kubevirt/kubevirt#13326 was merged) but 6% is closer to the original and apparently solves the issue we had with smaller PVCs.
Release note: