[PATCH v3 manager 71/76] ui: storage wizard: iscsi: reuse existing target connections

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Reuse a target's connection-only storage for additional shared LVM
groups instead of creating another login owner. Inherit its ID, nodes,
and multipath setting, restricting LVM to those nodes. Reject
conflicting configurations, targets serving raw disk images, and
connections excluding the setup node, which must activate the storage to
discover LUNs.

Select LUN identifies the reused connection. Back remains available
until setup creates an object or starts an operation. Going back
invalidates the inventory and restores the summary. Recheck target use
before submission and explain how reopening the wizard continues an
interrupted setup.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 www/manager6/storage/wizard/IScsi.js     | 184 +++++++++++++++++++----
 www/manager6/storage/wizard/LunSelect.js |   2 +-
 www/manager6/storage/wizard/SanLvm.js    |  71 ++++++++-
 www/manager6/storage/wizard/Wizard.js    |  67 ++++++++-
 4 files changed, 283 insertions(+), 41 deletions(-)

diff --git a/www/manager6/storage/wizard/IScsi.js b/www/manager6/storage/wizard/IScsi.js
index 9bab76cd0..777d491e7 100644
--- a/www/manager6/storage/wizard/IScsi.js
+++ b/www/manager6/storage/wizard/IScsi.js
@@ -70,7 +70,10 @@ Ext.define('PVE.storage.wizard.IScsiTarget', {
                 disabled: true,
                 showNodeSelector: false,
                 listeners: {
-                    change: (f, value) => me.updateMultipathDefault(f, value),
+                    change: function (f, value) {
+                        me.updateMultipathDefault(f, value);
+                        me.up('window').refreshTargetUse();
+                    },
                 },
             },
         ];
