SpaceMaker: support for calculating PVs for several VGs

src/lib/y2storage/planned/lvm_vg.rb

@@ -83,6 +83,11 @@ class LvmVg < Device
       # @return [Symbol]
       attr_accessor :size_strategy
 
+      # Disks where the proposal can create extra physical volumes to honor {#size_strategy}
+      #
+      # @return [Array<String>] names of partitionable devices
+      attr_accessor :pvs_candidate_devices
+
       # Builds a new instance based on a real VG
       #
       # The new instance represents the intention to reuse the real VG, so the
@@ -111,6 +116,7 @@ def initialize(volume_group_name: nil, lvs: [], pvs: [])
         @pvs = pvs
         @pvs_encryption_password = nil
         @make_space_policy = :needed
+        @pvs_candidate_devices = []
       end
 
       # Initializes the object taking the values from a real volume group
@@ -258,13 +264,15 @@ def self.to_string_attrs
       end
 
       # Device name of the disk-like device in which the volume group has to be
-      # physically located. If nil, the volume group can spread freely over any
-      # set of disks.
+      # physically located. If nil, the volume group can spread over a set of
+      # several disks (maybe even unlimited).
       #
       # @return [String, nil]
       def forced_disk_name
         forced_lv = lvs.find(&:disk)
-        forced_lv ? forced_lv.disk : nil
+        return forced_lv.disk if forced_lv
+
+        pvs_candidate_devices.size == 1 ? pvs_candidate_devices.first : nil
       end
 
       protected

src/lib/y2storage/proposal/autoinst_partitioner.rb

@@ -155,12 +155,21 @@ def reuse_partitions_in_devicegraph(reused_parts, graph)
       def best_distribution(planned_partitions, devices)
         spaces = devices.map(&:free_spaces).flatten
 
-        calculator = Proposal::PartitionsDistributionCalculator.new
-        dist = calculator.best_distribution(planned_partitions, spaces)
+        dist = distribute_partitions(planned_partitions, spaces)
         return dist if dist
 
         # Second try with more flexible planned partitions
-        calculator.best_distribution(flexible_partitions(planned_partitions), spaces)
+        distribute_partitions(flexible_partitions(planned_partitions), spaces)
+      end
+
+      # @see #best_distribution
+      #
+      # @param partitions [Array<Planned::Partition>] list of planned partitions to create
+      # @param spaces [Array<FreeDiskSpace>] spaces to distribute the partitions
+      # @return [Planned::PartitionsDistribution, nil]
+      def distribute_partitions(partitions, spaces)
+        calculator = Proposal::PartitionsDistributionCalculator.new(partitions)
+        calculator.best_distribution(spaces)
       end
 
       # Checks whether (re)formatting the given device is acceptable

src/lib/y2storage/proposal/devicegraph_generator.rb

@@ -190,7 +190,9 @@ def provide_space(planned_partitions, devicegraph, lvm_helper)
       # Variant of #provide_space when LVM is not involved
       # @see #provide_space
       def provide_space_no_lvm(planned_partitions, devicegraph)
-        result = space_maker.provide_space(devicegraph, default_disks, planned_partitions)
+        result = space_maker.provide_space(
+          devicegraph, default_disks: default_disks, partitions: planned_partitions
+        )
         log.info "Found enough space"
         result
       end
@@ -211,7 +213,8 @@ def provide_space_lvm(planned_partitions, devicegraph, lvm_helper)
           space_maker.protected_sids += lvm_sids
 
           result = space_maker.provide_space(
-            devicegraph, default_disks, planned_partitions, lvm_helper.volume_group
+            devicegraph, default_disks: default_disks,
+            partitions: planned_partitions, volume_groups: [lvm_helper.volume_group]
           )
           log.info "Found enough space including LVM, reusing #{vg}"
           return result
@@ -223,7 +226,8 @@ def provide_space_lvm(planned_partitions, devicegraph, lvm_helper)
 
         lvm_helper.reused_volume_group = nil
         result = space_maker.provide_space(
-          devicegraph, default_disks, planned_partitions, lvm_helper.volume_group
+          devicegraph, default_disks: default_disks,
+          partitions: planned_partitions, volume_groups: [lvm_helper.volume_group]
         )
         log.info "Found enough space including LVM"
 

