Fix use

src/lib/y2storage/autoinst_proposal.rb

@@ -74,10 +74,7 @@ def calculate_proposal
         return @devices
       end
 
-      space_maker = Proposal::AutoinstSpaceMaker.new(disk_analyzer, issues_list)
-      devicegraph = space_maker.cleaned_devicegraph(initial_devicegraph, drives)
-
-      @devices = propose_devicegraph(devicegraph, drives)
+      @devices = propose_devicegraph(initial_devicegraph, drives)
     end
 
     # Proposes a devicegraph based on given drives map
@@ -91,6 +88,10 @@ def calculate_proposal
     def propose_devicegraph(devicegraph, drives)
       if drives.partitions?
         @planned_devices = plan_devices(devicegraph, drives)
+
+        space_maker = Proposal::AutoinstSpaceMaker.new(disk_analyzer, issues_list)
+        devicegraph = space_maker.cleaned_devicegraph(devicegraph, drives, @planned_devices)
+
         create_devices(devicegraph, @planned_devices, drives.disk_names)
       else
         log.info "No partitions were specified. Falling back to guided setup planning."

src/lib/y2storage/proposal/autoinst_space_maker.rb

@@ -22,6 +22,8 @@
 # find current contact information at www.suse.com.
 
 require "y2storage/disk"
+require "y2storage/planned/lvm_vg"
+require "y2storage/planned/lvm_lv"
 
 module Y2Storage
   module Proposal
@@ -45,17 +47,22 @@ def initialize(disk_analyzer, issues_list = nil)
       # Performs all the delete operations specified in the AutoYaST profile
       #
       # @param original_devicegraph [Devicegraph] initial devicegraph
-      # @param drives_map           [Array<Planned::Partition>] set of partitions
+      # @param drives_map           [AutoinstDrivesMap] drives map
+      # @param planned_devices      [Array<Planned::Partition>] set of partitions
       #   to make space for.
-      def cleaned_devicegraph(original_devicegraph, drives_map)
+      def cleaned_devicegraph(original_devicegraph, drives_map, planned_devices)
         devicegraph = original_devicegraph.dup
 
+        reused_partitions = reused_partitions_by_disk(devicegraph, planned_devices)
+        sid_map = partitions_sid_map(devicegraph)
+
         drives_map.each_pair do |disk_name, drive_spec|
           disk = BlkDevice.find_by_name(devicegraph, disk_name)
           next unless disk
-          delete_stuff(devicegraph, disk, drive_spec)
+          delete_stuff(devicegraph, disk, drive_spec, reused_partitions[disk.name])
         end
 
+        adjust_reuse_values(devicegraph, planned_devices, sid_map)
         devicegraph
       end
 
@@ -65,60 +72,80 @@ def cleaned_devicegraph(original_devicegraph, drives_map)
 
       # Deletes unwanted partitions for the given disk
       #
-      # @param devicegraph [Devicegraph]
-      # @param disk        [Disk]
-      # @param drive_spec  [AutoinstProfile::DriveSection]
-      def delete_stuff(devicegraph, disk, drive_spec)
-        if drive_spec.initialize_attr
+      # @param devicegraph  [Devicegraph]
+      # @param disk         [Disk]
+      # @param drive_spec   [AutoinstProfile::DriveSection]
+      # @param reused_parts [Array<String>] Reused partitions names
+      def delete_stuff(devicegraph, disk, drive_spec, reused_parts)
+        reused_parts ||= []
+        if drive_spec.initialize_attr && reused_parts.empty?
           disk.remove_descendants
           return
         end
 
         # TODO: resizing of partitions
 
-        delete_by_use(devicegraph, disk, drive_spec)
+        delete_by_use(devicegraph, disk, drive_spec, reused_parts)
       end
 
       # Deletes unwanted partition according to the "use" element
       #
