Skip to content

Commit

Permalink
fix: UI crashes with Multipath or RAID (#1212)
Browse files Browse the repository at this point in the history
## Problem

The storage UI crashes when the system has Multipath or BIOS RAID
devices.

Source of the problem:

* `Storage1.Multipath` and `Storage1.RAID` D-Bus interfaces export the
path of the wires and RAID devices respectively.
* Neither Multipath wires nor RAID devices are exported on D-Bus.
* The UI fails to build the Multipath wires and/or RAID devices because
they are not found in the list of devices.

## Solution

* Modify the D-Bus interfaces in order to export the name of the
Multipath wires and RAID devices instead of their D-Bus path.
* Adapt the UI storage client to use the name of the Multipath wires and
RAID devices instead of trying to build a complete device.

Note: At this moment, the D-Bus storage service only exports the devices
useful for the installation. The Multipath wires or RAID devices are not
exported because they cannot be used for installing the system. In the
future we can consider to export those devices too, adapting the UI to
avoid selecting that devices for the installation.

<details>
<summary>Toggle screenshots</summary>

![localhost_8080_
(3)](https://github.com/openSUSE/agama/assets/1112304/f3c1baae-cb57-4ba0-817d-fab95cf76821)

![localhost_8080_
(4)](https://github.com/openSUSE/agama/assets/1112304/1b3ffd08-3a98-4c2a-8638-1bc54dd0ccd8)

![localhost_8080_
(5)](https://github.com/openSUSE/agama/assets/1112304/93a4cfc5-3127-49f4-b0f7-94ece4b37f05)

</details>
  • Loading branch information
joseivanlopez committed May 15, 2024
2 parents 742d5eb + 4c1808a commit e3ca84c
Show file tree
Hide file tree
Showing 9 changed files with 79 additions and 37 deletions.
4 changes: 2 additions & 2 deletions rust/agama-lib/src/storage/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ pub struct Md {
#[derive(Debug, Clone, Serialize, Deserialize, utoipa::ToSchema)]
#[serde(rename_all = "camelCase")]
pub struct Multipath {
pub wires: Vec<DeviceSid>,
pub wires: Vec<String>,
}

#[derive(Debug, Clone, Serialize, Deserialize, utoipa::ToSchema)]
Expand Down Expand Up @@ -653,5 +653,5 @@ impl TryFrom<zbus::zvariant::Value<'_>> for UnusedSlot {
#[derive(Debug, Clone, Serialize, Deserialize, utoipa::ToSchema)]
#[serde(rename_all = "camelCase")]
pub struct Raid {
pub devices: Vec<DeviceSid>,
pub devices: Vec<String>,
}
8 changes: 5 additions & 3 deletions service/lib/agama/dbus/storage/interfaces/device/multipath.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,19 @@ def self.apply?(storage_device)
MULTIPATH_INTERFACE = "org.opensuse.Agama.Storage1.Multipath"
private_constant :MULTIPATH_INTERFACE

# Paths of the D-Bus objects representing the multipath wires.
# Name of the multipath wires.
#
# @note: The multipath wires are not exported in D-Bus yet.
#
# @return [Array<String>]
def multipath_wires
storage_device.parents.map { |p| tree.path_for(p) }
storage_device.parents.map(&:name)
end

def self.included(base)
base.class_eval do
dbus_interface MULTIPATH_INTERFACE do
dbus_reader :multipath_wires, "ao", dbus_name: "Wires"
dbus_reader :multipath_wires, "as", dbus_name: "Wires"
end
end
end
Expand Down
8 changes: 5 additions & 3 deletions service/lib/agama/dbus/storage/interfaces/device/raid.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,19 @@ def self.apply?(storage_device)
RAID_INTERFACE = "org.opensuse.Agama.Storage1.RAID"
private_constant :RAID_INTERFACE

# Paths of the D-Bus objects representing the devices used by the DM RAID.
# Name of the devices used by the DM RAID.
#
# @note: The devices used by a DM RAID are not exported in D-Bus yet.
#
# @return [Array<String>]
def raid_devices
storage_device.parents.map { |p| tree.path_for(p) }
storage_device.parents.map(&:name)
end

def self.included(base)
base.class_eval do
dbus_interface RAID_INTERFACE do
dbus_reader :raid_devices, "ao", dbus_name: "Devices"
dbus_reader :raid_devices, "as", dbus_name: "Devices"
end
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 May 15 12:52:42 UTC 2024 - José Iván López González <jlopez@suse.com>

- Export the device name of the Multipath wires and RAID devices
instead of their D-Bus path (gh#openSUSE/agama#1212).

-------------------------------------------------------------------
Mon May 6 05:13:11 UTC 2024 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,9 @@
let(:device) { devicegraph.multipaths.first }

describe "#multipath_wires" do
it "returns the D-Bus path of the Multipath wires" do
sda = devicegraph.find_by_name("/dev/sda")
sdb = devicegraph.find_by_name("/dev/sdb")

it "returns the name of the Multipath wires" do
expect(subject.multipath_wires)
.to contain_exactly(tree.path_for(sda), tree.path_for(sdb))
.to contain_exactly("/dev/sda", "/dev/sdb")
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,9 @@
let(:device) { devicegraph.dm_raids.first }

describe "#raid_devices" do
it "returns the D-Bus path of the RAID devices" do
sdb = devicegraph.find_by_name("/dev/sdb")
sdc = devicegraph.find_by_name("/dev/sdc")

it "returns the name of the RAID devices" do
expect(subject.raid_devices)
.to contain_exactly(tree.path_for(sdb), tree.path_for(sdc))
.to contain_exactly("/dev/sdb", "/dev/sdc")
end
end
end
Expand Down
3 changes: 1 addition & 2 deletions service/test/agama/software/manager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,9 @@
describe "#products" do
it "returns the list of known products" do
products = subject.products
expect(products.size).to eq(4)
expect(products.size).to eq(3)
expect(products).to all(be_a(Agama::Software::Product))
expect(products).to contain_exactly(
an_object_having_attributes(id: "ALP-Dolomite"),
an_object_having_attributes(id: "Tumbleweed"),
an_object_having_attributes(id: "MicroOS"),
an_object_having_attributes(id: "MicroOS-Desktop")
Expand Down
33 changes: 21 additions & 12 deletions web/src/client/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,22 @@ class DevicesManager {
*/
async getDevices() {
const buildDevice = (jsonDevice, jsonDevices) => {
/** @type {() => StorageDevice} */
const buildDefaultDevice = () => {
return {
sid: 0,
name: "",
description: "",
isDrive: false,
type: ""
};
};

/** @type {(names: string[]) => StorageDevice[]} */
const buildCollectionFromNames = (names) => {
return names.map(name => ({ ...buildDefaultDevice(), name }));
};

/** @type {(sids: String[], jsonDevices: object[]) => StorageDevice[]} */
const buildCollection = (sids, jsonDevices) => {
if (sids === null || sids === undefined) return [];
Expand All @@ -251,20 +267,20 @@ class DevicesManager {

/** @type {(device: StorageDevice, info: object) => void} */
const addRaidInfo = (device, info) => {
device.devices = buildCollection(info.devices, jsonDevices);
device.devices = buildCollectionFromNames(info.devices);
};

/** @type {(device: StorageDevice, info: object) => void} */
const addMultipathInfo = (device, info) => {
device.wires = buildCollection(info.wires, jsonDevices);
device.wires = buildCollectionFromNames(info.wires);
};

/** @type {(device: StorageDevice, info: object) => void} */
const addMDInfo = (device, info) => {
device.type = "md";
device.level = info.level;
device.uuid = info.uuid;
addRaidInfo(device, info);
device.devices = buildCollection(info.devices, jsonDevices);
};

/** @type {(device: StorageDevice, info: object) => void} */
Expand All @@ -282,7 +298,7 @@ class DevicesManager {
};

/** @type {(device: StorageDevice, info: object) => void} */
const addLvInfo = (device, info) => {
const addLvInfo = (device, _info) => {
device.type = "lvmLv";
};

Expand Down Expand Up @@ -317,14 +333,7 @@ class DevicesManager {
};
};

/** @type {StorageDevice} */
const device = {
sid: 0,
name: "",
description: "",
isDrive: false,
type: ""
};
const device = buildDefaultDevice();

/** @type {(jsonProperty: String, info: function) => void} */
const process = (jsonProperty, method) => {
Expand Down
40 changes: 35 additions & 5 deletions web/src/client/storage.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -421,9 +421,39 @@ sdf1.component = {

md0.devices = [sda1, sda2];

raid.devices = [sdb, sdc];

multipath.wires = [sdd, sde];
raid.devices = [
{
sid: 0,
name: "/dev/sdb",
description: "",
isDrive: false,
type: ""
},
{
sid: 0,
name: "/dev/sdc",
description: "",
isDrive: false,
type: ""
}
];

multipath.wires = [
{
sid: 0,
name: "/dev/sdd",
description: "",
isDrive: false,
type: ""
},
{
sid: 0,
name: "/dev/sde",
description: "",
isDrive: false,
type: ""
}
];

lvmVg.logicalVolumes = [lvmLv1];
lvmVg.physicalVolumes = [sdf1];
Expand Down Expand Up @@ -905,7 +935,7 @@ const contexts = {
}
},
raid: {
devices: [62, 63]
devices: ["/dev/sdb", "/dev/sdc"]
}
},
{
Expand Down Expand Up @@ -938,7 +968,7 @@ const contexts = {
}
},
multipath: {
wires: [64, 65]
wires: ["/dev/sdd", "/dev/sde"]
}
},
{
Expand Down

0 comments on commit e3ca84c

Please sign in to comment.