src/lib/y2storage/proposal/partitions_distribution_calculator.rb

@@ -30,8 +30,14 @@ module Proposal
     class PartitionsDistributionCalculator
       include Yast::Logger
 
-      def initialize(planned_vg = nil)
-        @planned_vg = planned_vg
+      # Constructor
+      #
+      # @param partitions [Array<Planned::Partition>] see {#planned_partitions}
+      # @param planned_vgs [Array<Planned::LvmVg>] see {#planned_vgs}
+      def initialize(partitions = [], planned_vgs = [], default_disks = nil)
+        @planned_partitions = partitions
+        @planned_vgs = planned_vgs
+        @default_disks = default_disks
       end
 
       # Best possible distribution, nil if the planned partitions don't fit
@@ -41,30 +47,21 @@ def initialize(planned_vg = nil)
       # the LVM physical volumes that need to be created in order to reach
       # that size (within the max limits defined for the planned VG).
       #
-      # @param partitions [Array<Planned::Partition>]
-      # @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, extra_spaces = [])
-        log.info "Calculating best space distribution for #{partitions.inspect}"
+      # @param spaces [Array<FreeDiskSpace>] spaces that can be used to allocate partitions
+      # @return [Planned::PartitionsDistribution, nil]
+      def best_distribution(spaces)
+        log.info "Calculating best space distribution for #{planned_partitions.inspect}"
         # First, make sure the whole attempt makes sense
-        return nil if impossible?(partitions, spaces + extra_spaces)
+        return nil if impossible?(planned_partitions, spaces)
 
         begin
-          dist_hashes = distribute_partitions(partitions, spaces, extra_spaces)
+          dist_hashes = distribute_partitions(planned_partitions, spaces)
         rescue NoDiskSpaceError
           return nil
         end
         candidates = distributions_from_hashes(dist_hashes)
 
-        if lvm?
-          log.info "Calculate LVM posibilities for the #{candidates.size} candidate distributions"
-          pv_calculator = PhysVolCalculator.new(spaces, planned_vg)
-          candidates.map! { |dist| pv_calculator.add_physical_volumes(dist) }
-        end
+        add_physical_volumes(candidates, spaces)
         candidates.compact!
 
         best_candidate(candidates)
@@ -78,20 +75,21 @@ def best_distribution(partitions, spaces, extra_spaces = [])
       # from the partition.
       #
       # @param partition [Partition] partition to resize
-      # @param planned_partitions [Array<Planned::Partition>] planned
-      #     partitions to make space for
       # @param free_spaces [Array<FreeDiskSpace>] all free spaces in the system
       # @return [DiskSize]
-      def resizing_size(partition, planned_partitions, free_spaces)
+      def resizing_size(partition, free_spaces)
         # This is far more complex than "needed_space - current_space" because
         # we really have to find a distribution that is valid.
         #
         # The following code tries to find the minimal valid distribution
         # that would succeed, taking into account that resizing will introduce a
         # new space or make one of the existing spaces grow.
 
-        all_spaces = add_or_mark_growing_space(free_spaces, partition)
+        disk = partition.partitionable.name
+        all_spaces = free_spaces.select { |s| s.disk_name == disk }
+        all_spaces = add_or_mark_growing_space(all_spaces, partition)
         all_planned = all_planned_partitions(planned_partitions)
+        all_planned = all_planned.select { |p| compatible_disk?(p, disk) }
 
         begin
           dist_hashes = distribute_partitions(all_planned, all_spaces)
@@ -100,11 +98,7 @@ def resizing_size(partition, planned_partitions, free_spaces)
           # reclaim as much space as possible.
           #
           # FIXME: using the partition size as fallback value in situations
-          # where resizing the partition cannot provide a valid solution makes
-          # sense because, with the current SpaceMaker algorithm, we will not
-          # have another chance of resizing this partition.
-          # Revisit this if the global proposal algorithm is changed in the
-          # future.
+          # where resizing the partition cannot provide a valid solution.
           return partition.size
         end
 
