lib/repo: Allow min-free-space-size and -percent to co-exist #1685
Conversation
|
Can one of the admins verify this patch? |
|
bot, add author to whitelist |
|
I've submitted this for @uajain because he's off sick today and I wanted this to be "on the official record" before any releases or anything went out per #1683. We ran into nasty problems in Endless when an older flatpak/ostree accessed the repo (eg running GNOME Builder from a flatpak, or sharing a repo between different os version chroots), because the old flatpak code ignored -percent and re-added the min-free-space-size thing which the new code then bailed on. |
|
Hmm, would it be easier at this point to just respect both of them? That's the behaviour I'd expect anyway if we support both options to be set. E.g. when calculating |
|
@jlebon Heh... I guess... that's quite a reversal from prohibiting that both are set.... Does it adds much value, because if we do want to change the ostree default from a percentage to an absolute value, we're going to want to clear away the 3% default. Flatpak has been wiping the percentage default away for ages and only (potentially) wants an absolute value from here on out too. |
|
OK, so to recap here: on Endless, flatpak uses the host repo, and because flatpak just does I just want to make sure we don't make this more complicated than it already is. :) |
|
You can ignore the fact Endless shares the repo. This PR corrects the fact that ostree preventing the new and old options from co-existing is a backwards-incompatible change, because an old version that doesn't understand the new option can access/set it and render the repo broken. Ie, even though the new flatpak removes the setting, a fatal error occurs if an old version of Flatpak accesses the repo, re-adding the -percent variant. From then on, even the new Flatpak cannot open the repo, so everything is broken. We need to either allow the options to co-exist by having one win over the other (as set out in this PR) or by respecting both. I don't mind which approach is taken, because primarily, Flatpak will enforce that only the one it wants is set. Additionally, I recommend ostree considers changing the default in any case. |
src/libostree/ostree-repo.c
Outdated
| if (self->min_free_space_percent != 0 && self->min_free_space_mb != 0) | ||
| { | ||
| self->min_free_space_percent = 0; | ||
| g_warning ("Both min-free-space-percent and -size are mentioned in config. Enforcing min-free-space-size check only."); |
There was a problem hiding this comment.
I am not a fan of g_warning() in general. It seriously annoys me how prevalent it is in GNOME - there's a constant stream of warnings that we've shipped to users forever from apps and the core that end up spamming the journal and don't really get fixed.
It's pretty similar to what ends up happening with Python apps when a library deprecates an API - those aren't for users to fix, they're for developers.
rpm-ostree's test suite runs with G_DEBUG=fatal-warnings and I am tempted to turn that on by default too.
Since we know people are going to hit this for a while, let's just not emit a warning. At some point the newer ostree and flatpak will propagate most places and there won't be a problem anymore.
I am not too concerned about the case of an admin adding both manually. If they don't read the docs that's not our fault.
There was a problem hiding this comment.
(Sort of tangentially related...an interesting middle ground is what Linux has with e.g. WARN_ON_ONCE() - except that's hard to implement in userspace since it's very common for processes to be restarted and maintaining the warning state outside would get tricky - would probably require assistance from journald somehow)
Mmm...I think I'd vote to keep the logic from the current patch for now. If you have an absolute size, why would you also specify a percentage? |
|
Overall LGTM with the |
Previously, we would error out if both of the options were mentioned
in the config file (even if one of them is disabled with 0). There
were few suggestions that this behavior was not quite right.
Therefore, instead of throwing error and exiting, it's preferred to
warn the user. Hence, the solution that worked out is:
* Allow both options to exist simulateneously
* Check each config's value and decide:
* If both are present and are non-zero, warn the user. Also, prefer
to use min-free-space-size over the another.
* If both are absent, then use -percent=3% as fallback
* Every other case is valid hence, no warning
https://phabricator.endlessm.com/T13698
(cherry picked from commit be68991cf80f0aa1da7d36ab6e1d2c4d6c7cd3fb)
Signed-off-by: Robert McQueen <rob@endlessm.com>
uajain
force-pushed
the
min-space-coexist
branch
from
3efebbd
to
66a8d44
Compare
|
Thank you @cgwalters for the review. I now understand your point with |
|
@rh-atomic-bot r+ 66a6d44 |
|
🙀 |
Thanks, that clears it up nicely for me! |
|
☀️ Test successful - status-atomicjenkins |
Previously, we would error out if both of the options were mentioned
in the config file (even if one of them is disabled with 0). There
were few suggestions that this behavior was not quite right.
Therefore, instead of throwing error and exiting, it's preferred to
warn the user. Hence, the solution that worked out is:
to use min-free-space-size over the another.
https://phabricator.endlessm.com/T13698
(cherry picked from commit be68991cf80f0aa1da7d36ab6e1d2c4d6c7cd3fb)
Signed-off-by: Robert McQueen rob@endlessm.com