storage: don't suggest use-gap if we can't fit more partitions #2031
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Expect for manual partitioning, we should ideally suggest guided scenarios only if we are strongly confident that they can be applied. Currently, we only perform some basic checks before suggesting scenarios. Going forward, I think we should be more cautious by trying to apply the scenarios in a throwaway filesystem model. If the scenario fails to apply, we should not suggest it. There is a lot of work involved so this is marked as a TODO for now. Signed-off-by: Olivier Gayot <[email protected]>
|
While looking for bugs on LP, I found many other that are similar in essence but for different types of guided scenarios e.g., Resize, Reformat. We should probably implement the same type of check going forward. |
dbungert
approved these changes
Jul 17, 2024
There was a problem hiding this comment.
Thanks for this Olivier. I believe this is correct.
Two non-blocking comments:
- as you've already observed, this also applies to resize and reformat. My sense is that the primary place people run into this is with resize.
- To make this better down the road, the number of primaries required varies by capability, so that could be factored in as well
Chris-Peterson444
approved these changes
Jul 19, 2024
mwhudson
approved these changes
Jul 21, 2024
subiquity/common/filesystem/boot.py
Outdated
| @@ -63,6 +63,9 @@ class MakeBootDevicePlan(abc.ABC): | |||
| this a boot device" in sync. | |||
| """ | |||
|
|
|||
| def iterate_steps(self): | |||
There was a problem hiding this comment.
Seems like it would be a little cleaner to have a "new_partition_count()" method on MakeBootDevicePlan?
ogayot
force-pushed
the
use-gap-invalid-gap
branch
2 times, most recently
from
21124af
to
639e3b0
Compare
Signed-off-by: Olivier Gayot <[email protected]>
Signed-off-by: Olivier Gayot <[email protected]>
If a disk has already reached its limit of (primary) partitions, suggesting a use-gap scenario can only lead to failures. In the same vein, if a disk only has room for one more partition but we also need to add an ESP, the scenario would fail to apply too. We now try to prevent Subiquity from suggesting such, bad, use-gap scenarios ; by guessing the number of primary partitions needed ; and comparing it with the limit. Although it isn't a perfect solution (see below), this change should hopefully address the most common problems. What makes it difficult to properly suggest scenarios is that we do not have all the cards in hand with the current API. For instance, the user can choose to add a recovery partition, which of course influences the number of partitions required. Sadly, we don't have a way with the current API to allow a scenario without recovery partition ; while rejecting the same scenario with a recovery partition. Capabilities can also influence the number of required partitions, supposedly. LP: #2073378 Signed-off-by: Olivier Gayot <[email protected]>
ogayot
force-pushed
the
use-gap-invalid-gap
branch
from
639e3b0
to
4f012e0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If a disk has already reached its limit of (primary) partitions, suggesting a use-gap scenario can only lead to failures. In the same vein, if a disk only has room for one more partition but we also need to add an ESP, the scenario would fail to apply too.
We now try to prevent Subiquity from suggesting such, bad, use-gap scenarios ; by guessing the number of primary partitions needed ; and comparing it with the limit.
Although it isn't a perfect solution (see below), this change should hopefully address the most common problems.
What makes it difficult to properly suggest scenarios is that we do not have all the cards in hand with the current API. For instance, the user can choose to add a recovery partition, which of course influences the number of partitions required. Sadly, we don't have a way with the current API to allow a scenario without recovery partition ; while rejecting the same scenario with a recovery partition. Capabilities can also influence the number of required partitions, supposedly.
LP:#2073378