@@ -119,22 +113,21 @@ def resizing_size(partition, planned_partitions, free_spaces)
         end
       end
 
-      # Whether LVM should be taken into account
+      # When calculating an LVM proposal, this represents the projected volume groups for
+      # which is necessary to automatically allocate physical volumes (based on their respective
+      # values for {Planned::LvmVg#pvs_candidate_devices}.
       #
-      # @return [Boolean]
-      def lvm?
-        !!(planned_vg && planned_vg.missing_space > DiskSize.zero)
-      end
+      # Empty if LVM is not involved (partition-based proposal)
+      #
+      # @return [Array<Planned::LvmVg>]
+      attr_reader :planned_vgs
 
-      protected
+      # @return [Array<Planned::Partition>] planned partitions to find space for
+      attr_reader :planned_partitions
 
-      # When calculating an LVM proposal, this represents the projected "system"
-      # volume group to accommodate root and other volumes.
-      #
-      # Nil if LVM is not involved (partition-based proposal)
-      #
-      # @return [Planned::LvmVg, nil]
-      attr_reader :planned_vg
+      attr_reader :default_disks
+
+      protected
 
       # Checks whether there is any chance of producing a valid
       # PartitionsDistribution to accomodate the planned partitions and the
@@ -143,10 +136,8 @@ def lvm?
       # This check could be improved to detect more situations that make it impossible
       # to get a distribution, but the goal is to keep it relatively simple and fast.
       def impossible?(planned_partitions, free_spaces)
-        if lvm?
-          # Let's assume the best possible case - if we need to create a PV it will be only one
-          planned_partitions += [planned_vg.single_pv_partition]
-        end
+        # Let's assume the best possible case - if we need to create PVs it will be only one per VG
+        planned_partitions += single_pv_partitions
 
         # First, do the simplest calculation - checking total sizes
         needed = DiskSize.sum(planned_partitions.map(&:min))
@@ -169,6 +160,21 @@ def impossible_partitions?(planned_partitions, free_spaces)
         false
       end
 
+      # Planned volume groups that need some extra physical volume
+      #
+      # @return [Array<Planned::LvmVg]
+      def incomplete_planned_vgs
+        planned_vgs.select { |vg| vg.missing_space > DiskSize.zero }
+      end
+
+      # Simplest possible collection of missing physical volumes (only one per incomplete volume
+      # group)
+      #
+      # @return [Array<Planned::Partition>]
+      def single_pv_partitions
+        incomplete_planned_vgs.map(&:single_pv_partition)
+      end
+
       # Returns the sum of available spaces
       #
       # @param free_spaces [Array<FreeDiskSpace>] List of free disk spaces
@@ -184,11 +190,10 @@ def available_space(free_spaces)
       #
       # @param planned_partitions [Array<Planned::Partition>]
       # @param free_spaces [Array<FreeDiskSpace>]
-      # @param extra_spaces [Array<FreeDiskSpace>]
       # @return [Hash{Planned::Partition => Array<FreeDiskSpace>}]
-      def candidate_disk_spaces(planned_partitions, free_spaces, extra_spaces = [])
+      def candidate_disk_spaces(planned_partitions, free_spaces)
         planned_partitions.each_with_object({}) do |partition, hash|
-          spaces = partition_candidate_spaces(partition, free_spaces, extra_spaces)
+          spaces = partition_candidate_spaces(partition, free_spaces)
           if spaces.empty?
             log.error "No suitable free space for #{partition}"
             raise NoDiskSpaceError, "No suitable free space for the planned partition"
@@ -197,6 +202,37 @@ def candidate_disk_spaces(planned_partitions, free_spaces, extra_spaces = [])
         end
       end
 
