Skip to content

Validate storage show warnings #789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Validate storage show warnings #789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44fff4f
partitioning validate warnings
KKoukiou Mar 26, 2025
56b46f9
test: New test for storage warnings
adamkankovsky Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 37 additions & 5 deletions src/apis/storage_partitioning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,27 @@ export const partitioningConfigureWithTask = ({ partitioning }) => {
);
};

const partitioningValidate = async ({ onFail, onSuccess, partitioning }) => {
const tasks = await new StorageClient().client.call(
partitioning,
INTERFACE_NAME_PARTITIONING,
"ValidateWithTask", []
);
return runStorageTask({
onFail,
onSuccess: async () => {
const taskProxy = new StorageClient().client.proxy(
"org.fedoraproject.Anaconda.Task",
tasks[0]
);
const result = await taskProxy.GetResult();

return onSuccess(result.v);
},
task: tasks[0],
});
};

export const resetPartitioning = () => {
return callClient("ResetPartitioning", []);
};
Expand Down Expand Up @@ -280,13 +301,24 @@ export const applyStorage = async ({ devices, luks, onFail, onSuccess, partition
await setBootloaderDrive({ drive: "" });
}

const tasks = await partitioningConfigureWithTask({ partitioning });
const configureTasks = await partitioningConfigureWithTask({ partitioning });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider flattening the async flow by combining storage task calls into a single async/await block.

Consider flattening the async flow by combining the storage task calls into a single async/await block. For example, rather than nesting callbacks in both the onConfigureTaskSuccess and the partitioningValidate helper, inline the operations sequentially. This makes the flow of operations easier to follow. For instance:

const configureTasks = await partitioningConfigureWithTask({ partitioning });
try {
  await applyPartitioning({ partitioning });

  const tasks = await new StorageClient().client.call(
    partitioning,
    INTERFACE_NAME_PARTITIONING,
    "ValidateWithTask",
    []
  );

  const taskProxy = new StorageClient().client.proxy(
    "org.fedoraproject.Anaconda.Task",
    tasks[0]
  );
  const result = await taskProxy.GetResult();
  onSuccess(result.v);
} catch (error) {
  onFail(error);
}

In this refactoring, the sequence is linear without multiple layers of callbacks. This keeps all behavior intact while reducing indirection and simplifying error handling.


const onConfigureTaskSuccess = async () => {
try {
await applyPartitioning({ partitioning });
await partitioningValidate({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KKoukiou @adamkankovsky May I ask why we validate partitioning after applying it? In the Gtk backend we do it before: https://github.com/rhinstaller/anaconda/blob/1d9120e716aff354c7895ebde205f4ba6f9f347a/pyanaconda/ui/lib/storage.py#L307
Also we should handle also validation errors (not only warnings) before applying the partitioning?

onFail,
onSuccess,
partitioning,
});
} catch (error) {
onFail(error);
}
};

runStorageTask({
onFail,
onSuccess: () => applyPartitioning({ partitioning })
.then(onSuccess)
.catch(onFail),
task: tasks[0]
onSuccess: onConfigureTaskSuccess,
task: configureTasks[0],
});
};
55 changes: 39 additions & 16 deletions src/components/storage/CockpitStorageIntegration.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ const waitForNewSelectedDisks = ({ newSelectedDisks, selectedDisks, setNextCheck
}
};