-      # @param devicegraph [Devicegraph]
-      # @param disk        [Disk]
-      # @param drive_spec  [AutoinstProfile::DriveSection]
-      def delete_by_use(devicegraph, disk, drive_spec)
+      # @param devicegraph  [Devicegraph]
+      # @param disk         [Disk]
+      # @param drive_spec   [AutoinstProfile::DriveSection]
+      # @param reused_parts [Array<String>] Reused partitions names
+      def delete_by_use(devicegraph, disk, drive_spec, reused_parts)
         return if drive_spec.use == "free"
         case drive_spec.use
         when "all"
-          disk.partition_table.remove_descendants if disk.partition_table
+          delete_partitions(devicegraph, disk.partitions, reused_parts) if disk.partition_table
         when "linux"
-          delete_linux_partitions(devicegraph, disk)
+          delete_linux_partitions(devicegraph, disk, reused_parts)
         when Array
-          delete_partitions_by_number(devicegraph, disk, drive_spec.use)
+          delete_partitions_by_number(devicegraph, disk, drive_spec.use, reused_parts)
         else
           register_invalid_use_value(drive_spec)
         end
       end
 
+      # Search a partition by its sid
+      #
+      # @param devicegraph  [Devicegraph] Working devicegraph
+      def partition_by_sid(devicegraph, sid)
+        devicegraph.partitions.find { |p| p.sid == sid }
+      end
+
       # Deletes Linux partitions from a disk in the given devicegraph
       #
-      # @param devicegraph [Devicegraph] Working devicegraph
-      # @param disk        [Disk]        Disk to remove partitions from
-      def delete_linux_partitions(devicegraph, disk)
-        partition_killer = Proposal::PartitionKiller.new(devicegraph)
+      # @param devicegraph  [Devicegraph] Working devicegraph
+      # @param disk         [Disk]        Disk to remove partitions from
+      # @param reused_parts [Array<String>] Reused partitions names
+      def delete_linux_partitions(devicegraph, disk, reused_parts)
         parts = disk_analyzer.linux_partitions(disk)
-        # TODO: when introducing supporting for LVM, should we protect here
-        # the PVs of VGs that are going to be reused somehow?
-        parts.map(&:name).each { |n| partition_killer.delete(n) }
+        delete_partitions(devicegraph, parts, reused_parts)
       end
 
       # Deletes Linux partitions which number is included in a list
       #
       # @param devicegraph [Devicegraph]    Working devicegraph
       # @param disk        [Disk]           Disk to remove partitions from
       # @param partition_nrs [Array<Integer>] List of partition numbers
-      def delete_partitions_by_number(devicegraph, disk, partition_nrs)
-        partition_killer = Proposal::PartitionKiller.new(devicegraph)
+      def delete_partitions_by_number(devicegraph, disk, partition_nrs, reused_partitions)
         parts = disk.partitions.select { |n| partition_nrs.include?(n.number) }
-        parts.map(&:name).each { |n| partition_killer.delete(n) }
+        delete_partitions(devicegraph, parts, reused_partitions)
+      end
+
+      # @param devicegraph     [Devicegraph]               devicegraph
+      # @param parts           [Array<Planned::Partition>] parts to delete
+      # @param reused_parts    [Array<String>]             reused partitions names
+      def delete_partitions(devicegraph, parts, reused_parts)
+        partition_killer = Proposal::PartitionKiller.new(devicegraph)
+        parts_to_delete = parts.reject { |p| reused_parts.include?(p.name) }
+        parts_to_delete.map(&:sid).each do |sid|
+          partition = partition_by_sid(devicegraph, sid)
+          next unless partition
+          partition_killer.delete(partition.name)
+        end
       end
 
       # Register an invalid/missing value for 'use'
@@ -131,6 +158,76 @@ def register_invalid_use_value(drive_spec)
           issues_list.add(:missing_value, drive_spec, :use)
         end
       end
