Merge pull request #411 from yast/fix-use

Fix use

package/yast2-storage-ng.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Fri Nov  3 15:11:42 UTC 2017 - igonzalezsosa@suse.com
+
+- AutoYaST: do not remove partitions that are supposed to be reused
+  (bsc#1066398).
+- 4.0.21
+
 -------------------------------------------------------------------
 Fri Nov  3 15:04:24 UTC 2017 - jlopez@suse.com
 

package/yast2-storage-ng.spec

@@ -16,7 +16,7 @@
 #
 
 Name:		yast2-storage-ng
-Version:        4.0.20
+Version:        4.0.21
 Release:	0
 BuildArch:	noarch
 

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_devices_planner.rb

@@ -148,7 +148,7 @@ def planned_for_md(drive)
         part_section = drive.partitions.first
         device_config(md, part_section, drive)
         md.lvm_volume_group_name = part_section.lvm_group
-        add_device_reuse(md, md.name, !!part_section.format) if part_section.create == false
+        add_md_reuse(md, part_section) if part_section.create == false
 
         raid_options = part_section.raid_options
         if raid_options
@@ -285,6 +285,20 @@ def add_vg_reuse(vg, drive)
         vg.reuse = vg_to_reuse.vg_name if vg_to_reuse
       end
 
+      # Set 'reusing' attributes for a MD RAID
+      #
+      # @param md      [Planned::Md] Planned MD RAID
+      # @param section [AutoinstProfile::PartitionSection] AutoYaST specification
+      def add_md_reuse(md, section)
+        # TODO: fix when not using named raids
+        md_to_reuse = devicegraph.md_raids.find { |m| m.name == md.name }
+        if md_to_reuse.nil?
+          issues_list.add(:missing_reusable_device, section)
+          return
+        end
+        add_device_reuse(md, md_to_reuse.name, !!section.format)
+      end
+
       # @param devicegraph [Devicegraph] Devicegraph to search for the partition to reuse
       # @param part_section [AutoinstProfile::PartitionSection] Partition specification
       #   from AutoYaST

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)
+          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_issues/missing_reusable_device_test.rb

@@ -0,0 +1,45 @@
+#!/usr/bin/env rspec
+# encoding: utf-8
+
+# Copyright (c) [2017] SUSE LLC
+#
+# All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of version 2 of the GNU General Public License as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, contact SUSE LLC.
+#
+# To contact SUSE LLC about this file by physical or electronic mail, you may
+# find current contact information at www.suse.com.
+
+require_relative "../../spec_helper"
+require "y2storage/autoinst_issues/missing_reuse_info"
+require "y2storage/autoinst_profile/partition_section"
+
+describe Y2Storage::AutoinstIssues::MissingReusableDevice do
+  subject(:issue) { described_class.new(section) }
+
+  let(:section) do
+    instance_double(Y2Storage::AutoinstProfile::PartitionSection)
+  end
+
+  describe "#message" do
+    it "returns a description of the issue" do
+      expect(issue.message).to eq("Reusable device not found")
+    end
+  end
+
+  describe "#severity" do
+    it "returns :warn" do
+      expect(issue.severity).to eq(:warn)
+    end
+  end
+end

test/y2storage/autoinst_issues/missing_reuse_info_test.rb

@@ -0,0 +1,45 @@
+#!/usr/bin/env rspec
+# encoding: utf-8
+
+# Copyright (c) [2017] SUSE LLC
+#
+# All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of version 2 of the GNU General Public License as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, contact SUSE LLC.
+#
+# To contact SUSE LLC about this file by physical or electronic mail, you may
+# find current contact information at www.suse.com.
+
+require_relative "../../spec_helper"
+require "y2storage/autoinst_issues/missing_reuse_info"
+require "y2storage/autoinst_profile/partition_section"
+
+describe Y2Storage::AutoinstIssues::MissingReuseInfo do
+  subject(:issue) { described_class.new(section) }
+
+  let(:section) do
+    instance_double(Y2Storage::AutoinstProfile::PartitionSection)
+  end
+
+  describe "#message" do
+    it "returns a description of the issue" do
+      expect(issue.message).to include "Not enough information"
+    end
+  end
+
+  describe "#severity" do
+    it "returns :warn" do
+      expect(issue.severity).to eq(:warn)
+    end
+  end
+end