From 67081db3049bcfe9057cb356985d4a072ad8abdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Wed, 10 Apr 2024 18:01:48 +0200 Subject: [PATCH] Fixed setting unlimited maximum partition size (#1142) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem - https://github.com/openSUSE/agama/issues/1065 - The maximum partition size cannot be set to "unlimited", using empty value results in the default maximum size **Example:** Changing the default 2GB max size is not possible, it is always restored back: ![agama_max_size_broken](https://github.com/openSUSE/agama/assets/907998/04bb3f90-dbff-4a7e-99c2-32045162d64d) ## Details - The problem is that the "unlimited" value is represented using the `undefined` value in the web UI. - But `undefined` cannot be sent over D-Bus because D-Bus does not have concept for `undefined`, `nil` or `NULL` values, Iin that case the `MaxSize` is simply missing the sent D-Bus data. - When the D-Bus service creates a `Volume` object it first initializes it with the default data from the YAML product configuration file. Then it updates the attributes using the D-Bus data. Obviously when a value is missing in the D-Bus then it keeps the default value from the config. And that's the problem. ## Solution - Set the "unlimited" value when the `MaxSize` attribute is missing in the D-Bus data. ## Questions - The fix has a drawback, the current code allows sending only partial data set and ensures the defaults from the config file are used for missing values. - This won't work any more for the maximum size as a missing value won't be considered as a "use the default" but as "use the unlimited size". - Can this have some bad consequences? - The code works, but is it safe in all scenarios? ## Alternative Solution - If this fix is not correct then we should probably introduce a new key in the D-Bus data, something like `MaxSizeUnlimited` with `true` / `false` values which can be transferred via D-Bus... ## Testing - Tested manually Now it is possible to unset the maximum size: ![agama_max_size_fixed](https://github.com/openSUSE/agama/assets/907998/067c8108-75d8-40be-9eda-04828bd73e48) --------- Co-authored-by: José Iván López --- .../lib/agama/dbus/storage/volume_conversion/from_dbus.rb | 1 + service/package/rubygem-agama-yast.changes | 6 ++++++ service/test/agama/dbus/storage/manager_test.rb | 3 ++- .../agama/dbus/storage/volume_conversion/from_dbus_test.rb | 3 ++- 4 files changed, 11 insertions(+), 2 deletions(-) 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 b2440dab4f..6e6f7ecaef 100644 --- a/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb +++ b/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb @@ -53,6 +53,7 @@ def convert builder = Agama::Storage::VolumeTemplatesBuilder.new_from_config(config) builder.for(dbus_volume["MountPath"] || "").tap do |target| valid_dbus_properties.each { |p| conversion(target, p) } + target.max_size = Y2Storage::DiskSize.unlimited unless dbus_volume.key?("MaxSize") end end diff --git a/service/package/rubygem-agama-yast.changes b/service/package/rubygem-agama-yast.changes index 1e8f813afa..ee05a357fb 100644 --- a/service/package/rubygem-agama-yast.changes +++ b/service/package/rubygem-agama-yast.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Apr 10 11:35:53 UTC 2024 - Ladislav Slezák + +- Fixed setting unlimited maximum partition size + (gh#openSUSE/agama#1065) + ------------------------------------------------------------------- Wed Apr 3 15:12:05 UTC 2024 - José Iván López González diff --git a/service/test/agama/dbus/storage/manager_test.rb b/service/test/agama/dbus/storage/manager_test.rb index 7e85660587..d61bb7127a 100644 --- a/service/test/agama/dbus/storage/manager_test.rb +++ b/service/test/agama/dbus/storage/manager_test.rb @@ -391,7 +391,8 @@ 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)) + # missing maximum value means unlimited size + expect(volume.max_size.to_i).to eq(-1) expect(volume.btrfs.snapshots).to eq(false) end 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 e3d3e2bfa9..a5e584a1ef 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 @@ -113,7 +113,8 @@ 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)) - expect(volume.max_size.to_i).to eq(10 * (1024**3)) + # missing maximum value means unlimited size + expect(volume.max_size.to_i).to eq(-1) expect(volume.btrfs.snapshots?).to eq(false) end end