From c57edd2b70e952db7d60d748ef5823b0ed07923f Mon Sep 17 00:00:00 2001 From: Ancor Gonzalez Sosa Date: Tue, 8 Aug 2023 15:41:46 +0200 Subject: [PATCH] Update tests for DBus::Storage::Manager and add fixes --- service/lib/agama/dbus/storage/manager.rb | 5 +- .../proposal_settings_conversion/from_dbus.rb | 6 +- .../storage/volume_conversion/from_dbus.rb | 8 +- .../volume_conversion/from_y2storage.rb | 2 +- .../agama/storage/volume_templates_builder.rb | 77 +++--- .../test/agama/dbus/storage/manager_test.rb | 235 ++++++++++-------- 6 files changed, 199 insertions(+), 134 deletions(-) diff --git a/service/lib/agama/dbus/storage/manager.rb b/service/lib/agama/dbus/storage/manager.rb index f85afe2229..b38070c537 100644 --- a/service/lib/agama/dbus/storage/manager.rb +++ b/service/lib/agama/dbus/storage/manager.rb @@ -25,7 +25,7 @@ require "agama/dbus/storage/proposal" require "agama/dbus/storage/proposal_settings_conversion" require "agama/dbus/storage/volume_conversion" -require "agama/dbus/storage/volume_templates_builder" +require "agama/storage/volume_templates_builder" require "agama/dbus/base_object" require "agama/dbus/with_service_status" require "agama/dbus/interfaces/issues" @@ -343,7 +343,8 @@ def config # @return [Agama::VolumeTemplatesBuilder] def volume_templates_builder - @volume_templates_builder ||= VolumeTemplatesBuilder.new_from_config(config) + @volume_templates_builder ||= + Agama::Storage::VolumeTemplatesBuilder.new_from_config(config) end end end diff --git a/service/lib/agama/dbus/storage/proposal_settings_conversion/from_dbus.rb b/service/lib/agama/dbus/storage/proposal_settings_conversion/from_dbus.rb index 5fcd671cc4..0b307ee0a4 100644 --- a/service/lib/agama/dbus/storage/proposal_settings_conversion/from_dbus.rb +++ b/service/lib/agama/dbus/storage/proposal_settings_conversion/from_dbus.rb @@ -46,7 +46,11 @@ def convert settings.tap do |target| dbus_settings.each do |dbus_property, dbus_value| - send(CONVERSIONS[dbus_property], target, dbus_value) + meth = CONVERSIONS[dbus_property] + # FIXME: likely ignoring the wrong attribute is not the best + next unless meth + + send(meth, target, dbus_value) end end end 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 91a951e583..f732b76fec 100644 --- a/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb +++ b/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb @@ -42,11 +42,15 @@ def initialize(dbus_volume, config:) # @return [Agama::Storage::Volume] def convert builder = Agama::Storage::VolumeTemplatesBuilder.new_from_config(config) - volume = builder.for(dbus_volume["MountPath"]) + volume = builder.for(dbus_volume["MountPath"] || "") volume.tap do |target| dbus_volume.each do |dbus_property, dbus_value| - send(CONVERSIONS[dbus_property], target, dbus_value) + meth = CONVERSIONS[dbus_property] + # FIXME: likely ignoring the wrong attribute is not the best + next unless meth + + send(meth, target, dbus_value) end end end diff --git a/service/lib/agama/storage/volume_conversion/from_y2storage.rb b/service/lib/agama/storage/volume_conversion/from_y2storage.rb index c7404409c7..cccafd7808 100644 --- a/service/lib/agama/storage/volume_conversion/from_y2storage.rb +++ b/service/lib/agama/storage/volume_conversion/from_y2storage.rb @@ -38,7 +38,7 @@ def initialize(spec, config:) # # @return [Agama::Storage::Volume] def convert - volume = VolumeTemplatesBuilder.new_from_config(config).for(spec.mount_point) + volume = VolumeTemplatesBuilder.new_from_config(config).for(spec.mount_point || "") volume.tap do |target| target.device = spec.device diff --git a/service/lib/agama/storage/volume_templates_builder.rb b/service/lib/agama/storage/volume_templates_builder.rb index f6bbad50a0..aaad390bb1 100644 --- a/service/lib/agama/storage/volume_templates_builder.rb +++ b/service/lib/agama/storage/volume_templates_builder.rb @@ -52,7 +52,7 @@ def initialize(config_data) # @param path [String] # @return [Agama::Storage::Volume] def for(path) - data = @data[cleanpath(path)] || @data[""] || empty_data + data = @data[path_key(path)] || @data[""] || empty_data Volume.new(path).tap do |volume| volume.btrfs = data[:btrfs] @@ -72,9 +72,20 @@ def for(path) private + def fetch(hash, key, default = nil) + key_s = key.to_s + return hash[key_s] if hash.key?(key_s) + + hash.fetch(key.to_sym, default) + end + def key(data) - path = data["mount_path"] - return "" unless path + path = fetch(data, :mount_path, "") + path_key(path) + end + + def path_key(path) + return "" if path.empty? cleanpath(path) end @@ -93,28 +104,28 @@ def values(data) # rubocop:disable Metrics/AbcSize {}.tap do |values| values[:btrfs] = btrfs(data) values[:outline] = outline(data) - values[:mount_options] = data.fetch("mount_options", []) + values[:mount_options] = fetch(data, :mount_options, []) # TODO: maybe ensure consistency of values[:filesystem] and values[:outline].filesystems ? - fs = data["filesystem"] - values[:filesystem] = Y2Storage::Filesystems::Type.find(fs.downcase.to_sym) if fs + fs = fetch(data, :filesystem) + values[:filesystem] = fs_type(fs) if fs values[:filesystem] ||= values[:outline].filesystems.first values[:filesystem] ||= Y2Storage::Filesystems::Type::EXT4 - size = data.fetch("size", {}) - values[:auto_size] = size.fetch("auto", false) - values[:min_size] = parse_disksize(size["min"]) - values[:max_size] = parse_disksize(size["max"]) + size = fetch(data, :size, {}) + values[:auto_size] = fetch(size, :auto, false) + values[:min_size] = parse_disksize(fetch(size, :min)) + values[:max_size] = parse_disksize(fetch(size, :max)) end end def btrfs(data) - btrfs_data = data.fetch("btrfs", {}) + btrfs_data = fetch(data, "btrfs", {}) BtrfsSettings.new.tap do |btrfs| - btrfs.snapshots = btrfs_data.fetch("snapshots", false) - btrfs.read_only = btrfs_data.fetch("read_only", false) - btrfs.default_subvolume = btrfs_data.fetch("default_subvolume", "") - btrfs.subvolumes = btrfs_data["subvolumes"] + btrfs.snapshots = fetch(btrfs_data, "snapshots", false) + btrfs.read_only = fetch(btrfs_data, "read_only", false) + btrfs.default_subvolume = fetch(btrfs_data, "default_subvolume", "") + btrfs.subvolumes = fetch(btrfs_data, "subvolumes") btrfs.subvolumes&.map! { |subvol_data| subvolume(subvol_data) } end end @@ -123,30 +134,30 @@ def subvolume(data) return Y2Storage::SubvolSpecification.new(data) if data.is_a?(String) Y2Storage::SubvolSpecification.new( - data["path"], copy_on_write: data["copy_on_write"], archs: data["archs"] + fetch(data, :path), copy_on_write: fetch(data, :copy_on_write), archs: fetch(data, :archs) ) end def outline(data) # rubocop:disable Metrics/AbcSize - outline_data = data.fetch("outline", {}) + outline_data = fetch(data, "outline", {}) VolumeOutline.new.tap do |outline| - outline.required = outline_data.fetch("required", false) - outline.filesystems = outline_data.fetch("filesystems", []) - outline.filesystems.map! { |fs| Y2Storage::Filesystems::Type.find(fs.downcase.to_sym) } - outline.snapshots_configurable = outline_data.fetch("snapshots_configurable", true) - - size = outline_data.fetch("auto_size", {}) - min = parse_disksize(size["min"]) - max = parse_disksize(size["max"]) + outline.required = fetch(outline_data, "required", false) + outline.filesystems = fetch(outline_data, "filesystems", []) + outline.filesystems.map! { |fs| fs_type(fs) } + outline.snapshots_configurable = fetch(outline_data, "snapshots_configurable", true) + + size = fetch(outline_data, "auto_size", {}) + min = parse_disksize(fetch(size, :min)) + max = parse_disksize(fetch(size, :max)) outline.base_min_size = min if min outline.base_max_size = max if max - outline.adjust_by_ram = size.fetch("adjust_by_ram", false) - outline.min_size_fallback_for = Array(size["min_fallback_for"]) + outline.adjust_by_ram = fetch(size, :adjust_by_ram, false) + outline.min_size_fallback_for = Array(fetch(size, :min_fallback_for)) outline.min_size_fallback_for.map! { |p| cleanpath(p) } - outline.max_size_fallback_for = Array(size["max_fallback_for"]) + outline.max_size_fallback_for = Array(fetch(size, :max_fallback_for)) outline.max_size_fallback_for.map! { |p| cleanpath(p) } - assign_snapshots_increment(outline, size["snapshots_increment"]) + assign_snapshots_increment(outline, fetch(size, :snapshots_increment)) end end @@ -167,7 +178,13 @@ def parse_disksize(value) end def cleanpath(path) - Pathname.new(path).cleanpath + Pathname.new(path).cleanpath.to_s + end + + def fs_type(fs) + return fs if fs.is_a?(Y2Storage::Filesystems::Type) + + Y2Storage::Filesystems::Type.find(fs.downcase.to_sym) end end end diff --git a/service/test/agama/dbus/storage/manager_test.rb b/service/test/agama/dbus/storage/manager_test.rb index b5b99bfd62..7fb6653205 100644 --- a/service/test/agama/dbus/storage/manager_test.rb +++ b/service/test/agama/dbus/storage/manager_test.rb @@ -34,7 +34,7 @@ require "dbus" describe Agama::DBus::Storage::Manager do - subject { described_class.new(backend, logger) } + subject(:manager) { described_class.new(backend, logger) } let(:logger) { Logger.new($stdout, level: :warn) } @@ -43,6 +43,7 @@ proposal: proposal, iscsi: iscsi, software: software, + config: config, on_probe: nil, on_progress_change: nil, on_progress_finish: nil, @@ -50,8 +51,11 @@ on_deprecated_system_change: nil) end + let(:config) { Agama::Config.new(config_data) } + let(:config_data) { {} } + let(:proposal) do - instance_double(Agama::Storage::Proposal, on_calculate: nil, calculated_settings: settings) + instance_double(Agama::Storage::Proposal, on_calculate: nil, settings: settings) end let(:settings) { nil } @@ -121,75 +125,83 @@ end end - describe "#volume_templates" do - before do - allow(proposal).to receive(:volume_templates).and_return(templates) - end - - context "if there are no volume templates" do - let(:templates) { [] } - - it "returns an empty list" do - expect(subject.volume_templates).to eq([]) - end + describe "#default_volume" do + let(:config_data) do + { "storage" => { "volumes" => [], "volume_templates" => cfg_templates } } end - context "if there are volume templates" do - let(:templates) { [volume1_template, volume2_template] } + context "with no storage section in the configuration" do + let(:cfg_templates) { [] } - let(:volume1_template) do - spec = Y2Storage::VolumeSpecification.new({ - "min_size" => "5 GiB", - "max_size" => "10 GiB" - }) + it "returns the same generic default volume for any path" do + generic = { + "FsType" => "Ext4", "MountOptions" => [], + "MinSize" => 0, "MaxSize" => 0, "AutoSize" => false + } + generic_outline = { "Required" => false, "FsTypes" => [], "SupportAutoSize" => false} + + expect(subject.default_volume("/")).to include(generic) + expect(subject.default_volume("/")["Outline"]).to include(generic_outline) + + expect(subject.default_volume("swap")).to include(generic) + expect(subject.default_volume("swap")["Outline"]).to include(generic_outline) + + expect(subject.default_volume("/foo")).to include(generic) + expect(subject.default_volume("/foo")["Outline"]).to include(generic_outline) + end + end + + context "with a set of volume templates in the configuration" do + let(:cfg_templates) do + [ + { + "mount_path" => "/", "filesystem" => "btrfs", "size" => { "auto" => true }, + "outline" => { + "required" => true, + "auto_size" => { + "base_min" => "5 GiB", "base_max" => "20 GiB", "min_fallback_for" => "/home" + }, + "filesystems" => ["btrfs"] + } + }, + { + "mount_path" => "swap", "filesystem" => "swap", + "size" => { "auto" => false, "min" => "1 GiB", "max" => "2 GiB" }, + "outline" => { "required" => false, "filesystems" => ["swap"] } + }, + { + "mount_path" => "/home", "filesystem" => "xfs", + "size" => { "auto" => false, "min" => "10 GiB" }, + "outline" => { "required" => false, "filesystems" => ["xfs", "ext2"] } + }, + { + "filesystem" => "ext4", "size" => { "auto" => false, "min" => "10 GiB" }, + "outline" => { "filesystems" => ["ext3", "ext4", "xfs"] } + } + ] + end + + it "returns the appropriate volume if there is a corresponding template" do + expect(subject.default_volume("/")).to include("FsType" => "Btrfs", "AutoSize" => true) + expect(subject.default_volume("/")["Outline"]).to include( + "Required" => true, "FsTypes" => ["Btrfs"], + "SupportAutoSize" => true, "SizeRelevantVolumes" => ["/home"] + ) - Agama::Storage::Volume.new(spec).tap do |volume| - volume.mount_point = "/test" - volume.device_type = :partition - volume.encrypted = true - volume.fs_type = Y2Storage::Filesystems::Type::EXT3 - volume.fixed_size_limits = true - volume.snapshots = true - end + expect(subject.default_volume("swap")).to include( + "FsType" => "Swap", "AutoSize" => false, "MinSize" => 1024**3, "MaxSize" => 2*(1024**3) + ) + expect(subject.default_volume("swap")["Outline"]).to include( + "Required" => false, "FsTypes" => ["Swap"], "SupportAutoSize" => false + ) end - let(:volume2_template) { Agama::Storage::Volume.new } + it "returns the default volume for any path without a template" do + default = { "FsType" => "Ext4", "AutoSize" => false, "MinSize" => 10*(1024**3) } + default_outline = { "FsTypes" => ["Ext3", "Ext4", "XFS"], "SupportAutoSize" => false} - before do - allow(volume1_template).to receive(:size_relevant_volumes).and_return(["/home"]) - allow(volume1_template).to receive(:fs_types) - .and_return([Y2Storage::Filesystems::Type::EXT3]) - end - - it "returns a list with a hash for each volume template" do - expect(subject.volume_templates.size).to eq(2) - expect(subject.volume_templates).to all(be_a(Hash)) - - template1, template2 = subject.volume_templates - - expect(template1).to eq({ - "MountPoint" => "/test", - "Optional" => false, - "DeviceType" => "partition", - "Encrypted" => true, - "FsTypes" => ["Ext3"], - "FsType" => "Ext3", - "MinSize" => Y2Storage::DiskSize.GiB(5).to_i, - "MaxSize" => Y2Storage::DiskSize.GiB(10).to_i, - "FixedSizeLimits" => true, - "AdaptiveSizes" => true, - "Snapshots" => true, - "SnapshotsConfigurable" => false, - "SnapshotsAffectSizes" => false, - "SizeRelevantVolumes" => ["/home"] - }) - - expect(template2).to eq({ - "Optional" => true, - "AdaptiveSizes" => false, - "SnapshotsConfigurable" => false, - "SnapshotsAffectSizes" => false - }) + expect(subject.default_volume("/foo")).to include(default) + expect(subject.default_volume("/foo")["Outline"]).to include(default_outline) end end end @@ -221,7 +233,7 @@ describe "#calculate_proposal" do let(:dbus_settings) do { - "CandidateDevices" => ["/dev/vda"], + "BootDevice" => "/dev/vda", "LVM" => true, "EncryptionPassword" => "n0ts3cr3t", "Volumes" => dbus_volumes @@ -230,20 +242,22 @@ let(:dbus_volumes) do [ - { "MountPoint" => "/" }, - { "MountPoint" => "swap" } + { "MountPath" => "/" }, + { "MountPath" => "swap" } ] end it "calculates a proposal with settings having values from D-Bus" do expect(proposal).to receive(:calculate) do |settings| expect(settings).to be_a(Agama::Storage::ProposalSettings) - expect(settings.candidate_devices).to contain_exactly("/dev/vda") - expect(settings.lvm).to eq(true) - expect(settings.encryption_password).to eq("n0ts3cr3t") + expect(settings.boot_device).to eq "/dev/vda" + expect(settings.lvm).to be_a(Agama::Storage::LvmSettings) + expect(settings.lvm.enabled).to eq true + expect(settings.encryption).to be_a(Agama::Storage::EncryptionSettings) + expect(settings.encryption.password).to eq("n0ts3cr3t") expect(settings.volumes).to contain_exactly( - an_object_having_attributes(mount_point: "/"), - an_object_having_attributes(mount_point: "swap") + an_object_having_attributes(mount_path: "/"), + an_object_having_attributes(mount_path: "swap") ) end @@ -253,12 +267,14 @@ context "when the D-Bus settings does not include some values" do let(:dbus_settings) { {} } - it "calculates a proposal with settings having default values for the missing settings" do + it "calculates a proposal with default/empty values for the missing settings" do expect(proposal).to receive(:calculate) do |settings| expect(settings).to be_a(Agama::Storage::ProposalSettings) - expect(settings.candidate_devices).to eq([]) - expect(settings.lvm).to be_nil - expect(settings.encryption_password).to be_nil + expect(settings.boot_device).to eq nil + expect(settings.lvm).to be_a(Agama::Storage::LvmSettings) + expect(settings.lvm.enabled).to eq false + expect(settings.encryption).to be_a(Agama::Storage::EncryptionSettings) + expect(settings.encryption.password).to be_nil expect(settings.volumes).to eq([]) end @@ -266,19 +282,30 @@ end end + context "when the D-Bus settings include some unexpected attribute" do + let(:dbus_settings) { { "CandidateDevices" => ["/dev/vda"] } } + + # This is likely a temporary behavior + it "calculates a proposal ignoring the unknown attributes" do + expect(proposal).to receive(:calculate) do |settings| + expect(settings).to be_a(Agama::Storage::ProposalSettings) + end + + subject.calculate_proposal(dbus_settings) + end + end + context "when the D-Bus settings includes a volume" do let(:dbus_volumes) { [dbus_volume1] } let(:dbus_volume1) do { - "DeviceType" => "lvm_lv", - "Encrypted" => true, - "MountPoint" => "/", - "FixedSizeLimits" => true, - "MinSize" => 1024, - "MaxSize" => 2048, - "FsType" => "Ext3", - "Snapshots" => true + "MountPath" => "/", + "AutoSize" => false, + "MinSize" => 1024, + "MaxSize" => 2048, + "FsType" => "Btrfs", + "Snapshots" => true } end @@ -286,32 +313,44 @@ expect(proposal).to receive(:calculate) do |settings| volume = settings.volumes.first - expect(volume.device_type).to eq(:lvm_lv) - expect(volume.encrypted).to eq(true) - expect(volume.mount_point).to eq("/") - expect(volume.fixed_size_limits).to eq(true) + expect(volume.mount_path).to eq("/") + expect(volume.auto_size).to eq(false) expect(volume.min_size.to_i).to eq(1024) expect(volume.max_size.to_i).to eq(2048) - expect(volume.snapshots).to eq(true) + expect(volume.btrfs.snapshots).to eq(true) end subject.calculate_proposal(dbus_settings) end context "and the D-Bus volume does not include some values" do - let(:dbus_volume1) { { "MountPoint" => "/" } } + let(:dbus_volume1) { { "MountPath" => "/" } } + + let(:config_data) do + { "storage" => { "volumes" => [], "volume_templates" => cfg_templates } } + end + + let(:cfg_templates) do + [ + { + "mount_path" => "/", "filesystem" => "btrfs", + "size" => { "auto" => false, "min" => "5 GiB", "max" => "20 GiB" }, + "outline" => { + "filesystems" => ["btrfs"] + } + } + ] + end - it "calculates a proposal with settings having a volume with missing values" do + it "calculates a proposal with a volume completed with its default settings" do expect(proposal).to receive(:calculate) do |settings| volume = settings.volumes.first - expect(volume.device_type).to be_nil - expect(volume.encrypted).to be_nil - expect(volume.mount_point).to eq("/") - expect(volume.fixed_size_limits).to be_nil - expect(volume.min_size).to be_nil - expect(volume.max_size).to be_nil - expect(volume.snapshots).to be_nil + expect(volume.mount_path).to eq("/") + expect(volume.auto_size).to eq(false) + expect(volume.min_size.to_i).to eq(5*(1024**3)) + expect(volume.max_size.to_i).to eq(20*(1024**3)) + expect(volume.btrfs.snapshots).to eq(false) end subject.calculate_proposal(dbus_settings)