New strategy for SpaceMaker

[ancorgs]

Problem

The traditional behavior of Y2Storage::Proposal::SpaceMaker is coupled to the options offered by YaST in that regard. Somehow summarized in this screenshot.

But for Agama we want to offer other options.

First of all, we want to stop making a difference for partitions of type Windows/Linux/other. The criteria for classifying a partition in one group or the other is not so clear.

On the other hand, we want to make it possible for Agama to specify any of the four modes described at storage_ui.md. That is:

• Delete everything in the disk(s).
• Resize existing partition(s).
• Do not modify existing partition(s).
• Custom, with the user specifying what to do with every individual partition in the affected disks: resize it, delete it or keep it as it is.

Solution

This adds to the ProposalSetting the concept of different strategies for making space.

The default strategy is the classic one, now called auto and based on the known settings resize_windows, windows_delete_mode, linux_delete_mode and other_delete_mode.

A new strategy called bigger_resize is added. This strategy ignores the settings previously mentioned and works with a explicit list of actions specified in the proposal settings. Thus, the caller is responsible for deciding what to do with each partition or disk (:force_delete, :delete or :resize). Omitted devices are kept without modification. The only decision taken by SpaceMaker itself is the order in which those actions must be applied while looking for valid layout. As the name of the strategy suggests, it first executes all the :force_delete actions, then the :resize ones (sorted by "recoverable size") and finally the :delete ones.

With this solution, all the mentioned Agama modes can be easily implemented.

• Delete everything in the disk(s). Settings with :force_delete for all partitions in the affected disks.
• Resize existing partition(s). Settings with :resize for all partitions in the affected disks.
• Do not modify existing partition(s). Settings with an empty list of actions.
• Custom. Settings describing the exact actions decided by the user.

Implementation

The pull request is divided into several commits because the code was refactored into small steps before implementing the strategy support at SpaceMaker. Our comprehensive battery of unit tests survived all those reorganizations.

Testing

Added new unit tests.

[coveralls]

Coverage: 97.755% (+0.005%) from 97.75% when pulling 802b15d on ancorgs:space_maker_actions into 1a37459 on yast:master.

[joseivanlopez]

NP: I would explicitly check by SpaceMakerActions::Shrink, and loging/raising an exception in else.

[ancorgs]

I fear that would only lead to a line of code not covered by tests. This is quite internal, not something receiving direct input from the caller. I wouldn't expect a SpaceMakerActions::List to return an action from an unknown type.

[joseivanlopez]

Does it prevent to resize beyond the available free space?

[ancorgs]

Yes, it does. As you can see here for example, we sometimes use the size of the partition as shrink_size (so target becomes partition.size - partition.size).

yast-storage-ng/src/lib/y2storage/proposal/partitions_distribution_calculator.rb

Line 108 in 1a37459

return partition.size

Even if those cases, everything works as expected. So this is already proven by the existing tests.

[joseivanlopez]

Just as note, we have the Equatable mixin for custom comparison: https://github.com/yast/yast-yast2/blob/master/library/general/src/lib/yast2/equatable.rb

[ancorgs]

Well, EqualByInstanceVariables is used for ProposalSettings. And this class is basically a subset of those settings extracted to a separate object for clarity. So it makes sense to be consistent with the container class and use the same mechanism.

[joseivanlopez]

Yes, only mentioning to promote it for other cases :)

[joseivanlopez]

Version bump and changelog?

[joseivanlopez]

LGTM

[yast-bot]

❌ Public Jenkins job #464 failed