+
+      # Return a map of reused partitions
+      #
+      # Calculates which partitions are meant to be reused and, as a consequence, should not
+      # be deleted.
+      #
+      # @param devicegraph     [Devicegraph]               devicegraph
+      # @param planned_devices [Array<Planned::Partition>] set of partitions
+      # @return [Hash<String,Array<String>>] disk name to list of reused partitions map
+      def reused_partitions_by_disk(devicegraph, planned_devices)
+        find_reused_partitions(devicegraph, planned_devices).each_with_object({}) do |part, map|
+          disk_name = part.disk.name
+          map[disk_name] ||= []
+          map[disk_name] << part.name
+        end
+      end
+
+      # Determine which partitions will be reused
+      #
+      # @param devicegraph     [Devicegraph]               devicegraph
+      # @param planned_devices [Array<Planned::Partition>] set of partitions
+      # @return [Hash<String,Array<String>>] disk name to list of reused partitions map
+      def find_reused_partitions(devicegraph, planned_devices)
+        reused_devices = planned_devices.select(&:reuse).each_with_object([]) do |device, all|
+          case device
+          when Y2Storage::Planned::Partition
+            all << devicegraph.partitions.find { |p| device.reuse == p.name }
+          when Y2Storage::Planned::LvmVg
+            vg = devicegraph.lvm_vgs.find { |v| File.join("/dev", device.reuse) == v.name }
+            all.concat(vg.lvm_pvs.map(&:blk_device))
+          when Y2Storage::Planned::Md
+            all << devicegraph.md_raids.find { |r| device.reuse == r.name }
+          end
+        end
+
+        ancestors = reused_devices.map(&:ancestors).flatten
+        (reused_devices + ancestors).select { |p| p.is_a?(Y2Storage::Partition) }
+      end
+
+      # Build a device name to sid map
+      #
+      # @param devicegraph [Devicegraph] devicegraph
+      # @return [Hash<String,Integer>] Map with device names as keys and sid as values.
+      #
+      # @see adjust_reuse_values
+      def partitions_sid_map(devicegraph)
+        devicegraph.partitions.each_with_object({}) { |p, m| m[p.name] = p.sid }
+      end
+
+      # Adjust reuse values
+      #
+      # When using logical partitions (ms-dos), removing a partition might cause the rest of
+      # logical partitions numbers to shift. In such a situation, 'reuse' properties of planned
+      # devices will break (they might point to a non-existent device).
+      #
+      # This method takes care of setting the correct value for every 'reuse' property (thus
+      # planned_devices are modified).
+      #
+      # @param devicegraph     [Devicegraph]               devicegraph
+      # @param planned_devices [Array<Planned::Partition>] set of partitions to make space for
+      # @param sid_map         [Hash<String,Integer>]      device name to sid map
+      #
+      # @see sid_map
+      def adjust_reuse_values(devicegraph, planned_devices, sid_map)
+        planned_devices.select { |d| d.is_a?(Y2Storage::Planned::Partition) && d.reuse }.each do |device|
+          sid = sid_map[device.reuse]
+          partition = partition_by_sid(devicegraph, sid)
+          device.reuse = partition.name
+        end
+      end
     end
   end
 end

test/y2storage/autoinst_proposal_test.rb

@@ -262,11 +262,24 @@
       end
 
       let(:pv) do
-        { "create" => true, "lvm_group" => lvm_group, "size" => "max", "type" => :CT_LVM }
+        { "create" => false, "lvm_group" => lvm_group, "size" => "max", "type" => :CT_LVM }
       end
 
       it "reuses the volume group" do
         proposal.propose
+        devicegraph = proposal.devices
+        expect(devicegraph.partitions).to contain_exactly(
+          an_object_having_attributes("name" => "/dev/sda1"), # new pv
+          an_object_having_attributes("name" => "/dev/sda3"),
+          an_object_having_attributes("name" => "/dev/sda5")
+        )
+
+        vg = devicegraph.lvm_vgs.first
+        expect(vg.vg_name).to eq(lvm_group)
+        expect(vg.lvm_pvs.map(&:blk_device)).to contain_exactly(
+          an_object_having_attributes("name" => "/dev/sda1"), # new pv
+          an_object_having_attributes("name" => "/dev/sda5")
+        )
       end
     end