genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3710805651
##########
ui/src/views/infra/AddPrimaryStorage.vue:
##########
@@ -947,6 +950,9 @@ export default {
params['details[0].api_username'] = values.flashArrayUsername
params['details[0].api_password'] = values.flashArrayPassword
url = values.flashArrayURL
+ if (values.protocol === 'NVMeTCP') {
+ url = url + (url.indexOf('?') === -1 ? '?' : '&') +
'transport=nvme-tcp'
+ }
Review Comment:
Fixed in `ca9ca209cc` — the submit handler now replaces an existing
`transport=` parameter via regex instead of blindly appending, so a URL already
carrying `transport=` cannot end up with two conflicting values.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayVolume.java:
##########
@@ -107,6 +111,19 @@ public AddressType getAddressType() {
@JsonIgnore
public String getAddress() {
if (serial == null) return null;
+ if (AddressType.NVMETCP.equals(addressType)) {
+ // EUI-128 layout for FlashArray NVMe namespaces:
+ // 00 + serial[0:14] + <Pure OUI (24a937)> + serial[14:24]
+ // This is the value the Linux kernel exposes as
+ // /dev/disk/by-id/nvme-eui.<result>
+ if (serial.length() < 24) {
+ throw new RuntimeException("FlashArray serial [" + serial
+ + "] is too short to build an NVMe EUI-128 address "
+ + "(expected at least 24 hex characters, got "
+ + serial.length() + ")");
+ }
+ return ("00" + serial.substring(0, 14) + PURE_OUI_EUI +
serial.substring(14)).toLowerCase();
+ }
Review Comment:
Fixed in `ca9ca209cc` — now slices `serial.substring(14, 24)` instead of
`substring(14)`, so a longer-than-expected serial can no longer produce an EUI
over 32 hex characters.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -171,15 +189,32 @@ public String attach(ProviderAdapterContext context,
ProviderAdapterDataObject d
});
if (list != null && list.getItems() != null) {
for (FlashArrayConnection conn : list.getItems()) {
- if (conn.getHost() != null && conn.getHost().getName()
!= null &&
+ if (AddressType.NVMETCP.equals(volumeAddressType)) {
+ // Prefer a hostgroup-scoped match when a
hostgroup is configured
+ // on the pool; otherwise fall through to matching
the connection
+ // by host like the Fibre Channel branch below.
Covers both
+ // transport=nvme-tcp deployments with and without
hostgroup=.
+ if (hostgroup != null && conn.getHostGroup() !=
null
+ && conn.getHostGroup().getName() != null
+ &&
conn.getHostGroup().getName().equals(hostgroup)) {
+ return conn.getNsid() != null ? "" +
conn.getNsid() : "1";
+ }
+ if (conn.getHost() != null &&
conn.getHost().getName() != null
+ &&
(conn.getHost().getName().equals(hostname)
+ || (hostname.indexOf('.') > 0
+ && conn.getHost().getName()
+ .equals(hostname.substring(0,
hostname.indexOf('.')))))) {
+ return conn.getNsid() != null ? "" +
conn.getNsid() : "1";
+ }
+ } else if (conn.getHost() != null &&
conn.getHost().getName() != null &&
(conn.getHost().getName().equals(hostname) ||
conn.getHost().getName().equals(hostname.substring(0, hostname.indexOf('.'))))
&&
conn.getLun() != null) {
return "" + conn.getLun();
}
Review Comment:
Fixed in `ca9ca209cc` — the Fibre Channel branch now guards
`hostname.indexOf(char(39)+char(46)+char(39)) > 0` before slicing, mirroring
the NVMe-TCP branch. A hostname with no dot no longer throws.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFPool.java:
##########
@@ -0,0 +1,157 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.storage;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cloudstack.utils.qemu.QemuImg;
+import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
+import org.joda.time.Duration;
+
+import com.cloud.agent.api.to.HostTO;
+import com.cloud.hypervisor.kvm.resource.KVMHABase.HAStoragePool;
+import com.cloud.storage.Storage;
+import com.cloud.storage.Storage.ProvisioningType;
+
+/**
+ * KVMStoragePool for NVMe-over-Fabrics pools. Mirror of
+ * {@link MultipathSCSIPool} for adapters based on
+ * {@link MultipathNVMeOFAdapterBase}. Every data operation is delegated
+ * back to the adapter; the pool itself only tracks addressing/identity.
+ */
+public class MultipathNVMeOFPool implements KVMStoragePool {
+ private final String uuid;
+ private final String sourceHost;
+ private final int sourcePort;
+ private final String sourceDir;
+ private final Storage.StoragePoolType storagePoolType;
+ private final StorageAdaptor storageAdaptor;
+ private final Map<String, String> details;
+ private long capacity;
+ private long used;
+ private long available;
+
+ public MultipathNVMeOFPool(String uuid, String host, int port, String path,
+ Storage.StoragePoolType poolType, Map<String, String> poolDetails,
StorageAdaptor adaptor) {
+ this.uuid = uuid;
+ this.sourceHost = host;
+ this.sourcePort = port;
+ this.sourceDir = path;
+ this.storagePoolType = poolType;
+ this.storageAdaptor = adaptor;
+ this.details = poolDetails;
+ this.capacity = 0;
+ this.used = 0;
+ this.available = 0;
+ }
+
+ public MultipathNVMeOFPool(String uuid, StorageAdaptor adaptor) {
+ this.uuid = uuid;
+ this.sourceHost = null;
+ this.sourcePort = -1;
+ this.sourceDir = null;
+ this.storagePoolType = Storage.StoragePoolType.NVMeTCP;
+ this.storageAdaptor = adaptor;
+ this.details = new HashMap<>();
+ this.capacity = 0;
+ this.used = 0;
+ this.available = 0;
+ }
+
+ @Override
+ public KVMPhysicalDisk createPhysicalDisk(String volumeUuid,
ProvisioningType provisioningType, long size, byte[] passphrase) {
+ return null;
+ }
+
+ @Override
+ public KVMPhysicalDisk createPhysicalDisk(String volumeUuid,
PhysicalDiskFormat format, ProvisioningType provisioningType, long size, byte[]
passphrase) {
+ return null;
+ }
Review Comment:
Leaving these as-is intentionally. `MultipathNVMeOFPool` is a dummy pool
object: this adapter dispatches per-volume and never uses the pool for
connectivity, capacity, or disk lifecycle, so these `KVMStoragePool` methods
are genuinely not applicable rather than not-yet-implemented. The existing
`MultipathSCSIPool` (the FC/iSCSI sibling this class is modelled on) does
exactly the same thing — returns `null`/`false`/`true` placeholders for the
same set of methods. Throwing `UnsupportedOperationException` here would
diverge from that precedent and risks turning a currently harmless no-op call
into a hard failure somewhere in the KVM agent that we have not exercised.
Happy to revisit if a maintainer prefers the stricter behaviour applied to both
classes together.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFPool.java:
##########
@@ -0,0 +1,157 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.storage;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cloudstack.utils.qemu.QemuImg;
+import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
+import org.joda.time.Duration;
+
+import com.cloud.agent.api.to.HostTO;
+import com.cloud.hypervisor.kvm.resource.KVMHABase.HAStoragePool;
+import com.cloud.storage.Storage;
+import com.cloud.storage.Storage.ProvisioningType;
+
+/**
+ * KVMStoragePool for NVMe-over-Fabrics pools. Mirror of
+ * {@link MultipathSCSIPool} for adapters based on
+ * {@link MultipathNVMeOFAdapterBase}. Every data operation is delegated
+ * back to the adapter; the pool itself only tracks addressing/identity.
+ */
+public class MultipathNVMeOFPool implements KVMStoragePool {
+ private final String uuid;
+ private final String sourceHost;
+ private final int sourcePort;
+ private final String sourceDir;
+ private final Storage.StoragePoolType storagePoolType;
+ private final StorageAdaptor storageAdaptor;
+ private final Map<String, String> details;
+ private long capacity;
+ private long used;
+ private long available;
+
+ public MultipathNVMeOFPool(String uuid, String host, int port, String path,
+ Storage.StoragePoolType poolType, Map<String, String> poolDetails,
StorageAdaptor adaptor) {
+ this.uuid = uuid;
+ this.sourceHost = host;
+ this.sourcePort = port;
+ this.sourceDir = path;
+ this.storagePoolType = poolType;
+ this.storageAdaptor = adaptor;
+ this.details = poolDetails;
+ this.capacity = 0;
+ this.used = 0;
+ this.available = 0;
+ }
+
+ public MultipathNVMeOFPool(String uuid, StorageAdaptor adaptor) {
+ this.uuid = uuid;
+ this.sourceHost = null;
+ this.sourcePort = -1;
+ this.sourceDir = null;
+ this.storagePoolType = Storage.StoragePoolType.NVMeTCP;
+ this.storageAdaptor = adaptor;
+ this.details = new HashMap<>();
+ this.capacity = 0;
+ this.used = 0;
+ this.available = 0;
+ }
+
+ @Override
+ public KVMPhysicalDisk createPhysicalDisk(String volumeUuid,
ProvisioningType provisioningType, long size, byte[] passphrase) {
+ return null;
+ }
+
+ @Override
+ public KVMPhysicalDisk createPhysicalDisk(String volumeUuid,
PhysicalDiskFormat format, ProvisioningType provisioningType, long size, byte[]
passphrase) {
+ return null;
+ }
+
+ @Override
+ public boolean connectPhysicalDisk(String volumeUuid, Map<String, String>
details) {
+ return storageAdaptor.connectPhysicalDisk(volumeUuid, this, details,
false);
+ }
+
+ @Override
+ public KVMPhysicalDisk getPhysicalDisk(String volumeId) {
+ return storageAdaptor.getPhysicalDisk(volumeId, this);
+ }
+
+ @Override
+ public boolean disconnectPhysicalDisk(String volumeUuid) {
+ return storageAdaptor.disconnectPhysicalDisk(volumeUuid, this);
+ }
+
+ @Override
+ public boolean deletePhysicalDisk(String volumeUuid, Storage.ImageFormat
format) {
+ return true;
+ }
+
+ @Override
+ public List<KVMPhysicalDisk> listPhysicalDisks() {
+ return null;
+ }
Review Comment:
Leaving these as-is intentionally. `MultipathNVMeOFPool` is a dummy pool
object: this adapter dispatches per-volume and never uses the pool for
connectivity, capacity, or disk lifecycle, so these `KVMStoragePool` methods
are genuinely not applicable rather than not-yet-implemented. The existing
`MultipathSCSIPool` (the FC/iSCSI sibling this class is modelled on) does
exactly the same thing — returns `null`/`false`/`true` placeholders for the
same set of methods. Throwing `UnsupportedOperationException` here would
diverge from that precedent and risks turning a currently harmless no-op call
into a hard failure somewhere in the KVM agent that we have not exercised.
Happy to revisit if a maintainer prefers the stricter behaviour applied to both
classes together.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.storage;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.cloudstack.utils.qemu.QemuImg;
+import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
+import org.apache.cloudstack.utils.qemu.QemuImgException;
+import org.apache.cloudstack.utils.qemu.QemuImgFile;
+import org.libvirt.LibvirtException;
+
+import com.cloud.storage.Storage;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.script.OutputInterpreter;
+import com.cloud.utils.script.Script;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+/**
+ * Base class for KVM storage adapters that surface remote block volumes over
+ * NVMe-over-Fabrics (NVMe-oF). It is the NVMe-oF counterpart of
+ * {@link MultipathSCSIAdapterBase}: it does not drive device-mapper multipath
+ * and does not rescan the SCSI bus, because NVMe-oF has its own multipath
+ * (the kernel's native NVMe multipath) and namespaces show up via
+ * asynchronous event notifications as soon as the target grants access.
+ *
+ * Volumes are identified on the host by their EUI-128 NGUID, which udev
+ * exposes as {@code /dev/disk/by-id/nvme-eui.<eui>}.
+ */
+public abstract class MultipathNVMeOFAdapterBase implements StorageAdaptor {
+ protected static Logger LOGGER =
LogManager.getLogger(MultipathNVMeOFAdapterBase.class);
+ static final Map<String, KVMStoragePool> MapStorageUuidToStoragePool = new
HashMap<>();
Review Comment:
Fixed in `ca9ca209cc` — `MapStorageUuidToStoragePool` is now a
`ConcurrentHashMap` populated via `computeIfAbsent`, so concurrent callers
cannot race to double-create a pool or corrupt the map.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:
##########
@@ -0,0 +1,465 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.storage;
+
+import java.io.File;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.cloudstack.utils.qemu.QemuImg;
+import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
+import org.apache.cloudstack.utils.qemu.QemuImgException;
+import org.apache.cloudstack.utils.qemu.QemuImgFile;
+import org.libvirt.LibvirtException;
+
+import com.cloud.storage.Storage;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.script.OutputInterpreter;
+import com.cloud.utils.script.Script;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+/**
+ * Base class for KVM storage adapters that surface remote block volumes over
+ * NVMe-over-Fabrics (NVMe-oF). It is the NVMe-oF counterpart of
+ * {@link MultipathSCSIAdapterBase}: it does not drive device-mapper multipath
+ * and does not rescan the SCSI bus, because NVMe-oF has its own multipath
+ * (the kernel's native NVMe multipath) and namespaces show up via
+ * asynchronous event notifications as soon as the target grants access.
+ *
+ * Volumes are identified on the host by their EUI-128 NGUID, which udev
+ * exposes as {@code /dev/disk/by-id/nvme-eui.<eui>}.
+ */
+public abstract class MultipathNVMeOFAdapterBase implements StorageAdaptor {
+ protected static Logger LOGGER =
LogManager.getLogger(MultipathNVMeOFAdapterBase.class);
+ static final Map<String, KVMStoragePool> MapStorageUuidToStoragePool = new
HashMap<>();
+
+ static final int DEFAULT_DISK_WAIT_SECS = 240;
+ static final long NS_RESCAN_TIMEOUT_SECS = 5;
+ private static final long POLL_INTERVAL_MS = 2000;
+
+ @Override
+ public KVMStoragePool getStoragePool(String uuid) {
+ KVMStoragePool pool = MapStorageUuidToStoragePool.get(uuid);
+ if (pool == null) {
+ // Dummy pool - adapters that dispatch per-volume don't need
+ // connectivity information on the pool itself.
+ pool = new MultipathNVMeOFPool(uuid, this);
+ MapStorageUuidToStoragePool.put(uuid, pool);
+ }
+ return pool;
+ }
Review Comment:
Fixed in `ca9ca209cc` — `MapStorageUuidToStoragePool` is now a
`ConcurrentHashMap` populated via `computeIfAbsent`, so concurrent callers
cannot race to double-create a pool or corrupt the map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
