Merge pull request #1335 from ancorgs/agama_adaptations

Adaptations to have more flexible proposal (useful for Agama)

package/yast2-storage-ng.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Wed May  3 11:51:09 UTC 2023 - Ancor Gonzalez Sosa <ancor@suse.com>
+
+- GuidedProposal adapted to support assigning volumes to specific
+  devices when volumes_allocate_mode is :auto (useful for Agama).
+
 -------------------------------------------------------------------
 Mon Apr 24 14:47:33 UTC 2023 - Ancor Gonzalez Sosa <ancor@suse.com>
 

src/lib/y2storage/proposal/devices_planner.rb

@@ -298,14 +298,12 @@ def adjust_btrfs_sizes(planned_device, volume)
       # @param planned_device [Planned::Device]
       # @param volume [VolumeSpecification]
       def adjust_device(planned_device, volume)
-        if settings.allocate_mode?(:device)
-          planned_device.disk = volume.device if planned_device.respond_to?(:disk=)
-        elsif planned_device.root?
-          # Forcing this when planned_device is a LV would imply the new VG
-          # can only be located in that disk (preventing it to spread over
-          # several disks). We likely don't want that.
-          planned_device.disk = settings.root_device if planned_device.is_a?(Planned::Partition)
-        end
+        planned_device.disk = volume.device if planned_device.respond_to?(:disk=)
+        return unless settings.allocate_mode?(:auto) && planned_device.root?
+
+        # Forcing this when planned_device is a LV would imply the new VG can only be located
+        # in that disk (preventing it to spread over several disks). We likely don't want that.
+        planned_device.disk ||= settings.root_device if planned_device.is_a?(Planned::Partition)
       end
 
       # Adjusts values when planned device is swap

src/lib/y2storage/proposal/partitions_distribution_calculator.rb

@@ -42,16 +42,19 @@ def initialize(planned_vg = nil)
       # that size (within the max limits defined for the planned VG).
       #
       # @param partitions [Array<Planned::Partition>]
-      # @param spaces [Array<FreeDiskSpace>]
+      # @param spaces [Array<FreeDiskSpace>] spaces that can be used to allocate partitions targeted
+      #   to the corresponding disk but also partitions with no specific disk and partitions for LVM
+      # @param extra_spaces [Array<FreeDiskSpace>] spaces that can only be used to allocate
+      #   partitions explicitly targeted to the corresponding disk
       #
       # @return [Planned::PartitionsDistribution]
-      def best_distribution(partitions, spaces)
+      def best_distribution(partitions, spaces, extra_spaces = [])
         log.info "Calculating best space distribution for #{partitions.inspect}"
         # First, make sure the whole attempt makes sense
-        return nil if impossible?(partitions, spaces)
+        return nil if impossible?(partitions, spaces + extra_spaces)
 
         begin
-          dist_hashes = distribute_partitions(partitions, spaces)
+          dist_hashes = distribute_partitions(partitions, spaces, extra_spaces)
         rescue NoDiskSpaceError
           return nil
         end
@@ -116,6 +119,13 @@ def resizing_size(partition, planned_partitions, free_spaces)
         end
       end
 
+      # Whether LVM should be taken into account
+      #
+      # @return [Boolean]
+      def lvm?
+        !!(planned_vg && planned_vg.missing_space > DiskSize.zero)
+      end
+
       protected
 
       # When calculating an LVM proposal, this represents the projected "system"
@@ -126,13 +136,6 @@ def resizing_size(partition, planned_partitions, free_spaces)
       # @return [Planned::LvmVg, nil]
       attr_reader :planned_vg
 
-      # Whether LVM should be taken into account
-      #
-      # @return [Boolean]
-      def lvm?
-        !!(planned_vg && planned_vg.missing_space > DiskSize.zero)
-      end
-
       # Checks whether there is any chance of producing a valid
       # PartitionsDistribution to accomodate the planned partitions and the
       # missing LVM part in the free spaces
@@ -181,13 +184,12 @@ def available_space(free_spaces)
       #
       # @param planned_partitions [Array<Planned::Partition>]
       # @param free_spaces [Array<FreeDiskSpace>]
-      # @param raise_if_empty [Boolean] raise a {NoDiskSpaceError} if there is
-      #   any planned partition that doesn't fit in any of the spaces
+      # @param extra_spaces [Array<FreeDiskSpace>]
       # @return [Hash{Planned::Partition => Array<FreeDiskSpace>}]
-      def candidate_disk_spaces(planned_partitions, free_spaces, raise_if_empty: true)
+      def candidate_disk_spaces(planned_partitions, free_spaces, extra_spaces = [])
         planned_partitions.each_with_object({}) do |partition, hash|
