You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "genegr (via GitHub)" <gi...@apache.org> on 2026/04/22 19:33:59 UTC
[PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
genegr opened a new pull request, #13061:
URL: https://github.com/apache/cloudstack/pull/13061
### Description
Adds an end-to-end NVMe-over-TCP data path for CloudStack on KVM, using the FlashArray adaptive plugin as the first (and currently only) consumer. The change is opt-in — existing Fibre Channel FlashArray / Primera deployments continue to work unchanged.
A FlashArray pool is switched to NVMe-TCP by adding a single `transport=nvme-tcp` query parameter to the pool URL on `createStoragePool`:
```
url=https://<user>:<pass>@<fa-ip>:443/api?pod=<pod>&transport=nvme-tcp&hostgroup=<hg>
```
When that parameter is present the adaptive lifecycle stamps the pool with the new `StoragePoolType.NVMeTCP`, the KVM agent dispatches to a brand-new `MultipathNVMeOFAdapterBase` / `NVMeTCPAdapter` pair, and the FlashArray adapter attaches volumes as host-group-scoped NVMe connections, builds EUI-128 NGUIDs in the layout `/dev/disk/by-id/nvme-eui.<32-hex>` that udev emits for a Pure namespace, and reverses that layout when CloudStack looks up a volume by address.
The six commits are split along natural seams (address type, FA REST-side support, storage pool type, KVM adapter, adaptive lifecycle routing, docs) so each can be reviewed independently.
Why a separate `NVMeTCP` pool type (and a separate `MultipathNVMeOFAdapterBase`) rather than reusing `FiberChannel` / `MultipathSCSIAdapterBase`?
- NVMe-oF is a different command set (NVMe, not SCSI), identifies namespaces by EUI-128 NGUIDs (not SCSI WWNs), and on Linux is multipathed natively by the `nvme` driver rather than by device-mapper multipath. Keeping it out of the SCSI code path avoids special-casing inside every method that handles paths, connect, disconnect, or size lookup.
- The new base class is fabric-agnostic: a future NVMe-RoCE or NVMe-FC adapter would only need a concrete subclass and a new pool-type value, without touching the SCSI code.
### Types of changes
- [x] Enhancement (non-breaking change which adds functionality)
- [ ] Bugfix
- [ ] Breaking change
### Feature/Enhancement Scale or Bug Severity
Feature. Opt-in via `transport=nvme-tcp` URL parameter on pool registration. Defaults are unchanged.
### How Has This Been Tested?
Validated end-to-end on a 4.23-SNAPSHOT lab against a Pure Storage FlashArray running Purity 6.7.7:
- Pre-requisites on each KVM host: an OVS bridge `cloudbr-nvme` with an IP on the NVMe subnet, `nvme-cli` + `nvme_tcp` kernel module, a persistent `/etc/nvme/hostnqn`, a populated `/etc/nvme/discovery.conf` and `nvme connect-all` enabled at boot.
- Pre-requisites on the array: a pod (`cloudstack`), a hostgroup matching the CloudStack cluster name (`cluster1`), one host per KVM host inside the hostgroup bound to the host's NQN.
- Registered a FlashArray primary pool with `provider="Flash Array"`, `transport=nvme-tcp`, `hostgroup=cluster1` → pool enters `Up` state, `type: NVMeTCP`.
- Created and attached a 20 GiB `tags=nvme` disk offering volume to a Rocky 9 VM: the volume's path carried `type=NVMETCP; address=<EUI-128>; connid.kvm01=1; connid.kvm02=1;`; both hosts saw `/dev/disk/by-id/nvme-eui.<that EUI>` via the host-group NVMe connection; libvirt presented the namespace to the guest as `/dev/vdb`.
- Inside the guest: `mkfs.ext4 /dev/vdb`, wrote 16 MiB of `/dev/urandom` with `conv=fsync`, recorded SHA-256, unmounted/remounted, re-checksummed → hash matched.
- Live-migrated the VM between the two KVM hosts while a `sha256sum` probe loop was running against `/mnt/nvme/pattern.bin` every 2 s. Migration completed in 6 s, the loop output showed the same hash across the migration window with no gap (multi-path/hostgroup-scope proof).
- Default-path Fibre Channel registrations (no `transport=` parameter) continue to work — `type: FiberChannel`, FC WWN addressing, same `MultipathSCSIAdapterBase` code path as before.
### Notes
- This PR stacks logically on top of #13050 (FlashArray empty-pod capacity fallback) for a clean empty-pod registration experience, but does **not** require it — `capacitybytes=` on `createStoragePool` is a workaround without #13050 merged. Another PR is pending to fix `AdaptiveDataStoreLifeCycleImpl.initialize()` to honor user-supplied capacity even when provider stats are unavailable.
- Future work (not in this PR): implement `copyPhysicalDisk` on `MultipathNVMeOFAdapterBase` so NVMe-TCP pools can also host system/VR root disks. Today the adapter deliberately throws `UnsupportedOperationException` for `copyPhysicalDisk` and callers are expected to keep ROOT volumes on a conventional NFS/file pool and place only DATA volumes on the NVMe-TCP pool.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4341110734
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5176667826
@DaanHoogland yes, ready from my side. Pushed `ca9ca209cc` which rebases onto current `main` and addresses the Copilot round:
- **AddPrimaryStorage.vue** — set/replace the `transport=` query parameter instead of blindly appending, so a URL already carrying `transport=` cannot end up with two conflicting values.
- **FlashArrayVolume** — EUI-128 now slices `serial.substring(14, 24)` rather than `substring(14)`; the previous form guarded short serials but a longer-than-expected one could yield an EUI over 32 hex chars.
- **FlashArrayAdapter** — the Fibre Channel connection match now guards `hostname.indexOf('.') > 0` before slicing, mirroring the NVMe-TCP branch (a dotless hostname previously threw).
- **MultipathNVMeOFAdapterBase** — pool cache is a `ConcurrentHashMap` via `computeIfAbsent`; `key=value` parsing uses `split("=", 2)`; and `rescanAllControllers()` is throttled to once per 10s during namespace discovery instead of firing on every 2s poll.
I pushed back on one item (replies inline): the placeholder `KVMStoragePool` methods on `MultipathNVMeOFPool`. That class is a dummy pool — this adapter dispatches per-volume and never uses the pool for connectivity/capacity/disk lifecycle — and the existing `MultipathSCSIPool` sibling returns the same `null`/`false`/`true` placeholders for the same methods. Switching only this class to `UnsupportedOperationException` would diverge from that precedent and could turn a currently harmless no-op into a hard failure in an agent path we have not exercised. Happy to change it if you would rather both classes move together.
The rebase picked up one conflict in `AddPrimaryStorage.vue` where `main` had added the NetApp ONTAP provider — resolved by keeping both. Verified `mvn -pl plugins/hypervisors/kvm,plugins/storage/volume/flasharray -am -DskipTests compile` clean, including checkstyle.
Re: testing — @slavkap confirmed StorPool is unaffected, and @rp- reported "works here" earlier. I have exercised the full FlashArray NVMe-TCP path on my lab (pool registration through the UI, VM deploy on an NVMe-TCP root disk, volume attach/detach, snapshot + revert with the correct EUI-128 `install_path`, and live migration between two KVM hosts).
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6387744724
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4306390407
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4351023989
@rp- @slavkap , can you test this one on your set-ups?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3710806062
##########
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 on `hostname.indexOf('.') > 0` before slicing, mirroring the NVMe-TCP branch above it. A hostname with no dot no longer throws `StringIndexOutOfBoundsException`.
##########
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:
Fixed in `ca9ca209cc` — now `part.split("=", 2)` so only the first `=` delimits key from value, and a value containing `=` is no longer silently discarded.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3785442823
##########
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:
Thanks @DaanHoogland — appreciated. Resolving these three threads then; the rest of the Copilot rounds are applied and already resolved.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6339973611
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19677
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4306381001
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4347984984
<b>[SF] Trillian test result (tid-15978)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 50803 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13061-t15978-kvm-ol8.zip
Smoke tests completed. 151 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3718753507
##########
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:
These placeholders are deliberate and match the in-tree sibling exactly. `MultipathNVMeOFPool` is modelled on `MultipathSCSIPool` (the FC/iSCSI pool that has shipped for several releases), and every method flagged here has a byte-identical body there:
| Method | `MultipathNVMeOFPool` | `MultipathSCSIPool` |
|---|---|---|
| `createPhysicalDisk` (both overloads) | `return null` | `return null` |
| `deletePhysicalDisk` | `return true` | `return true` |
| `listPhysicalDisks` | `return null` | `return null` |
| `refresh()` | `return false` | `return false` |
| `isExternalSnapshot()` | `return true` | `return true` |
| `getLocalPath()` | `return null` | `return null` |
| `createFolder()` | `return false` | `return false` |
On the specific concern that `refresh()` returning `false` is "incorrect behavior even when the pool is usable" — `MultipathSCSIPool.refresh()` also returns `false`, so if that were harmful it would already be affecting every Fibre Channel deployment. The methods that actually matter for this pool type (`connectPhysicalDisk`, `disconnectPhysicalDisk`, `getPhysicalDisk`) all delegate to the adaptor and are exercised end-to-end.
The reason these are no-ops rather than unimplemented is that this is a *dummy* pool object: the adapter dispatches per volume, and an NVMe-TCP namespace is not a host-local artifact the way a qcow2 file or an LV is. There is no pool-level disk to create, list, or delete on the host.
I'd rather not switch only this class to `CloudRuntimeException`/`UnsupportedOperationException`, because it would (a) diverge from the sibling that this class is intentionally symmetric with, and (b) convert currently-harmless no-ops into hard failures on paths I cannot exercise — note that all three in-tree callers of `deletePhysicalDisk` ignore the return value and catch only `CloudRuntimeException`, so throwing genuinely changes behaviour on volume-delete and backup-delete.
@DaanHoogland this is the one item from the Copilot rounds I have pushed back on rather than applied — flagging it explicitly since you asked me to resolve or apply. If the project would prefer the stricter contract I'm happy to do it, but I think it should land as a separate change covering `MultipathSCSIPool` and `MultipathNVMeOFPool` together rather than making the two inconsistent here.
##########
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:
Same finding as the thread above — see [my reply there](https://github.com/apache/cloudstack/pull/13061#discussion_r3715037519) for the method-by-method comparison with `MultipathSCSIPool` and the rationale.
##########
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;
+ }
+
+ @Override
+ public String getUuid() {
+ return uuid;
+ }
+
+ public void setCapacity(long capacity) { this.capacity = capacity; }
+ @Override public long getCapacity() { return this.capacity; }
+ public void setUsed(long used) { this.used = used; }
+ @Override public long getUsed() { return this.used; }
+ public void setAvailable(long available) { this.available = available; }
+ @Override public long getAvailable() { return this.available; }
+
+ @Override public boolean refresh() { return false; }
Review Comment:
Same finding as the thread above — see [my reply there](https://github.com/apache/cloudstack/pull/13061#discussion_r3715037519) for the method-by-method comparison with `MultipathSCSIPool` and the rationale.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5467823887
Gentle bump on this one — no rush if people are away, just want to make sure it hasn't fallen off the radar.
Current state, as far as I can tell nothing is outstanding from my side:
- All review threads resolved (the Copilot rounds are applied; the one item I pushed back on was [confirmed fine by @DaanHoogland](https://github.com/apache/cloudstack/pull/13061#discussion_r3715037519)).
- Approved by @slavkap on 27 Jul.
- `build` is green. The remaining red checks are `codecov/patch`, `codecov/project`, `Sonar JaCoCo Coverage` (this adds a plugin with no unit tests, same as the other FlashArray PRs that merged) and `test_mm_domain_limits` → *"Deploy VM with specified RAM & verify the usage"*, which is resource-usage accounting and unrelated to storage.
- No merge conflicts, though the branch is now ~80 commits behind `main`. I've deliberately left it alone rather than force-pushing, so as not to invalidate a regression run if one is in flight — happy to rebase the moment it's useful.
@vladimirpetrov @sureshanaparti — re @DaanHoogland's question above about regression testing: is there anything you'd like from me to move that along? I have a two-host KVM lab with a FlashArray behind it and I'm glad to run any specific suite and post the results, or to add unit tests for the pure logic (EUI-128 construction/parsing, volume-path parsing) if that would help with the coverage checks.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6338755832
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4302329496
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13061?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0.50251%` with `396 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 19.15%. Comparing base ([`3166e64`](https://app.codecov.io/gh/apache/cloudstack/commit/3166e64891fc75d4d32b66d874cff3f613b09b52?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c0cdfa4`](https://app.codecov.io/gh/apache/cloudstack/commit/c0cdfa41da668507b8b4437cf171beea5b68e896?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/cloudstack/pull/13061?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rvisor/kvm/storage/MultipathNVMeOFAdapterBase.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fhypervisors%2Fkvm%2Fsrc%2Fmain%2Fjava%2Fcom%2Fcloud%2Fhypervisor%2Fkvm%2Fstorage%2FMultipathNVMeOFAdapterBase.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9oeXBlcnZpc29ycy9rdm0vc3JjL21haW4vamF2YS9jb20vY2xvdWQvaHlwZXJ2aXNvci9rdm0vc3RvcmFnZS9NdWx0aXBhdGhOVk1lT0ZBZGFwdGVyQmFzZS5qYXZh) | 0.00% | [244 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ud/hypervisor/kvm/storage/MultipathNVMeOFPool.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fhypervisors%2Fkvm%2Fsrc%2Fmain%2Fjava%2Fcom%2Fcloud%2Fhypervisor%2Fkvm%2Fstorage%2FMultipathNVMeOFPool.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9oeXBlcnZpc29ycy9rdm0vc3JjL21haW4vamF2YS9jb20vY2xvdWQvaHlwZXJ2aXNvci9rdm0vc3RvcmFnZS9NdWx0aXBhdGhOVk1lT0ZQb29sLmphdmE=) | 0.00% | [79 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...atastore/adapter/flasharray/FlashArrayAdapter.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fstorage%2Fvolume%2Fflasharray%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcloudstack%2Fstorage%2Fdatastore%2Fadapter%2Fflasharray%2FFlashArrayAdapter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9zdG9yYWdlL3ZvbHVtZS9mbGFzaGFycmF5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jbG91ZHN0YWNrL3N0b3JhZ2UvZGF0YXN0b3JlL2FkYXB0ZXIvZmxhc2hhcnJheS9GbGFzaEFycmF5QWRhcHRlci5qYXZh) | 0.00% | [38 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...m/cloud/hypervisor/kvm/storage/NVMeTCPAdapter.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fhypervisors%2Fkvm%2Fsrc%2Fmain%2Fjava%2Fcom%2Fcloud%2Fhypervisor%2Fkvm%2Fstorage%2FNVMeTCPAdapter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9oeXBlcnZpc29ycy9rdm0vc3JjL21haW4vamF2YS9jb20vY2xvdWQvaHlwZXJ2aXNvci9rdm0vc3RvcmFnZS9OVk1lVENQQWRhcHRlci5qYXZh) | 0.00% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tore/lifecycle/AdaptiveDataStoreLifeCycleImpl.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fstorage%2Fvolume%2Fadaptive%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcloudstack%2Fstorage%2Fdatastore%2Flifecycle%2FAdaptiveDataStoreLifeCycleImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9zdG9yYWdlL3ZvbHVtZS9hZGFwdGl2ZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2xvdWRzdGFjay9zdG9yYWdlL2RhdGFzdG9yZS9saWZlY3ljbGUvQWRhcHRpdmVEYXRhU3RvcmVMaWZlQ3ljbGVJbXBsLmphdmE=) | 0.00% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ud/hypervisor/kvm/storage/KVMStorageProcessor.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fhypervisors%2Fkvm%2Fsrc%2Fmain%2Fjava%2Fcom%2Fcloud%2Fhypervisor%2Fkvm%2Fstorage%2FKVMStorageProcessor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9oeXBlcnZpc29ycy9rdm0vc3JjL21haW4vamF2YS9jb20vY2xvdWQvaHlwZXJ2aXNvci9rdm0vc3RvcmFnZS9LVk1TdG9yYWdlUHJvY2Vzc29yLmphdmE=) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...store/adapter/flasharray/FlashArrayConnection.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fstorage%2Fvolume%2Fflasharray%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcloudstack%2Fstorage%2Fdatastore%2Fadapter%2Fflasharray%2FFlashArrayConnection.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9zdG9yYWdlL3ZvbHVtZS9mbGFzaGFycmF5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jbG91ZHN0YWNrL3N0b3JhZ2UvZGF0YXN0b3JlL2FkYXB0ZXIvZmxhc2hhcnJheS9GbGFzaEFycmF5Q29ubmVjdGlvbi5qYXZh) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tack/storage/datastore/adapter/ProviderVolume.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fstorage%2Fvolume%2Fadaptive%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcloudstack%2Fstorage%2Fdatastore%2Fadapter%2FProviderVolume.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9zdG9yYWdlL3ZvbHVtZS9hZGFwdGl2ZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2xvdWRzdGFjay9zdG9yYWdlL2RhdGFzdG9yZS9hZGFwdGVyL1Byb3ZpZGVyVm9sdW1lLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...datastore/adapter/flasharray/FlashArrayVolume.java](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&filepath=plugins%2Fstorage%2Fvolume%2Fflasharray%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcloudstack%2Fstorage%2Fdatastore%2Fadapter%2Fflasharray%2FFlashArrayVolume.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9zdG9yYWdlL3ZvbHVtZS9mbGFzaGFycmF5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jbG91ZHN0YWNrL3N0b3JhZ2UvZGF0YXN0b3JlL2FkYXB0ZXIvZmxhc2hhcnJheS9GbGFzaEFycmF5Vm9sdW1lLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/13061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #13061 +/- ##
============================================
+ Coverage 18.01% 19.15% +1.13%
+ Complexity 16607 16603 -4
============================================
Files 6029 5568 -461
Lines 542160 502404 -39756
Branches 66451 58940 -7511
============================================
- Hits 97682 96245 -1437
+ Misses 433461 395337 -38124
+ Partials 11017 10822 -195
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13061/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13061/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13061/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `19.15% <0.50%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/13061?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3128670648
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/NVMeTCPAdapter.java:
##########
@@ -0,0 +1,51 @@
+// 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 com.cloud.storage.Storage;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+/**
+ * StorageAdaptor for the {@link Storage.StoragePoolType#NVMeTCP} pool type.
+ * All operational logic lives in {@link MultipathNVMeOFAdapterBase}; this
+ * class just binds that logic to a pool type so
+ * {@link KVMStoragePoolManager} can find it via reflection.
+ */
+public class NVMeTCPAdapter extends MultipathNVMeOFAdapterBase {
+ private static final Logger LOGGER_NVMETCP = LogManager.getLogger(NVMeTCPAdapter.class);
Review Comment:
```suggestion
private static final Logger LOGGER = LogManager.getLogger(NVMeTCPAdapter.class);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4700228395
Thanks @slavkap — great catch. The interface drift was real: between the last rebase of this branch (05-22) and main today, `KVMStoragePool` had two method renames:
- `checkingHeartBeat(HAStoragePool, HostTO)` → `hasHeartBeat(HAStoragePool, HostTO)`
- `vmActivityCheck(...)` → `hasVmActivity(...)` (same signature)
Rebased the branch onto current `upstream/main` and pushed `93eb9d94e1 kvm: align MultipathNVMeOFPool with KVMStoragePool API change`, which renames the two `@Override` methods on `MultipathNVMeOFPool` to match (mirrors how `MultipathSCSIPool` implements them). Both still return `null` since `MultipathNVMeOFPool` doesn't participate in KVM-side HA heartbeat.
Verified locally with `mvn -pl plugins/hypervisors/kvm -am -DskipTests compile`. Should be clean for you on the next pull — happy to repackage if useful.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6374239790
<b>[SF] Trillian test result (tid-17214)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 107447 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13061-t17214-kvm-ol8.zip
Smoke tests completed. 142 look OK, 15 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_CRUD_operations_userdata | `Error` | 1523.96 | test_register_userdata.py
test_deploy_vm_with_registered_userdata | `Error` | 7.06 | test_register_userdata.py
test_deploy_vm_with_registered_userdata_with_override_policy_allow | `Error` | 6.66 | test_register_userdata.py
test_deploy_vm_with_registered_userdata_with_override_policy_append | `Error` | 6.55 | test_register_userdata.py
test_deploy_vm_with_registered_userdata_with_override_policy_deny | `Error` | 6.58 | test_register_userdata.py
test_deploy_vm_with_registered_userdata_with_params | `Error` | 6.61 | test_register_userdata.py
test_link_and_unlink_userdata_to_template | `Error` | 6.57 | test_register_userdata.py
test_user_userdata_crud | `Error` | 8.78 | test_register_userdata.py
ContextSuite context=TestResetVmOnReboot>:setup | `Error` | 0.00 | test_reset_vm_on_reboot.py
ContextSuite context=TestRAMCPUResourceAccounting>:setup | `Error` | 0.00 | test_resource_accounting.py
test_03_register_template | `Error` | 1.15 | test_resource_names.py
ContextSuite context=TestRestoreVM>:setup | `Error` | 0.00 | test_restore_vm.py
ContextSuite context=TestRouterDHCPHosts>:setup | `Error` | 0.00 | test_router_dhcphosts.py
ContextSuite context=TestRouterDHCPOpts>:setup | `Error` | 0.00 | test_router_dhcphosts.py
ContextSuite context=TestRouterDns>:setup | `Error` | 0.00 | test_router_dns.py
ContextSuite context=TestRouterDnsService>:setup | `Error` | 0.00 | test_router_dnsservice.py
ContextSuite context=TestSslOffloading>:setup | `Error` | 0.00 | test_ssl_offloading.py
ContextSuite context=TestRouterIpTablesPolicies>:setup | `Error` | 0.00 | test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setup | `Error` | 0.00 | test_routers_iptables_default_policy.py
ContextSuite context=TestIsolatedNetworks>:setup | `Error` | 0.00 | test_routers_network_ops.py
ContextSuite context=TestRedundantIsolateNetworks>:setup | `Error` | 0.00 | test_routers_network_ops.py
ContextSuite context=TestRouterServices>:setup | `Error` | 0.00 | test_routers.py
ContextSuite context=TestCpuCapServiceOfferings>:setup | `Error` | 0.00 | test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setup | `Error` | 0.52 | test_service_offerings.py
ContextSuite context=TestSetSourceNatIp>:setup | `Error` | 0.00 | test_set_sourcenat.py
ContextSuite context=TestSnapshotRootDisk>:setup | `Error` | 0.00 | test_snapshots.py
ContextSuite context=TestSnapshotStandaloneBackup>:setup | `Error` | 0.00 | test_snapshots.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6287536461
> Thanks @DaanHoogland. @abh1sar, anything else you need from me on this one?
no.
@abh1sar is this alright by you?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3528308974
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/NVMeTCPAdapter.java:
##########
@@ -0,0 +1,51 @@
+// 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 com.cloud.storage.Storage;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+/**
+ * StorageAdaptor for the {@link Storage.StoragePoolType#NVMeTCP} pool type.
+ * All operational logic lives in {@link MultipathNVMeOFAdapterBase}; this
+ * class just binds that logic to a pool type so
+ * {@link KVMStoragePoolManager} can find it via reflection.
+ */
+public class NVMeTCPAdapter extends MultipathNVMeOFAdapterBase {
+ private static final Logger LOGGER_NVMETCP = LogManager.getLogger(NVMeTCPAdapter.class);
Review Comment:
Fixed in `71b494c897` — renamed to `LOGGER`.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:
##########
@@ -0,0 +1,448 @@
+// 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) {
+ 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)) {
+ waitSecs = Integer.parseInt(waitTime);
Review Comment:
Fixed in `71b494c897` — `Integer.parseInt(waitTime)` is now guarded; a non-integer STORAGE_POOL_DISK_WAIT logs a warning and falls back to DEFAULT_DISK_WAIT instead of aborting the connect.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -140,22 +144,36 @@ public String attach(ProviderAdapterContext context, ProviderAdapterDataObject d
String volumeName = normalizeName(pod, dataObject.getExternalName());
try {
FlashArrayList<FlashArrayConnection> list = null;
- FlashArrayHost host = getHost(hostname);
- if (host != null) {
- list = POST("/connections?host_names=" + host.getName() + "&volume_names=" + volumeName, null,
+ if (AddressType.NVMETCP.equals(volumeAddressType) && hostgroup != null) {
+ // NVMe-TCP pod volumes are connected at the host-group level so the
+ // array assigns a consistent NSID visible to every member host.
+ list = POST("/connections?host_group_names=" + hostgroup + "&volume_names=" + volumeName, null,
new TypeReference<FlashArrayList<FlashArrayConnection>>() {
});
+ } else {
+ FlashArrayHost host = getHost(hostname);
+ if (host != null) {
+ list = POST("/connections?host_names=" + host.getName() + "&volume_names=" + volumeName, null,
+ new TypeReference<FlashArrayList<FlashArrayConnection>>() {
+ });
+ }
}
if (list == null || list.getItems() == null || list.getItems().size() == 0) {
throw new RuntimeException("Volume attach did not return lun information");
}
Review Comment:
Fixed in `71b494c897` — reworded to transport-agnostic "did not return connection information (lun/nsid)".
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayVolume.java:
##########
@@ -107,6 +111,13 @@ 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>
Review Comment:
Fixed in `71b494c897` — `FlashArrayVolume` now validates `serial.length() == 24` before slicing and fails fast with a clear message.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -167,13 +185,18 @@ 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)) {
+ if (conn.getHostGroup() != null && conn.getHostGroup().getName() != null
+ && conn.getHostGroup().getName().equals(hostgroup)) {
+ 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) {
Review Comment:
Fixed in `71b494c897` — the retry lookup now checks both host-group and host-scoped connections so `transport=nvme-tcp` without `hostgroup` gets a matching retry.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:
##########
@@ -0,0 +1,448 @@
+// 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) {
+ 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)) {
+ waitSecs = Integer.parseInt(waitTime);
+ }
+ }
+ 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) {
+ Thread.currentThread().interrupt();
+ return false;
+ }
+ }
+ LOGGER.debug("NVMe namespace did not appear at " + address.getPath() + " within " + waitSecs + "s");
+ return false;
+ }
+
+ private void rescanAllControllers() {
+ try {
+ File sysClass = new File("/sys/class/nvme");
+ File[] ctrls = sysClass.listFiles();
+ if (ctrls == null) {
+ return;
+ }
+ for (File ctrl : ctrls) {
+ Process p = new ProcessBuilder("nvme", "ns-rescan", "/dev/" + ctrl.getName())
+ .redirectErrorStream(true).start();
+ p.waitFor(NS_RESCAN_TIMEOUT_SECS, TimeUnit.SECONDS);
+ }
Review Comment:
Fixed in `71b494c897` — `rescanAllControllers()` now checks the `waitFor()` return, calls `destroyForcibly()` on timeout, and logs a warning so hung `nvme ns-rescan` processes cannot accumulate.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -781,6 +816,13 @@ private FlashArrayVolume getSnapshot(String snapshotName) {
return (FlashArrayVolume) getFlashArrayItem(list);
Review Comment:
Fixed in `71b494c897` — both `getSnapshot()` overloads and the `snapshot()` creator all apply `withAddressType(...)` now. Verified end-to-end: snapshot on an NVMe-TCP pool now persists the EUI-128 (`006c1b16ce1c034d24a9371c05ab334a`) instead of the FC WWN default.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "slavkap (via GitHub)" <gi...@apache.org>.
slavkap commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4656872932
everything works with StorPool, there is only a problem with the build
```
[ERROR] /Users/slavkapeleva/Projects/cloudstack-main/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFPool.java:[39,7] error: MultipathNVMeOFPool is not abstract and does not override abstract method hasVmActivity(HAStoragePool,HostTO,Duration,String,String,long) in KVMStoragePool
[ERROR] /Users/slavkapeleva/Projects/cloudstack-main/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFPool.java:[149,4] error: method does not override or implement a method from a supertype
[ERROR] /Users/slavkapeleva/Projects/cloudstack-main/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFPool.java:[152,4] error: method does not override or implement a method from a supertype
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4381533612
> @rp- @slavkap , can you test this one on your set-ups?
works here
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3715037519
##########
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 required `KVMStoragePool` methods currently return `null`/`false` unconditionally. This can cause immediate NPEs (e.g., callers expecting a `KVMPhysicalDisk`), or incorrect behavior (e.g., a failed `refresh()` even when the pool is usable). Prefer implementing these methods (even if delegating to the adaptor), or fail deterministically with a well-explained runtime exception (e.g., `CloudRuntimeException`) rather than returning `null`/`false`.
##########
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 required `KVMStoragePool` methods currently return `null`/`false` unconditionally. This can cause immediate NPEs (e.g., callers expecting a `KVMPhysicalDisk`), or incorrect behavior (e.g., a failed `refresh()` even when the pool is usable). Prefer implementing these methods (even if delegating to the adaptor), or fail deterministically with a well-explained runtime exception (e.g., `CloudRuntimeException`) rather than returning `null`/`false`.
##########
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;
+ }
+
+ @Override
+ public String getUuid() {
+ return uuid;
+ }
+
+ public void setCapacity(long capacity) { this.capacity = capacity; }
+ @Override public long getCapacity() { return this.capacity; }
+ public void setUsed(long used) { this.used = used; }
+ @Override public long getUsed() { return this.used; }
+ public void setAvailable(long available) { this.available = available; }
+ @Override public long getAvailable() { return this.available; }
+
+ @Override public boolean refresh() { return false; }
Review Comment:
Several required `KVMStoragePool` methods currently return `null`/`false` unconditionally. This can cause immediate NPEs (e.g., callers expecting a `KVMPhysicalDisk`), or incorrect behavior (e.g., a failed `refresh()` even when the pool is usable). Prefer implementing these methods (even if delegating to the adaptor), or fail deterministically with a well-explained runtime exception (e.g., `CloudRuntimeException`) rather than returning `null`/`false`.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6338734681
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6163898936
Thanks @DaanHoogland. @abh1sar, anything else you need from me on this one?
On the red checks, in case they are what is holding it up: the two integration suites
did not actually fail a test. Both were **cancelled at the 6 hour job limit during
`Setup Environment`** - every later step, including `Run Integration Tests with
Simulator`, is marked `skipped`, so no test ran. The other twelve suites, covering the
same areas, passed. A re-run of just those two should clear it, and I am happy for
someone to kick that off.
`codecov/patch` and `codecov/project` are red too, but those also failed on #13060 in
this same series and it merged, so I assume they are not a gate.
Nothing has changed on the branch since the test matrix above, and I would rather not
push anything now and reset CI again unless you want a change.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6387765317
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4341471833
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17643
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4341707575
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5860682798
Thanks @abh1sar. I worked through all seven. Summary first, detail below.
| # | Test | Result |
|---|------|--------|
| 1 | Snapshot attached NVMe volume, then createVolume from it | Pass (**found and fixed a bug**) |
| 2 | Resize attached volume, VM running | Pass (**needed two fixes**) |
| 3 | Same resize with VM stopped, then start | Pass |
| 4 | createSnapshot with backup to secondary storage | Works as supported; `locationType=SECONDARY` is **not supported on KVM at all** - see below |
| 5 | createTemplate from NVMe volume, deploy VM from it onto the NVMe pool | Pass (**needed fixes**) |
| 6 | migrateVolume to a second NVMe-TCP pool, and to NFS | Pass (two pre-existing constraints worth knowing) |
| 7 | Live-migrate under continuous verified fio load | Pass (**found and fixed a real bug**) |
Environment: two KVM hosts (Rocky 9.7) plus a separate management server, on `24.0.0-SNAPSHOT` built
from this branch, FlashArray X90 over NVMe-TCP, plus an NFS pool and a second NVMe-TCP pool for
test 6.
Your invitation to flag things that are not expected to work turned out to matter for test 4 and for
parts of test 6, and running the matrix surfaced four code defects. Those are now fixed on the branch,
along with direct-download template support, which the test 5 audit showed was missing entirely.
---
### 1. Snapshot + createVolume from snapshot - pass, and it exposed a bug
Both halves work: `createSnapshot` on an attached NVMe-TCP data volume of a running VM reaches
`BackedUp`, and `createVolume` from that snapshot lands `Ready` on the same pool, attaches to a
running VM, and shows up on the host as its EUI-128 udev symlink.
While auditing this path I found that `FlashArrayAdapter.copy()` and `revert()` returned a volume
whose address type had never been stamped. `FlashArrayVolume`'s constructor defaults `addressType`
to `FIBERWWN`, so on an NVMe-TCP pool those two paths emitted an **FC-style NAA WWN** instead of an
EUI-128. `copy()` is the damaging one, because `AdaptiveDataStoreDriverImpl.copyAsync()` persists
`generatePathInfo(outVolume, ...)` - so the address was written to the database and the host could
never resolve the namespace.
This was not hypothetical; it was already in my lab database from an earlier run. Before and after
the fix, same pool:
```
t1-from-snap type=FIBERWWN; address=624a93706c1b16ce1c034d1c06221ea1 <- old code
t2-from-snap type=NVMETCP; address=006c1b16ce1c034d24a9371c0622dee2 <- fixed
```
Rather than add a third `withAddressType()` call site, the fix stamps inside the private
`getVolume()` accessor so every path is correct by construction, matching the private
`getSnapshot()` accessor which already did this. No behavioural change for Fibre Channel, since
`FIBERWWN` is what the constructor already defaulted to.
### 2. Online resize - pass, needed two fixes
This originally failed at the API layer before any host-side work was attempted:
`VolumeApiServiceImpl.validateVolumeResizeWithSize()` refuses to resize a managed volume on a
running VM unless the pool type is allowlisted, and that list held PowerFlex and FiberChannel but
not NVMeTCP. Second, the KVM side had no resize implementation for NVMe-oF at all.
Both are fixed. Verified end to end - all four layers agree:
| Layer | Value |
|---|---|
| CloudStack | 40 G |
| FlashArray `provisioned` | 42949672960 |
| Host `blockdev --getsize64` | 42949672960 |
| Guest (qemu `Capacity`) | 42949672960 |
### 3. Offline resize then start - pass
40 G -> 50 G with the VM stopped, then started cleanly. Array, host and guest all report
53687091200, and the guest sees `/dev/vdb` as 50 G. The filesystem itself still needs the usual
in-guest `resize2fs`, which is expected.
### 4. Snapshot with backup to secondary - needs flagging
The supported form works: `createSnapshot` on an NVMe-TCP volume succeeds and reaches `BackedUp`,
held array-native on primary. That is by design for managed storage -
`StorageSystemSnapshotStrategy.backupSnapshot()` marks the snapshot backed up and returns as soon
as `locationType != SECONDARY`.
Asking explicitly for `locationType=SECONDARY` fails, but **not** for any NVMe-TCP reason:
```java
if (snapshotInfo.getLocationType() == Snapshot.LocationType.SECONDARY && volumeInfo.getFormat() != ImageFormat.VHD) {
throw new CloudRuntimeException("Only the '" + ImageFormat.VHD + "' image type can be used when 'LocationType' is set to 'SECONDARY'.");
}
```
`StorageSystemSnapshotStrategy.verifyLocationType()` gates purely on `format != VHD`, with no pool
type or transport condition. VHD means XenServer, so every KVM managed-storage volume is rejected
here - Fibre Channel and PowerFlex included. So I would call this out of scope for this PR rather
than an NVMe-TCP gap. Happy to be told otherwise if you think it should be widened.
### 5. createTemplate and deploy from it - pass, needed fixes
`createTemplate` from an NVMe-TCP volume produced a `Download Complete` template, and a VM deployed
from it came up `Running` with its ROOT volume on the NVMe-TCP pool carrying a correct EUI-128, which
the host resolved normally.
This needed the managed-storage allowlists in `StorageSystemDataMotionStrategy`. When Fibre Channel
support was added (#7889), `FiberChannel` was added to a number of allowlists in shared
managed-storage code; the NVMe-TCP work added the pool type, adaptor and lifecycle pivot but not
those. The gaps were:
* `verifyFormatWithPoolType()` - RAW was accepted only on PowerFlex and FiberChannel, so any
data-motion of a RAW NVMe-TCP volume was rejected outright.
* `handleCreateTemplateFromManagedVolume()` - same format check, which made createTemplate fail
with the misleading "you can only create a template from a volume on KVM currently".
* the `grantAccess()`/`revokeAccess()` pair around the template copy only ran for a detached volume
or for PowerFlex/FiberChannel, so an attached NVMe-TCP volume was never granted host access.
* `getSnapshotDetails()` - the snapshot path is the device address for array-native snapshots.
I found these as a family by grepping every `StoragePoolType.FiberChannel` reference rather than one
failing test at a time. `ApiDBUtils.getHypervisorTypeFromZone()` needed NVMeTCP too, since it is a
KVM-only pool type. I deliberately left `StatsCollector`'s volume-stats format check alone: RAW is
already in its accepted list, so adding the pool type there would be dead weight.
### 6. migrateVolume - pass, with two pre-existing constraints
Both directions work:
* NVMe-TCP pool -> second NVMe-TCP pool: 7 s, new volume carries a fresh EUI-128, old row `Expunged`.
* NVMe-TCP pool -> NFS: 32 s, and the migrated volume correctly switches from an EUI-128 address to
an NFS file path.
Two things blocked this before I got it working, both pre-existing and transport-agnostic, worth
knowing if you reproduce:
* **Storage tags must match.** My second NVMe-TCP pool initially had no tags while the volume's disk
offering required `nvme`, so the allocator refused it. Lab configuration, not code.
* **A volume with snapshots that exist only on primary storage cannot be migrated.**
`SnapshotHelper.checkKvmVolumeSnapshotsOnlyInPrimaryStorage()` gates on `HypervisorType.KVM` with
no pool-type condition. Since managed-storage snapshots are array-native and therefore always
primary-only, this means any snapshotted managed volume is unmigratable on KVM - Fibre Channel
included. I tested with a snapshot-free volume. This one might deserve its own discussion, but it
is not specific to this PR.
### 7. Live migration under load - pass, after fixing a real bug
This one genuinely failed, and thank you for asking for it, because the cause was a design
inconsistency rather than a small oversight.
CloudStack grants and revokes managed-storage access **one host at a time**:
`AdaptiveDataStoreDriverImpl.grantAccess()` calls `attach(volume, hostname)` and `revokeAccess()`
calls `detach(volume, hostname)`. But `attach()` created a **host-group scoped** connection when
`hostgroup=` was configured, while `detach()` still ran per host and deleted that shared connection.
So:
```
grantAccess(V, dst) -> POST /connections?host_group_names=cluster1 ("already exists")
... guest migrates, now running on dst, doing I/O to V ...
revokeAccess(V, src) -> DELETE /connections?host_group_names=cluster1
```
The DELETE removes the only connection there is, so the volume is revoked from the whole group
including the destination. The namespace disappears under the running guest and, with libvirt's
`werror=stop`, the VM freezes while CloudStack still reports it Running. On the array both member
hosts show the *same* nsid, which is the giveaway that it is one shared connection rather than two
per-host ones.
Group scoping was justified in a comment as giving "a consistent NSID visible to every member host",
but the NSID is not what locates the namespace - the EUI-128 address is, as the same method says
twenty lines further down. So the justification did not hold.
Worth noting there is no way to fix this in `detach()`: it receives only a context carrying
domain/zone/account, a volume identity and a host name - no VM and no reason code - so "revoke
because the VM migrated off" and "revoke because the VM stopped" are the same call with the same
arguments. Keeping the volume connected cluster-wide for the VM's lifetime is therefore not
expressible at this layer; it would degrade to never detaching. Per-host maps one-to-one onto
grantAccess/revokeAccess, which is exactly what Fibre Channel has always done in this same adaptor.
So NVMe-TCP now connects and disconnects per host. `hostgroup=` remains an accepted pool parameter
and is used only where the field's own comment always said it would be - removing leftover group
connections when a volume is deleted - plus a demoted fallback in the "Connection already exists"
branch so volumes carrying a group connection from an earlier release still resolve.
Verified with your scenario exactly: continuous `fio` randwrite load in the guest, live-migrated
mid-run, then left running a further five minutes on the destination.
```
fio --name=migtest --filename=/mnt/pgdata/fio.test --size=2G --rw=randwrite --bs=64k \
--direct=1 --verify=crc32c --verify_backlog=128 --do_verify=1 \
--time_based --runtime=600 --ioengine=libaio --iodepth=8
```
fio's own summary for the whole run, which spans the migration:
```
migtest: (groupid=0, jobs=1): err= 0: pid=1481
write: IOPS=3770, BW=236MiB/s (247MB/s)(138GiB/600000msec); 0 zone resets
```
* **`err= 0` across the full 600 s - 138 GiB written with crc32c verification, no failures**
* migration completed in 7 s under load; CloudStack reported `Running` on the destination
* array connection moved host-scoped, one entry per volume, `hostgroup` null
* destination domain `running` throughout, never paused; `cpu.time` and `block.1.wr.reqs` both
advancing (657 k write requests observed post-migration)
* ~73 GiB of that was written during the five-minute post-migration soak, with zero guest
`dmesg` I/O errors and the filesystem writable throughout
* source host released both namespaces
For the avoidance of doubt about the destination's qemu log: it does contain seven
`Remote I/O error` lines, but they are all timestamped from the pre-fix run the day before. Counting
only from this boot onwards gives zero, so the same file happens to hold a clean before/after.
---
### Also addressed in this round: direct download templates now work
Auditing test 5 turned up that `createTemplateFromDirectDownloadFile()` was unimplemented, so a
template registered with `directdownload=true` downloaded fine and then failed on the host at
deployment. That is now implemented and tested.
The thing that makes it look harder than it is: the two path arguments are different kinds of thing.
`templateFilePath` is a plain local file from the direct-download helper, while `destTemplatePath` is
a managed volume path (`type=NVMETCP;address=...`). Only the destination may go through
`KVMStoragePool.getPhysicalDisk()`. `KVMStorageProcessor` has already called `connectPhysicalDisk()`
for the destination by then, so the namespace is present.
Worth flagging that the **Fibre Channel implementation conflates them**, and I do not think it can
work as written. It calls `destPool.getPhysicalDisk(templateFilePath)`, and
`FiberChannelAdapter.parseAndValidatePath()` treats any string without a `;` as a bare WWN:
```java
if (parts.length == 1) { type = "FIBERWWN"; address = parts[0]; }
...
path = "/dev/mapper/3" + address;
```
so a local file path becomes `/dev/mapper/3/var/lib/libvirt/...` and the lookup cannot succeed. I
have not tried to reproduce that on FC hardware, so I am only reporting what the code does - but you
may want a separate look at it. This implementation follows `ScaleIOStorageAdaptor` instead, which is
the working precedent for block-backed managed storage.
The template is written as QCOW2 onto the raw namespace rather than as RAW, for three reasons: it
matches ScaleIO; it agrees with the QCOW2 format the template is registered with; and
`KVMStorageProcessor` runs `Qcow2Inspector.validateQcow2File()` on the path we return, which for an
existing block device would fail on RAW content and then try to delete the device. Compressed
templates are handled via `TemplateDownloaderUtil`, and a template that will not fit the namespace is
refused up front rather than part way through the convert.
Verified end to end: QCOW2 template registered with `directdownload=true`, VM deployed with a storage
offering tagged for the NVMe-TCP pool, guest booted from the namespace.
```
ROOT-28 Everpure X90 NVMe-TCP primary NVMeTCP Ready
type=NVMETCP; address=006c1b16ce1c034d24a9371c06232755
i-2-28-VM hda -> /dev/disk/by-id/nvme-eui.006c1b16ce1c034d24a9371c06232755
```
### An unrelated bug found while testing - flagging rather than fixing in this PR
Cleaning up test templates afterwards surfaced a separate pre-existing bug that is **not specific
to NVMe-TCP and affects Fibre Channel identically**, so I have deliberately left it out of this PR
rather than widen the scope. Flagging it in case someone wants it tracked.
`deleteTemplate` on an adaptive managed pool returns `{"success": true}` and removes the
`vm_template` and `template_spool_ref` rows, but **leaves the volume live on the array**. It is a
silent orphan, and `storage.cleanup` cannot recover it because CloudStack no longer has a record of
the volume.
The cause is ordering. `HypervisorTemplateAdapter.delete()` soft-removes the template before
evicting it from primary storage:
```java
_tmpltDao.remove(template.getId());
...
templateMgr.evictTemplateFromStoragePoolsForZones(template.getId(), profile.getZoneIdList());
```
and the eviction path reaches `AdaptiveDataStoreDriverImpl.newManagedVolumeContext()`:
```java
} else if (obj instanceof TemplateInfo) {
VMTemplateVO template = _vmTemplateDao.findById(obj.getId());
ctx.setAccountId(template.getAccountId()); // NPE
```
`GenericDaoBase.findById()` excludes soft-removed rows, so this returns null and throws. Stack:
```
newManagedVolumeContext:919 <- deleteAsync:298 <- TemplateServiceImpl.deleteTemplateOnPrimary:1583
<- TemplateManagerImpl.evictTemplateFromStoragePool:1210 <- HypervisorTemplateAdapter.delete:586
```
`findByIdIncludingRemoved()` there, or a null guard that skips rather than throws, would likely be
enough. Happy to open a separate PR for it if you agree that is the right shape - it touches the
adaptive framework rather than this transport, which is why I have not bundled it.
### Minor change: stub parity with the Fibre Channel adaptor
I also aligned the NVMe-oF adaptor's unimplemented stubs with the Fibre Channel adaptor, which threw
`UnsupportedOperationException` where FC returns a benign value (`deletePhysicalDisk`,
`createTemplateFromDisk`, `listPhysicalDisks`, `createFolder`). A caller that degrades gracefully on
FC would previously abort on NVMe-TCP. The three stubs that FC also throws from still throw.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5620310328
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4890370073
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5175472972
@sureshanaparti , any blocker on this one?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4305048423
Pushed `723bf1445f kvm/flasharray: address review feedback on NVMe-TCP PR` to address this round of feedback:
### Code fixes
- **`NVMeTCPAdapter` logger rename** — `LOGGER_NVMETCP` → `LOGGER` to match the project convention (@sureshanaparti).
- **`rescanAllControllers()` now honours `waitFor(...)`** and `destroyForcibly()`s the `nvme ns-rescan` process on timeout, so hung rescans cannot accumulate under load (@Copilot).
- **`Integer.parseInt(waitTime)` is now guarded** in `connectPhysicalDisk()` — a non-integer `STORAGE_POOL_DISK_WAIT` detail no longer aborts the connect; we log a warning and fall back to `DEFAULT_DISK_WAIT` (@Copilot).
- **Retry path recognises host-scoped NVMe-TCP connections** — the "Connection already exists" fallback was only matching host-group connections, so a `transport=nvme-tcp` pool without `hostgroup` would never hit its retry branch. Now checks both (@Copilot).
- **Attach-failure error message is transport-agnostic** — the message used to say "did not return a LUN"; under NVMe-TCP we return an NSID. Reworded to "did not return connection information (lun/nsid)" so NVMe-TCP debugging isn't misleading (@Copilot).
- **EUI-128 length validation in `getVolumeByAddress()`** — NVMe-TCP path no longer blind-slices `substring(2,16)/substring(22)`; an address that isn't a 32-hex EUI-128 is now rejected explicitly rather than surfacing as a mislabelled "not found" via `StringIndexOutOfBoundsException` (@Copilot).
- **`FlashArrayVolume` serial length check** — NGUID construction now validates `serial.length() == 24` before slicing, and fails fast with a clear message instead of throwing `StringIndexOutOfBoundsException` deep in the address getter (@Copilot).
- **`getSnapshot()` now applies `withAddressType(...)`** so the returned `FlashArrayVolume` carries the pool's transport type (NVMe-TCP vs FC) — without this, `ProviderSnapshot.getAddress()` on an NVMe-TCP pool emitted an FC-style WWN instead of the EUI-128, and adaptive's `takeSnapshot` / `revertSnapshot` paths persisted the wrong address (@Copilot).
### Extra fix in the same area
While wiring `withAddressType(...)` into `getSnapshot()` I noticed the same pattern was **also missing in `snapshot()`** (the method that actually *creates* the snapshot), and in the other `getSnapshot()` overload. Both now apply `withAddressType(...)` as well, so the address stored at snapshot-create time is already the NVMe EUI-128 on an NVMe-TCP pool rather than the FC-WWN default set by `FlashArrayVolume`'s constructor.
Verified end-to-end on an NVMe-TCP pool: snapshotting a volume now persists `install_path=006c1b16ce1c034d24a9371c05ab334a` (NVMe EUI-128) rather than the previous `624a93706c1b16ce1c034d1c05ab3346` (FC WWN), and subsequent `revertSnapshot` uses the correct transport-specific address.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4299811970
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4299805074
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5100650415
@genegr is this ready?
Can you address (resolve or apply) the co-pilot comments?
@slavkap, did/will you test?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5600778994
Hi @genegr
Please check if the below tests are passing (Please flag if some of these are not expected to work or are not supported):
1. Take a volume snapshot of an attached NVMe-TCP data volume, then createVolume from that snapshot on the same pool.
2. Resize an attached NVMe volume to a bigger size on a running VM.
3. Same resize with the VM Stopped, then start the VM.
4. createSnapshot on an NVMe volume with backup to secondary storage.
5. createTemplate from an NVMe-TCP volume, then deploy a VM from that template onto the NVMe pool.
6. migrateVolume of an attached NVMe volume to a second NVMe-TCP pool, and to an NFS pool.
7. VM with an NVMe data volume on kvm01, continuous dd/fio write loop inside the guest. Live-migrate to kvm02. Run I/O on kvm02 for a further 5 minutes. Guest I/O should not error out after migration.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez merged PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6340127412
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6340168061
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4306791218
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17589
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4341711873
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3712514453
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayVolume.java:
##########
@@ -107,6 +111,22 @@ 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 24 hex characters, got "
+ + serial.length() + ")");
+ }
+ // Slice exact ranges rather than substring(14) so a serial with unexpected trailing
+ // characters cannot produce an EUI longer than 32 hex chars (which would not match
+ // /dev/disk/by-id/nvme-eui.<eui> on Linux).
+ return ("00" + serial.substring(0, 14) + PURE_OUI_EUI + serial.substring(14, 24)).toLowerCase();
Review Comment:
Good catch — you're right that my previous change traded one problem for another. Rejecting only `length() < 24` while slicing `serial[0:24]` meant two distinct serials sharing a 24-character prefix would collapse onto the same EUI-128, which is a volume-identity bug rather than just a cosmetic one.
Fixed in `43a15b5857`: the serial must now match `[0-9a-fA-F]{24}` exactly, so over-long and non-hex serials are both rejected up front.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -307,14 +345,24 @@ public ProviderVolume getVolumeByAddress(ProviderAdapterContext context, Address
throw new RuntimeException("Invalid search criteria provided for getVolumeByAddress");
}
- // only support WWN type addresses at this time.
- if (!ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ String serial;
+ if (ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ // Strip the NAA prefix (1 char) + Pure OUI to recover the volume serial.
+ serial = address.substring(FlashArrayVolume.PURE_OUI.length() + 1).toUpperCase();
+ } else if (ProviderVolume.AddressType.NVMETCP.equals(addressType)) {
+ // Reverse the EUI-128 layout: serial = eui[2:16] + eui[22:32], after
+ // stripping the optional "eui." prefix that appears in udev paths.
+ String eui = address.startsWith("eui.") ? address.substring(4) : address;
+ if (eui == null || eui.length() != 32) {
+ throw new RuntimeException("Invalid NVMe-TCP EUI-128 address ["
+ + address + "]: expected 32 hex characters, got "
+ + (eui == null ? "null" : String.valueOf(eui.length())));
+ }
+ serial = (eui.substring(2, 16) + eui.substring(22)).toUpperCase();
Review Comment:
Fixed in `43a15b5857`. `getVolumeByAddress` now validates the full FlashArray EUI-128 layout before reversing it into a serial:
- exactly 32 hexadecimal characters (was: any 32-character string)
- a `00` prefix
- the Pure Storage OUI (`24a937`) at offset 16, which is where `getAddress()` places it
Sanity-checked the offsets against a real namespace from my lab: `006c1b16ce1c034d24a9371c05ab334a` passes all three checks and round-trips to serial `6C1B16CE1C034D1C05AB334A` and back unchanged.
##########
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;
+ }
Review Comment:
I'd like to keep this one as `return true`, for two reasons.
**It matches the sibling adapter.** `MultipathSCSIPool.deletePhysicalDisk` (the FC/iSCSI class this one is modelled on) is byte-identical — `return true;` — as is its `listPhysicalDisks() { return null; }`. Changing only the NVMe class would leave the two out of step.
**Nothing is actually leaked here.** For managed storage the namespace is deleted on the *provider* side: the management server drives `FlashArrayAdapter.delete()`, which is what issues the destroy against the array. The KVM agent's pool-level `deletePhysicalDisk` exists for hypervisor-local disk *files* (a qcow2 on NFS, an LV on CLVM); an NVMe-TCP namespace is not a host-local object, so there is genuinely nothing for this method to do. It isn't "reporting success for work it skipped" so much as "no host-side artifact to remove".
Worth noting too that all three in-tree callers ignore the return value — `KVMStorageProcessor.deleteVolume` (twice) and `deleteBackup` call it for side effects only and catch just `CloudRuntimeException`. So `true` vs `false` changes nothing observable, while throwing `UnsupportedOperationException` *would* change behaviour: it would turn a harmless no-op into a hard failure on the volume-delete and backup-delete paths.
Happy to switch both classes together if a maintainer would rather have the stricter contract — I just don't want to introduce a new failure mode in this PR for a path I can't exercise.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3712514453
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayVolume.java:
##########
@@ -107,6 +111,22 @@ 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 24 hex characters, got "
+ + serial.length() + ")");
+ }
+ // Slice exact ranges rather than substring(14) so a serial with unexpected trailing
+ // characters cannot produce an EUI longer than 32 hex chars (which would not match
+ // /dev/disk/by-id/nvme-eui.<eui> on Linux).
+ return ("00" + serial.substring(0, 14) + PURE_OUI_EUI + serial.substring(14, 24)).toLowerCase();
Review Comment:
Good catch — you're right that my previous change traded one problem for another. Rejecting only while slicing meant two distinct serials sharing a 24-character prefix would collapse onto the same EUI-128, which is a volume-identity bug rather than just a cosmetic one.
Fixed in : the serial must now match exactly, so over-long and non-hex serials are both rejected up front.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -307,14 +345,24 @@ public ProviderVolume getVolumeByAddress(ProviderAdapterContext context, Address
throw new RuntimeException("Invalid search criteria provided for getVolumeByAddress");
}
- // only support WWN type addresses at this time.
- if (!ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ String serial;
+ if (ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ // Strip the NAA prefix (1 char) + Pure OUI to recover the volume serial.
+ serial = address.substring(FlashArrayVolume.PURE_OUI.length() + 1).toUpperCase();
+ } else if (ProviderVolume.AddressType.NVMETCP.equals(addressType)) {
+ // Reverse the EUI-128 layout: serial = eui[2:16] + eui[22:32], after
+ // stripping the optional "eui." prefix that appears in udev paths.
+ String eui = address.startsWith("eui.") ? address.substring(4) : address;
+ if (eui == null || eui.length() != 32) {
+ throw new RuntimeException("Invalid NVMe-TCP EUI-128 address ["
+ + address + "]: expected 32 hex characters, got "
+ + (eui == null ? "null" : String.valueOf(eui.length())));
+ }
+ serial = (eui.substring(2, 16) + eui.substring(22)).toUpperCase();
Review Comment:
Fixed in . now validates the full FlashArray EUI-128 layout before reversing it into a serial:
- exactly 32 hexadecimal characters (was: any 32-character string)
- a prefix
- the Pure Storage OUI () at offset 16, which is where places it
Sanity-checked the offsets against a real namespace from my lab: passes all three checks and round-trips to serial and back unchanged.
##########
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;
+ }
Review Comment:
I'd like to keep this one as , for two reasons.
**It matches the sibling adapter.** (the FC/iSCSI class this one is modelled on) is byte-identical — — as is its . Changing only the NVMe class would leave the two out of step.
**Nothing is actually leaked here.** For managed storage the namespace is deleted on the *provider* side: the management server drives , which is what issues the destroy against the array. The KVM agent's pool-level exists for hypervisor-local disk *files* (a qcow2 on NFS, an LV on CLVM); an NVMe-TCP namespace is not a host-local object, so there is genuinely nothing for this method to do. It isn't reporting success for work it skipped so much as no host-side artifact to remove.
Worth noting too that all three in-tree callers ignore the return value — (twice) and call it for side effects only and catch just . So vs changes nothing observable, while throwing *would* change behaviour: it would turn a harmless no-op into a hard failure on the volume-delete and backup-delete paths.
Happy to switch both classes together if a maintainer would rather have the stricter contract — I just don't want to introduce a new failure mode in this PR for a path I can't exercise.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5316383158
@vladimirpetrov @sureshanaparti , only regression testing needed for this one?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5934860865
both @slavkap and @rp- approive, I think we can merge this , @abh1sar ?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3459549235
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:
##########
@@ -0,0 +1,462 @@
+// 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) {
+ String type = null;
+ String address = null;
+ String connectionId = null;
+ String path = null;
+ String hostname = resolveHostnameShort();
+ String hostnameFq = resolveHostnameFq();
+ String[] parts = inPath.split(";");
Review Comment:
Good catch @slavkap. The two public callers in this class (`getPhysicalDisk` and `connectPhysicalDisk`) already pre-guard via `StringUtils.isEmpty(volumePath)` before calling `parseAndValidatePath`, so the current code paths can't reach it with a null `inPath` — but as a public method it deserves defense-in-depth.
Pushed `e503a1439d kvm: guard parseAndValidatePath against null inPath`: throws `CloudRuntimeException` with a clear message instead of letting it NPE on `inPath.split(...)`. Mirrors how the FC sibling `FiberChannelAdapter` throws on an invalid address type.
Also rebased onto current `upstream/main` to clear the merge-conflict bot. The new conflict was `KVMStorageProcessor.java` — main added `CLVM` to the RAW-format list and we add `NVMeTCP`; resolved by keeping both. Compile verified via `mvn -pl plugins/hypervisors/kvm -am -DskipTests compile`.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4718267712
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3711135887
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -307,14 +345,24 @@ public ProviderVolume getVolumeByAddress(ProviderAdapterContext context, Address
throw new RuntimeException("Invalid search criteria provided for getVolumeByAddress");
}
- // only support WWN type addresses at this time.
- if (!ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ String serial;
+ if (ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ // Strip the NAA prefix (1 char) + Pure OUI to recover the volume serial.
+ serial = address.substring(FlashArrayVolume.PURE_OUI.length() + 1).toUpperCase();
+ } else if (ProviderVolume.AddressType.NVMETCP.equals(addressType)) {
+ // Reverse the EUI-128 layout: serial = eui[2:16] + eui[22:32], after
+ // stripping the optional "eui." prefix that appears in udev paths.
+ String eui = address.startsWith("eui.") ? address.substring(4) : address;
+ if (eui == null || eui.length() != 32) {
+ throw new RuntimeException("Invalid NVMe-TCP EUI-128 address ["
+ + address + "]: expected 32 hex characters, got "
+ + (eui == null ? "null" : String.valueOf(eui.length())));
+ }
+ serial = (eui.substring(2, 16) + eui.substring(22)).toUpperCase();
Review Comment:
For NVMETCP, `getVolumeByAddress` accepts any 32-character string and deterministically maps it into a serial without validating the expected FlashArray EUI-128 layout (e.g., leading `00` and the embedded Pure OUI at the expected offset). This makes it easier for a malformed/tampered address to resolve to an unintended volume serial. Recommend validating the EUI structure (prefix + expected OUI segment, and hex-only characters) before deriving the serial.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayVolume.java:
##########
@@ -107,6 +111,22 @@ 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 24 hex characters, got "
+ + serial.length() + ")");
+ }
+ // Slice exact ranges rather than substring(14) so a serial with unexpected trailing
+ // characters cannot produce an EUI longer than 32 hex chars (which would not match
+ // /dev/disk/by-id/nvme-eui.<eui> on Linux).
+ return ("00" + serial.substring(0, 14) + PURE_OUI_EUI + serial.substring(14, 24)).toLowerCase();
Review Comment:
The code expects a 24-hex-character serial (per the exception message/comments) but only rejects serials shorter than 24. If a longer serial ever appears, the EUI will be derived from only the first 24 chars, which can cause collisions (two different serials with the same first 24 chars map to the same EUI) and break volume identity. Consider enforcing `serial.length() == 24` (and optionally validating hex) when building the EUI-128 address.
##########
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;
+ }
Review Comment:
This method currently returns `true` unconditionally, which tells callers the disk was deleted even though no delete occurred. That can lead to incorrect orchestration state and leaked namespaces on the provider. Instead, either delegate to the adaptor (and let it throw/handle unsupported deletion) or throw an `UnsupportedOperationException`/return `false` to avoid reporting a successful deletion.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4341116220
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4299974292
Heads-up: pushed an additional commit `c0cdfa41da — kvm: implement copyPhysicalDisk on MultipathNVMeOFAdapterBase` to this PR. The original description noted this as "future work, not in this PR", but after validating the rest of the NVMe-TCP path end-to-end I wanted VMs to be fully deployable on an NVMe-TCP pool (root + data), which requires `copyPhysicalDisk` to land the template as a raw image on the provisioned namespace.
Implementation mirrors `MultipathSCSIAdapterBase.copyPhysicalDisk`: resolve the destination device path via the existing `getPhysicalDisk` plumbing (which triggers `nvme ns-rescan` and waits for the `by-id/nvme-eui.<NGUID>` symlink), then `qemu-img convert` the source image into the raw block device. User-space encrypted source or destination volumes are rejected by design — the FlashArray already encrypts at rest and layering qemu-img LUKS on top of a hostgroup-scoped namespace is not a sensible layering (and would break across live-migration).
With this commit I was able to:
- Deploy a Rocky 9 VM with `pooltype: NVMeTCP` on the root volume (previously the deploy failed as soon as the root disk tried to land on the NVMe-TCP pool).
- Attach an additional `tags=nvme` data disk, so both `vda` and `vdb` are NVMe-backed.
- `createVMSnapshot` with `quiescevm=true, snapshotmemory=false` → array-side snapshots on both volumes, CloudStack `state: Ready`, `type: Disk`.
- `revertToVMSnapshot` → both volumes came back with identical SHA-256 content to pre-snapshot.
I've also updated the PR description to reflect the 7-commit set and add the full-NVMe test evidence. Happy to split this commit into a separate follow-up PR if reviewers prefer — let me know.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4300127195
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17578
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4305039105
Pushed `723bf1445f kvm/flasharray: address review feedback on NVMe-TCP PR` to address this round of feedback:
### Code fixes
- **`NVMeTCPAdapter` logger rename** — `LOGGER_NVMETCP` → `LOGGER` to match the project convention (@sureshanaparti).
- **`rescanAllControllers()` now honours `waitFor(...)`** and `destroyForcibly()`s the `nvme ns-rescan` process on timeout, so hung rescans cannot accumulate under load (@Copilot).
- **`Integer.parseInt(waitTime)` is now guarded** in `connectPhysicalDisk()` — a non-integer `STORAGE_POOL_DISK_WAIT` detail no longer aborts the connect; we log a warning and fall back to `DEFAULT_DISK_WAIT` (@Copilot).
- **Retry path recognises host-scoped NVMe-TCP connections** — the "Connection already exists" fallback was only matching host-group connections, so a `transport=nvme-tcp` pool without `hostgroup` would never hit its retry branch. Now checks both (@Copilot).
- **Attach-failure error message is transport-agnostic** — the message used to say "did not return a LUN"; under NVMe-TCP we return an NSID. Reworded to "did not return connection information (lun/nsid)" so NVMe-TCP debugging isn't misleading (@Copilot).
- **EUI-128 length validation in `getVolumeByAddress()`** — NVMe-TCP path no longer blind-slices `substring(2,16)/substring(22)`; an address that isn't a 32-hex EUI-128 is now rejected explicitly rather than surfacing as a mislabelled "not found" via `StringIndexOutOfBoundsException` (@Copilot).
- **`FlashArrayVolume` serial length check** — NGUID construction now validates `serial.length() == 24` before slicing, and fails fast with a clear message instead of throwing `StringIndexOutOfBoundsException` deep in the address getter (@Copilot).
- **`getSnapshot()` now applies `withAddressType(...)`** so the returned `FlashArrayVolume` carries the pool's transport type (NVMe-TCP vs FC) — without this, `ProviderSnapshot.getAddress()` on an NVMe-TCP pool emitted an FC-style WWN instead of the EUI-128, and adaptive's `takeSnapshot` / `revertSnapshot` paths persisted the wrong address (@Copilot).
### Extra fix in the same area
While wiring `withAddressType(...)` into `getSnapshot()` I noticed the same pattern was **also missing in `snapshot()`** (the method that actually *creates* the snapshot), and in the other `getSnapshot()` overload. Both now apply `withAddressType(...)` as well, so the address stored at snapshot-create time is already the NVMe EUI-128 on an NVMe-TCP pool rather than the FC-WWN default set by `FlashArrayVolume`'s constructor.
Verified end-to-end on an NVMe-TCP pool: snapshotting a volume now persists `install_path=006c1b16ce1c034d24a9371c05ab334a` (NVMe EUI-128) rather than the previous `624a93706c1b16ce1c034d1c05ab3346` (FC WWN), and subsequent `revertSnapshot` uses the correct transport-specific address.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-6395793788
<b>[SF] Trillian test result (tid-17241)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 50988 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13061-t17241-kvm-ol8.zip
Smoke tests completed. 157 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4335656501
Pushed `0608223402 ui: expose NVMe-TCP transport for FlashArray primary storage` on top of the round-1 backend fixes. Three small edits in `ui/src/views/infra/AddPrimaryStorage.vue`:
- The protocol dropdown for the FlashArray provider previously pinned to `['FiberChannel']`, leaving NVMe-TCP only reachable via a hand-crafted URL. It now offers `['FiberChannel', 'NVMeTCP']`. Primera stays FC-only.
- When `NVMeTCP` is the selected protocol, the submit handler appends `transport=nvme-tcp` to the FlashArray URL (using `?` or `&` based on whether the URL already has a query string), so the adaptive lifecycle pivot in `pickPoolType()` resolves `StoragePoolType.NVMeTCP` server-side.
- The generic `Path` input — already hidden under the `FiberChannel` protocol — is also hidden under `NVMeTCP` so the FlashArray form looks consistent across transports.
Verified end-to-end on a live FlashArray: registered a new pool through the form (Provider: Flash Array, Protocol: NVMeTCP), submitted to backend resolved `StoragePoolType.NVMeTCP` correctly, attached a volume, snapshot+revert produced the right NVMe EUI-128 `install_path`.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4519493404
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3967635565
##########
PendingReleaseNotes:
##########
@@ -39,3 +39,15 @@ example.ver.1 > example.ver.2:
which can now be attached to Instances. This is to prevent the Secondary
Storage to grow to enormous sizes as Linux Distributions keep growing in
size while a stripped down Linux should fit on a 2.88MB floppy.
+
+
+4.22.0 > 4.23.0:
Review Comment:
This should be 4.23.0 -> 24.0
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "slavkap (via GitHub)" <gi...@apache.org>.
slavkap commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3435435412
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:
##########
@@ -0,0 +1,462 @@
+// 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) {
+ String type = null;
+ String address = null;
+ String connectionId = null;
+ String path = null;
+ String hostname = resolveHostnameShort();
+ String hostnameFq = resolveHostnameFq();
+ String[] parts = inPath.split(";");
Review Comment:
is it possible the `inPath` to be null?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3528310090
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -260,14 +283,19 @@ public ProviderVolume getVolumeByAddress(ProviderAdapterContext context, Address
throw new RuntimeException("Invalid search criteria provided for getVolumeByAddress");
}
- // only support WWN type addresses at this time.
- if (!ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ String serial;
+ if (ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ // Strip the NAA prefix (1 char) + Pure OUI to recover the volume serial.
+ serial = address.substring(FlashArrayVolume.PURE_OUI.length() + 1).toUpperCase();
+ } else if (ProviderVolume.AddressType.NVMETCP.equals(addressType)) {
+ // Reverse the EUI-128 layout: serial = eui[2:16] + eui[22:32], after
+ // stripping the optional "eui." prefix that appears in udev paths.
+ String eui = address.startsWith("eui.") ? address.substring(4) : address;
+ serial = (eui.substring(2, 16) + eui.substring(22)).toUpperCase();
+ } else {
Review Comment:
Fixed in `71b494c897` — an address that is not a 32-hex EUI-128 is now rejected explicitly instead of surfacing as a `StringIndexOutOfBoundsException`.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3719457061
##########
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:
no @genegr , it is fine. we push back on it a lot. I’ll stop running it for this PR.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3710807041
##########
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:
Fixed in `ca9ca209cc` — now `part.split(char(34)=char(34), 2)` so only the first `=` delimits key from value and a value containing `=` is no longer silently discarded.
##########
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:
Fixed in `ca9ca209cc` — `rescanAllControllers()` is now throttled to once per 10s (`RESCAN_INTERVAL_MS`) while the symlink poll stays at 2s. The first iteration still rescans immediately, so discovery latency in the common case is unchanged, but a long wait no longer spawns `nvme ns-rescan` per controller every 2s.
##########
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:
Fixed in `ca9ca209cc` — `rescanAllControllers()` is now throttled to once per 10s (`RESCAN_INTERVAL_MS`) while the symlink poll stays at 2s. The first iteration still rescans immediately, so discovery latency in the common case is unchanged, but a long wait no longer spawns `nvme ns-rescan` per controller every 2s.
##########
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:
Fixed in `ca9ca209cc` — `rescanAllControllers()` is now throttled to once per 10s (`RESCAN_INTERVAL_MS`) while the symlink poll stays at 2s. The first iteration still rescans immediately, so discovery latency in the common case is unchanged, but a long wait no longer spawns `nvme ns-rescan` per controller every 2s.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "genegr (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3128714419
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -167,13 +185,18 @@ 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)) {
+ if (conn.getHostGroup() != null && conn.getHostGroup().getName() != null
+ && conn.getHostGroup().getName().equals(hostgroup)) {
+ 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) {
Review Comment:
In NVMe-TCP mode, the "Connection already exists" retry path only matches host-group connections (`conn.getHostGroup().getName().equals(hostgroup)`). If `transport=nvme-tcp` is used without `hostgroup`, the initial attach falls back to host-scoped connections but this retry lookup will never match them, causing attach to fail. Either require `hostgroup` for NVMe-TCP upfront, or also match host-scoped NVMe connections here when `hostgroup` is null.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -140,22 +144,36 @@ public String attach(ProviderAdapterContext context, ProviderAdapterDataObject d
String volumeName = normalizeName(pod, dataObject.getExternalName());
try {
FlashArrayList<FlashArrayConnection> list = null;
- FlashArrayHost host = getHost(hostname);
- if (host != null) {
- list = POST("/connections?host_names=" + host.getName() + "&volume_names=" + volumeName, null,
+ if (AddressType.NVMETCP.equals(volumeAddressType) && hostgroup != null) {
+ // NVMe-TCP pod volumes are connected at the host-group level so the
+ // array assigns a consistent NSID visible to every member host.
+ list = POST("/connections?host_group_names=" + hostgroup + "&volume_names=" + volumeName, null,
new TypeReference<FlashArrayList<FlashArrayConnection>>() {
});
+ } else {
+ FlashArrayHost host = getHost(hostname);
+ if (host != null) {
+ list = POST("/connections?host_names=" + host.getName() + "&volume_names=" + volumeName, null,
+ new TypeReference<FlashArrayList<FlashArrayConnection>>() {
+ });
+ }
}
if (list == null || list.getItems() == null || list.getItems().size() == 0) {
throw new RuntimeException("Volume attach did not return lun information");
}
Review Comment:
The exception message here is NVMe-unfriendly: in NVMe-TCP mode the code returns an NSID (or a default) rather than a LUN. Consider changing the message to something transport-agnostic (e.g., "did not return connection information") or conditional on `volumeAddressType` so debugging NVMe-TCP attach failures isn’t misleading.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -781,6 +816,13 @@ private FlashArrayVolume getSnapshot(String snapshotName) {
return (FlashArrayVolume) getFlashArrayItem(list);
Review Comment:
`getSnapshot()` returns a `FlashArrayVolume` without applying `withAddressType(...)`, so the returned object keeps the constructor default `AddressType.FIBERWWN`. On an NVMe-TCP pool this makes `ProviderSnapshot.getAddress()` emit an FC-style WWN instead of the NVMe EUI-128, and adaptive’s `takeSnapshot` persists that address as the snapshot path. Wrap this return (and the public `snapshot()` path) with `withAddressType(...)` so snapshot addressing matches the pool transport.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -260,14 +283,19 @@ public ProviderVolume getVolumeByAddress(ProviderAdapterContext context, Address
throw new RuntimeException("Invalid search criteria provided for getVolumeByAddress");
}
- // only support WWN type addresses at this time.
- if (!ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ String serial;
+ if (ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ // Strip the NAA prefix (1 char) + Pure OUI to recover the volume serial.
+ serial = address.substring(FlashArrayVolume.PURE_OUI.length() + 1).toUpperCase();
+ } else if (ProviderVolume.AddressType.NVMETCP.equals(addressType)) {
+ // Reverse the EUI-128 layout: serial = eui[2:16] + eui[22:32], after
+ // stripping the optional "eui." prefix that appears in udev paths.
+ String eui = address.startsWith("eui.") ? address.substring(4) : address;
+ serial = (eui.substring(2, 16) + eui.substring(22)).toUpperCase();
+ } else {
Review Comment:
The NVMe-TCP branch assumes `address` (after optional `eui.` stripping) is a full 32-hex EUI-128 and immediately slices fixed offsets (`substring(2,16)` / `substring(22)`). If a malformed/short address reaches here, this throws `StringIndexOutOfBoundsException` and is caught as "not found", potentially masking a real input problem. Add explicit length/format validation (and ideally verify the embedded Pure OUI) and fail with a clear error when the address doesn’t match the expected EUI-128 layout.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:
##########
@@ -0,0 +1,448 @@
+// 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) {
+ 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)) {
+ waitSecs = Integer.parseInt(waitTime);
+ }
+ }
+ 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) {
+ Thread.currentThread().interrupt();
+ return false;
+ }
+ }
+ LOGGER.debug("NVMe namespace did not appear at " + address.getPath() + " within " + waitSecs + "s");
+ return false;
+ }
+
+ private void rescanAllControllers() {
+ try {
+ File sysClass = new File("/sys/class/nvme");
+ File[] ctrls = sysClass.listFiles();
+ if (ctrls == null) {
+ return;
+ }
+ for (File ctrl : ctrls) {
+ Process p = new ProcessBuilder("nvme", "ns-rescan", "/dev/" + ctrl.getName())
+ .redirectErrorStream(true).start();
+ p.waitFor(NS_RESCAN_TIMEOUT_SECS, TimeUnit.SECONDS);
+ }
Review Comment:
`rescanAllControllers()` starts an `nvme ns-rescan` process per controller and waits up to `NS_RESCAN_TIMEOUT_SECS`, but ignores the boolean result of `waitFor(...)` and never terminates the process if it times out. This can leave hung `nvme` processes accumulating under load. Consider checking the `waitFor` return value and calling `destroyForcibly()` (and optionally logging) when the timeout is hit.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayVolume.java:
##########
@@ -107,6 +111,13 @@ 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>
Review Comment:
NVMe-TCP address generation assumes `serial` is long enough for `substring(0, 14)` / `substring(14)` (and per comment effectively expects a 24-hex serial). If FlashArray ever returns an unexpected/short serial, this will throw at runtime. Please validate `serial` length/format before slicing and fail with a clear exception (or return null) when it doesn’t match the expected layout.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:
##########
@@ -0,0 +1,448 @@
+// 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) {
+ 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)) {
+ waitSecs = Integer.parseInt(waitTime);
Review Comment:
`waitSecs = Integer.parseInt(waitTime)` can throw `NumberFormatException` if the STORAGE_POOL_DISK_WAIT detail is set to a non-integer value, which would abort connect rather than falling back to the default. Consider guarding this parse with a try/catch (logging a warning and keeping `DEFAULT_DISK_WAIT_SECS`) to match the more defensive parsing used elsewhere in the codebase for adapter parameters.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "slavkap (via GitHub)" <gi...@apache.org>.
slavkap commented on PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-5100746171
@DaanHoogland, I have tested it with StorPool even though that the changes are not related to other storage plugins. I cannot test it with FlashArray
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org