const prepareAndApplyPartitioning = ({ devices, newMountPoints, onFail, setNextCheckStep, useConfiguredStorage }) => {
const prepareAndApplyPartitioning = ({ devices, newMountPoints, onFail, setNextCheckStep, setStorageValidationReport, useConfiguredStorage }) => {
// If "Use configured storage" is not available, skip Manual partitioning creation
if (!useConfiguredStorage) {
setNextCheckStep();
Expand Down Expand Up @@ -496,7 +496,10 @@ const prepareAndApplyPartitioning = ({ devices, newMountPoints, onFail, setNextC
applyStorage({
devices,
onFail,
onSuccess: setNextCheckStep,
onSuccess: (report) => {
setStorageValidationReport(report);
setNextCheckStep();
},
partitioning,
});
} catch (exc) {
Expand Down Expand Up @@ -525,7 +528,7 @@ const scanDevices = ({ dispatch, onFail, setNextCheckStep }) => {
});
};

const useStorageSetup = ({ dispatch, newMountPoints, onCritFail, setError, useConfiguredStorage }) => {
const useStorageSetup = ({ dispatch, newMountPoints, onCritFail, setError, setStorageValidationReport, useConfiguredStorage }) => {
const [checkStep, setCheckStep] = useState("rescan");
const refCheckStep = useRef();
const devices = useOriginalDevices();
Expand Down Expand Up @@ -607,6 +610,7 @@ const useStorageSetup = ({ dispatch, newMountPoints, onCritFail, setError, useCo
newMountPoints,
onFail,
setNextCheckStep: () => setCheckStep(),
setStorageValidationReport,
useConfiguredStorage,
});
break;
Expand All @@ -629,6 +633,7 @@ const CheckStorageDialog = ({
const devices = useOriginalDevices();
const selectedDisks = diskSelection.selectedDisks;

const [storageValidationReport, setStorageValidationReport] = useState({});
const [error, setError] = useState();
const diskTotalSpace = useDiskTotalSpace({ devices, selectedDisks });
const diskFreeSpace = useDiskFreeSpace({ devices, selectedDisks });
Expand Down Expand Up @@ -695,6 +700,7 @@ const CheckStorageDialog = ({
newMountPoints,
onCritFail,
setError,
setStorageValidationReport,
useConfiguredStorage,
});
const loading = !error && storageStepsInProgress;
Expand Down Expand Up @@ -728,6 +734,19 @@ const CheckStorageDialog = ({
modalProps["aria-label"] = _("Checking storage configuration");
}

const WarningMessages = ({ warnings }) => {
if (!warnings?.length) return null;
return (
<>
{warnings.map((warning, index) => (
<HelperTextItem key={`warning-${index}`} variant="warning" isDynamic>
{warning}
</HelperTextItem>
))}
</>
);
};

return (
<Modal
className={idPrefix + "-check-storage-dialog" + (loading ? "--loading" : "")}
Expand Down Expand Up @@ -780,19 +799,23 @@ const CheckStorageDialog = ({
<>
{storageRequirementsNotMet ? error?.message : null}
<HelperText>
{!storageRequirementsNotMet &&
<HelperTextItem variant="success" isDynamic>
{useConfiguredStorage
? (
<Stack hasGutter>
<span>{_("Detected valid storage layout:")}</span>
{useConfiguredStorageReview}
</Stack>
)
: (
useEntireSoftwareDisk ? _("Use the RAID device for automatic partitioning") : _("Use free space")
)}
</HelperTextItem>}
{!storageRequirementsNotMet && (
<>
<WarningMessages warnings={storageValidationReport?.["warning-messages"]?.v} />
<HelperTextItem variant="success" isDynamic>
{useConfiguredStorage
? (
<Stack hasGutter>
<span>{_("Detected valid storage layout:")}</span>
{useConfiguredStorageReview}
</Stack>
)
: (
(useEntireSoftwareDisk ? _("Use free space") : _("Use the RAID device for automatic partitioning"))
)}
</HelperTextItem>
</>
)}
</HelperText>
</>}
</>
Expand Down
87 changes: 87 additions & 0 deletions test/check-storage-cockpit
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,93 @@ class TestStorageCockpitIntegration(VirtInstallMachineCase, StorageCase):
self.assertTrue("/mnt/sysroot/boot auto noauto 0 0" in fstab)
self.assertTrue("/mnt/sysroot auto noauto 0 0" in fstab)

@nondestructive
def testStorageValidationWarnings(self):
b = self.browser
m = self.machine
i = Installer(b, m, scenario="use-configured-storage")
s = Storage(b, m)
r = Review(b, m)

vgname = "vgwarnings"

i.open()
i.reach(i.steps.INSTALLATION_METHOD)
s.wait_scenario_visible("use-configured-storage", False)
s.check_disk_selected("vda")
s.modify_storage()
s.confirm_entering_cockpit_storage()
b.wait_visible(".cockpit-storage-integration-sidebar")

frame = "iframe[name='cockpit-storage']"
b._wait_present(frame)
b.switch_to_frame("cockpit-storage")
b._wait_present("#storage.ct-page-fill")

self.click_dropdown(self.card_row("Storage", 1), "Create partition table")
self.confirm()

self.click_dropdown(self.card_row("Storage", 2), "Create partition")
self.dialog({"size": 1, "type": "biosboot"})

self.click_dropdown(self.card_row("Storage", 3), "Create partition")
self.dialog({"size": 100, "type": "ext4", "mount_point": "/boot"})

self.click_dropdown(self.card_row("Storage", 4), "Create partition")
self.dialog({"size": 100, "type": "ext4", "mount_point": "/boot/efi"})

self.click_devices_dropdown("Create LVM2 volume group")
self.dialog({"name": vgname, "disks": {"vda": True}})

self.click_card_row("Storage", name=vgname)

# Create root LV
b.click(self.card_button("LVM2 logical volumes", "Create new logical volume"))
self.dialog({"name": "lv_root", "size": 4096})
self.click_card_row("LVM2 logical volumes", name="lv_root")
self.click_card_dropdown("Unformatted data", "Format")
self.dialog({"type": "ext4", "mount_point": "/"})

b.click(self.card_parent_link())

# Create small /home
b.click(self.card_button("LVM2 logical volumes", "Create new logical volume"))
self.dialog({"name": "lv_home", "size": 100})
self.click_card_row("LVM2 logical volumes", name="lv_home")
self.click_card_dropdown("Unformatted data", "Format")
self.dialog({"type": "ext4", "mount_point": "/home"})

b.click(self.card_parent_link())

# Create swap LV without UUID by skipping format
b.click(self.card_button("LVM2 logical volumes", "Create new logical volume"))
self.dialog({"name": "lv_swap", "size": 512})

b.click(self.card_parent_link())
b.switch_to_top()

s.return_to_installation()

# Pixel test before confirming
b.assert_pixels(
"#cockpit-storage-integration-check-storage-dialog",
"cockpit-storage-integration-check-storage-dialog-warnings",
ignore=pixel_tests_ignore,
)

# Also check warning modal contents via text
b.wait_visible("#cockpit-storage-integration-check-storage-dialog")
warnings_text = b.text("#cockpit-storage-integration-check-storage-dialog")
assert "/boot partition is less than" in warnings_text.lower()
assert "/boot/efi partition" in warnings_text.lower()
assert "/home partition is less than" in warnings_text.lower()
assert "recommended to create a new file system" in warnings_text.lower()

s.return_to_installation_confirm()
s.set_scenario("use-configured-storage")

i.reach(i.steps.REVIEW)

@nondestructive
def testStandardPartitionExt4(self):
self._testStandardPartition("ext4")
Expand Down
Loading