Skip to content

Commit

Permalink
web: Reuse devices (#1165)
Browse files Browse the repository at this point in the history
Implement feature for reusing an existing device (i.e., allow formatting
a block device or mounting a file system). Main changes:

* Redesign the dialog for changing location:
* Shows a table with all the devices that can be selected as location
for the volumes.
* Allows selecting how to allocate the file system at selected device
(create partition, create dedicated LVM, format, mount).
* Add a menu option in the table of volumes for resetting the location
(i.e., to use the installation device instead of a custom location).

Note: We have detected some problems with the UI for changing location
implemented in this PR:

* The table is scrollable. If there are too many devices, then there is
a chance of overlooking some devices if you don't realize about the
scroll.
* Having a table and group of radios at the end looks quite
overwhelming. The very same information could be provided in a simpler
way.
* Ideally, this change location option should be integrated in the form
of the volume.

Check some ideas in the thread of this PR and at
#1176.

<details>
<summary>Toggle screenshots</summary> 
 
![localhost_8080_
(44)](https://github.com/openSUSE/agama/assets/1112304/e93882df-a8ab-4c48-8df9-63154011adb0)

![localhost_8080_
(45)](https://github.com/openSUSE/agama/assets/1112304/f9cd0c6f-484c-4903-8961-94169fffe9b4)

</details>
  • Loading branch information
joseivanlopez authored May 3, 2024
2 parents 0535723 + ca7e244 commit aa89adb
Show file tree
Hide file tree
Showing 41 changed files with 1,697 additions and 1,020 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def filesystem_sid
#
# @return [String] e.g., "ext4"
def filesystem_type
storage_device.filesystem.type.to_s
storage_device.filesystem.type.to_human_string
end

# Mount path of the file system.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,9 @@ def missing_volumes
def volume_device_conversion(target)
return unless settings.device.is_a?(DeviceSettings::Disk)

target.volumes.select(&:proposed?).each { |v| v.device ||= settings.device.name }
target.volumes
.select { |v| v.proposed? && !v.reuse_name }
.each { |v| v.device ||= settings.device.name }
end

# @param target [Y2Storage::ProposalSettings]
Expand Down
4 changes: 2 additions & 2 deletions service/lib/agama/storage/volume_conversion/from_y2storage.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ def sizes_conversion(target)
planned = planned_device_for(target.mount_path)
return unless planned

target.min_size = planned.min
target.max_size = planned.max
target.min_size = planned.min if planned.respond_to?(:min)
target.max_size = planned.max if planned.respond_to?(:max)
end

# Planned device for the given mount path.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

describe "#filesystem_type" do
it "returns the file system type" do
expect(subject.filesystem_type).to eq("ext4")
expect(subject.filesystem_type).to eq("Ext4")
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,25 @@
expect(y2storage_settings.candidate_devices).to contain_exactly("/dev/sda")
end
end

context "and a volume is reusing a device" do
before do
settings.volumes = [
Agama::Storage::Volume.new("/test1").tap do |volume|
volume.location.target = :device
volume.location.device = "/dev/sdb"
end
]
end

it "does not set the target device as device for the volume with missing device" do
y2storage_settings = subject.convert

expect(y2storage_settings.volumes).to contain_exactly(
an_object_having_attributes(mount_point: "/test1", device: nil)
)
end
end
end

context "when the device settings is set to create a new LVM volume group" do
Expand Down
1 change: 1 addition & 0 deletions web/cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
"subvolume",
"subvolumes",
"svgrrc",
"scrollbox",
"teleporter",
"testfile",
"testsuite",
Expand Down
6 changes: 6 additions & 0 deletions web/package/cockpit-agama.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Fri May 3 09:45:36 UTC 2024 - José Iván López González <[email protected]>

- Allow reusing an existing device or file system
(gh#openSUSE/agama#1165).

-------------------------------------------------------------------
Thu May 02 11:07:23 UTC 2024 - Balsa Asanovic <[email protected]>

Expand Down
34 changes: 34 additions & 0 deletions web/src/assets/styles/composition.scss
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,40 @@
flex-wrap: wrap;
}

// TODO: make it less specific.
.location-layout > div {
display: flex;

form {
display: flex;
flex-direction: column;
flex: 1 1 0;
gap: 0
}

form > div:nth-child(2) {
overflow-y: auto;
min-block-size: 120px;
margin-block-end: var(--spacer-medium);
table { background: transparent; }
}

form > div:last-child {
min-block-size: min-content;
}
}

// FIXME: Temporary solution to hide expand button. The button should be removed instead.
.location-layout table button[id^="expand-toggle"] {
display: none;
}

.location-layout table tbody tr:not(:first-child) {
td:nth-child(3) {
padding-inline-start: 5ch;
}
}

[data-state="reversed"] {
flex-direction: row-reverse;
}
Expand Down
8 changes: 8 additions & 0 deletions web/src/assets/styles/patternfly-overrides.scss
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,14 @@ ul {
vertical-align: middle;
}

.pf-v5-c-radio {
align-items: center;
}

.pf-v5-c-radio__label {
font-size: var(--pf-v5-c-form__label--FontSize);
}

@media screen and (width <= 768px) {
.pf-m-grid-md.pf-v5-c-table tr:where(.pf-v5-c-table__tr):not(.pf-v5-c-table__expandable-row) {
padding-inline: 0;
Expand Down
12 changes: 12 additions & 0 deletions web/src/assets/styles/utilities.scss
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,18 @@
max-inline-size: calc(var(--ui-max-inline-size) + var(--spacer-large))
}

.scrollbox {
background:
linear-gradient(#fff 33%, rgb(255 255 255 / 0%)),
linear-gradient(rgb(255 255 255 / 0%), #fff 66%) 0 100%,
radial-gradient(farthest-side at 50% 0, rgb(102 102 102 / 50%), rgb(0 0 0 / 0%)),
radial-gradient(farthest-side at 50% 100%, rgba(102 102 102 / 50%), rgb(0 0 0 / 0%)) 0 100%;
background-color: #fff;
background-repeat: no-repeat;
background-attachment: local, local, scroll, scroll;
background-size: 100% 48px, 100% 48px, 100% 16px, 100% 16px;
}

.height-75 {
height: 75dvh;
}
Expand Down
45 changes: 43 additions & 2 deletions web/src/client/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,40 @@ class ProposalManager {
return proxy.AvailableDevices.map(path => findDevice(systemDevices, path)).filter(d => d);
}

/**
* Gets the devices that can be selected as target for a volume.
*
* A device can be selected as target for a volume if either it is an available device for
* installation or it is a device built over the available devices for installation. For example,
* a MD RAID is a possible target only if all its members are available devices or children of the
* available devices.
*
* @returns {Promise<StorageDevice[]>}
*/
async getVolumeDevices() {
/** @type {StorageDevice[]} */
const availableDevices = await this.getAvailableDevices();

/** @type {(device: StorageDevice) => boolean} */
const isAvailable = (device) => {
const isChildren = (device, parentDevice) => {
const partitions = parentDevice.partitionTable?.partitions || [];
return !!partitions.find(d => d.name === device.name);
};

return !!availableDevices.find(d => d.name === device.name || isChildren(device, d));
};

/** @type {(device: StorageDevice[]) => boolean} */
const allAvailable = (devices) => devices.every(isAvailable);

const system = await this.system.getDevices();
const mds = system.filter(d => d.type === "md" && allAvailable(d.devices));
const vgs = system.filter(d => d.type === "lvmVg" && allAvailable(d.physicalVolumes));

return [...availableDevices, ...mds, ...vgs];
}

/**
* Gets the list of meaningful mount points for the selected product
*
Expand Down Expand Up @@ -537,7 +571,7 @@ class ProposalManager {
return dbusTargetPVDevices.v.map(d => d.v);
};

// TODO: Read installation devices from D-Bus.
/** @todo Read installation devices from D-Bus. */
const buildInstallationDevices = (dbusSettings, devices) => {
const findDevice = (name) => {
const device = devices.find(d => d.name === name);
Expand All @@ -547,10 +581,17 @@ class ProposalManager {
return device;
};

// Only consider the device assigned to a volume as installation device if it is needed
// to find space in that device. For example, devices directly formatted or mounted are not
// considered as installation devices.
const volumes = dbusSettings.Volumes.v.filter(vol => (
[VolumeTargets.NEW_PARTITION, VolumeTargets.NEW_VG].includes(vol.v.Target.v))
);

const values = [
dbusSettings.TargetDevice?.v,
buildTargetPVDevices(dbusSettings.TargetPVDevices),
dbusSettings.Volumes.v.map(vol => vol.v.TargetDevice.v)
volumes.map(vol => vol.v.TargetDevice.v)
].flat();

if (dbusSettings.ConfigureBoot.v) values.push(dbusSettings.BootDevice.v);
Expand Down
43 changes: 29 additions & 14 deletions web/src/components/core/ExpandableSelector.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@
import React, { useState } from "react";
import { Table, Thead, Tr, Th, Tbody, Td, ExpandableRowContent, RowSelectVariant } from "@patternfly/react-table";

/**
* @typedef {import("@patternfly/react-table").TableProps} TableProps
* @typedef {import("react").RefAttributes<HTMLTableElement>} HTMLTableProps
*/

/**
* An object for sharing data across nested maps
*
Expand Down Expand Up @@ -93,23 +98,26 @@ const sanitizeSelection = (selection, allowMultiple) => {
};

/**
* Build a expandable table with selectable items
* Build a expandable table with selectable items.
* @component
*
* @note It only accepts one nesting level.
*
* @param {object} props
* @param {ExpandableSelectorColumn[]} props.columns - Collection of objects defining columns.
* @param {boolean} [props.isMultiple=false] - Whether multiple selection is allowed.
* @param {object[]} props.items - Collection of items to be rendered.
* @param {string} [props.itemIdKey="id"] - The key for retrieving the item id.
* @param {(item: object) => Array<object>} [props.itemChildren=() => []] - Lookup method to retrieve children from given item.
* @param {(item: object) => boolean} [props.itemSelectable=() => true] - Whether an item will be selectable or not.
* @param {(item: object) => (string|undefined)} [props.itemClassNames=() => ""] - Callback that allows adding additional CSS class names to item row.
* @param {object[]} [props.itemsSelected=[]] - Collection of selected items.
* @param {string[]} [props.initialExpandedKeys=[]] - Ids of initially expanded items.
* @param {(selection: Array<object>) => void} [props.onSelectionChange=noop] - Callback to be triggered when selection changes.
* @param {object} [props.tableProps] - Props for {@link https://www.patternfly.org/components/table/#table PF/Table}.
* @typedef {object} ExpandableSelectorBaseProps
* @property {ExpandableSelectorColumn[]} [columns=[]] - Collection of objects defining columns.
* @property {boolean} [isMultiple=false] - Whether multiple selection is allowed.
* @property {object[]} [items=[]] - Collection of items to be rendered.
* @property {string} [itemIdKey="id"] - The key for retrieving the item id.
* @property {(item: object) => Array<object>} [itemChildren=() => []] - Lookup method to retrieve children from given item.
* @property {(item: object) => boolean} [itemSelectable=() => true] - Whether an item will be selectable or not.
* @property {(item: object) => (string|undefined)} [itemClassNames=() => ""] - Callback that allows adding additional CSS class names to item row.
* @property {object[]} [itemsSelected=[]] - Collection of selected items.
* @property {any[]} [initialExpandedKeys=[]] - Ids of initially expanded items.
* @property {(selection: Array<object>) => void} [onSelectionChange=noop] - Callback to be triggered when selection changes.
*
* @typedef {ExpandableSelectorBaseProps & TableProps & HTMLTableProps} ExpandableSelectorProps
*
* @param {ExpandableSelectorProps} props
*/
export default function ExpandableSelector({
columns = [],
Expand All @@ -126,7 +134,14 @@ export default function ExpandableSelector({
}) {
const [expandedItemsKeys, setExpandedItemsKeys] = useState(initialExpandedKeys);
const selection = sanitizeSelection(itemsSelected, isMultiple);
const isItemSelected = (item) => selection.includes(item);
const isItemSelected = (item) => {
const selected = selection.find((selectionItem) => {
return Object.hasOwn(selectionItem, itemIdKey) &&
selectionItem[itemIdKey] === item[itemIdKey];
});

return selected !== undefined || selection.includes(item);
};
const isItemExpanded = (key) => expandedItemsKeys.includes(key);
const toggleExpanded = (key) => {
if (isItemExpanded(key)) {
Expand Down
10 changes: 5 additions & 5 deletions web/src/components/core/TreeTable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ import { Table, Thead, Tr, Th, Tbody, Td, TreeRowWrapper } from '@patternfly/rea

/**
* @typedef {object} TreeTableColumn
* @property {string} title
* @property {(any) => React.ReactNode} content
* @property {string} name
* @property {(object) => React.ReactNode} value
* @property {string} [classNames]
*/

Expand Down Expand Up @@ -82,14 +82,14 @@ export default function TreeTable({
const renderColumns = (item, treeRow) => {
return columns.map((c, cIdx) => {
const props = {
dataLabel: c.title,
dataLabel: c.name,
className: c.classNames
};

if (cIdx === 0) props.treeRow = treeRow;

return (
<Td key={cIdx} {...props}>{c.content(item)}</Td>
<Td key={cIdx} {...props}>{c.value(item)}</Td>
);
});
};
Expand Down Expand Up @@ -138,7 +138,7 @@ export default function TreeTable({
>
<Thead>
<Tr>
{ columns.map((c, i) => <Th key={i} className={c.classNames}>{c.title}</Th>) }
{ columns.map((c, i) => <Th key={i} className={c.classNames}>{c.name}</Th>) }
</Tr>
</Thead>
<Tbody>
Expand Down
20 changes: 11 additions & 9 deletions web/src/components/storage/BootConfigField.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,23 +56,25 @@ const Button = ({ isBold = false, onClick }) => {
* Allows to select the boot config.
* @component
*
* @param {object} props
* @param {boolean} props.configureBoot
* @param {StorageDevice|undefined} props.bootDevice
* @param {StorageDevice|undefined} props.defaultBootDevice
* @param {StorageDevice[]} props.devices
* @param {boolean} props.isLoading
* @param {(boot: BootConfig) => void} props.onChange
* @typedef {object} BootConfigFieldProps
* @property {boolean} configureBoot
* @property {StorageDevice|undefined} bootDevice
* @property {StorageDevice|undefined} defaultBootDevice
* @property {StorageDevice[]} availableDevices
* @property {boolean} isLoading
* @property {(boot: BootConfig) => void} onChange
*
* @typedef {object} BootConfig
* @property {boolean} configureBoot
* @property {StorageDevice} bootDevice
*
* @param {BootConfigFieldProps} props
*/
export default function BootConfigField({
configureBoot,
bootDevice,
defaultBootDevice,
devices,
availableDevices,
isLoading,
onChange
}) {
Expand Down Expand Up @@ -113,7 +115,7 @@ export default function BootConfigField({
configureBoot={configureBoot}
bootDevice={bootDevice}
defaultBootDevice={defaultBootDevice}
devices={devices}
availableDevices={availableDevices}
onAccept={onAccept}
onCancel={closeDialog}
/>
Expand Down
Loading

0 comments on commit aa89adb

Please sign in to comment.