-
Notifications
You must be signed in to change notification settings - Fork 19
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
Improve the :bigger_resize SpaceMaker strategy #1382
Merged
Merged
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
ancorgs
force-pushed
the
better_bigger_resize
branch
from
April 25, 2024 10:58
93d6135
to
6e77e1c
Compare
ancorgs
force-pushed
the
better_bigger_resize
branch
from
April 25, 2024 12:30
6e77e1c
to
a74ff39
Compare
ancorgs
force-pushed
the
better_bigger_resize
branch
from
April 25, 2024 13:18
a74ff39
to
88340d6
Compare
joseivanlopez
approved these changes
Apr 25, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
✔️ Internal Jenkins job #1155 successfully finished |
ancorgs
added a commit
to openSUSE/agama
that referenced
this pull request
Apr 25, 2024
## Problem Agama uses the `Y2Storage` strategy to find space called `:bigger_resize`. That strategy was found to be inconsistent, so it was slightly modified. See more details at yast/yast-storage-ng#1382 ## Solution This pull request adapts Agama to the new approach, in which the space actions are only expected for partitions (and maybe in the future for logical volumes), but not for whole disks. 1) The actions generated for the pre-defined policies do not longer include the disks, only partitions. 2) The "Find Space" form only offers the action selector for the partitions. In the case of disks without partitions, some static texts are added to help the user understand what can happen with the content of the disk. ![partition_actions](https://github.com/openSUSE/agama/assets/3638289/d1996c0b-a8de-4cea-8a76-14d8b3240199) Note we plan to rethink the whole interface of "Find Space" in the short term. This pull request only aims to keep the UI consistent with the new logic in the proposal, while the bigger changes arrive. ## Testing - Unit tests adapted. - Tested manually together with yast/yast-storage-ng#1382
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.
IMPORTANT: This should be merged in sync with openSUSE/agama#1164, which includes the needed adaptations in the Agama side to keep everything consistent.
Problem
The
:bigger_resize
strategy was added to give Agama a more fine-grained control of the actions to be performed by the SpaceMaker.But we found several inconsistencies in the way we originally defined
:bigger_resize
.Before this change, the SpaceMaker strategy expected a list like this.
:delete
or:force_delete
(ie. wipe the content).:delete or
:force_delete` (ie. remove the partition):resize
(ie. shrinking the partition)First problem
The behavior described above turned out to be quite confusing since actions on non-partitioned disks are about the content (filesystems, PVs, etc.) and actions on partitions are about the devices themselves, offering no option for things like keeping the device but allowing to destroy the content.
Second problem
The first problem refers to disks that are part of the candidate devices or are selected as root disk. But the situation of disks or RAIDs that were chosen to be formatted (ie. reused by a volume) also needed clarification.
Additional unrelated problem
While testing this I found that installing on a disk that previously contained a RAID could result in a proposal that didn't include the partitions needed for booting. After some debugging, turns out the culprit was some code introduced many years ago in order to support non-standard RAID1 setups (#788).
Solution
Solution to the first problem
We agreed that having explicit actions for non-partitioned disks has very little value. If a non-partitioned device is selected to be formatted or to be a candidate disk or root disk, it’s content will be deleted if needed no matter whether there is such action. In other words, actions refer to the devices themselves, not to their content. As such, actions only make sense for partitions and maybe for other devices that can be destroyed (like logical volumes if we implement reusing of LVM VGs in the future).
Solution to the second problem
We came to the conclusion that selecting a disk to be directly formatted (or any other partitionable device like a RAID) already implies an statement about deleting all its content (partitions or whatever is there). There is no need to have space actions for those cases (they would be an open door for conflicts). Space actions only make sense for candidate disks and for the root disk. They are not needed for reused devices or its descendants.
Solution to the additional problem
If the root device is explicitly set (something that always happens in the Agama proposals), that value is now honored, ignoring any possible RAID consideration.
Testing