Copilot commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3659302164
##########
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:
This always appends an additional `transport=nvme-tcp` even if the URL
already includes a `transport=` query parameter, potentially resulting in
duplicate/ambiguous transport selection. Consider parsing the query string and
replacing/setting the `transport` key instead of blindly appending.
##########
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:
The Fibre Channel match uses `hostname.substring(0, hostname.indexOf('.'))`
without guarding for hosts where `hostname` has no dot (i.e., `indexOf('.') ==
-1`), which will throw `StringIndexOutOfBoundsException`. Mirror the NVMe
branch’s guarding (`hostname.indexOf('.') > 0`) or precompute a safe short
hostname.
##########
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:
The EUI-128 construction assumes a 24-hex-character serial (`serial[0:24]`),
but it only checks `serial.length() < 24` and then appends
`serial.substring(14)` (to end). If the serial is longer than 24, the computed
EUI will be longer than 32 hex chars and won’t match
`/dev/disk/by-id/nvme-eui.<32-hex>`. Consider enforcing `serial.length() == 24`
or explicitly using `serial.substring(14, 24)`.
##########
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:
The static storage-pool cache is a plain `HashMap` accessed without
synchronization; concurrent calls can race (double-create pools) and `HashMap`
is not safe for concurrent writes. Use a `ConcurrentHashMap` and
`computeIfAbsent`, or otherwise synchronize access.
##########
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:
Several unimplemented `KVMStoragePool` methods return `null`/`false`/`true`
placeholders, which can mask errors and lead to NPEs if any caller expects real
behavior (e.g., `createPhysicalDisk`, `listPhysicalDisks`). Prefer throwing
`UnsupportedOperationException` for operations that are intentionally
unsupported on managed NVMeTCP pools, and only return `true` where the
operation truly succeeded.
##########
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:
Several unimplemented `KVMStoragePool` methods return `null`/`false`/`true`
placeholders, which can mask errors and lead to NPEs if any caller expects real
behavior (e.g., `createPhysicalDisk`, `listPhysicalDisks`). Prefer throwing
`UnsupportedOperationException` for operations that are intentionally
unsupported on managed NVMeTCP pools, and only return `true` where the
operation truly succeeded.
##########
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;
Review Comment:
During namespace discovery, this polls every 2 seconds and triggers
`rescanAllControllers()` on every iteration. Since `rescanAllControllers()`
spawns `nvme ns-rescan` processes per controller, this can become expensive on
hosts with many controllers and can create bursty process load. Consider
rate-limiting rescans (e.g., rescan every N polls or with a minimum interval)
or only rescanning after an initial grace period when the device hasn't
appeared.
##########
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:
The static storage-pool cache is a plain `HashMap` accessed without
synchronization; concurrent calls can race (double-create pools) and `HashMap`
is not safe for concurrent writes. Use a `ConcurrentHashMap` and
`computeIfAbsent`, or otherwise synchronize access.
##########
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;
+ }
+
+ @Override
+ public KVMStoragePool getStoragePool(String uuid, boolean refreshInfo) {
+ return getStoragePool(uuid);
+ }
+
+ public abstract String getName();
+
+ @Override
+ public abstract Storage.StoragePoolType getStoragePoolType();
+
+ public abstract boolean isStoragePoolTypeSupported(Storage.StoragePoolType
type);
+
+ /**
+ * Parse a {@code type=NVMETCP; address=<eui>; connid.<host>=<nsid>; ...}
+ * volume path and produce an {@link AddressInfo} with the host-side device
+ * path set to {@code /dev/disk/by-id/nvme-eui.<eui>}.
+ */
+ public AddressInfo parseAndValidatePath(String inPath) {
+ if (inPath == null) {
+ throw new CloudRuntimeException("Cannot parse null volume path");
+ }
+ String type = null;
+ String address = null;
+ String connectionId = null;
+ String path = null;
+ String hostname = resolveHostnameShort();
+ String hostnameFq = resolveHostnameFq();
+ String[] parts = inPath.split(";");
+ for (String part : parts) {
+ String[] pair = part.split("=");
+ if (pair.length != 2) {
+ continue;
+ }
Review Comment:
Parsing `key=value` tokens with `part.split(\"=\")` will split on every `=`;
if any value ever contains `=` (e.g., encoded data), `pair.length` will exceed
2 and the token is silently ignored. Use a 2-part split (limit=2) so only the
first `=` delimits key from value.
##########
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;
+ }
+
+ @Override
+ public KVMStoragePool getStoragePool(String uuid, boolean refreshInfo) {
+ return getStoragePool(uuid);
+ }
+
+ public abstract String getName();
+
+ @Override
+ public abstract Storage.StoragePoolType getStoragePoolType();
+
+ public abstract boolean isStoragePoolTypeSupported(Storage.StoragePoolType
type);
+
+ /**
+ * Parse a {@code type=NVMETCP; address=<eui>; connid.<host>=<nsid>; ...}
+ * volume path and produce an {@link AddressInfo} with the host-side device
+ * path set to {@code /dev/disk/by-id/nvme-eui.<eui>}.
+ */
+ public AddressInfo parseAndValidatePath(String inPath) {
+ if (inPath == null) {
+ throw new CloudRuntimeException("Cannot parse null volume path");
+ }
+ String type = null;
+ String address = null;
+ String connectionId = null;
+ String path = null;
+ String hostname = resolveHostnameShort();
+ String hostnameFq = resolveHostnameFq();
+ String[] parts = inPath.split(";");
+ for (String part : parts) {
+ String[] pair = part.split("=");
+ if (pair.length != 2) {
+ continue;
+ }
+ String key = pair[0].trim();
+ String value = pair[1].trim();
+ if (key.equals("type")) {
+ type = value.toUpperCase();
+ } else if (key.equals("address")) {
+ address = value;
+ } else if (key.equals("connid")) {
+ connectionId = value;
+ } else if (key.startsWith("connid.")) {
+ String inHostname = key.substring("connid.".length());
+ if (inHostname.equals(hostname) ||
inHostname.equals(hostnameFq)) {
+ connectionId = value;
+ }
+ }
+ }
+
+ if (!"NVMETCP".equals(type)) {
+ throw new CloudRuntimeException("Invalid address type provided for
NVMe-oF target disk: " + type);
+ }
+ if (address == null) {
+ throw new CloudRuntimeException("NVMe-oF volume path is missing
the required address field");
+ }
+ path = "/dev/disk/by-id/nvme-eui." + address.toLowerCase();
+ return new AddressInfo(type, address, connectionId, path);
+ }
+
+ @Override
+ public KVMPhysicalDisk getPhysicalDisk(String volumePath, KVMStoragePool
pool) {
+ if (StringUtils.isEmpty(volumePath) || pool == null) {
+ LOGGER.error("Unable to get physical disk, volume path or pool not
specified");
+ return null;
+ }
+ return getPhysicalDisk(parseAndValidatePath(volumePath), pool);
+ }
+
+ private KVMPhysicalDisk getPhysicalDisk(AddressInfo address,
KVMStoragePool pool) {
+ KVMPhysicalDisk disk = new KVMPhysicalDisk(address.getPath(),
address.toString(), pool);
+ disk.setFormat(QemuImg.PhysicalDiskFormat.RAW);
+
+ if (!isConnected(address.getPath())) {
+ if (!connectPhysicalDisk(address, pool, null)) {
+ throw new CloudRuntimeException("Unable to connect to NVMe
namespace at " + address.getPath());
+ }
+ }
+ long diskSize = getPhysicalDiskSize(address.getPath());
+ disk.setSize(diskSize);
+ disk.setVirtualSize(diskSize);
+ return disk;
+ }
+
+ @Override
+ public KVMStoragePool createStoragePool(String uuid, String host, int
port, String path, String userInfo, Storage.StoragePoolType type, Map<String,
String> details, boolean isPrimaryStorage) {
+ LOGGER.info(String.format("createStoragePool(uuid,host,port,path,type)
called with args (%s, %s, %d, %s, %s)", uuid, host, port, path, type));
+ MultipathNVMeOFPool pool = new MultipathNVMeOFPool(uuid, host, port,
path, type, details, this);
+ MapStorageUuidToStoragePool.put(uuid, pool);
+ return pool;
+ }
+
+ @Override
+ public boolean deleteStoragePool(String uuid) {
+ MapStorageUuidToStoragePool.remove(uuid);
+ return true;
+ }
+
+ @Override
+ public boolean deleteStoragePool(KVMStoragePool pool) {
+ return deleteStoragePool(pool.getUuid());
+ }
+
+ @Override
+ public boolean connectPhysicalDisk(String volumePath, KVMStoragePool pool,
Map<String, String> details, boolean isVMMigrate) {
+ if (StringUtils.isEmpty(volumePath) || pool == null) {
+ LOGGER.error("Unable to connect NVMe-oF physical disk:
insufficient arguments");
+ return false;
+ }
+ return connectPhysicalDisk(parseAndValidatePath(volumePath), pool,
details);
+ }
+
+ private boolean connectPhysicalDisk(AddressInfo address, KVMStoragePool
pool, Map<String, String> details) {
+ if (address.getConnectionId() == null) {
+ LOGGER.error("NVMe-oF volume " + address.getPath() + " on pool " +
pool.getUuid() + " is missing a connid.<host> token in its path");
+ return false;
+ }
+ long waitSecs = DEFAULT_DISK_WAIT_SECS;
+ if (details != null &&
details.containsKey(com.cloud.storage.StorageManager.STORAGE_POOL_DISK_WAIT.toString()))
{
+ String waitTime =
details.get(com.cloud.storage.StorageManager.STORAGE_POOL_DISK_WAIT.toString());
+ if (StringUtils.isNotEmpty(waitTime)) {
+ try {
+ waitSecs = Integer.parseInt(waitTime);
+ } catch (NumberFormatException e) {
+ LOGGER.warn("Ignoring non-numeric " +
com.cloud.storage.StorageManager.STORAGE_POOL_DISK_WAIT.toString()
+ + "=[" + waitTime + "] on pool " + pool.getUuid()
+ ", falling back to default "
+ + DEFAULT_DISK_WAIT_SECS + "s");
+ }
+ }
+ }
+ return waitForNamespace(address, pool, waitSecs);
+ }
+
+ /**
+ * Poll for the EUI-keyed udev symlink to show up. On every iteration also
+ * nudge the kernel with {@code nvme ns-rescan} on every local NVMe
+ * controller, to cover arrays / firmware combinations that do not emit a
+ * reliable asynchronous event notification when a new namespace is
+ * mapped.
+ */
+ private boolean waitForNamespace(AddressInfo address, KVMStoragePool pool,
long waitSecs) {
+ if (waitSecs < 60) {
+ waitSecs = 60;
+ }
+ long deadline = System.currentTimeMillis() + (waitSecs * 1000);
+ File dev = new File(address.getPath());
+ while (System.currentTimeMillis() < deadline) {
+ if (dev.exists() && isConnected(address.getPath())) {
+ long size = getPhysicalDiskSize(address.getPath());
+ if (size > 0) {
+ LOGGER.debug("Found NVMe namespace at " +
address.getPath());
+ return true;
+ }
+ }
+ rescanAllControllers();
+ try {
+ Thread.sleep(POLL_INTERVAL_MS);
+ } catch (InterruptedException ie) {
Review Comment:
During namespace discovery, this polls every 2 seconds and triggers
`rescanAllControllers()` on every iteration. Since `rescanAllControllers()`
spawns `nvme ns-rescan` processes per controller, this can become expensive on
hosts with many controllers and can create bursty process load. Consider
rate-limiting rescans (e.g., rescan every N polls or with a minimum interval)
or only rescanning after an initial grace period when the device hasn't
appeared.
##########
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;
+ }
+
+ @Override
+ public KVMStoragePool getStoragePool(String uuid, boolean refreshInfo) {
+ return getStoragePool(uuid);
+ }
+
+ public abstract String getName();
+
+ @Override
+ public abstract Storage.StoragePoolType getStoragePoolType();
+
+ public abstract boolean isStoragePoolTypeSupported(Storage.StoragePoolType
type);
+
+ /**
+ * Parse a {@code type=NVMETCP; address=<eui>; connid.<host>=<nsid>; ...}
+ * volume path and produce an {@link AddressInfo} with the host-side device
+ * path set to {@code /dev/disk/by-id/nvme-eui.<eui>}.
+ */
+ public AddressInfo parseAndValidatePath(String inPath) {
+ if (inPath == null) {
+ throw new CloudRuntimeException("Cannot parse null volume path");
+ }
+ String type = null;
+ String address = null;
+ String connectionId = null;
+ String path = null;
+ String hostname = resolveHostnameShort();
+ String hostnameFq = resolveHostnameFq();
+ String[] parts = inPath.split(";");
+ for (String part : parts) {
+ String[] pair = part.split("=");
+ if (pair.length != 2) {
+ continue;
+ }
+ String key = pair[0].trim();
+ String value = pair[1].trim();
+ if (key.equals("type")) {
+ type = value.toUpperCase();
+ } else if (key.equals("address")) {
+ address = value;
+ } else if (key.equals("connid")) {
+ connectionId = value;
+ } else if (key.startsWith("connid.")) {
+ String inHostname = key.substring("connid.".length());
+ if (inHostname.equals(hostname) ||
inHostname.equals(hostnameFq)) {
+ connectionId = value;
+ }
+ }
+ }
+
+ if (!"NVMETCP".equals(type)) {
+ throw new CloudRuntimeException("Invalid address type provided for
NVMe-oF target disk: " + type);
+ }
+ if (address == null) {
+ throw new CloudRuntimeException("NVMe-oF volume path is missing
the required address field");
+ }
+ path = "/dev/disk/by-id/nvme-eui." + address.toLowerCase();
+ return new AddressInfo(type, address, connectionId, path);
+ }
+
+ @Override
+ public KVMPhysicalDisk getPhysicalDisk(String volumePath, KVMStoragePool
pool) {
+ if (StringUtils.isEmpty(volumePath) || pool == null) {
+ LOGGER.error("Unable to get physical disk, volume path or pool not
specified");
+ return null;
+ }
+ return getPhysicalDisk(parseAndValidatePath(volumePath), pool);
+ }
+
+ private KVMPhysicalDisk getPhysicalDisk(AddressInfo address,
KVMStoragePool pool) {
+ KVMPhysicalDisk disk = new KVMPhysicalDisk(address.getPath(),
address.toString(), pool);
+ disk.setFormat(QemuImg.PhysicalDiskFormat.RAW);
+
+ if (!isConnected(address.getPath())) {
+ if (!connectPhysicalDisk(address, pool, null)) {
+ throw new CloudRuntimeException("Unable to connect to NVMe
namespace at " + address.getPath());
+ }
+ }
+ long diskSize = getPhysicalDiskSize(address.getPath());
+ disk.setSize(diskSize);
+ disk.setVirtualSize(diskSize);
+ return disk;
+ }
+
+ @Override
+ public KVMStoragePool createStoragePool(String uuid, String host, int
port, String path, String userInfo, Storage.StoragePoolType type, Map<String,
String> details, boolean isPrimaryStorage) {
+ LOGGER.info(String.format("createStoragePool(uuid,host,port,path,type)
called with args (%s, %s, %d, %s, %s)", uuid, host, port, path, type));
+ MultipathNVMeOFPool pool = new MultipathNVMeOFPool(uuid, host, port,
path, type, details, this);
+ MapStorageUuidToStoragePool.put(uuid, pool);
+ return pool;
+ }
+
+ @Override
+ public boolean deleteStoragePool(String uuid) {
+ MapStorageUuidToStoragePool.remove(uuid);
+ return true;
+ }
+
+ @Override
+ public boolean deleteStoragePool(KVMStoragePool pool) {
+ return deleteStoragePool(pool.getUuid());
+ }
+
+ @Override
+ public boolean connectPhysicalDisk(String volumePath, KVMStoragePool pool,
Map<String, String> details, boolean isVMMigrate) {
+ if (StringUtils.isEmpty(volumePath) || pool == null) {
+ LOGGER.error("Unable to connect NVMe-oF physical disk:
insufficient arguments");
+ return false;
+ }
+ return connectPhysicalDisk(parseAndValidatePath(volumePath), pool,
details);
+ }
+
+ private boolean connectPhysicalDisk(AddressInfo address, KVMStoragePool
pool, Map<String, String> details) {
+ if (address.getConnectionId() == null) {
+ LOGGER.error("NVMe-oF volume " + address.getPath() + " on pool " +
pool.getUuid() + " is missing a connid.<host> token in its path");
+ return false;
+ }
+ long waitSecs = DEFAULT_DISK_WAIT_SECS;
+ if (details != null &&
details.containsKey(com.cloud.storage.StorageManager.STORAGE_POOL_DISK_WAIT.toString()))
{
+ String waitTime =
details.get(com.cloud.storage.StorageManager.STORAGE_POOL_DISK_WAIT.toString());
+ if (StringUtils.isNotEmpty(waitTime)) {
+ try {
+ waitSecs = Integer.parseInt(waitTime);
+ } catch (NumberFormatException e) {
+ LOGGER.warn("Ignoring non-numeric " +
com.cloud.storage.StorageManager.STORAGE_POOL_DISK_WAIT.toString()
+ + "=[" + waitTime + "] on pool " + pool.getUuid()
+ ", falling back to default "
+ + DEFAULT_DISK_WAIT_SECS + "s");
+ }
+ }
+ }
+ return waitForNamespace(address, pool, waitSecs);
+ }
+
+ /**
+ * Poll for the EUI-keyed udev symlink to show up. On every iteration also
+ * nudge the kernel with {@code nvme ns-rescan} on every local NVMe
+ * controller, to cover arrays / firmware combinations that do not emit a
+ * reliable asynchronous event notification when a new namespace is
+ * mapped.
+ */
+ private boolean waitForNamespace(AddressInfo address, KVMStoragePool pool,
long waitSecs) {
+ if (waitSecs < 60) {
+ waitSecs = 60;
+ }
+ long deadline = System.currentTimeMillis() + (waitSecs * 1000);
+ File dev = new File(address.getPath());
+ while (System.currentTimeMillis() < deadline) {
Review Comment:
During namespace discovery, this polls every 2 seconds and triggers
`rescanAllControllers()` on every iteration. Since `rescanAllControllers()`
spawns `nvme ns-rescan` processes per controller, this can become expensive on
hosts with many controllers and can create bursty process load. Consider
rate-limiting rescans (e.g., rescan every N polls or with a minimum interval)
or only rescanning after an initial grace period when the device hasn't
appeared.
--
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]
