diff --git a/ui/webui/src/actions/storage-actions.js b/ui/webui/src/actions/storage-actions.js index fb3b89caaf4..c77a7a4d2e1 100644 --- a/ui/webui/src/actions/storage-actions.js +++ b/ui/webui/src/actions/storage-actions.js @@ -85,12 +85,12 @@ export const getDiskSelectionAction = () => { }; }; -export const getPartitioningDataAction = ({ requests, partitioning, updateOnly }) => { +export const getPartitioningDataAction = ({ requests, partitioning }) => { return async (dispatch) => { const props = { path: partitioning }; const convertRequests = reqs => reqs.map(request => Object.entries(request).reduce((acc, [key, value]) => ({ ...acc, [key]: value.v }), {})); - if (!updateOnly) { + if (!requests) { props.method = await getPartitioningMethod({ partitioning }); if (props.method === "MANUAL") { const reqs = await gatherRequests({ partitioning }); diff --git a/ui/webui/src/apis/storage.js b/ui/webui/src/apis/storage.js index d7c851b8fdd..3394d4aca97 100644 --- a/ui/webui/src/apis/storage.js +++ b/ui/webui/src/apis/storage.js @@ -488,7 +488,7 @@ export const startEventMonitorStorage = ({ dispatch }) => { if (args[0] === "org.fedoraproject.Anaconda.Modules.Storage.DiskSelection") { dispatch(getDiskSelectionAction()); } else if (args[0] === "org.fedoraproject.Anaconda.Modules.Storage.Partitioning.Manual" && Object.hasOwn(args[1], "Requests")) { - dispatch(getPartitioningDataAction({ requests: args[1].Requests.v, partitioning: path, updateOnly: true })); + dispatch(getPartitioningDataAction({ requests: args[1].Requests.v, partitioning: path })); } else if (args[0] === "org.fedoraproject.Anaconda.Modules.Storage" && Object.hasOwn(args[1], "CreatedPartitioning")) { const last = args[1].CreatedPartitioning.v.length - 1; dispatch(getPartitioningDataAction({ partitioning: args[1].CreatedPartitioning.v[last] })); diff --git a/ui/webui/src/components/AnacondaWizard.jsx b/ui/webui/src/components/AnacondaWizard.jsx index 5cbdf99500b..cd2f89c33a1 100644 --- a/ui/webui/src/components/AnacondaWizard.jsx +++ b/ui/webui/src/components/AnacondaWizard.jsx @@ -15,7 +15,7 @@ * along with This program; If not, see . */ import cockpit from "cockpit"; -import React, { useState, useMemo } from "react"; +import React, { useEffect, useState, useMemo } from "react"; import { ActionList, @@ -56,6 +56,20 @@ export const AnacondaWizard = ({ dispatch, isBootIso, osRelease, storageData, lo const [isInProgress, setIsInProgress] = useState(false); const [storageEncryption, setStorageEncryption] = useState(getStorageEncryptionState()); const [storageScenarioId, setStorageScenarioId] = useState(window.sessionStorage.getItem("storage-scenario-id") || getDefaultScenario().id); + const [reusePartitioning, setReusePartitioning] = useState(false); + + const availableDevices = useMemo(() => { + return Object.keys(storageData.devices); + }, [storageData.devices]); + + useEffect(() => { + /* + * When disk selection changes or the user re-scans the devices we need to re-create the partitioning. + * For Automatic partitioning we do it each time we go to review page, + * but for custom mount assignment we try to reuse the partitioning when possible. + */ + setReusePartitioning(false); + }, [availableDevices, storageData.diskSelection.selectedDisks]); const language = useMemo(() => { for (const l of Object.keys(localizationData.languages)) { @@ -86,7 +100,7 @@ export const AnacondaWizard = ({ dispatch, isBootIso, osRelease, storageData, lo label: _("Disk configuration"), steps: [{ component: MountPointMapping, - data: { deviceData: storageData.devices, diskSelection: storageData.diskSelection, partitioningData: storageData.partitioning, dispatch }, + data: { deviceData: storageData.devices, diskSelection: storageData.diskSelection, partitioningData: storageData.partitioning, dispatch, reusePartitioning, setReusePartitioning }, id: "mount-point-mapping", label: _("Manual disk configuration"), isHidden: storageScenarioId !== "mount-point-mapping" diff --git a/ui/webui/src/components/storage/EncryptedDevices.jsx b/ui/webui/src/components/storage/EncryptedDevices.jsx index 351d443f670..cbacce6514c 100644 --- a/ui/webui/src/components/storage/EncryptedDevices.jsx +++ b/ui/webui/src/components/storage/EncryptedDevices.jsx @@ -41,7 +41,6 @@ import { ModalError } from "cockpit-components-inline-notification.jsx"; import { getDevicesAction } from "../../actions/storage-actions.js"; import { - createPartitioning, unlockDevice, } from "../../apis/storage.js"; @@ -111,9 +110,6 @@ const UnlockDialog = ({ lockedLUKSDevices, onClose, dispatch }) => { dispatch(getDevicesAction()); if (res.every(r => r.value[0])) { - // Also refresh the partitioning data which will now show the children - // of the unlocked device. - createPartitioning({ method: "MANUAL" }); onClose(); } else { dialogErrorSet(_("Incorrect passphrase")); diff --git a/ui/webui/src/components/storage/MountPointMapping.jsx b/ui/webui/src/components/storage/MountPointMapping.jsx index bc7a4eb64c9..3e55f1242d2 100644 --- a/ui/webui/src/components/storage/MountPointMapping.jsx +++ b/ui/webui/src/components/storage/MountPointMapping.jsx @@ -55,6 +55,21 @@ const requiredMountPointOptions = [ { value: "/", name: "root" }, ]; +const getDeviceChildren = ({ deviceData, device }) => { + const children = []; + const deviceChildren = deviceData[device]?.children?.v || []; + + if (deviceChildren.length === 0) { + children.push(device); + } else { + deviceChildren.forEach(child => { + children.push(...getDeviceChildren({ deviceData, device: child })); + }); + } + + return children; +}; + const getInitialRequests = (partitioningData) => { const bootOriginalRequest = partitioningData.requests.find(r => r["mount-point"] === "/boot"); const rootOriginalRequest = partitioningData.requests.find(r => r["mount-point"] === "/"); @@ -81,6 +96,26 @@ const isDuplicateRequestField = (requests, fieldName, fieldValue) => { return requests.filter((request) => request[fieldName] === fieldValue).length > 1; }; +const isReformatValid = (deviceData, request, requests) => { + const device = request["device-spec"]; + + if (!device || !request.reformat) { + return true; + } + + const children = getDeviceChildren({ deviceData, device }); + + /* When parent device is re-formatted all children must: + * - either exist in the mount points mapper table and be re-formatted + * - or not exist in the mountpoints mapper table + */ + return children.every(child => { + const childRequest = requests.find(r => r["device-spec"] === child); + + return !childRequest || childRequest.reformat === true; + }); +}; + const getLockedLUKSDevices = (requests, deviceData) => { const devs = requests?.map(r => r["device-spec"]) || []; @@ -145,6 +180,7 @@ const DeviceColumnSelect = ({ deviceData, devices, idPrefix, lockedLUKSDevices, return ( { +const FormatColumn = ({ deviceData, handleRequestChange, idPrefix, request, requests }) => { const mountpoint = request["mount-point"]; const isRootMountPoint = mountpoint === "/"; + const reformatInvalid = !isReformatValid(deviceData, request, requests); const FormatSwitch = () => { return ( { content={_("The root partition is always re-formatted by the installer.")}> } + {reformatInvalid && + + + {_("Mismatch between parent device and child device reformat selection.")} + + } ); }; @@ -294,6 +337,7 @@ const RequestsTable = ({ handleRequestChange={handleRequestChange} idPrefix={rowId + "-format"} request={request} + requests={requests} /> ), props: { className: columnClassName } @@ -377,8 +421,10 @@ const MountPointMappingContent = ({ deviceData, partitioningData, dispatch, idPr if (requests) { const mountPoints = requests.map(r => r["mount-point"]); const devices = requests.map(r => r["device-spec"]); + const reformatInvalid = requests.some(request => !isReformatValid(deviceData, request, requests)); const isFormValid = ( + !reformatInvalid && new Set(mountPoints).size === mountPoints.length && new Set(devices).size === devices.length && mountPoints.every(m => m) && @@ -390,7 +436,7 @@ const MountPointMappingContent = ({ deviceData, partitioningData, dispatch, idPr setUpdateRequestCnt(updateRequestCnt => updateRequestCnt + 1); } } - }, [requests, setIsFormValid]); + }, [deviceData, requests, setIsFormValid]); const handleRequestChange = (mountpoint, device, newRequestId, reformat) => { const data = deviceData[device]; @@ -457,18 +503,11 @@ const MountPointMappingContent = ({ deviceData, partitioningData, dispatch, idPr } }; -export const MountPointMapping = ({ deviceData, diskSelection, partitioningData, dispatch, idPrefix, setIsFormValid, onAddErrorNotification, stepNotification }) => { - const [creatingPartitioning, setCreatingPartitioning] = useState(true); - - // If device selection changed since the last partitioning request redo the partitioning - const selectedDevices = diskSelection.selectedDisks; - const partitioningDevices = partitioningData?.requests?.map(r => r["device-spec"]) || []; - const canReusePartitioning = selectedDevices.length === partitioningDevices.length && selectedDevices.every(d => partitioningDevices.includes(d)); +export const MountPointMapping = ({ deviceData, diskSelection, partitioningData, dispatch, idPrefix, setIsFormValid, onAddErrorNotification, reusePartitioning, setReusePartitioning, stepNotification }) => { + const [usedPartitioning, setUsedPartitioning] = useState(partitioningData?.path); useEffect(() => { - if (canReusePartitioning) { - setCreatingPartitioning(false); - } else { + if (!reusePartitioning || partitioningData?.method !== "MANUAL") { /* Reset the bootloader drive before we schedule partitions * The bootloader drive is automatically set during the partitioning, so * make sure we always reset the previous value before we run another one, @@ -477,11 +516,14 @@ export const MountPointMapping = ({ deviceData, diskSelection, partitioningData, */ setBootloaderDrive({ drive: "" }) .then(() => createPartitioning({ method: "MANUAL" })) - .then(() => setCreatingPartitioning(false)); + .then(path => { + setUsedPartitioning(path[0]); + setReusePartitioning(true); + }); } - }, [canReusePartitioning]); + }, [reusePartitioning, setReusePartitioning, partitioningData?.method, partitioningData?.path]); - if (creatingPartitioning || !partitioningData?.path || (partitioningData?.requests?.length || 0) < 1) { + if (!reusePartitioning || usedPartitioning !== partitioningData.path) { return ; } diff --git a/ui/webui/src/reducer.js b/ui/webui/src/reducer.js index 5ed4bda0754..9e8aaba23a2 100644 --- a/ui/webui/src/reducer.js +++ b/ui/webui/src/reducer.js @@ -73,7 +73,7 @@ export const storageReducer = (state = storageInitialState, action) => { } else if (action.type === "GET_DISK_SELECTION") { return { ...state, diskSelection: action.payload.diskSelection }; } else if (action.type === "GET_PARTITIONING_DATA") { - return { ...state, partitioning: action.payload.partitioningData }; + return { ...state, partitioning: { ...state.partitioning, ...action.payload.partitioningData } }; } else { return state; } diff --git a/ui/webui/test/check-storage b/ui/webui/test/check-storage index 194f5a9630b..74199b22c99 100755 --- a/ui/webui/test/check-storage +++ b/ui/webui/test/check-storage @@ -325,7 +325,7 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): selector = f"{self.table_row(row)} .pf-c-select__toggle" self.browser.click(f"{selector}:not([disabled]):not([aria-disabled=true])") - select_entry = f"{selector} + ul button:contains('{device}')" + select_entry = f"{selector} + ul button[data-value='{device}']" self.browser.click(select_entry) self.browser.wait_in_text(f"{selector} .pf-c-select__toggle-text", device) @@ -364,8 +364,11 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): self.browser.click("#unlock-device-dialog-cancel-btn") self.browser.wait_not_present("#unlock-device-dialog") - def select_reformat(self, row): - self.browser.click(f"{self.table_row(row)} td[data-label='Reformat'] input") + def select_reformat(self, row, selected=True): + self.browser.set_checked(f"{self.table_row(row)} td[data-label='Reformat'] input", selected) + + def remove_row(self, row): + self.browser.click(f"{self.table_row(row)} button[aria-label='Remove']") def check_reformat(self, row, checked): checked_selector = "input:checked" if checked else "input:not(:checked)" @@ -386,7 +389,6 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): def unlock_all_encrypted_skip(self): self.browser.click("button:contains('Skip')") - def assert_inline_error(self, text): self.browser.wait_in_text(".pf-c-alert.pf-m-inline.pf-m-danger", text) @@ -429,6 +431,14 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): self.machine.execute(command) + def wait_mount_point_table_column_helper(self, row, column, text=None, present=True): + b = self.browser + + if present: + b.wait_in_text(f"#mount-point-mapping-table-row-{row}-{column} .pf-c-helper-text__item.pf-m-error", text) + else: + b.wait_not_present(f"#mount-point-mapping-table-row-{row}-{column} .pf-c-helper-text__item.pf-m-error") + @nondestructive def testBasic(self): b = self.browser @@ -463,7 +473,7 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): self.check_row_mountpoint(2, "/") self.check_row_device(2, "Select a device") self.check_reformat(2, True) - self.select_row_device(2, f"btrfs") + self.select_row_device(2, f"btrfstest") self.check_format_type(2, "btrfs") self.add_mount() @@ -485,13 +495,30 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): i.next() # verify review screen - disk = "vda" - r.check_disk(disk, "16.1 GB vda (0x1af4)") + r.check_disk(dev, "16.1 GB vda (0x1af4)") - r.check_disk_row(disk, 1, "vda1, 1.05 MB: biosboot") - r.check_disk_row(disk, 2, "vda2, 1.07 GB: format as ext4, /boot") - r.check_disk_row(disk, 3, "btrfstest, 10.7 GB: format as btrfs, /") - r.check_disk_row(disk, 4, "vda4, 4.29 GB: mount, /home") + r.check_disk_row(dev, 1, "vda1, 1.05 MB: biosboot") + r.check_disk_row(dev, 2, "vda2, 1.07 GB: format as ext4, /boot") + r.check_disk_row(dev, 3, "btrfstest, 10.7 GB: format as btrfs, /") + r.check_disk_row(dev, 4, "vda4, 4.29 GB: mount, /home") + + applied_partitioning = s.dbus_get_applied_partitioning() + + # When adding a new partition a new partitioning should be created + i.back(previous_page=i.steps.CUSTOM_MOUNT_POINT) + i.back() + + m.execute(f"sgdisk --new=0:0:0 {disk}") + s.rescan_disks() + self.select_mountpoint(i, s, [(dev, True)]) + self.check_row_device(1, "Select a device") + self.check_row_device(2, "Select a device") + self.select_row_device(1, f"{dev}2") + self.select_row_device(2, f"btrfs") + + i.next() + new_applied_partitioning = s.dbus_get_applied_partitioning() + self.assertNotEqual(new_applied_partitioning, applied_partitioning) @nondestructive def testNoRootMountPoint(self): @@ -688,7 +715,7 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): tmp_mount = "/tmp/btrfs-mount-test" self.partition_disk(disk, [("1MiB", "biosboot"), ("1GB", "ext4"), ("", "btrfs")]) m.execute(f""" - mkdir {tmp_mount} + mkdir -p {tmp_mount} mount {disk}3 {tmp_mount} btrfs subvolume create {tmp_mount}/root btrfs subvolume create {tmp_mount}/home @@ -735,13 +762,72 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): i.next() # verify review screen - disk = "vda" - r.check_disk(disk, "16.1 GB vda (0x1af4)") + r.check_disk(dev, "16.1 GB vda (0x1af4)") - r.check_disk_row(disk, 1, "vda1, 1.05 MB: biosboot") - r.check_disk_row(disk, 2, "vda2, 1.07 GB: format as ext4, /boot") - r.check_disk_row(disk, 3, "root, 15.0 GB: format as btrfs, /") - r.check_disk_row(disk, 4, "home, 15.0 GB: mount, /home") + r.check_disk_row(dev, 1, "vda1, 1.05 MB: biosboot") + r.check_disk_row(dev, 2, "vda2, 1.07 GB: format as ext4, /boot") + r.check_disk_row(dev, 3, "root, 15.0 GB: format as btrfs, /") + r.check_disk_row(dev, 4, "home, 15.0 GB: mount, /home") + + i.back(previous_page=i.steps.CUSTOM_MOUNT_POINT) + i.back() + + # Checks for nested btrfs subvolume + tmp_mount = "/tmp/btrfs-mount-test" + m.execute(f""" + mkdir -p {tmp_mount} + mount {disk}3 {tmp_mount} + btrfs subvolume create {tmp_mount}/home/Movies + btrfs subvolume create {tmp_mount}/home/Movies/Good_Movies + btrfs subvolume create {tmp_mount}/home/Movies/Bad_Movies + umount {tmp_mount} + rmdir {tmp_mount} + """) + s.rescan_disks() + self.select_mountpoint(i, s, [(dev, True)]) + + self.select_row_device(1, f"{dev}2") + self.select_row_device(2, "root") + self.add_mount() + self.select_row_device(3, "home") + self.select_row_mountpoint(3, "/home") + self.add_mount() + self.select_row_device(4, "home/Movies") + self.select_row_mountpoint(4, "/home/Movies") + self.add_mount() + self.select_row_device(5, "home/Movies/Good_Movies") + self.select_row_mountpoint(5, "/home/Movies/Good_Movies") + self.add_mount() + self.select_row_device(6, "home/Movies/Bad_Movies") + self.select_row_mountpoint(6, "/home/Movies/Bad_Movies") + + # No error when no devices are reformatted + for row in range(3, 6): + self.wait_mount_point_table_column_helper(row, "format", present=False) + + # When parent is re-formatted all child devices must be reformatted + self.select_row_device(4, "home/Movies") + self.select_reformat(4) + self.wait_mount_point_table_column_helper(4, "format", text="Mismatch") + self.select_reformat(5) + self.select_reformat(6) + self.wait_mount_point_table_column_helper(4, "format", present=False) + + # Check also that the rules apply to children deeper in the device tree + self.select_reformat(3) + self.wait_mount_point_table_column_helper(3, "format", present=False) + self.select_reformat(6, False) + self.wait_mount_point_table_column_helper(3, "format", text="Mismatch") + + # When parent is re-formmated all child devices should be + # * either also reformated if selected + # * either not selected (not part of the mountpoint assignment table) + self.remove_row(5) + self.remove_row(6) + self.wait_mount_point_table_column_helper(3, "format", present=False) + self.wait_mount_point_table_column_helper(4, "format", present=False) + + i.check_next_disabled(False) @nondestructive def testLVM(self): diff --git a/ui/webui/test/helpers/installer.py b/ui/webui/test/helpers/installer.py index d0ab67d78cd..9041d4cbe67 100644 --- a/ui/webui/test/helpers/installer.py +++ b/ui/webui/test/helpers/installer.py @@ -102,7 +102,7 @@ def check_next_disabled(self, disabled=True): self.browser.wait_visible(f"#installation-next-btn:not([aria-disabled={value}]") @log_step(snapshot_before=True) - def back(self, should_fail=False): + def back(self, should_fail=False, previous_page=""): current_page = self.get_current_page() self.browser.click("button:contains(Back)") @@ -110,8 +110,10 @@ def back(self, should_fail=False): if should_fail: self.wait_current_page(current_page) else: - prev = [k for k, v in self.steps._steps_jump.items() if current_page in v][0] - self.wait_current_page(prev) + if not previous_page: + previous_page = [k for k, v in self.steps._steps_jump.items() if current_page in v][0] + + self.wait_current_page(previous_page) @log_step() def open(self, step="installation-language"):