Skip to content

Commit

Permalink
Fixed setting unlimited maximum partition size (#1142)
Browse files Browse the repository at this point in the history
## Problem

- #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 <jlopez@suse.com>
  • Loading branch information
lslezak and joseivanlopez authored Apr 10, 2024
1 parent 05ea8cd commit 67081db
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 6 additions & 0 deletions service/package/rubygem-agama-yast.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Wed Apr 10 11:35:53 UTC 2024 - Ladislav Slezák <lslezak@suse.com>

- 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 <jlopez@suse.com>

Expand Down
3 changes: 2 additions & 1 deletion service/test/agama/dbus/storage/manager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 67081db

Please sign in to comment.