Merge pull request #1382 from ancorgs/better_bigger_resize

Improve the :bigger_resize SpaceMaker strategy

package/yast2-storage-ng.changes

@@ -1,3 +1,12 @@
+-------------------------------------------------------------------
+Wed Apr 24 07:48:08 UTC 2024 - Ancor Gonzalez Sosa <ancor@suse.com>
+
+- GuidedProposal: refine the :bigger_resize SpaceMaker strategy
+  (gh#openSUSE/agama#1164).
+- Fixed a bug related to the calculation of partitions required
+  for booting when RAID is involved.
+- 5.0.13
+
 -------------------------------------------------------------------
 Wed Apr 17 15:56:25 UTC 2024 - Ladislav Slezák <lslezak@suse.com>
 

package/yast2-storage-ng.spec

@@ -16,7 +16,7 @@
 #
 
 Name:           yast2-storage-ng
-Version:        5.0.12
+Version:        5.0.13
 Release:        0
 Summary:        YaST2 - Storage Configuration
 License:        GPL-2.0-only OR GPL-3.0-only

src/lib/y2storage/boot_requirements_strategies/analyzer.rb

@@ -85,6 +85,8 @@ def boot_disk
         return @boot_disk if @boot_disk
 
         @boot_disk = devicegraph.find_by_name(boot_disk_name) if boot_disk_name
+        # If the disk was explicitly chosen via boot_disk_name, we are all set
+        return @boot_disk if @boot_disk
 
         @boot_disk ||= boot_disk_from_planned_dev
         @boot_disk ||= boot_disk_from_devicegraph

src/lib/y2storage/proposal/space_maker_actions/bigger_resize_strategy.rb

@@ -34,18 +34,21 @@ def initialize(settings, _disk_analyzer)
           @settings = settings
           @to_delete_mandatory = []
           @to_delete_optional = []
+          @to_wipe = []
           @to_resize = []
         end
 
         # @param disk [Disk] see {List}
         def add_mandatory_actions(disk)
-          devices = disk.partition_table? ? partitions(disk) : [disk]
-          devices.select! { |d| configured?(d, :force_delete) }
+          return unless disk.partition_table?
+
+          devices = partitions(disk).select { |p| configured?(p, :force_delete) }
           to_delete_mandatory.concat(devices)
         end
 
         # @param disk [Disk] see {List}
         def add_optional_actions(disk, _lvm_helper)
+          add_wipe(disk)
           add_resize(disk)
           add_optional_delete(disk)
         end
@@ -57,8 +60,9 @@ def next
           return unless dev
 
           return Shrink.new(dev) if source == :to_resize
+          return Wipe.new(dev) if source == :to_wipe
 
-          dev.is?(:partition) ? Delete.new(dev, related_partitions: false) : Wipe.new(dev)
+          Delete.new(dev, related_partitions: false)
         end
 
         # @param deleted_sids [Array<Integer>] see {List}
@@ -74,15 +78,26 @@ def done(deleted_sids)
         # @return [ProposalSpaceSettings] proposal settings for making space
         attr_reader :settings
 
-        # @return [Array<BlkDevice>] list of devices to be deleted or emptied (mandatory)
+        # @return [Array<BlkDevice>] list of devices to be deleted (mandatory)
         attr_reader :to_delete_mandatory
 
-        # @return [Array<BlkDevice>] list of devices to be deleted or emptied (optionally)
+        # @return [Array<BlkDevice>] list of devices to be deleted (optionally)
         attr_reader :to_delete_optional
 
         # @return [Array<Partition>] list of partitions to be shrunk
         attr_reader :to_resize
 
+        # @return [Array<BlkDevice>] list of disks to be emptied if needed
+        attr_reader :to_wipe
+
+        # @see #add_optional_actions
+        # @param disk [Disk]
+        def add_wipe(disk)
+          return if disk.partition_table?
+
+          to_wipe << disk
+        end
+
         # @see #add_optional_actions
         # @param disk [Disk]
         def add_resize(disk)
@@ -110,12 +125,10 @@ def preferred_resize(part1, part2)
         #
         # @param disk [Disk]
         def add_optional_delete(disk)
-          if disk.partition_table?
-            partitions = partitions(disk).select { |p| configured?(p, :delete) }
-            to_delete_optional.concat(partitions.sort { |a, b| preferred_delete(a, b) })
-          elsif configured?(disk, :delete)
-            to_delete_optional << disk
-          end
+          return unless disk.partition_table?
+
+          partitions = partitions(disk).select { |p| configured?(p, :delete) }
+          to_delete_optional.concat(partitions.sort { |a, b| preferred_delete(a, b) })
         end
 
         # Compares two partitions to decide which one should be deleted first
@@ -152,6 +165,8 @@ def cleanup(collection, deleted_sids)
         def source_for_next
           if to_delete_mandatory.any?
             :to_delete_mandatory
+          elsif to_wipe.any?
+            :to_wipe
           elsif to_resize.any?
             :to_resize
           else

src/lib/y2storage/proposal_space_settings.rb

@@ -82,13 +82,12 @@ def self.delete_modes
     # @return [Boolean]
     attr_accessor :delete_resize_configurable
 
-    # What to do with existing partitions and disks if they are involved in the process of making
-    # space.
+    # What to do with existing partitions if they are involved in the process of making space.
     #
     # Keys are device names (like in BlkDevice#name, no alternative names) that correspond to a
-    # partition or to a disk with no partitions.
+    # partition.
     #
-    # The value for each key specifies what to do with the corresponding device if the storage
+    # The value for each key specifies what to do with the corresponding partition if the storage
     # proposal needs to process the corresponding disk. If the device is not explicitly mentioned,
     # nothing will be done. Possible values are :resize, :delete and :force_delete.
     #

test/y2storage/proposal/space_maker_bigger_resize_test.rb

@@ -56,6 +56,7 @@ def probed_partition(name)
       end
     end
 
+    # :force_delete for disks should be ignored, actions only make sense for partitions and LVs
     context "if :force_delete is specified for a disk that contains partitions" do
       let(:settings_actions) { { "/dev/sda" => :force_delete } }
 
@@ -79,6 +80,7 @@ def probed_partition(name)
       end
     end
 
+    # :force_delete for disks should be ignored, actions only make sense for partitions and LVs
     context "if :force_delete is specified for a directly formatted disk (no partition table)" do
       let(:scenario) { "multipath-formatted.xml" }
 
@@ -88,13 +90,15 @@ def probed_partition(name)
         settings.root_device = "/dev/mapper/0QEMU_QEMU_HARDDISK_mpath1"
       end
 
-      it "empties the device deleting the filesystem" do
-        expect(fake_devicegraph.filesystems.size).to eq 1
+      it "does not modify the content of the disk" do
+        original_filesystems = fake_devicegraph.filesystems
+        expect(original_filesystems.size).to eq 1
 
         result = maker.prepare_devicegraph(fake_devicegraph)
-        disk = result.disk_devices.first
-        expect(disk.has_children?).to eq false
-        expect(result.filesystems).to be_empty
+        filesystems = result.filesystems
+        expect(filesystems.size).to eq 1
+        device = filesystems.first.blk_devices.first
+        expect(device.name).to eq "/dev/mapper/0QEMU_QEMU_HARDDISK_mpath1"
       end
     end
 
@@ -185,36 +189,23 @@ def probed_partition(name)
       let(:scenario) { "lvm-disk-as-pv.xml" }
       let(:vol1) { planned_vol(mount_point: "/1", type: :ext4, min: 5.GiB) }
 
-      context "and the disk is not mentioned in the settings" do
-        let(:settings_actions) { { "/dev/sda1" => :delete } }
+      it "empties the disk deleting the LVM VG" do
+        expect(fake_devicegraph.lvm_vgs.size).to eq 1
 
-        it "raises an Error exception" do
-          expect { maker.provide_space(fake_devicegraph, volumes, lvm_helper) }
-            .to raise_error Y2Storage::Error
-        end
+        result = maker.provide_space(fake_devicegraph, volumes, lvm_helper)
+        disk = result[:devicegraph].disks.first
+        expect(disk.has_children?).to eq false
+        expect(result[:devicegraph].lvm_vgs).to be_empty
       end
 
-      context "and the disk is marked to be deleted" do
-        let(:settings_actions) { { "/dev/sda" => :delete } }
-
-        it "empties the disk deleting the LVM VG" do
-          expect(fake_devicegraph.lvm_vgs.size).to eq 1
-
-          result = maker.provide_space(fake_devicegraph, volumes, lvm_helper)
-          disk = result[:devicegraph].disks.first
-          expect(disk.has_children?).to eq false
-          expect(result[:devicegraph].lvm_vgs).to be_empty
-        end
-
-        it "assumes a (future) GPT partition table" do
-          gpt_size = 1.MiB
-          # The final 16.5 KiB are reserved by GPT
-          gpt_final_space = 16.5.KiB
+      it "assumes a (future) GPT partition table" do
+        gpt_size = 1.MiB
+        # The final 16.5 KiB are reserved by GPT
+        gpt_final_space = 16.5.KiB
 
-          result = maker.provide_space(fake_devicegraph, volumes, lvm_helper)
-          space = result[:partitions_distribution].spaces.first
-          expect(space.disk_size).to eq(space.disk.size - gpt_size - gpt_final_space)
-        end
+        result = maker.provide_space(fake_devicegraph, volumes, lvm_helper)
+        space = result[:partitions_distribution].spaces.first
+        expect(space.disk_size).to eq(space.disk.size - gpt_size - gpt_final_space)
       end
     end
 
@@ -227,36 +218,23 @@ def probed_partition(name)
         settings.root_device = "/dev/mapper/0QEMU_QEMU_HARDDISK_mpath1"
       end
 
-      context "and the device is not mentioned in the settings" do
-        let(:settings_actions) { {} }
+      it "empties the device deleting the filesystem" do
+        expect(fake_devicegraph.filesystems.size).to eq 1
 
-        it "raises an Error exception" do
-          expect { maker.provide_space(fake_devicegraph, volumes, lvm_helper) }
-            .to raise_error Y2Storage::Error
-        end
+        result = maker.provide_space(fake_devicegraph, volumes, lvm_helper)
+        disk = result[:devicegraph].disk_devices.first
+        expect(disk.has_children?).to eq false
+        expect(result[:devicegraph].filesystems).to be_empty
       end
 
-      context "and the disk is marked to be deleted" do
-        let(:settings_actions) { { "/dev/mapper/0QEMU_QEMU_HARDDISK_mpath1" => :delete } }
-
-        it "empties the device deleting the filesystem" do
-          expect(fake_devicegraph.filesystems.size).to eq 1
-
-          result = maker.provide_space(fake_devicegraph, volumes, lvm_helper)
-          disk = result[:devicegraph].disk_devices.first
-          expect(disk.has_children?).to eq false
-          expect(result[:devicegraph].filesystems).to be_empty
-        end
-
-        it "assumes a (future) GPT partition table" do
-          gpt_size = 1.MiB
-          # The final 16.5 KiB are reserved by GPT
-          gpt_final_space = 16.5.KiB
+      it "assumes a (future) GPT partition table" do
+        gpt_size = 1.MiB
+        # The final 16.5 KiB are reserved by GPT
+        gpt_final_space = 16.5.KiB
 
-          result = maker.provide_space(fake_devicegraph, volumes, lvm_helper)
-          space = result[:partitions_distribution].spaces.first
-          expect(space.disk_size).to eq(space.disk.size - gpt_size - gpt_final_space)
-        end
+        result = maker.provide_space(fake_devicegraph, volumes, lvm_helper)
+        space = result[:partitions_distribution].spaces.first
+        expect(space.disk_size).to eq(space.disk.size - gpt_size - gpt_final_space)
       end
     end
 

test/y2storage/proposal_agama_advanced_test.rb

@@ -52,6 +52,7 @@
       fake_devicegraph.mount_points.each { |i| i.parents.first.remove_mount_point }
 
       settings.space_settings.strategy = :bigger_resize
+      settings.lvm_vg_reuse = false
       # Agama uses homogeneous weights for all volumes
       settings.volumes.each { |v| v.weight = 100 }
       # Activate support for separate LVM VGs
@@ -80,5 +81,98 @@
         expect { proposal.propose }.to_not raise_error
       end
     end
+
+    context "with one disk containing partitions and another directly formatted" do
+      let(:scenario) { "gpt_msdos_and_empty" }
+
+      let(:lvm) { true }
+
+      before do
+        settings.candidate_devices = ["/dev/sdc", "/dev/sdf"]
+        settings.root_device = "/dev/sdc"
+      end
+
+      let(:volumes) { [{ "mount_point" => "/", "fs_type" => "xfs", "min_size" => size }] }
+
+      context "if there is no need to use the formatted disk (everything fits in the other)" do
+        let(:size) { "200 GiB" }
+
+        it "does not modify the formatted disk" do
+          proposal.propose
+          disk = proposal.devices.find_by_name("/dev/sdf")
+          expect(disk.filesystem.type.is?(:xfs)).to eq true
+          expect(disk.partitions).to be_empty
+        end
+      end
+
+      context "if the formatted disk needs to be used" do
+        let(:size) { "970 GiB" }
+
+        it "empties the disk deleting the filesystem" do
+          proposal.propose
+          disk = proposal.devices.find_by_name("/dev/sdf")
+          expect(disk.filesystem).to be_nil
+          expect(disk.partitions).to_not be_empty
+        end
+      end
+
+      context "if non-mandatory actions are possible to make space" do
+        let(:size) { "100 GiB" }
+
+        before do
+          settings.candidate_devices = ["/dev/sda", "/dev/sdf"]
+          settings.root_device = "/dev/sda"
+        end
+
+        it "tries to use the formatted disk before trying an optional delete" do
+          sda1_sid = fake_devicegraph.find_by_name("/dev/sda1").sid
+
+          settings.space_settings.actions = { "/dev/sda1" => :delete }
+          proposal.propose
+          expect(proposal.failed?).to eq false
+
+          disk = proposal.devices.find_by_name("/dev/sdf")
+          expect(disk.filesystem).to be_nil
+          expect(disk.partitions).to_not be_empty
+
+          expect(proposal.devices.find_by_name("/dev/sda1").sid).to eq sda1_sid
+        end
+
+        it "tries to use the formatted disk before trying an optional resize" do
+          orig_sda1 = fake_devicegraph.find_by_name("/dev/sda1")
+
+          settings.space_settings.actions = { "/dev/sda1" => :resize }
+          proposal.propose
+          expect(proposal.failed?).to eq false
+
+          disk = proposal.devices.find_by_name("/dev/sdf")
+          expect(disk.filesystem).to be_nil
+          expect(disk.partitions).to_not be_empty
+
+          sda1 = proposal.devices.find_by_name("/dev/sda1")
+          expect(sda1.sid).to eq orig_sda1.sid
+          expect(sda1.size).to eq orig_sda1.size
+        end
+      end
+    end
+
+    context "when installing on a disk that previously contained a RAID1" do
+      let(:scenario) { "windows-pc-raid1.xml" }
+
+      before do
+        settings.candidate_devices = ["/dev/sda"]
+        settings.root_device = "/dev/sda"
+        # Let's ensure a bios_boot partition is needed
+        allow(storage_arch).to receive(:efiboot?).and_return(false)
+      end
+
+      # In the past, the pre-existing RAID1 was considered to be the booting disk due to some
+      # false asumptions made by the BootRequirementsChecker.
+      it "creates the partitions needed for booting in the correct disk" do
+        proposal.propose
+        disk = proposal.devices.find_by_name("/dev/sda")
+        expect(disk.partitions.map(&:id)).to include Y2Storage::PartitionId::BIOS_BOOT
+      end
+    end
   end
 end