-          spaces = free_spaces.select { |space| suitable_disk_space?(space, partition) }
-          if spaces.empty? && raise_if_empty
+          spaces = partition_candidate_spaces(partition, free_spaces, extra_spaces)
+          if spaces.empty?
             log.error "No suitable free space for #{partition}"
             raise NoDiskSpaceError, "No suitable free space for the planned partition"
           end
@@ -229,6 +231,17 @@ def suitable_disk_space?(space, partition)
         true
       end
 
+      # @see #candidate_disk_spaces
+      #
+      # @param partition [Planned::Partition]
+      # @param candidate_spaces [Array<FreeDiskSpace>]
+      # @param extra_spaces [Array<FreeDiskSpace>]
+      # @return [Array<FreeDiskSpace>]
+      def partition_candidate_spaces(partition, candidate_spaces, extra_spaces)
+        spaces = partition.disk ? candidate_spaces + extra_spaces : candidate_spaces
+        spaces.select { |space| suitable_disk_space?(space, partition) }
+      end
+
       # @param partition [Planned::Partition]
       # @param space [FreeDiskSpace]
       #
@@ -267,10 +280,11 @@ def compatible_ptable?(partition, space)
       #
       # @param partitions [Array<Planned::Partitions>]
       # @param spaces [Array<FreeDiskSpace>]
+      # @param extra_spaces [Array<FreeDiskSpace>]
       # @return [Array<Hash{FreeDiskSpace => Array<Planned::Partition>}>]
-      def distribute_partitions(partitions, spaces)
+      def distribute_partitions(partitions, spaces, extra_spaces = [])
         log.info "Selecting the candidate spaces for each planned partition"
-        disk_spaces_by_part = candidate_disk_spaces(partitions, spaces)
+        disk_spaces_by_part = candidate_disk_spaces(partitions, spaces, extra_spaces)
 
         log.info "Calculate all the possible distributions of planned partitions into spaces"
         dist_hashes = distribution_hashes(disk_spaces_by_part)

src/lib/y2storage/proposal/space_maker.rb

@@ -131,6 +131,11 @@ def delete_unwanted_partitions(original_graph)
 
       attr_reader :disk_analyzer, :dist_calculator
 
+      # Disks that are not candidate devices but still must be considered because
+      # there are planned partitions explicitly targeted to those disks
+      # @return [Array<String>]
+      attr_reader :extra_disk_names
+
       # New devicegraph calculated by {#provide_space}
       # @return [Devicegraph]
       attr_reader :new_graph
@@ -164,6 +169,7 @@ def calculate_new_graph(partitions, keep, lvm_helper)
         @new_graph = original_graph.duplicate
         @new_graph_part_killer = PartitionKiller.new(@new_graph, candidate_disk_names)
         @new_graph_deleted_sids = []
+        @extra_disk_names = partitions.map(&:disk).compact.uniq - settings.candidate_devices
 
         # To make sure we are not freeing space in useless places first
         # restrict the operations to disks with particular disk
@@ -191,6 +197,14 @@ def calculate_new_graph(partitions, keep, lvm_helper)
           #
           parts_by_disk.each do |disk, parts|
             resize_and_delete(parts, keep, lvm_helper, disk_name: disk)
+          rescue Error
+            # If LVM was involved, maybe there is still hope if we don't abort on this error.
+            raise unless dist_calculator.lvm?
+
+            # dist_calculator tried to allocate the specific partitions for this disk but also
+            # all new physical volumes for the LVM. If the physical volumes were the culprit, we
+            # should keep trying to delete/resize stuff in other disks.
+            raise unless find_distribution(parts, ignore_lvm: true)
           end
         end
 
@@ -223,17 +237,27 @@ def planned_partitions_by_disk(planned_partitions)
       # @return [Boolean]
       def success?(planned_partitions)
         # Once a distribution has been found we don't have to look for another one.
-        if !@distribution
-          spaces = free_spaces(new_graph)
-          @distribution = dist_calculator.best_distribution(planned_partitions, spaces)
-        end
+        @distribution ||= find_distribution(planned_partitions)
         !!@distribution
       rescue Error => e
         log.info "Exception while trying to distribute partitions: #{e}"
         @distribution = nil
         false
       end
 
+      # Finds the best distribution to allocate the given set of planned partitions
+      #
+      # In case of an LVM-based proposal, the distribution will also include any needed physical
+      # volume for the system LVM. The argument ignore_lvm can be used to disable that requirement.
+      #
+      # @param planned_partitions [Array<Planned::Partition>]
+      # @param ignore_lvm [Boolean]
+      # @return [Planned::PartitionsDistribution, nil] nil if no valid distribution was found
+      def find_distribution(planned_partitions, ignore_lvm: false)
+        calculator = ignore_lvm ? non_lvm_dist_calculator : dist_calculator
+        calculator.best_distribution(planned_partitions, free_spaces, extra_free_spaces)
+      end
+
       # Perform all the needed operations to make space for the partitions
       #
       # @param planned_partitions [Array<Planned::Partition>] partitions
