flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage - #13061
flasharray/kvm/adaptive: NVMe-TCP transport for FlashArray primary storage#13061genegr wants to merge 13 commits into
Conversation
|
@blueorangutan package |
|
@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. |
|
Heads-up: pushed an additional commit Implementation mirrors With this commit I was able to:
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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17578 |
There was a problem hiding this comment.
Pull request overview
Adds opt-in NVMe-over-TCP (NVMe-oF/TCP) support for KVM managed primary storage via the adaptive storage framework, with the FlashArray adaptive plugin as the first consumer. This introduces a new StoragePoolType.NVMeTCP, NVMe EUI-128 addressing, and a KVM-side NVMe-oF adapter base to surface namespaces via /dev/disk/by-id/nvme-eui.<eui>.
Changes:
- Introduces NVMe-TCP transport selection (
transport=nvme-tcp) and maps it to a newStoragePoolType.NVMeTCP. - Extends FlashArray adapter to generate/parse NVMe EUI-128 addresses and use host-group scoped connections for consistent namespace identity.
- Adds KVM NVMe-oF adapter/pool implementations and updates KVM storage processor handling (RAW format + path derivation) for the new pool type.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayVolume.java | Adds NVMe EUI-128 address construction for NVMe-TCP volumes. |
| plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayConnection.java | Adds nsid field to model NVMe namespace IDs in connection payloads. |
| plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java | Adds transport selection, NVMe attach/lookup behavior, and address-type stamping for returned volumes. |
| plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/AdaptiveDataStoreLifeCycleImpl.java | Chooses pool type from provider URL transport= query parameter (defaults to FiberChannel). |
| plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/adapter/ProviderVolume.java | Adds AddressType.NVMETCP for provider volume addressing. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/NVMeTCPAdapter.java | Registers a KVM storage adapter for StoragePoolType.NVMeTCP. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFPool.java | Adds a pool implementation delegating operations back to the NVMe-oF adapter. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java | Implements NVMe-oF attach/wait-for-namespace and qemu-img convert copy into namespaces. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java | Treats NVMeTCP pools like other managed/shared block pools for RAW format and path derivation. |
| api/src/main/java/com/cloud/storage/Storage.java | Adds new enum value StoragePoolType.NVMeTCP. |
| PendingReleaseNotes | Documents the new NVMe-oF/TCP support and required components. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) { |
There was a problem hiding this 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.
| if (list == null || list.getItems() == null || list.getItems().size() == 0) { | ||
| throw new RuntimeException("Volume attach did not return lun information"); | ||
| } |
There was a problem hiding this comment.
Fixed in 71b494c897 — reworded to transport-agnostic "did not return connection information (lun/nsid)".
| // 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 { |
There was a problem hiding this comment.
Fixed in 71b494c897 — an address that is not a 32-hex EUI-128 is now rejected explicitly instead of surfacing as a StringIndexOutOfBoundsException.
| 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> |
There was a problem hiding this comment.
Fixed in 71b494c897 — FlashArrayVolume now validates serial.length() == 24 before slicing and fails fast with a clear message.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13061 +/- ##
============================================
- Coverage 19.65% 19.63% -0.02%
+ Complexity 19792 19791 -1
============================================
Files 6368 6371 +3
Lines 574881 575295 +414
Branches 70351 70411 +60
============================================
- Hits 112970 112961 -9
- Misses 449639 450066 +427
+ Partials 12272 12268 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Apply the review comments from the first round on apache#13061: * FlashArrayAdapter.snapshot() and both getSnapshot() entry points now wrap the returned FlashArrayVolume in withAddressType(). Without this, snapshots taken against an NVMe-TCP pool had the constructor-default AddressType.FIBERWWN and ProviderSnapshot.getAddress() emitted an FC style WWN instead of the NVMe EUI-128, which the adaptive driver then persisted as the snapshot path. Verified end-to-end against Purity 6.7.7: a fresh NVMe-TCP snapshot now lands with install_path starting 006c... , matching the source volume's EUI (previously it was 6-24a9370...). * FlashArrayAdapter.attach() - retry path after 'Connection already exists' no longer requires a hostgroup-scoped match for NVMe-TCP. If hostgroup is not configured, or the existing connection is host-scoped, fall back to matching by host name, same as the Fibre Channel branch. Also normalize the 'volume lun is not found' message when no connection list is returned. * FlashArrayAdapter.attach() - initial 'Volume attach did not return lun information' exception message now mentions both lun (FC) and nsid (NVMe-TCP) so the error is not misleading on NVMe deployments. * FlashArrayAdapter.getVolumeByAddress() - validate the EUI-128 length before slicing. A short/malformed address used to throw StringIndexOutOfBoundsException deep inside getFlashArrayItem and be swallowed as 'not found'; now a clear RuntimeException is raised with the expected vs actual length. * FlashArrayVolume.getAddress() - same defensive check when building an EUI-128 from the FlashArray volume serial; if the serial is shorter than 24 hex chars, fail with a clear message instead of SIOOBE. * MultipathNVMeOFAdapterBase.connectPhysicalDisk() - Integer.parseInt of the STORAGE_POOL_DISK_WAIT detail is now guarded; a non-numeric value falls back to the default rather than aborting the connect. * MultipathNVMeOFAdapterBase.rescanAllControllers() - honour the boolean return from Process.waitFor(). If an nvme ns-rescan invocation does not complete in NS_RESCAN_TIMEOUT_SECS we destroyForcibly() it, so hung nvme-cli processes do not accumulate while the namespace poll loop retries. * NVMeTCPAdapter - rename LOGGER_NVMETCP to LOGGER to match the naming convention used in the other KVM adapters. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
|
Pushed Code fixes
Extra fix in the same areaWhile wiring Verified end-to-end on an NVMe-TCP pool: snapshotting a volume now persists |
1 similar comment
|
Pushed Code fixes
Extra fix in the same areaWhile wiring Verified end-to-end on an NVMe-TCP pool: snapshotting a volume now persists |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17589 |
|
Pushed
Verified end-to-end on a live FlashArray: registered a new pool through the form (Provider: Flash Array, Protocol: NVMeTCP), submitted to backend resolved |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17643 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15978)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@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 |
|
@sureshanaparti , any blocker on this one? |
Preparatory data-model changes for NVMe-TCP support on the adaptive
storage framework. No behaviour change for existing Fibre Channel
users - the extra enum value, field, and getter/setter are only
exercised by callers that explicitly use them.
ProviderVolume.AddressType gains a NVMETCP value alongside FIBERWWN,
so adapters can declare that a volume is addressed by an NVMe EUI-128
(NGUID) rather than a SCSI WWN.
FlashArrayVolume.getAddress() produces the NGUID layout expected by
the Linux kernel for a FlashArray NVMe namespace:
00 + serial[0:14] + 24a937 (Pure 6-hex OUI) + serial[14:24]
which matches the /dev/disk/by-id/nvme-eui.<id> symlink emitted by
udev. Fibre Channel callers (addressType != NVMETCP) still get the
existing 6 + 24a9370 + serial form.
FlashArrayConnection gains a nsid field to carry the namespace id the
FlashArray REST API attaches to host-group-scoped NVMe connections,
when it is present.
Teach FlashArrayAdapter to talk to a pool over NVMe over TCP instead of
Fibre Channel.
The transport is selected from a new transport= option on the storage
pool URL (or the equivalent storage_pool_details entry), e.g.
https://user:pass@fa:443/api?pod=cs&transport=nvme-tcp&hostgroup=cluster1
Defaults remain Fibre Channel / WWN addressing when transport is absent
or anything other than nvme-tcp, so existing FC pools are unaffected.
Beyond the transport parsing itself the adapter now:
* Tracks a per-pool volumeAddressType (AddressType.NVMETCP or
FIBERWWN) and stamps every volume it hands back to the framework
with it (withAddressType), so the adaptive driver path stores the
correct type=... field in the CloudStack volume path (used later
by the KVM driver to locate the device).
* Attaches pod-backed NVMe-TCP volumes at the host-group level
(POST /connections?host_group_names=...) instead of per-host, so
the array assigns a consistent NSID to every member host; falls
back to per-host attach for FC or when no hostgroup is configured.
* Tolerates a missing nsid in the FlashArray connections response
for NVMe-TCP - Purity does not return one for host-group NVMe
connections; the namespace is identified on the host by EUI-128
from FlashArrayVolume.getAddress(), so a placeholder value is
returned to the caller purely for informational tracking.
* Resolves NVMETCP addresses back to volumes in getVolumeByAddress
by reversing the EUI-128 layout (strip optional eui. prefix, drop
leading 00 and the embedded Pure OUI).
* Indexes NVMe connections in getConnectionIdMap by host name (the
array returns one entry per host inside a host-group connection),
so connid.<hostname> tokens in the path still match in
parseAndValidatePath on the KVM side.
Followed by a matching adaptive/KVM driver change (separate commit).
NVMe-oF over TCP (NVMe-TCP) is conceptually a separate storage fabric from Fibre Channel / iSCSI: it speaks the NVMe command set rather than SCSI, identifies namespaces by EUI-128 NGUIDs rather than WWNs, and on Linux is multipathed natively by the nvme driver rather than by device-mapper multipath. Giving it its own StoragePoolType lets the KVM agent dispatch the adaptive driver to a dedicated NVMe-oF adapter (added in the next commit) without polluting the existing Fibre Channel code path. The new value is wired into the same format-routing and derivePath fall-through paths that already special-case FiberChannel in KVMStorageProcessor: NVMe-TCP volumes are also RAW and carry their device path in DataObjectTO.path rather than in a managedStoreTarget detail.
Introduce an NVMe-over-Fabrics counterpart to the existing
MultipathSCSIAdapterBase / FiberChannelAdapter pair.
NVMe-oF is conceptually distinct from SCSI - it speaks the NVMe command
set, identifies namespaces by EUI-128 NGUIDs, and is multipathed by the
kernel natively rather than by device-mapper - so keeping it out of the
SCSI code path avoids special-casing inside every method that handles
volume paths, connect, disconnect, or size lookup.
MultipathNVMeOFAdapterBase (abstract)
* Parses volume paths of the form
type=NVMETCP; address=<eui>; connid.<host>=<nsid>; ...
into an AddressInfo whose path is
/dev/disk/by-id/nvme-eui.<eui>
which is the udev symlink the kernel emits for every NVMe namespace.
* connectPhysicalDisk polls the udev path and, on every iteration,
triggers nvme ns-rescan on all local NVMe controllers, to cover
target/firmware combinations that do not send an asynchronous event
notification when a new namespace is mapped.
* disconnectPhysicalDisk is a no-op; the kernel drops the namespace
when the target removes the host-group connection. The
ByPath variant only claims paths starting with
/dev/disk/by-id/nvme-eui. so foreign paths still fall through to
other adapters.
* Delegates getPhysicalDisk, isConnected, and getPhysicalDiskSize to
plain test -b / blockdev --getsize64 calls - no SCSI rescan, no dm
multipath, no multipath-map cleanup timer.
* createPhysicalDisk / createTemplateFromDisk / listPhysicalDisks /
copyPhysicalDisk all throw UnsupportedOperationException - these
are the responsibility of the storage provider, not the KVM
adapter, same as the SCSI base.
MultipathNVMeOFPool
* KVMStoragePool mirror of MultipathSCSIPool. Defaults to
Storage.StoragePoolType.NVMeTCP in the parameterless-fallback
constructor.
NVMeTCPAdapter
* Concrete adapter that registers itself for
Storage.StoragePoolType.NVMeTCP via the reflection-based scan in
KVMStoragePoolManager. Carries no logic of its own beyond binding
the base to the pool type.
A similar MultipathNVMeOFAdapterBase-derived NVMeRoCEAdapter (or
NVMeFCAdapter) can later be added by adding one concrete subclass and a
new pool-type value; the base does not assume any particular
fabric-level transport.
The adaptive storage framework hard-coded FiberChannel as the KVM-side pool type for every provider it fronts. With a separate NVMeTCP pool type now available (and a dedicated NVMe-oF adapter on the KVM side), teach the lifecycle to route a pool to the right adapter based on a transport= URL parameter: https://user:pass@host/api?...&transport=nvme-tcp -> StoragePoolType.NVMeTCP -> NVMeTCPAdapter on the KVM host When the query parameter is absent the default stays FiberChannel, so existing FC deployments on Primera or FlashArray continue to work unchanged. The choice is made in the shared AdaptiveDataStoreLifeCycleImpl rather than inside each vendor plugin so every adaptive provider (FlashArray, Primera, any future one) speaks the same configuration vocabulary.
The NVMe-oF KVM adapter refused every template copy request from the adaptive storage orchestrator with UnsupportedOperationException, which made it impossible to use an NVMe-TCP pool as primary storage for a VM root disk: every deploy that landed a root volume on the pool failed as soon as CloudStack tried to lay down the template. Implement it the same way FiberChannel (SCSI) does: the storage provider creates and connects a raw namespace ahead of time, then the adapter resolves the host-side /dev/disk/by-id/nvme-eui.<NGUID> path via the existing getPhysicalDisk plumbing (which will nvme ns-rescan and wait for the symlink if the kernel has not yet picked it up) and qemu-img converts the source image into the raw block device. User-space encrypted source or destination volumes are rejected: the FlashArray already encrypts at rest and layering qemu-img LUKS on top of a hostgroup-scoped namespace shared between hosts is not a sensible layering. Source encryption would also break on migration because the passphrase does not travel. With this change a CloudStack KVM VM can have its ROOT volume on an NVMe-TCP pool (tested end-to-end on 4.23-SNAPSHOT against Purity 6.7.7: template copy, first boot, live migrate with data disk, VM snapshot with quiesce, and revert all work). Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
Apply the review comments from the first round on apache#13061: * FlashArrayAdapter.snapshot() and both getSnapshot() entry points now wrap the returned FlashArrayVolume in withAddressType(). Without this, snapshots taken against an NVMe-TCP pool had the constructor-default AddressType.FIBERWWN and ProviderSnapshot.getAddress() emitted an FC style WWN instead of the NVMe EUI-128, which the adaptive driver then persisted as the snapshot path. Verified end-to-end against Purity 6.7.7: a fresh NVMe-TCP snapshot now lands with install_path starting 006c... , matching the source volume's EUI (previously it was 6-24a9370...). * FlashArrayAdapter.attach() - retry path after 'Connection already exists' no longer requires a hostgroup-scoped match for NVMe-TCP. If hostgroup is not configured, or the existing connection is host-scoped, fall back to matching by host name, same as the Fibre Channel branch. Also normalize the 'volume lun is not found' message when no connection list is returned. * FlashArrayAdapter.attach() - initial 'Volume attach did not return lun information' exception message now mentions both lun (FC) and nsid (NVMe-TCP) so the error is not misleading on NVMe deployments. * FlashArrayAdapter.getVolumeByAddress() - validate the EUI-128 length before slicing. A short/malformed address used to throw StringIndexOutOfBoundsException deep inside getFlashArrayItem and be swallowed as 'not found'; now a clear RuntimeException is raised with the expected vs actual length. * FlashArrayVolume.getAddress() - same defensive check when building an EUI-128 from the FlashArray volume serial; if the serial is shorter than 24 hex chars, fail with a clear message instead of SIOOBE. * MultipathNVMeOFAdapterBase.connectPhysicalDisk() - Integer.parseInt of the STORAGE_POOL_DISK_WAIT detail is now guarded; a non-numeric value falls back to the default rather than aborting the connect. * MultipathNVMeOFAdapterBase.rescanAllControllers() - honour the boolean return from Process.waitFor(). If an nvme ns-rescan invocation does not complete in NS_RESCAN_TIMEOUT_SECS we destroyForcibly() it, so hung nvme-cli processes do not accumulate while the namespace poll loop retries. * NVMeTCPAdapter - rename LOGGER_NVMETCP to LOGGER to match the naming convention used in the other KVM adapters. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
AddPrimaryStorage previously pinned the protocol to FiberChannel whenever the operator picked the FlashArray provider, leaving NVMe-TCP backends only reachable by hand-crafting the URL with ?transport=nvme-tcp. Surface the choice in the form: - protocols dropdown for FlashArray now offers FiberChannel and NVMeTCP (Primera stays FC-only). - when NVMeTCP is selected, the submit handler appends transport=nvme-tcp to the FlashArray URL so the adaptive lifecycle pivot in pickPoolType() resolves StoragePoolType.NVMeTCP server-side. - the generic Path field, already hidden for FiberChannel, is also hidden for NVMeTCP for parity. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
Reported by @slavkap: building this branch against current main produces a compile error in MultipathNVMeOFPool because the KVMStoragePool interface drifted between when this PR was last rebased (2026-05-22) and main today (2026-06-09): - checkingHeartBeat(HAStoragePool, HostTO) was renamed to hasHeartBeat(HAStoragePool, HostTO) - vmActivityCheck(...) was renamed to hasVmActivity(...) with the same signature Update the two overrides accordingly (both still return null as before — MultipathNVMeOFPool does not participate in the KVM-side HA heartbeat). Mirrors how MultipathSCSIPool implements them. Verified compile via mvn -pl plugins/hypervisors/kvm -am -DskipTests compile. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
@slavkap noted that parseAndValidatePath has no internal null guard. The two callers in this base class (getPhysicalDisk and connectPhysicalDisk) already pre-check via StringUtils.isEmpty(volumePath), so the current code path is safe, but as a public method it deserves defense-in-depth. Throw 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. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
Address the 2026-07-27 Copilot review comments:
- AddPrimaryStorage.vue: set (or replace) the transport= query parameter
instead of blindly appending it, so a FlashArray URL that already carries
a hand-entered transport= is not left with two conflicting values.
- FlashArrayVolume: slice the EUI-128 serial with explicit substring(14, 24)
rather than substring(14). The length guard rejected short serials but a
longer-than-expected serial would previously yield an EUI over 32 hex
characters, which cannot match /dev/disk/by-id/nvme-eui.<eui>.
- FlashArrayAdapter: guard the Fibre Channel connection match against
hostnames with no dot. hostname.substring(0, hostname.indexOf('.')) would
throw StringIndexOutOfBoundsException when indexOf('.') == -1. Mirrors the
guard already present on the NVMe-TCP branch.
- MultipathNVMeOFAdapterBase: MapStorageUuidToStoragePool is now a
ConcurrentHashMap populated via computeIfAbsent, so concurrent callers
cannot race to create duplicate pool objects or corrupt the map.
- MultipathNVMeOFAdapterBase: split key=value tokens with a limit of 2 so a
value containing '=' is no longer silently discarded.
- MultipathNVMeOFAdapterBase: throttle rescanAllControllers() during
namespace discovery to once per 10s instead of on every 2s poll. Spawning
one nvme ns-rescan per controller every 2s is bursty on hosts with many
controllers; the first iteration still rescans immediately.
No functional change to the happy path.
Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
691649a to
ca9ca20
Compare
|
@DaanHoogland yes, ready from my side. Pushed
I pushed back on one item (replies inline): the placeholder The rebase picked up one conflict in 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 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFAdapterBase.java:261
- Processes started in
rescanAllControllers()have their output redirected but never consumed, and streams are not closed. In edge cases this can causenvmesubprocesses to block on a full pipe or accumulate resource usage under repeated polling. Consider explicitly draining/closing the process streams (or redirecting output to/dev/null), and afterdestroyForcibly()ensure the process is reaped (e.g., a follow-upwaitFor) to avoid zombie processes.
for (File ctrl : ctrls) {
Process p = new ProcessBuilder("nvme", "ns-rescan", "/dev/" + ctrl.getName())
.redirectErrorStream(true).start();
if (!p.waitFor(NS_RESCAN_TIMEOUT_SECS, TimeUnit.SECONDS)) {
// Kill runaway nvme-cli invocations so they do not pile
// up under the JVM on every poll iteration while we
// are still waiting for the namespace to appear.
LOGGER.debug("nvme ns-rescan /dev/" + ctrl.getName()
+ " did not complete within " + NS_RESCAN_TIMEOUT_SECS
+ "s; terminating");
p.destroyForcibly();
}
}
ui/src/views/infra/AddPrimaryStorage.vue:975
- The replacement only updates the first
transport=occurrence; an operator-provided URL that already contains multipletransport=parameters would still keep conflicting values after this change (contradicting the comment). Consider parsing the URL and query params (or applying a global cleanup) to ensure there is exactly onetransport=nvme-tcp, and also handle#fragmentURLs so the query parameter is inserted before the fragment.
if (values.protocol === 'NVMeTCP') {
// Set (or replace) the transport= query parameter rather than blindly appending,
// so a URL already carrying transport= from a hand-crafted operator entry is not
// left with two conflicting values.
if (/[?&]transport=/.test(url)) {
url = url.replace(/([?&])transport=[^&]*/, '$1transport=nvme-tcp')
} else {
url = url + (url.indexOf('?') === -1 ? '?' : '&') + 'transport=nvme-tcp'
}
}
| 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(); |
There was a problem hiding this 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.
| // 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(); |
There was a problem hiding this 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
00prefix - the Pure Storage OUI (
24a937) at offset 16, which is wheregetAddress()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.
| @Override | ||
| public boolean deletePhysicalDisk(String volumeUuid, Storage.ImageFormat format) { | ||
| return true; | ||
| } |
There was a problem hiding this 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.
Address the 2026-08-04 Copilot follow-up: - FlashArrayVolume.getAddress() now requires the serial to match exactly 24 hexadecimal characters instead of merely rejecting shorter ones. The previous form sliced serial[0:24], so two distinct serials sharing a 24-character prefix would have produced the same EUI-128 and therefore the same volume identity. Non-hex serials are now rejected as well. - FlashArrayAdapter.getVolumeByAddress() validates the FlashArray EUI-128 layout before reversing it into a serial: 32 hexadecimal characters, a 00 prefix, and the Pure Storage OUI at offset 16. Previously any 32-character string was accepted and deterministically mapped onto a volume serial, so a malformed or tampered address could resolve to an unintended volume. Verified against a real FlashArray namespace: EUI 006c1b16ce1c034d24a9371c05ab334a passes both checks and round-trips to serial 6C1B16CE1C034D1C05AB334A and back unchanged. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
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-tcpquery parameter to the pool URL oncreateStoragePool:When that parameter is present the adaptive lifecycle stamps the pool with the new
StoragePoolType.NVMeTCP, the KVM agent dispatches to a brand-newMultipathNVMeOFAdapterBase/NVMeTCPAdapterpair, 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 seven commits are split along natural seams (address type, FA REST-side support, storage pool type, KVM adapter, adaptive lifecycle routing, docs,
copyPhysicalDisk) so each can be reviewed independently.Why a separate
NVMeTCPpool type (and a separateMultipathNVMeOFAdapterBase) rather than reusingFiberChannel/MultipathSCSIAdapterBase?nvmedriver 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.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature. Opt-in via
transport=nvme-tcpURL 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:
cloudbr-nvmewith an IP on the NVMe subnet,nvme-cli+nvme_tcpkernel module, a persistent/etc/nvme/hostnqn, a populated/etc/nvme/discovery.confandnvme connect-allenabled at boot.cloudstack), a hostgroup matching the CloudStack cluster name (cluster1), one host per KVM host inside the hostgroup bound to the host's NQN.provider="Flash Array",transport=nvme-tcp,hostgroup=cluster1→ pool entersUpstate,type: NVMeTCP.tags=nvmedisk offering volume to a Rocky 9 VM: the volume's path carriedtype=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.mkfs.ext4 /dev/vdb, wrote 16 MiB of/dev/urandomwithconv=fsync, recorded SHA-256, unmounted/remounted, re-checksummed → hash matched.sha256sumprobe loop was running against/mnt/nvme/pattern.binevery 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).copyPhysicalDiskconverted the Rocky 9 cloud template qcow2 into a raw NVMe namespace (10 GB root), the VM booted from it, cloud-init injected an SSH key, and a 20 GBtags=nvmedata disk was attached.lsblkinside the guest showed bothvdaandvdbas NVMe-backed virtio block devices.vdaandvdbwith a known SHA-256, took acreateVMSnapshotwithquiescevm=true, snapshotmemory=false, deleted both sentinel files, issuedrevertToVMSnapshot, restarted, and confirmed both files reappeared with the identical SHA-256 content. Array-side snapshotscloudstack::vol-4-1-2-<id>.1for both volumes visible on Purity during the window. TheStorageVMSnapshotStrategypath is what CloudStack dispatches here, so any adaptive-plugin consumer gets the same behaviour.transport=parameter) continue to work —type: FiberChannel, FC WWN addressing, sameMultipathSCSIAdapterBasecode path as before.Notes
capacitybytes=oncreateStoragePoolis a workaround without flasharray: fall back to array capacity when pod has no quota #13050 merged. A companion fix sits in adaptive: honor user-provided capacityBytes when provider stats are unavailable #13059 (AdaptiveDataStoreLifeCycleImplhonouring user-supplied capacity when provider stats are null).snapshotmemory=trueis not yet supported on an NVMe-TCP pool (or on any managed pool):StorageVMSnapshotStrategy.canHandlein core CloudStack explicitly rejects memory snapshots. Disk-only snapshots (with or without quiesce) work. Lifting that restriction would be a separate feature PR touching core CloudStack, not the adaptive/NVMe plugin.