+      # @see #best_distribution
+      #
+      # @param candidates [Array<Planned::PartitionsDistribution>]
+      # @param all_spaces [Array<FreeDiskSpace>]
+      def add_physical_volumes(candidates, spaces)
+        candidates.map! do |dist|
+          incomplete_planned_vgs.inject(dist) do |res, planned_vg|
+            pv_spaces = spaces_for_vg(spaces, planned_vg)
+            pv_calculator = PhysVolCalculator.new(pv_spaces, planned_vg)
+            pv_calculator.add_physical_volumes(res)
+          end
+        end
+      end
+
+      # Subset of spaces that are located at devices that are acceptable for the given
+      # planed volume group
+      #
+      # @param all_spaces [Array<FreeDiskSpace>] full set of spaces
+      # @param volume_group [Planned::VolumeGroup]
+      # @return [Array<FreeDiskSpace>] subset of spaces that could contain a
+      #   physical volume
+      def spaces_for_vg(all_spaces, volume_group)
+        disk_name = volume_group.forced_disk_name
+        return all_spaces.select { |i| i.disk_name == disk_name } if disk_name
+
+        disk_names = volume_group.pvs_candidate_devices
+        disk_names = default_disks if disk_names.empty? && default_disks
+
+        all_spaces.select { |s| disk_names.include?(s.disk_name) }
+      end
+
       # All possible combinations of spaces and planned partitions.
       #
       # The result is an array in which each entry represents a potential
@@ -221,7 +257,7 @@ def distribution_hashes(disk_spaces_by_partition)
       #
       # @return [Boolean]
       def suitable_disk_space?(space, partition)
-        return false unless compatible_disk?(partition, space)
+        return false unless compatible_disk?(partition, space.disk_name)
         return false unless compatible_ptable?(partition, space)
         return false unless partition_fits_space?(partition, space)
 
@@ -235,19 +271,32 @@ def suitable_disk_space?(space, partition)
       #
       # @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) }
+      def partition_candidate_spaces(partition, candidate_spaces)
+        candidate_spaces.select { |space| suitable_disk_space?(space, partition) }
       end
 
       # @param partition [Planned::Partition]
-      # @param space [FreeDiskSpace]
+      # @param disk_name [String]
       #
       # @return [Boolean]
-      def compatible_disk?(partition, space)
-        return true unless partition.disk && partition.disk != space.disk_name
+      def compatible_disk?(partition, disk_name)
+        return partition.disk == disk_name if partition.disk
+
+        pv_candidates = planned_vg_for(partition)&.pvs_candidate_devices
+        return pv_candidates.include?(disk_name) if pv_candidates&.any?
+
+        return true unless default_disks
+
+        default_disks.include?(disk_name)
+      end
+
+      # Planned volume group associated to the given partition, if any
+      #
+      # @param planned_partition [Planned::Partition]
+      # @return [Planned::LvmVg, nil] nil if the partition is not meant as an LVM PV
+      def planned_vg_for(planned_partition)
+        planned_vgs.find { |vg| vg.volume_group_name == planned_partition.lvm_volume_group_name }
       end
 
       # @param partition [Planned::Partition]
@@ -280,11 +329,10 @@ 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, extra_spaces = [])
+      def distribute_partitions(partitions, spaces)
         log.info "Selecting the candidate spaces for each planned partition"
-        disk_spaces_by_part = candidate_disk_spaces(partitions, spaces, extra_spaces)
+        disk_spaces_by_part = candidate_disk_spaces(partitions, spaces)
 
         log.info "Calculate all the possible distributions of planned partitions into spaces"
         dist_hashes = distribution_hashes(disk_spaces_by_part)
@@ -438,11 +486,19 @@ def space_right_after_partition?(free_space, partition)
       # @return [Array<Planned::Partition] original set (in the non-LVM case) or
       #   an extended set including partitions needed for LVM
       def all_planned_partitions(planned_partitions)
-        return planned_partitions unless lvm?
+        # If some of the heuristic turns to be counterproductive, we may run more than one
+        # and choose the smaller
 
         # In the LVM case, assume the worst case - that there will be only
-        # one big PV and we have to make room for it as well.
-        planned_partitions + [planned_vg.single_pv_partition]
+        # one big PV per volume group and we have to make room for them as well.
+        planned_partitions + single_pv_partitions
+
+        # Limiting the worst case
+        # One VG case
+        #  no planned_partitions
+        #  all the not_growing_spaces are smaller than single_pv_partition
+        #  -> generate one partition per not_growing_space with the space size and
+        #    then one additional with the missing
       end
 
       # Size that is missing in the space marked as "growing" in order to