Skip to content
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

Changes to allow automatic generation of LVM PVs at the AgamaProposal #1392

Merged
merged 6 commits into from
Oct 9, 2024

Conversation

ancorgs
Copy link
Contributor

@ancorgs ancorgs commented Sep 26, 2024

We want to offer new possibilities with the so-called AgamaProposal (that will be used to implement all the features described at this document.

For that we need a more powerful SpaceMaker that can automatically calculate physical volumes for an arbitrary number of LVM volume groups extended over different sets of disks.

This pull request generalizes the existing SpaceMaker to gain that capability and to be less coupled with the traditional YaST GuidedProposal.

Original API

class Y2Storage::Proposal::SpaceMaker
  # @param settings [ProposalSettings] Note the class
  def initialize(disk_analyzer, settings); end

  # @param planned_partitions [Array<Planned::Partition>]
  # @param lvm_helper [Proposal::LvmHelper]
  def provide_space(original_graph, planned_partitions, lvm_helper); end

   # @param planned_partitions [Array<Planned::Partition>]
   def prepare_devicegraph(original_graph, planned_partitions = []); end
end

New API.

Does not use LvmHelper or ProposalSettings. Accepts different sets of partitions and/or volume groups. Many arguments can be omitted on the calls. The caller is more explicit about what to do instead of relying on SpaceMaker to take decisions (like which disks to clean) based on the proposal settings.

class Y2Storage::Proposal::SpaceMaker
  # @param settings [ProposalSpaceSettings] Note the class
  def initialize(disk_analyzer, settings); end

  # @param partitions [Array<Planned::Partition>]
  # @param volume_groups [Array<Planned::LvmVg>]
  # @param default_disks [Array<String>]
  def provide_space(original_graph, partitions: [], volume_groups: [], default_disks: [])
  end

  # @param disks [Array<String>]
   def prepare_devicegraph(original_graph, disks); end
end

The behavior is unchanged except one corner case related to how it calculates the space needed to subtract from existing partitions in order to make space. The new behavior looks more correct. Anyways, that corner case should not affect YaST.

Bonus

This also modifies the mentioned calculation of how much a partition must be resized.

The previous algorithm was quite aggressive when LVM is involved. A perfect size would be computationally too expensive to find, but now we do two attempts, one with the original logic and another one with a logic that can still succeed but is less aggressive when there are several spaces in the disk.

Notes

As usual, this is structured in separate logical commits to easy review.

@coveralls
Copy link

coveralls commented Sep 26, 2024

Coverage Status

coverage: 97.814% (+0.004%) from 97.81%
when pulling 2aa1c76 on ancorgs:less_lvm_helper
into d984221 on yast:master.

@ancorgs ancorgs marked this pull request as ready for review October 8, 2024 12:28
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ancorgs ancorgs merged commit fdc1614 into yast:master Oct 9, 2024
7 of 9 checks passed
Copy link

github-actions bot commented Oct 9, 2024

✅ Autosubmission job #11251174448 successfully finished
✅ Created submit request #1206481

ancorgs added a commit to agama-project/agama that referenced this pull request Oct 9, 2024
#1652 adds support for configuring the automatic generation of LVM
physical volumes. Read there the details about how the feature works.

This pull request implements a first version of the functionality
described there, including a couple of unit tests.

Depends on yast/yast-storage-ng#1392
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants