From 2eecbfb7dfdedbbec06f59f939dd52bc675bac75 Mon Sep 17 00:00:00 2001 From: Ancor Gonzalez Sosa Date: Wed, 31 Jul 2024 16:08:47 +0200 Subject: [PATCH] WIP: tests and fixes for size calculation --- service/lib/agama/storage/config.rb | 15 +- .../filesystem_type/from_json.rb | 2 +- .../config_conversions/from_json_test.rb | 162 ++++++++++++++++-- 3 files changed, 159 insertions(+), 20 deletions(-) diff --git a/service/lib/agama/storage/config.rb b/service/lib/agama/storage/config.rb index cf1b6df89c..7f41be1bf9 100644 --- a/service/lib/agama/storage/config.rb +++ b/service/lib/agama/storage/config.rb @@ -100,11 +100,12 @@ def default_size_devices end def default_size(device, attr, builder) - # TODO: what to do if path is nil or empty? - path = device.filesystem&.path - # TODO: what to do if there is no default volume? + path = device.filesystem&.path || "" vol = builder.for(path) + return fallback_size(attr) unless vol + # Theoretically, neither Volume#min_size or Volume#max_size can be nil + # At most they will be zero or unlimited, respectively return vol.send(:"#{attr}_size") unless vol.auto_size? outline = vol.outline @@ -113,6 +114,14 @@ def default_size(device, attr, builder) size_with_snapshots(size, device, outline) end + # TODO: these are the fallbacks used when constructing volumes, not sure if repeating them + # here is right + def fallback_size(attr) + return Y2Storage::DiskSize.zero if attr == :min + + Y2Storage::DiskSize.unlimited + end + def size_with_fallbacks(device, outline, attr, builder) proposed_paths = filesystems.map(&:path) diff --git a/service/lib/agama/storage/config_conversions/filesystem_type/from_json.rb b/service/lib/agama/storage/config_conversions/filesystem_type/from_json.rb index 8d0d4e7cc0..f1a97e9c2f 100644 --- a/service/lib/agama/storage/config_conversions/filesystem_type/from_json.rb +++ b/service/lib/agama/storage/config_conversions/filesystem_type/from_json.rb @@ -72,7 +72,7 @@ def convert_btrfs(default = nil) default_config.tap do |config| snapshots = btrfs_json[:snapshots] - config.snapshots = snapshots if snapshots + config.snapshots = snapshots unless snapshots.nil? end end end diff --git a/service/test/agama/storage/config_conversions/from_json_test.rb b/service/test/agama/storage/config_conversions/from_json_test.rb index 48bb205f25..386cae557c 100644 --- a/service/test/agama/storage/config_conversions/from_json_test.rb +++ b/service/test/agama/storage/config_conversions/from_json_test.rb @@ -43,6 +43,10 @@ "volume_templates" => [ { "mount_path" => "/", "filesystem" => "btrfs", "size" => { "auto" => true }, + "btrfs" => { + "snapshots" => true, "default_subvolume" => "@", + "subvolumes" => ["home", "opt", "root", "srv"] + }, "outline" => { "required" => true, "snapshots_configurable" => true, "auto_size" => { @@ -53,22 +57,29 @@ } }, { - "mount_path" => "/home", - "outline" => { - "required" => false, "filesystem" => "xfs", - "size" => { "auto" => false, "min" => "5 GiB" } - } + "mount_path" => "/home", "size" => { "auto" => false, "min" => "5 GiB" }, + "outline" => { "required" => false, "filesystem" => "xfs" } }, { "mount_path" => "swap", - "outline" => { "required" => false } - } + "outline" => { "required" => false, "filesystem" => "swap" } + }, + { "mount_path" => "", "size" => { "min" => "100 MiB" } } ] } } end describe "#convert" do + using Y2Storage::Refinements::SizeCasts + + # TODO: + # Encryption + # Filesystem type (btrfs, etc) + # Filesystem at disk (including default types based on config, etc.) + # Filesystem at partition + # Partition id + context "with an empty JSON configuration" do let(:config_json) { {} } @@ -94,18 +105,11 @@ context "with some drives and boot configuration at JSON" do let(:config_json) do { - boot: { - configure: true, - device: "/dev/sdb" - }, + boot: { configure: true, device: "/dev/sdb" }, drives: [ { ptableType: "gpt", - partitions: [ - { - filesystem: { path: "/", type: { btrfs: { snapshots: false } } } - } - ] + partitions: [{ filesystem: { path: "/" } }] } ] } @@ -122,6 +126,132 @@ expect(config.boot.configure).to eq true expect(config.boot.device).to eq "/dev/sdb" end + + it "includes the corresponding drives" do + config = subject.convert + expect(config.drives.size).to eq 1 + drive = config.drives.first + expect(drive).to be_a(Agama::Storage::Configs::Drive) + expect(drive.ptable_type).to eq Y2Storage::PartitionTables::Type::GPT + expect(drive.partitions.size).to eq 1 + partition = drive.partitions.first + expect(partition.filesystem.path).to eq "/" + end + end + + context "omitting sizes for the partitions" do + let(:config_json) do + { + drives: [ + { + partitions: [ + { + filesystem: { path: "/", type: { btrfs: { snapshots: false } } } + }, + { + filesystem: { path: "/home" } + }, + { + filesystem: { path: "/opt" } + }, + { + filesystem: { path: "swap" } + } + ] + } + ] + } + end + + it "uses default sizes" do + config = subject.convert + partitions = config.drives.first.partitions + expect(partitions).to contain_exactly( + an_object_having_attributes( + filesystem: have_attributes(path: "/"), + size: have_attributes(default: true, min: 5.GiB, max: 10.GiB) + ), + an_object_having_attributes( + filesystem: have_attributes(path: "/home"), + size: have_attributes(default: true, min: 5.GiB, max: Y2Storage::DiskSize.unlimited) + ), + an_object_having_attributes( + filesystem: have_attributes(path: "/opt"), + size: have_attributes(default: true, min: 100.MiB, max: Y2Storage::DiskSize.unlimited) + ), + an_object_having_attributes( + filesystem: have_attributes(path: "swap"), + size: have_attributes( + default: true, min: Y2Storage::DiskSize.zero, max: Y2Storage::DiskSize.unlimited + ) + ) + ) + end + end + + # Note the min is mandatory + context "specifying size limits for the partitions" do + let(:config_json) do + { + drives: [ + { + partitions: [ + { + filesystem: { path: "/", type: { btrfs: { snapshots: false } } }, + size: { min: "3 GiB" } + }, + { + filesystem: { path: "/home" }, + size: { min: "6 GiB", max: "9 GiB" } + } + ] + } + ] + } + end + + it "sets both min and max limits as requested" do + config = subject.convert + partitions = config.drives.first.partitions + expect(partitions).to include( + an_object_having_attributes( + filesystem: have_attributes(path: "/home"), + size: have_attributes(default: false, min: 6.GiB, max: 9.GiB) + ) + ) + end + + it "uses unlimited for the omitted max sizes" do + config = subject.convert + partitions = config.drives.first.partitions + expect(partitions).to include( + an_object_having_attributes( + filesystem: have_attributes(path: "/"), + size: have_attributes(default: false, min: 3.GiB, max: Y2Storage::DiskSize.unlimited) + ) + ) + end + end + + context "specifying a filesystem for a drive" do + let(:config_json) do + { + drives: [{ filesystem: filesystem }] + } + end + + context "if the filesystem specification only contains a path" do + let(:filesystem) { { path: "/" } } + + it "uses the default type and btrfs attributes for that path" do + config = subject.convert + filesystem = config.drives.first.filesystem + expect(filesystem.type.fstype).to eq Y2Storage::Filesystems::Type::BTRFS + expect(filesystem.type.btrfs.snapshots).to eq true + expect(filesystem.type.btrfs.default_subvolume).to eq "@" + expect(filesystem.type.btrfs.subvolumes.map(&:path)).to eq ["home", "opt", "root", "srv"] + end + end end end end