Disk add dialog: Change format together with the storage pool

Otherwise we create a race condition: The ModalBody already gets re-rendered with the new pool name, but with possibly with a format that is invalid for the new pool type. That triggers our new assertion. This is very redundant with the effect which fine-tunes the format and disk type according to the other data. This effect is evil and should be dropped, but doing so requires more intrusive code cleanup. Introduce a helper function to avoid repeated code.
cockpit-project · Nov 29, 2023 · 4deef0d · 4deef0d
1 parent db53620
commit 4deef0d
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 19 deletions.
diff --git a/src/components/vm/disks/diskAdd.jsx b/src/components/vm/disks/diskAdd.jsx
@@ -395,6 +395,19 @@ export const AddDiskModalBody = ({ disk, idPrefix, isMediaInsertion, vm, vms, su
     }), [defaultPool, mode, vm]);
     const storagePoolName = diskParams.storagePool?.name;
 
+    const getPoolFormatAndDevice = (pool, volName) => {
+        const params = { format: getDefaultVolumeFormat(pool), device: "disk" };
+        if (['dir', 'fs', 'netfs', 'gluster', 'vstorage'].indexOf(pool.type) > -1) {
+            const volume = pool.volumes.find(vol => vol.name === volName);
+            if (volume?.format) {
+                params.format = volume.format;
+                if (volume.format === "iso")
+                    params.device = "cdrom";
+            }
+        }
+        return params;
+    };
+
     useEffect(() => {
         // Refresh storage volume list before displaying the dialog.
         // There are recently no Libvirt events for storage volumes and polling is ugly.
@@ -423,20 +436,9 @@ export const AddDiskModalBody = ({ disk, idPrefix, isMediaInsertion, vm, vms, su
             return;
         }
 
-        let format = getDefaultVolumeFormat(diskParams.storagePool);
-        let deviceType = "disk";
-        if (['dir', 'fs', 'netfs', 'gluster', 'vstorage'].indexOf(diskParams.storagePool.type) > -1) {
-            const volume = diskParams.storagePool.volumes.find(vol => vol.name === diskParams.existingVolumeName);
-            if (volume && volume.format) {
-                format = volume.format;
-                if (volume.format === "iso")
-                    deviceType = "cdrom";
-            }
-        }
         setDiskParams(diskParams => ({
             ...diskParams,
-            format,
-            device: deviceType,
+            ...getPoolFormatAndDevice(diskParams.storagePool, diskParams.existingVolumeName),
         }));
     }, [storagePools, diskParams.storagePool, diskParams.existingVolumeName]);
 
@@ -513,7 +515,14 @@ export const AddDiskModalBody = ({ disk, idPrefix, isMediaInsertion, vm, vms, su
         switch (key) {
         case 'storagePoolName': {
             const currentPool = storagePools.find(pool => pool.name === value && pool.connectionName === vm.connectionName);
-            setDiskParams(diskParams => ({ ...diskParams, storagePool: currentPool, existingVolumeName: getDefaultVolumeName(currentPool, vm) }));
+            const existingVolumeName = getDefaultVolumeName(currentPool, vm);
+
+            setDiskParams(diskParams => ({
+                ...diskParams,
+                storagePool: currentPool,
+                existingVolumeName,
+                ...getPoolFormatAndDevice(currentPool, existingVolumeName),
+            }));
             break;
         }
         case 'mode': {

diff --git a/test/check-machines-disks b/test/check-machines-disks
@@ -953,12 +953,6 @@ class TestMachinesDisks(VirtualMachinesCase):
         if m.image in ["debian-testing"]:
             self.allow_journal_messages(f'.* type=1400 .* apparmor="DENIED" operation="open" profile="libvirt.* name="{self.vm_tmpdir}.*')
 
-        # HACK: Switching the pool relies on useEffect() to update "Format:" to the default format of the new pool
-        # type; there is an intermediade rendering with the new pool and the old format. This is unfortunately
-        # hard to fix due to the useEffect() maze in the "Add disk" dialog. Until we clean this up, ignore this
-        # particular instance
-        self.allow_browser_errors("VolumeDetails internal error: format qcow2 is not valid for storage pool type disk")
-
     @timeout(900)
     def testAddDiskDirPool(self):
         b = self.browser