@@ -378,21 +402,33 @@ def remove_content(disk)
       #
       # @return [DiskSize]
       def resizing_size(partition, planned_partitions, disk_name)
-        spaces = free_spaces(new_graph, disk_name)
-        dist_calculator.resizing_size(partition, planned_partitions, spaces)
+        spaces = free_spaces(disk_name)
+        if disk_name && extra_disk_names.include?(disk_name)
+          # Operating in a disk that is not a candidate_device, no need to make extra space for LVM
+          return non_lvm_dist_calculator.resizing_size(partition, planned_partitions, spaces)
+        end
+
+        # As explained above, don't assume we will make space for LVM on non-candidate devices
+        partitions = planned_partitions.reject { |p| extra_disk_names.include?(p.disk) }
+        dist_calculator.resizing_size(partition, partitions, spaces)
       end
 
-      # List of free spaces in the given devicegraph
+      # List of free spaces from the candidate devices in the new devicegraph
       #
-      # @param graph [Devicegraph]
       # @param disk [String, nil] optional disk name to restrict result to
       # @return [Array<FreeDiskSpace>]
-      def free_spaces(graph, disk = nil)
-        disks_for(graph, disk).each_with_object([]) do |d, list|
+      def free_spaces(disk = nil)
+        disks_for(new_graph, disk).each_with_object([]) do |d, list|
           list.concat(d.as_not_empty { d.free_spaces })
         end
       end
 
+      # List of free spaces from extra disks (see {#extra_disk_names})
+      # @return [Array<FreeDiskSpace>]
+      def extra_free_spaces
+        extra_disk_names.flat_map { |d| free_spaces(d) }
+      end
+
       # List of candidate disk devices in the given devicegraph
       #
       # @param devicegraph [Devicegraph]
@@ -409,6 +445,16 @@ def candidate_disk_names
         settings.candidate_devices
       end
 
+      # Distribution calculator to use in special cases in which any implication related
+      # to LVM must be ignored
+      #
+      # Used for example when operating in extra (non-candidate) disks
+      #
+      # @return [PartitionsDistributionCalculator]
+      def non_lvm_dist_calculator
+        @non_lvm_dist_calculator ||= PartitionsDistributionCalculator.new
+      end
+
       # Whether {#resize_and_delete} should be executed several times,
       # see {#calculate_new_graph} for details.
       #

src/lib/y2storage/volume_specification.rb

@@ -140,11 +140,8 @@ class VolumeSpecification
     # @return [String]
     attr_accessor :separate_vg_name
 
-    # Device name of the disk (DiskDevice to be precise) in which the volume
-    # must be located. If nil, there are no restrictions allocating the volume.
-    #
-    # Note this is only relevant when {ProposalSettings#allocate_volume_mode} is
-    # set to :device.
+    # Optional device name of the disk (DiskDevice to be precise) in which the volume
+    # must be located.
     #
     # @return [String, nil]
     attr_accessor :device

test/data/devicegraphs/output/agama-basis-distributed.yml

@@ -0,0 +1,64 @@
+---
+- disk:
+    name: /dev/sda
+    size: 1 TiB
+    partition_table:  ms-dos
+    partitions:
+
+    - partition:
+        name:         /dev/sda1
+        size:         1033215 MiB (0.99 TiB)
+        id:           0x7
+        file_system:  ntfs
+        label:        windows
+    - partition:
+        name:         /dev/sda2
+        id:           linux
+        file_system:  xfs
+        mount_point:  /home
+
+- disk:
+    name: /dev/sdb
+    size: 400 GiB
+    partition_table:  gpt
+    partitions:
+
+    - partition:
+        name:         /dev/sdb1
+        size:         398332 MiB (389.00 GiB)
+        id: windows_basic_data
+        file_system:  ntfs
+        label:        windows
+    - partition:
+        size:         2 MiB
+        name:         /dev/sdb2
+        id:           bios_boot
+    - partition:
+        size:         10 GiB
+        name:         /dev/sdb3
+        id:           linux
+        file_system:  xfs
+        mount_point:  /
+    - partition:
+        name:         /dev/sdb4
+        id:           swap
+        file_system:  swap
+        mount_point:  swap
+
+- disk:
+    name: /dev/sdc
+    size: 400 GiB
+    partition_table:  ms-dos
+    partitions:
+
+    - partition:
+        size:         404479 MiB (395.00 GiB)
+        name:         /dev/sdc1
+        id:           0x7
+        file_system:  ntfs
+        label:        windows
+    - partition:
+        name:         /dev/sdc2
+        id:           linux
+        file_system:  xfs
+        mount_point:  /srv