From df5896ba9e37718bb6bb373692e4b79076ae8bf3 Mon Sep 17 00:00:00 2001 From: Katerina Koukiou Date: Wed, 27 Mar 2024 10:12:15 +0100 Subject: [PATCH] Simplify logic for possibly reusing partitioning If the type matches, (MANUAL vs AUTOMATIC) and the devices also match then we can reuse the partitioning. --- src/components/AnacondaWizard.jsx | 58 +------------------ .../storage/InstallationDestination.jsx | 11 ++++ src/components/storage/MountPointMapping.jsx | 45 +++++++++++--- src/helpers/storage.js | 2 +- test/check-storage | 12 ++-- 5 files changed, 56 insertions(+), 72 deletions(-) diff --git a/src/components/AnacondaWizard.jsx b/src/components/AnacondaWizard.jsx index cfa5fcf466..1216de00fc 100644 --- a/src/components/AnacondaWizard.jsx +++ b/src/components/AnacondaWizard.jsx @@ -16,7 +16,7 @@ */ import cockpit from "cockpit"; -import React, { useContext, useEffect, useMemo, useState } from "react"; +import React, { useContext, useEffect, useState } from "react"; import { PageSection, PageSectionTypes, @@ -25,26 +25,21 @@ import { WizardStep, } from "@patternfly/react-core"; -import { resetPartitioning } from "../apis/storage_partitioning.js"; - import { AnacondaPage } from "./AnacondaPage.jsx"; import { AnacondaWizardFooter } from "./AnacondaWizardFooter.jsx"; import { FooterContext, StorageContext } from "./Common.jsx"; import { InstallationProgress } from "./installation/InstallationProgress.jsx"; import { getSteps } from "./steps.js"; import { CockpitStorageIntegration } from "./storage/CockpitStorageIntegration.jsx"; -const N_ = cockpit.noop; export const AnacondaWizard = ({ dispatch, onCritFail, setShowStorage, showStorage }) => { const [isFormDisabled, setIsFormDisabled] = useState(false); const [isFormValid, setIsFormValid] = useState(false); - const [reusePartitioning, setReusePartitioning] = useState(false); const [stepNotification, setStepNotification] = useState(); const [showWizard, setShowWizard] = useState(true); const [currentStepId, setCurrentStepId] = useState(); const storageData = useContext(StorageContext); const storageScenarioId = storageData.storageScenarioId; - const selectedDisks = storageData.diskSelection.selectedDisks; const [scenarioPartitioningMapping, setScenarioPartitioningMapping] = useState({}); useEffect(() => { @@ -56,10 +51,6 @@ export const AnacondaWizard = ({ dispatch, onCritFail, setShowStorage, showStora } }, [storageData.partitioning.path, storageScenarioId]); - const availableDevices = useMemo(() => { - return Object.keys(storageData.devices); - }, [storageData.devices]); - useEffect(() => { if (!currentStepId) { return; @@ -67,51 +58,19 @@ export const AnacondaWizard = ({ dispatch, onCritFail, setShowStorage, showStora cockpit.location.go([currentStepId]); }, [currentStepId]); - 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, selectedDisks]); - const componentProps = { dispatch, isFormDisabled, onCritFail, - reusePartitioning, scenarioPartitioningMapping, setIsFormDisabled, setIsFormValid, - setReusePartitioning, setShowStorage, }; - const getFlattenedStepsIds = (steps) => { - const stepIds = []; - for (const step of steps) { - stepIds.push(step.id); - if (step.steps) { - for (const childStep of step.steps) { - if (childStep?.isHidden !== true) { - stepIds.push(childStep.id); - } - } - } - } - return stepIds; - }; const stepsOrder = getSteps(); - const flattenedStepsIds = getFlattenedStepsIds(stepsOrder); const firstStepId = stepsOrder[0].id; - const isStepFollowedBy = (earlierStepId, laterStepId) => { - const earlierStepIdx = flattenedStepsIds.findIndex(s => s === earlierStepId); - const laterStepIdx = flattenedStepsIds.findIndex(s => s === laterStepId); - return earlierStepIdx < laterStepIdx; - }; - const createSteps = (stepsOrder, componentProps) => { return stepsOrder.map(s => { const isVisited = firstStepId === s.id || currentStepId === s.id; @@ -156,20 +115,7 @@ export const AnacondaWizard = ({ dispatch, onCritFail, setShowStorage, showStora setIsFormValid(false); } - // Reset the applied partitioning when going back from a step after creating partitioning to a step - // before creating partitioning. - if ((prevStep.id === "accounts" || isStepFollowedBy("accounts", prevStep.id)) && - isStepFollowedBy(newStep.id, "accounts")) { - setIsFormDisabled(true); - resetPartitioning() - .then( - () => setCurrentStepId(newStep.id), - () => onCritFail({ context: cockpit.format(N_("Error was hit when going back from $0."), prevStep.prevName) }) - ) - .always(() => setIsFormDisabled(false)); - } else { - setCurrentStepId(newStep.id); - } + setCurrentStepId(newStep.id); }; if (!showWizard) { diff --git a/src/components/storage/InstallationDestination.jsx b/src/components/storage/InstallationDestination.jsx index 83db7d4658..bab319dcc3 100644 --- a/src/components/storage/InstallationDestination.jsx +++ b/src/components/storage/InstallationDestination.jsx @@ -47,6 +47,7 @@ import { scanDevicesWithTask, } from "../../apis/storage.js"; import { setSelectedDisks } from "../../apis/storage_disks_selection.js"; +import { resetPartitioning } from "../../apis/storage_partitioning.js"; import { getDevicesAction, getDiskSelectionAction } from "../../actions/storage-actions.js"; @@ -335,6 +336,16 @@ export const InstallationDestination = ({ } }, [isRescanningDisks, diskSelection.usableDisks]); + useEffect(() => { + // Always reset the partitioning when entering the installation destination page + const resetPartitioningAsync = async () => { + setIsFormDisabled(true); + await resetPartitioning(); + setIsFormDisabled(false); + }; + resetPartitioningAsync(); + }, [setIsFormDisabled]); + useEffect(() => { // Select default disks for the partitioning on component mount if (refUsableDisks.current !== undefined) { diff --git a/src/components/storage/MountPointMapping.jsx b/src/components/storage/MountPointMapping.jsx index f3310ee43d..be74037abf 100644 --- a/src/components/storage/MountPointMapping.jsx +++ b/src/components/storage/MountPointMapping.jsx @@ -46,6 +46,7 @@ import { } from "../../apis/storage_partitioning.js"; import { + getDeviceAncestors, getDeviceChildren, getLockedLUKSDevices, hasDuplicateFields, @@ -611,13 +612,34 @@ const isUsableDevice = (devSpec, deviceData) => { const MountPointMapping = ({ dispatch, idPrefix, - reusePartitioning, setIsFormValid, - setReusePartitioning, setStepNotification, }) => { - const { devices, partitioning } = useContext(StorageContext); - const [usedPartitioning, setUsedPartitioning] = useState(partitioning?.path); + const { devices, diskSelection, partitioning } = useContext(StorageContext); + const [usedPartitioning, setUsedPartitioning] = useState(); + const canReusePartitioning = useMemo(() => { + // The partitioning can be reused if the devices in the partitioning requests and children + // of the selected disks are the same + // + if (usedPartitioning === partitioning?.path) { + return true; + } + + const usableDevices = Object.keys(devices).filter(device => { + const children = devices[device].children.v; + const ancestors = getDeviceAncestors(devices, device); + + return children.length === 0 && diskSelection.selectedDisks.some(disk => ancestors.includes(disk)); + }); + + const usedDevices = partitioning?.requests?.map(r => r["device-spec"]) || []; + + if (usedDevices.every(d => usableDevices.includes(d)) && usableDevices.every(d => usedDevices.includes(d))) { + return true; + } + return false; + }, [devices, diskSelection.selectedDisks, partitioning.path, partitioning.requests, usedPartitioning]); + const mountPointConstraints = useMountPointConstraints(); const [skipUnlock, setSkipUnlock] = useState(false); const lockedLUKSDevices = useMemo( @@ -636,7 +658,13 @@ const MountPointMapping = ({ useWizardFooter(getFooter); useEffect(() => { - if (!reusePartitioning || partitioning?.method !== "MANUAL") { + let recreatedPartitioning = false; + + if (partitioning?.method !== "MANUAL" || !canReusePartitioning) { + recreatedPartitioning = true; + } + + if (recreatedPartitioning) { /* 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, @@ -647,12 +675,13 @@ const MountPointMapping = ({ .then(() => createPartitioning({ method: "MANUAL" })) .then(path => { setUsedPartitioning(path); - setReusePartitioning(true); }); + } else { + setUsedPartitioning(partitioning.path); } - }, [reusePartitioning, setReusePartitioning, partitioning?.method, partitioning?.path]); + }, [canReusePartitioning, partitioning?.method, partitioning?.path]); - const isLoadingNewPartitioning = !reusePartitioning || usedPartitioning !== partitioning.path; + const isLoadingNewPartitioning = usedPartitioning !== partitioning.path; const showLuksUnlock = lockedLUKSDevices?.length > 0 && !skipUnlock; return ( diff --git a/src/helpers/storage.js b/src/helpers/storage.js index fca98ba1a9..eb075c6423 100644 --- a/src/helpers/storage.js +++ b/src/helpers/storage.js @@ -21,7 +21,7 @@ * @param {string} device - The name of the device * @returns {Array} */ -const getDeviceAncestors = (deviceData, device) => { +export const getDeviceAncestors = (deviceData, device) => { // device ancestors including the device itself const ancestors = []; const deviceParents = deviceData[device]?.parents?.v || []; diff --git a/test/check-storage b/test/check-storage index 5cf9617bde..0f38d9d72b 100755 --- a/test/check-storage +++ b/test/check-storage @@ -1369,14 +1369,12 @@ class TestStorageCockpitIntegration(anacondalib.VirtInstallMachineCase, StorageC i.next(next_page=i.steps.CUSTOM_MOUNT_POINT) - s.select_mountpoint_row_device(1, "root") - s.select_mountpoint_row_device(2, "vda2") - s.select_mountpoint_row_reformat(2) - s.add_mountpoint_row() + s.check_mountpoint_row(1, "/", "root", True, "btrfs") + s.check_mountpoint_row(2, "/boot", "vda2", False, "ext4") + s.check_mountpoint_row(3, "/home", "home", False, "btrfs") - # Selecting first device, then mount point and then reformat - s.select_mountpoint_row_device(3, "home") - s.select_mountpoint_row_mountpoint(3, "/home") + s.select_mountpoint_row_reformat(1) + s.select_mountpoint_row_reformat(2) s.select_mountpoint_row_reformat(3, False) # Root should be preselected to reformat