@@ -125,9 +128,18 @@ Ext.define('PVE.storage.wizard.IScsiUsage', {
     blockedReason: function () {
         let me = this;
         let wizard = me.up('window');
-        if (wizard.getViewModel().get('usage') !== 'lvm') {
+        let usage = wizard.getViewModel().get('usage');
+        let use = wizard.targetUse;
+        if (use?.conflict || (usage === 'direct' && use?.reuse)) {
+            return { text: use.conflict ?? use.inUse };
+        }
+        if (usage !== 'lvm') {
             return undefined;
         }
+        let unavailable = PVE.storage.wizard.reuseNodeRefusal(use?.reuse, Proxmox.NodeName);
+        if (unavailable) {
+            return { text: unavailable };
+        }
         if (wizard.getScanNode() !== Proxmox.NodeName) {
             return {
                 text: Ext.String.format(
@@ -143,7 +155,7 @@ Ext.define('PVE.storage.wizard.IScsiUsage', {
         // without multipath LVM sees the PV once per portal and ignores the volume group
         let field = wizard.down('field[name=multipath]');
         let portals = wizard.down('pveStorageWizardIScsiTarget').targetPortals;
-        if (portals < 2 || field.getValue()) {
+        if (wizard.targetUse?.reuse || portals < 2 || field.getValue()) {
             return undefined;
         }
         return {
@@ -276,9 +288,11 @@ PVE.storage.wizard.types.iscsi = {
     viewModel: {
         data: {
             usage: 'lvm',
+            reuseBase: false,
         },
         formulas: {
             isLvmUsage: (get) => get('usage') === 'lvm',
+            newBase: (get) => get('usage') === 'lvm' && !get('reuseBase'),
         },
     },
     steps: () => [
@@ -313,8 +327,8 @@ PVE.storage.wizard.types.iscsi = {
                 vtype: 'StorageId',
                 allowBlank: true,
                 bind: {
-                    disabled: '{!isLvmUsage}',
-                    hidden: '{!isLvmUsage}',
+                    disabled: '{!newBase}',
+                    hidden: '{!newBase}',
                 },
             },
         ],
@@ -336,16 +350,99 @@ PVE.storage.wizard.types.iscsi = {
         if (wizard.getValues().usage !== 'lvm') {
             return undefined;
         }
-        return gettext('Connect and Select LUN');
+        return wizard.targetUse?.reuse ? gettext('Select LUN') : gettext('Connect and Select LUN');
+    },
+
+    // a storage serving the LUNs as disk images rules the target out, they may be guest disks
+    // without a signature
+    targetUse: function (entries, values, wizard) {
+        let enc = Ext.htmlEncode;
+        let users = entries.filter((entry) => entry.target && entry.target === values.target);
+        if (!users.length) {
+            return undefined;
+        }
+        let connects = (entry) => entry.type === 'iscsi' && `${entry.content}` === 'none';
+        let serving = users.filter((entry) => !connects(entry));
+        if (serving.length) {
+            return {
+                conflict: Ext.String.format(
+                    ngettext(
+                        'Storage {0} already uses the LUNs of this target as disk images, so the wizard cannot set up another storage on it.',
+                        'Storages {0} already use the LUNs of this target as disk images, so the wizard cannot set up another storage on it.',
+                        serving.length,
+                    ),
+                    enc(serving.map((entry) => `"${entry.storage}"`).join(', ')),
+                ),
+            };
+        }
+        if (users.length > 1) {
+            return {
+                conflict: Ext.String.format(
+                    gettext('Several storages connect to this target: {0}.'),
+                    enc(users.map((entry) => entry.storage).join(', ')),
+                ),
+            };
+        }
+        let base = users[0];
+        let inUse = Ext.String.format(
+            gettext('Storage "{0}" already connects to this target.'),
+            enc(base.storage),
+        );
+        if (base.disable) {
+            return {
+                inUse,
+                conflict: Ext.String.format(
+                    gettext(
+                        'Storage "{0}" connects to this target, but it is disabled. Enable that storage first.',
+                    ),
+                    enc(base.storage),
+                ),
+            };
+        }
+        if (!base.multipath && wizard.down('pveStorageWizardIScsiTarget').targetPortals > 1) {
+            return {
+                inUse,
+                conflict: Ext.String.format(
+                    gettext(
+                        'Storage "{0}" connects to this target without multipath, but shared LVM on a target with several portals needs multipath. Enable the Multipath option of that storage first.',
+                    ),
+                    enc(base.storage),
+                ),
+            };
+        }
+        return {
+            inUse,
+            reuse: {
+                storage: base.storage,
+                portal: base.portal,
+                multipath: !!base.multipath,
+                nodes: base.nodes ? `${base.nodes}`.split(',') : [],
+            },
+        };
+    },
+
+    targetUseChanged: function (wizard, use) {
+        let panel = wizard.down('pveStorageWizardIScsiTarget');
+        let field = panel.down('field[name=multipath]');
+        if (use?.reuse) {
+            panel.settingMultipathDefault = true;
+            field.setValue(use.reuse.multipath);
+            panel.settingMultipathDefault = false;
+        }
+        field.setDisabled(!!use?.reuse);
+        wizard.getViewModel().set('reuseBase', !!use?.reuse);
     },
 
     summaryRows: function (values, wizard) {
         if (values.usage !== 'lvm') {
             return {};
         }
+        let reuse = wizard.targetUse?.reuse;
         let created = wizard.created ?? {};
-        let base = values['base-storage'] || `${values.storage}-base`;
-        if (created.baseStorage) {
+        let base = reuse?.storage ?? (values['base-storage'] || `${values.storage}-base`);
+        if (reuse) {
+            base = Ext.String.format(gettext('{0} (existing)'), base);
+        } else if (created.baseStorage) {
             base = Ext.String.format(gettext('{0} (created)'), created.baseStorage);
         }
         let rows = [
@@ -358,17 +455,33 @@ PVE.storage.wizard.types.iscsi = {
             },
         ];
         let skip = ['storage', 'base-storage'];
+        if (reuse) {
+            rows.push(
+                { label: 'Portal', value: reuse.portal, weight: 50 },
+                {
+                    label: gettext('Multipath'),
+                    value: Proxmox.Utils.format_boolean(reuse.multipath),
+                    weight: 50,
+                },
+            );
+            skip.push('portal', 'multipath');
+        }
         return { rows, skip };
     },
 
     summaryNotes: function (values, wizard) {
         if (values.usage === 'lvm') {
-            let base = values['base-storage'] || `${values.storage}-base`;
+            let reuse = wizard.targetUse?.reuse;
+            let base = reuse?.storage ?? (values['base-storage'] || `${values.storage}-base`);
             return [
                 Ext.String.format(
-                    gettext(
-                        'Press Connect and Select LUN to add the iSCSI storage "{0}" and list its LUNs. The volume group is created on the LUN you select next.',
-                    ),
+                    reuse
+                        ? gettext(
+                              'Press Select LUN to list the LUNs of the existing iSCSI storage "{0}". The volume group is created on the LUN you select next.',
+                          )
+                        : gettext(
+                              'Press Connect and Select LUN to add the iSCSI storage "{0}" and list its LUNs. The volume group is created on the LUN you select next.',
+                          ),
                     Ext.htmlEncode(base),
                 ),
             ];
@@ -389,6 +502,23 @@ PVE.storage.wizard.types.iscsi = {
         let usage = values.usage;
         delete values.usage;
 
+        // the storages of the target as they are now, the steps saw an older state
+        let checkTarget = (entries) => {
+            let use = PVE.storage.wizard.types.iscsi.targetUse(entries, values, wizard);
+            if (use?.conflict || (usage === 'direct' && use?.inUse)) {
+                return use.conflict ?? use.inUse;
+            }
+            if (
+                usage === 'lvm' &&
+                (use?.reuse?.storage ?? '') !== (wizard.targetUse?.reuse?.storage ?? '')
+            ) {
+                return gettext(
+                    'The storages that use the target have changed in the meantime. Go back to the Target step to review them.',
+                );
+            }
+            return undefined;
+        };
+
         if (usage === 'direct') {
             delete values['base-storage'];
             if (!values.multipath) {
@@ -396,11 +526,15 @@ PVE.storage.wizard.types.iscsi = {
             }
             values.type = 'iscsi';
             values.content = 'images';
-            wizard.runSubmitChain([(next) => wizard.createStorage(values, next)]);
+            wizard.runSubmitChain([
+                (next) => wizard.assertStorageIdsFree([values.storage], next, checkTarget),
+                (next) => wizard.createStorage(values, next),
+            ]);
             return;
         }
 
-        let multipath = !!values.multipath;
+        let reuse = wizard.targetUse?.reuse;
+        let multipath = reuse ? reuse.multipath : !!values.multipath;
         let portals = wizard.down('pveStorageWizardIScsiTarget').targetPortals;
         PVE.storage.wizard.submitSanLvm(wizard, values, {
             sameIdText: gettext('The iSCSI storage and the LVM storage need different IDs.'),
@@ -411,29 +545,15 @@ PVE.storage.wizard.types.iscsi = {
                       )
                     : undefined,
             baseExistsText: gettext(
-                'The iSCSI storage "{0}" exists. Press {1} to retry the remaining steps, or complete the setup under Datacenter > Storage > Add > LVM.',
+                'The iSCSI storage "{0}" exists. Press {1} to retry the remaining steps. If you close the wizard, running it again on the same target continues the setup.',
             ),
             volumeGroupText: gettext(
                 'Press {2} to retry the remaining steps. If the volume group "{0}" already exists on the LUN, you can also complete the setup under Datacenter > Storage > Add > LVM, with base volume "{1}" and volume group "{0}".',
             ),
             sameUnitText: gettext(
-                'Press {1} to try the same LUN again, for example after wiping it. The iSCSI storage "{0}" remains after closing the wizard. Complete its setup under Datacenter > Storage > Add > LVM.',
+                'Press {1} to try the same LUN again, for example after wiping it. To use another LUN or storage ID, close the wizard and run it again on the same target. The wizard reuses the iSCSI storage "{0}".',
             ),
-            // the LUNs of a target that another storage already logs in to are that storage's, and
-            // may be in use through it
-            checkTarget: (entries) => {
-                let other = entries.find(
-                    (entry) => entry.type === 'iscsi' && entry.target === values.target,
-                );
-                return other
-                    ? Ext.String.format(
-                          gettext(
-                              'The iSCSI storage "{0}" already uses this target. Create the LVM storage on top of it under Datacenter > Storage > Add > LVM.',
-                          ),
-                          enc(other.storage),
-                      )
-                    : undefined;
-            },
+            checkTarget,
             baseParams: (baseId) => ({
                 storage: baseId,
                 type: 'iscsi',
@@ -448,7 +568,7 @@ PVE.storage.wizard.types.iscsi = {
                 xtype: 'pveStorageWizardLunSelect',
                 footnote: Ext.String.format(
                     gettext(
-                        'Closing the wizard keeps storage "{0}". Complete the setup under Datacenter > Storage > Add > LVM if the wizard is closed.',
+                        'Closing the wizard keeps storage "{0}". Running the wizard again on the same target continues from there.',
                     ),
                     enc(baseId),
                 ),
diff --git a/www/manager6/storage/wizard/LunSelect.js b/www/manager6/storage/wizard/LunSelect.js
index 270daaa73..4e138f787 100644
--- a/www/manager6/storage/wizard/LunSelect.js
+++ b/www/manager6/storage/wizard/LunSelect.js
@@ -97,7 +97,7 @@ Ext.define('PVE.storage.wizard.LunSelect', {
         grid.setDisabled(!!pinned || blocked || me.scanning || me.scanError);
         me.down('#rescan').setHidden(!!pinned);
         me.down('#rescan').setDisabled(blocked || me.scanning);
-        me.down('#footnote').setHidden(!!pinned);
+        me.down('#footnote').setHidden(!!pinned || !!wizard.targetUse?.reuse);
         let text;
         if (wizard.unconfirmedStorage) {
             text = wizard.storageUnknownText(false);
diff --git a/www/manager6/storage/wizard/SanLvm.js b/www/manager6/storage/wizard/SanLvm.js
index 054725753..7050730e0 100644
--- a/www/manager6/storage/wizard/SanLvm.js
+++ b/www/manager6/storage/wizard/SanLvm.js
@@ -1,7 +1,22 @@
+// listing the LUNs and adding the LVM storage both activate the connection storage on that node
+PVE.storage.wizard.reuseNodeRefusal = function (reuse, node) {
+    if (!reuse?.nodes.length || reuse.nodes.includes(node)) {
+        return undefined;
+    }
+    return Ext.String.format(
+        gettext(
+            'Storage "{0}" is not available on node "{1}", which sets up the volume group. Add the node to that storage, or open the web UI on one of its nodes.',
+        ),
+        Ext.htmlEncode(reuse.storage),
+        Ext.htmlEncode(node),
+    );
+};
+
 PVE.storage.wizard.submitSanLvm = function (wizard, values, transport) {
     let enc = Ext.htmlEncode;
     let created = (wizard.created ??= {});
-    let baseId = values['base-storage'] || `${values.storage}-base`;
+    let reuse = wizard.targetUse?.reuse;
+    let baseId = reuse?.storage ?? (values['base-storage'] || `${values.storage}-base`);
     if (baseId === values.storage) {
         Ext.Msg.alert(gettext('Error'), transport.sameIdText);
         return;
@@ -13,6 +28,11 @@ PVE.storage.wizard.submitSanLvm = function (wizard, values, transport) {
 
     // the storage API creates the volume group on the node it runs on
     let node = Proxmox.NodeName;
+    let unavailable = PVE.storage.wizard.reuseNodeRefusal(reuse, node);
+    if (unavailable) {
+        Ext.Msg.alert(gettext('Error'), unavailable);
+        return;
+    }
     let nodesList = values.nodes ? [].concat(values.nodes).join(',').split(',') : [];
     nodesList = nodesList.filter((entry) => entry !== '');
     if (wizard.getScanNode() !== node || (nodesList.length && !nodesList.includes(node))) {
@@ -27,9 +47,35 @@ PVE.storage.wizard.submitSanLvm = function (wizard, values, transport) {
         );
         return;
     }
+    // the LVM storage can only be active where its connection storage is
+    let lvmNodes = values.nodes;
+    if (reuse?.nodes.length) {
+        if (!nodesList.length) {
+            lvmNodes = reuse.nodes.join(',');
+        } else if (nodesList.some((entry) => !reuse.nodes.includes(entry))) {
+            Ext.Msg.alert(
+                gettext('Error'),
+                Ext.String.format(
+                    ngettext(
+                        'Storage "{0}" is only available on node {1}.',
+                        'Storage "{0}" is only available on nodes {1}.',
+                        reuse.nodes.length,
+                    ),
+                    enc(baseId),
+                    enc(reuse.nodes.join(', ')),
+                ),
+            );
+            return;
+        }
+    }
 
     let retryHint = () =>
-        Ext.String.format(transport.baseExistsText, enc(baseId), enc(wizard.actionText()));
+        reuse
+            ? Ext.String.format(
+                  gettext('Press {0} to retry the remaining steps.'),
+                  enc(wizard.actionText()),
+              )
+            : Ext.String.format(transport.baseExistsText, enc(baseId), enc(wizard.actionText()));
     let volumeGroupHint = () =>
         Ext.String.format(
             transport.volumeGroupText,
@@ -48,13 +94,20 @@ PVE.storage.wizard.submitSanLvm = function (wizard, values, transport) {
                 next();
                 return;
             }
-            wizard.assertStorageIdsFree([baseId, values.storage], next, transport.checkTarget);
+            let ids = reuse ? [values.storage] : [baseId, values.storage];
+            wizard.assertStorageIdsFree(ids, next, transport.checkTarget);
         },
         (next) => {
             if (created.baseStorage) {
                 next();
                 return;
             }
+            if (reuse) {
+                created.baseStorage = baseId;
+                wizard.freeze();
+                next();
+                return;
+            }
             wizard.createStorage(transport.baseParams(baseId), () => {
                 created.baseStorage = baseId;
                 wizard.freeze();
@@ -70,9 +123,13 @@ PVE.storage.wizard.submitSanLvm = function (wizard, values, transport) {
             let active = wizard.activateLunView({
                 ...transport.lunView(node, baseId),
                 recap: Ext.String.format(
-                    gettext(
-                        'Connection storage "{1}" created. Setting up volume group and LVM storage "{0}".',
-                    ),
+                    reuse
+                        ? gettext(
+                              'Using existing connection storage "{1}" to set up volume group and LVM storage "{0}".',
+                          )
+                        : gettext(
+                              'Connection storage "{1}" created. Setting up volume group and LVM storage "{0}".',
+                          ),
                     enc(values.storage),
                     enc(baseId),
                 ),
@@ -112,7 +169,7 @@ PVE.storage.wizard.submitSanLvm = function (wizard, values, transport) {
                     base: created.baseVolume,
                     shared: 1,
                     content: values.content,
-                    nodes: values.nodes,
+                    nodes: lvmNodes,
                     disable: values.disable,
                 },
                 next,
diff --git a/www/manager6/storage/wizard/Wizard.js b/www/manager6/storage/wizard/Wizard.js
index c5ffa2b8e..433612b1a 100644
--- a/www/manager6/storage/wizard/Wizard.js
+++ b/www/manager6/storage/wizard/Wizard.js
@@ -165,6 +165,17 @@ Ext.define('PVE.storage.wizard.RemoteStorage', {
         return !me.lunView || me.lunView.canSubmit();
     },
 
+    canReuseBack: function () {
+        let me = this;
+        return !!(
+            me.lunView &&
+            me.targetUse?.reuse &&
+            !me.busy &&
+            !me.unconfirmedStorage &&
+            Object.keys(me.created ?? {}).every((key) => key === 'baseStorage')
+        );
+    },
+
     actionText: function () {
         let me = this;
         if (me.unconfirmedStorage) {
@@ -185,7 +196,7 @@ Ext.define('PVE.storage.wizard.RemoteStorage', {
         if (me.lunView) {
             me.lunView.updateState();
             me.down('#submit').setDisabled(!me.canSubmit());
-            me.down('#back').setDisabled(true);
+            me.down('#back').setDisabled(!me.canReuseBack());
         }
         // a type with its own submit condition decides the state of the confirm buttons
         let confirmActive = me.down('#wizcontent').getActiveTab() === me.down('#confirm');
@@ -214,6 +225,24 @@ Ext.define('PVE.storage.wizard.RemoteStorage', {
         return false;
     },
 
+    reuseBack: function () {
+        let me = this;
+        if (!me.canReuseBack()) {
+            return;
+        }
+        let view = me.lunView;
+        view.invalidateScan();
+        delete me.lunView;
+        delete me.created.baseStorage;
+        let panel = me.down('#confirm');
+        panel.getLayout().setActiveItem(0);
+        panel.remove(view, true);
+        panel.setTitle(gettext('Confirm'));
+        me.unfreeze();
+        me.down('#submit').enable();
+        me.updateSummary();
+    },
+
     // keeps the window busy, so it cannot close with the continuation pending
     request: function (options) {
         let me = this;
@@ -420,6 +449,33 @@ Ext.define('PVE.storage.wizard.RemoteStorage', {
         });
     },
 
+    // { reuse, conflict, inUse } from targetUse() of the type, only from the storages the user may
+    // see
+    targetUse: undefined,
+
+    refreshTargetUse: function () {
+        let me = this;
+        let typeEntry = PVE.storage.wizard.types[me.wizardType];
+        let request = (me.targetUseRequest = (me.targetUseRequest ?? 0) + 1);
+        let update = function (use) {
+            // a newer selection decides, and the wizard may be gone by now
+            if (me.destroyed || request !== me.targetUseRequest) {
+                return;
+            }
+            me.targetUse = use;
+            me.down('#wizcontent').items.each((tab) => tab.updateBlockedReason?.());
+            typeEntry.targetUseChanged?.(me, use);
+        };
+        update(undefined);
+        Proxmox.Utils.API2Request({
+            url: '/storage',
+            method: 'GET',
+            failure: () => update(undefined),
+            success: (response) =>
+                update(typeEntry.targetUse(response.result.data || [], me.getValues(), me)),
+        });
+    },
+
     storageUnknownText: function (html = true) {
         let encode = html ? Ext.htmlEncode : (value) => value;
         return Ext.String.format(
@@ -787,6 +843,15 @@ Ext.define('PVE.storage.wizard.RemoteStorage', {
         });
 
         me.callParent();
+        let back = me.down('#back');
+        let backHandler = back.handler;
+        back.setHandler(function () {
+            if (me.lunView) {
+                me.reuseBack();
+            } else {
+                backHandler.call(back);
+            }
+        });
         me.query('field').forEach((field) => {
             field.on('change', () => me.updateActions());
             field.on('validitychange', () => me.updateActions());
-- 
2.47.3