From c302e1e6a6e481cd4a157caeead79499a4b30114 Mon Sep 17 00:00:00 2001 From: Ancor Gonzalez Sosa Date: Tue, 19 Mar 2024 15:41:21 +0100 Subject: [PATCH 1/4] [Service] Alternative location for volumes --- .../storage/volume_conversion/from_dbus.rb | 12 ++- .../dbus/storage/volume_conversion/to_dbus.rb | 4 +- .../to_y2storage.rb | 5 +- service/lib/agama/storage/volume.rb | 13 +-- .../volume_conversion/from_y2storage.rb | 14 ++- .../storage/volume_conversion/to_y2storage.rb | 28 +++++- service/lib/agama/storage/volume_location.rb | 78 +++++++++++++++ .../to_dbus_test.rb | 2 +- .../volume_conversion/from_dbus_test.rb | 24 ++++- .../storage/volume_conversion/to_dbus_test.rb | 8 +- .../to_y2storage_test.rb | 5 +- .../volume_conversion/from_y2storage_test.rb | 18 ++-- .../volume_conversion/to_y2storage_test.rb | 98 ++++++++++++++++++- 13 files changed, 266 insertions(+), 43 deletions(-) create mode 100644 service/lib/agama/storage/volume_location.rb diff --git a/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb b/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb index 3499c599fc..e66f361a3a 100644 --- a/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb +++ b/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb @@ -20,6 +20,7 @@ # find current contact information at www.suse.com. require "agama/storage/volume" +require "agama/storage/volume_location" require "agama/storage/volume_templates_builder" require "y2storage/disk_size" require "y2storage/filesystems/type" @@ -67,8 +68,8 @@ def convert CONVERSIONS = { "MountPath" => :mount_path_conversion, "MountOptions" => :mount_options_conversion, + "Target" => :target_conversion, "TargetDevice" => :target_device_conversion, - "TargetVG" => :target_vg_conversion, "FsType" => :fs_type_conversion, "MinSize" => :min_size_conversion, "MaxSize" => :max_size_conversion, @@ -92,13 +93,16 @@ def mount_options_conversion(target, value) # @param target [Agama::Storage::Volume] # @param value [String] def target_device_conversion(target, value) - target.device = value + target.location.device = value end # @param target [Agama::Storage::Volume] # @param value [String] - def target_vg_conversion(target, value) - target.separate_vg_name = value + def target_conversion(target, value) + target_value = value.downcase.to_sym + return unless Agama::Storage::VolumeLocation.targets.include?(target_value) + + target.location.target = target_value end # @param target [Agama::Storage::Volume] diff --git a/service/lib/agama/dbus/storage/volume_conversion/to_dbus.rb b/service/lib/agama/dbus/storage/volume_conversion/to_dbus.rb index 3f586f7d2e..9c4c51533b 100644 --- a/service/lib/agama/dbus/storage/volume_conversion/to_dbus.rb +++ b/service/lib/agama/dbus/storage/volume_conversion/to_dbus.rb @@ -37,8 +37,8 @@ def convert { "MountPath" => volume.mount_path.to_s, "MountOptions" => volume.mount_options, - "TargetDevice" => volume.device.to_s, - "TargetVG" => volume.separate_vg_name.to_s, + "Target" => volume.location.target.to_s, + "TargetDevice" => volume.location.device.to_s, "FsType" => volume.fs_type&.to_human_string || "", "MinSize" => volume.min_size&.to_i, "AutoSize" => volume.auto_size?, diff --git a/service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb b/service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb index e7ca55e481..0f695d0774 100644 --- a/service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb +++ b/service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb @@ -83,7 +83,8 @@ def lvm_conversion(target) lvm = settings.lvm.enabled? target.lvm = lvm - target.separate_vgs = lvm + # Activate support for dedicated volume groups + target.separate_vgs = true # Prevent VG reuse target.lvm_vg_reuse = false end @@ -170,7 +171,7 @@ def find_max_size_fallback(mount_path) def all_devices devices = [settings.boot_device] + settings.lvm.system_vg_devices + - settings.volumes.map(&:device) + settings.volumes.map(&:location).reject(&:reuse_device?).map(&:device) devices.compact.uniq.map { |d| device_or_partitions(d) }.flatten end diff --git a/service/lib/agama/storage/volume.rb b/service/lib/agama/storage/volume.rb index 6b6c231dc2..2ff73300d7 100644 --- a/service/lib/agama/storage/volume.rb +++ b/service/lib/agama/storage/volume.rb @@ -23,6 +23,7 @@ require "y2storage/disk_size" require "agama/storage/btrfs_settings" require "agama/storage/volume_outline" +require "agama/storage/volume_location" module Agama module Storage @@ -58,15 +59,10 @@ class Volume # @return [Array] attr_accessor :mount_options - # Used to locate the volume in a separate disk + # Location of the volume # - # @return [String, nil] - attr_accessor :device - - # Used to locate the volume in a separate VG - # - # @return [String, nil] - attr_accessor :separate_vg_name + # @return [VolumeLocation] + attr_accessor :location # Min size for the volume # @@ -98,6 +94,7 @@ def initialize(mount_path) @max_size = Y2Storage::DiskSize.unlimited @btrfs = BtrfsSettings.new @outline = VolumeOutline.new + @location = VolumeLocation.new end def_delegators :outline, diff --git a/service/lib/agama/storage/volume_conversion/from_y2storage.rb b/service/lib/agama/storage/volume_conversion/from_y2storage.rb index 364531699c..ee4dd9651d 100644 --- a/service/lib/agama/storage/volume_conversion/from_y2storage.rb +++ b/service/lib/agama/storage/volume_conversion/from_y2storage.rb @@ -41,13 +41,12 @@ def convert volume = VolumeTemplatesBuilder.new_from_config(config).for(spec.mount_point || "") volume.tap do |target| - target.device = spec.device - target.separate_vg_name = spec.separate_vg_name target.mount_options = spec.mount_options target.fs_type = spec.fs_type sizes_conversion(target) btrfs_conversion(target) + location_conversion(target) end end @@ -83,6 +82,17 @@ def btrfs_conversion(target) target.btrfs.read_only = spec.btrfs_read_only end + # @param target [Agama::Storage::Volume] + def location_conversion(target) + if spec.reuse? + target.location.target = spec.reformat? ? :device : :filesystem + target.location.device = spec.reuse_name + elsif !!spec.device + target.location.target = spec.separate_vg? ? :new_vg : :new_partition + target.location.device = spec.device + end + end + # Planned device for the given mount path. # @param mount_path [String] diff --git a/service/lib/agama/storage/volume_conversion/to_y2storage.rb b/service/lib/agama/storage/volume_conversion/to_y2storage.rb index 605da35778..0e62261f3c 100644 --- a/service/lib/agama/storage/volume_conversion/to_y2storage.rb +++ b/service/lib/agama/storage/volume_conversion/to_y2storage.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright (c) [2023] SUSE LLC +# Copyright (c) [2023-2024] SUSE LLC # # All Rights Reserved. # @@ -37,8 +37,6 @@ def initialize(volume) # @return [Y2Storage::VolumeSpecification] def convert # rubocop:disable Metrics/AbcSize Y2Storage::VolumeSpecification.new({}).tap do |target| - target.device = volume.device - target.separate_vg_name = volume.separate_vg_name target.mount_point = volume.mount_path target.mount_options = volume.mount_options.join(",") target.proposed = true @@ -50,6 +48,7 @@ def convert # rubocop:disable Metrics/AbcSize sizes_conversion(target) btrfs_conversion(target) + location_conversion(target) end end @@ -88,6 +87,29 @@ def btrfs_conversion(target) target.btrfs_default_subvolume = volume.btrfs.default_subvolume target.btrfs_read_only = volume.btrfs.read_only? end + + # @param target [Y2Storage::VolumeSpecification] + def location_conversion(target) + location = volume.location + return if location.default? + + if location.reuse_device? + target.reuse_name = location.device + target.reformat = location.target == :device + return + end + + target.device = location.device + target.separate_vg_name = vg_name(target) if location.target == :new_vg + end + + # Name to be used as separate_vg_name for the given Y2Storage volume + # + # @param target [Y2Storage::VolumeSpecification] + def vg_name(target) + mount_point = target.root? ? "root" : target.mount_point.sub(%r{^/}, "") + "vg-#{mount_point.tr("/", "_")}" + end end end end diff --git a/service/lib/agama/storage/volume_location.rb b/service/lib/agama/storage/volume_location.rb new file mode 100644 index 0000000000..1d0a5656b4 --- /dev/null +++ b/service/lib/agama/storage/volume_location.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +# Copyright (c) [2024] 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. + +module Agama + module Storage + # Settings specifying what device should be used for a given Volume and how + class VolumeLocation + # @see .targets + TARGETS = [:default, :new_partition, :new_vg, :device, :filesystem].freeze + private_constant :TARGETS + + # What to do to allocate the volume + # + # @return [Symbol] see .targets + attr_reader :target + + # Concrete device to allocate the volume, the exact meaning depends on {#target} + # + # @return [String, nil] + attr_accessor :device + + # All possible values for #target: + # + # - :default new partition or logical volume in the default device + # - :new_partition new partition at the disk pointed by {#device} + # - :new_vg new LV in a new dedicated VG created at a the disk pointed by {#device} + # - :device the existing block device specified by {#device} is used and reformatted + # - :filesystem: the existing filesystem on the device specified by {#device} is mounted + # + # @return [Array] + def self.targets + TARGETS + end + + # Constructor + def initialize + @target = :default + end + + # Sets the value of {#target} ensuring it is valid + def target=(value) + return unless TARGETS.include?(value) + + @target = value + end + + # @return [Boolean] + def default? + target == :default + end + + # Whether the chosen target implies reusing an existing device (formatting it or not) + # + # @return [Boolean] + def reuse_device? + [:device, :filesystem].include?(target) + end + end + end +end diff --git a/service/test/agama/dbus/storage/proposal_settings_conversion/to_dbus_test.rb b/service/test/agama/dbus/storage/proposal_settings_conversion/to_dbus_test.rb index 44ef43b0ca..c5961e1585 100644 --- a/service/test/agama/dbus/storage/proposal_settings_conversion/to_dbus_test.rb +++ b/service/test/agama/dbus/storage/proposal_settings_conversion/to_dbus_test.rb @@ -80,7 +80,7 @@ "MountPath" => "/test", "MountOptions" => [], "TargetDevice" => "", - "TargetVG" => "", + "Target" => "default", "FsType" => "", "MinSize" => 0, "AutoSize" => false, diff --git a/service/test/agama/dbus/storage/volume_conversion/from_dbus_test.rb b/service/test/agama/dbus/storage/volume_conversion/from_dbus_test.rb index c441879372..5512783c33 100644 --- a/service/test/agama/dbus/storage/volume_conversion/from_dbus_test.rb +++ b/service/test/agama/dbus/storage/volume_conversion/from_dbus_test.rb @@ -67,7 +67,7 @@ "MountPath" => "/test", "MountOptions" => ["rw", "default"], "TargetDevice" => "/dev/sda", - "TargetVG" => "/dev/system", + "Target" => "new_vg", "FsType" => "Ext4", "MinSize" => 1024, "MaxSize" => 2048, @@ -89,8 +89,8 @@ expect(volume).to be_a(Agama::Storage::Volume) expect(volume.mount_path).to eq("/test") expect(volume.mount_options).to contain_exactly("rw", "default") - expect(volume.device).to eq("/dev/sda") - expect(volume.separate_vg_name).to eq("/dev/system") + expect(volume.location.device).to eq("/dev/sda") + expect(volume.location.target).to eq(:new_vg) expect(volume.fs_type).to eq(Y2Storage::Filesystems::Type::EXT4) expect(volume.auto_size?).to eq(false) expect(volume.min_size.to_i).to eq(1024) @@ -107,8 +107,7 @@ expect(volume).to be_a(Agama::Storage::Volume) expect(volume.mount_path).to eq("/test") expect(volume.mount_options).to contain_exactly("data=ordered") - expect(volume.device).to be_nil - expect(volume.separate_vg_name).to be_nil + expect(volume.location.target).to eq :default expect(volume.fs_type).to eq(Y2Storage::Filesystems::Type::BTRFS) expect(volume.auto_size?).to eq(false) expect(volume.min_size.to_i).to eq(5 * (1024**3)) @@ -243,5 +242,20 @@ expect(volume.btrfs.snapshots?).to eq(false) end end + + context "when the D-Bus settings provide a Target that makes no sense" do + let(:dbus_volume) do + { + "MountPath" => "/test", + "Target" => "new_disk" + } + end + + it "ignores the Target value provided from D-Bus and uses :default" do + volume = subject.convert + + expect(volume.location.target).to eq :default + end + end end end diff --git a/service/test/agama/dbus/storage/volume_conversion/to_dbus_test.rb b/service/test/agama/dbus/storage/volume_conversion/to_dbus_test.rb index 9fffa181a2..2356c0c94e 100644 --- a/service/test/agama/dbus/storage/volume_conversion/to_dbus_test.rb +++ b/service/test/agama/dbus/storage/volume_conversion/to_dbus_test.rb @@ -47,8 +47,8 @@ volume.btrfs.snapshots = true volume.btrfs.read_only = true volume.mount_options = ["rw", "default"] - volume.device = "/dev/sda" - volume.separate_vg_name = "/dev/system" + volume.location.device = "/dev/sda" + volume.location.target = :new_partition volume.min_size = Y2Storage::DiskSize.new(1024) volume.max_size = Y2Storage::DiskSize.new(2048) volume.auto_size = true @@ -61,7 +61,7 @@ "MountPath" => "/test", "MountOptions" => [], "TargetDevice" => "", - "TargetVG" => "", + "Target" => "default", "FsType" => "", "MinSize" => 0, "AutoSize" => false, @@ -82,7 +82,7 @@ "MountPath" => "/test", "MountOptions" => ["rw", "default"], "TargetDevice" => "/dev/sda", - "TargetVG" => "/dev/system", + "Target" => "new_partition", "FsType" => "Ext4", "MinSize" => 1024, "MaxSize" => 2048, diff --git a/service/test/agama/storage/proposal_settings_conversion/to_y2storage_test.rb b/service/test/agama/storage/proposal_settings_conversion/to_y2storage_test.rb index 404cb716d6..8eae303204 100644 --- a/service/test/agama/storage/proposal_settings_conversion/to_y2storage_test.rb +++ b/service/test/agama/storage/proposal_settings_conversion/to_y2storage_test.rb @@ -44,7 +44,10 @@ settings.encryption.pbkd_function = Y2Storage::PbkdFunction::ARGON2ID settings.space.policy = :custom settings.space.actions = { "/dev/sda" => :force_delete } - volume = Agama::Storage::Volume.new("/test").tap { |v| v.device = "/dev/sdc" } + volume = Agama::Storage::Volume.new("/test").tap do |vol| + vol.location.target = :new_partition + vol.location.device = "/dev/sdc" + end settings.volumes = [volume] end end diff --git a/service/test/agama/storage/volume_conversion/from_y2storage_test.rb b/service/test/agama/storage/volume_conversion/from_y2storage_test.rb index c21495500d..22108805de 100644 --- a/service/test/agama/storage/volume_conversion/from_y2storage_test.rb +++ b/service/test/agama/storage/volume_conversion/from_y2storage_test.rb @@ -57,14 +57,16 @@ expect(volume).to be_a(Agama::Storage::Volume) expect(volume).to have_attributes( - mount_path: "/", - device: "/dev/sda", - separate_vg_name: "/dev/vg0", - mount_options: contain_exactly("defaults", "ro"), - fs_type: Y2Storage::Filesystems::Type::BTRFS, - min_size: Y2Storage::DiskSize.GiB(5), - max_size: Y2Storage::DiskSize.GiB(20), - btrfs: an_object_having_attributes( + mount_path: "/", + location: an_object_having_attributes( + device: "/dev/sda", + target: :new_vg + ), + mount_options: contain_exactly("defaults", "ro"), + fs_type: Y2Storage::Filesystems::Type::BTRFS, + min_size: Y2Storage::DiskSize.GiB(5), + max_size: Y2Storage::DiskSize.GiB(20), + btrfs: an_object_having_attributes( snapshots?: true, subvolumes: contain_exactly("@/home", "@/var"), default_subvolume: "@", diff --git a/service/test/agama/storage/volume_conversion/to_y2storage_test.rb b/service/test/agama/storage/volume_conversion/to_y2storage_test.rb index ba8263aaca..a08ce862eb 100644 --- a/service/test/agama/storage/volume_conversion/to_y2storage_test.rb +++ b/service/test/agama/storage/volume_conversion/to_y2storage_test.rb @@ -29,8 +29,8 @@ describe "#convert" do let(:volume) do Agama::Storage::Volume.new("/").tap do |volume| - volume.device = "/dev/sda" - volume.separate_vg_name = "/dev/vg0" + volume.location.device = "/dev/sda" + volume.location.target = :new_vg volume.mount_options = ["defaults"] volume.fs_type = btrfs volume.auto_size = false @@ -59,7 +59,7 @@ expect(spec).to be_a(Y2Storage::VolumeSpecification) expect(spec).to have_attributes( device: "/dev/sda", - separate_vg_name: "/dev/vg0", + separate_vg_name: "vg-root", mount_point: "/", mount_options: "defaults", proposed?: true, @@ -102,5 +102,97 @@ ) end end + + context "when the default target is used" do + before { volume.location.target = :default } + + it "sets both #device and #reuse_name to nil" do + spec = subject.convert + + expect(spec.device).to be_nil + expect(spec.reuse_name).to be_nil + end + end + + context "when the target is a new dedicated partition" do + before { volume.location.target = :new_partition } + + it "sets #device to the expected disk name" do + expect(subject.convert.device).to eq "/dev/sda" + end + + it "sets #separate_vg_name and #reuse_name to nil" do + spec = subject.convert + + expect(spec.reuse_name).to be_nil + expect(spec.separate_vg_name).to be_nil + end + end + + context "when the target is a new dedicated volume group" do + before { volume.location.target = :new_vg } + + context "when the mount point is /" do + it "sets #device, #separate_vg_name and #reuse_name to the expected values" do + spec = subject.convert + + expect(spec.device).to eq "/dev/sda" + expect(spec.reuse_name).to be_nil + expect(spec.separate_vg_name).to eq "vg-root" + end + end + + context "when the mount point is not the root one" do + let(:volume) do + Agama::Storage::Volume.new("/var/log").tap do |volume| + volume.location.device = "/dev/sda" + end + end + + it "sets #device, #separate_vg_name and #reuse_name to the expected values" do + spec = subject.convert + + expect(spec.device).to eq "/dev/sda" + expect(spec.reuse_name).to be_nil + expect(spec.separate_vg_name).to eq "vg-var_log" + end + end + end + + context "when the target is an existing block device" do + before { volume.location.target = :device } + + it "sets #reuse_name and #reformat to the proper values" do + spec = subject.convert + + expect(spec.reuse_name).to eq "/dev/sda" + expect(spec.reformat).to eq true + end + + it "sets #device and #separate_vg_name to nil" do + spec = subject.convert + + expect(spec.device).to be_nil + expect(spec.separate_vg_name).to be_nil + end + end + + context "when the target is an existing file system" do + before { volume.location.target = :filesystem } + + it "sets #reuse_name and #reformat to the proper values" do + spec = subject.convert + + expect(spec.reuse_name).to eq "/dev/sda" + expect(spec.reformat).to eq false + end + + it "sets #device and #separate_vg_name to nil" do + spec = subject.convert + + expect(spec.device).to be_nil + expect(spec.separate_vg_name).to be_nil + end + end end end From 7d23a24040fd22746f52d9e230397a79020f6d7d Mon Sep 17 00:00:00 2001 From: Ancor Gonzalez Sosa Date: Wed, 20 Mar 2024 15:02:52 +0100 Subject: [PATCH 2/4] [Service] Update D-Bus documentation --- .../org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml | 4 ++-- doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml b/doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml index 6b5130f2f6..254aa14124 100644 --- a/doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml +++ b/doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml @@ -18,8 +18,8 @@