Conversation
Member
|
@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. |
Contributor
Author
|
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 |
Contributor
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.
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 575302 +421
Branches 70351 70413 +62
============================================
- Hits 112970 112969 -1
- Misses 449639 450061 +422
Partials 12272 12272
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:
|
genegr
added a commit
to genegr/cloudstack
that referenced
this pull request
Apr 23, 2026
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>
Contributor
Author
|
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
Contributor
Author
|
Pushed Code fixes
Extra fix in the same areaWhile wiring Verified end-to-end on an NVMe-TCP pool: snapshotting a volume now persists |
Contributor
|
@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 |
Contributor
Author
|
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 |
Contributor
|
@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 |
Contributor
|
@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)
|
Contributor
Contributor
|
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):
|
abh1sar
reviewed
Sep 9, 2026
| size while a stripped down Linux should fit on a 2.88MB floppy. | ||
|
|
||
|
|
||
| 4.22.0 > 4.23.0: |
Contributor
There was a problem hiding this comment.
This should be 4.23.0 -> 24.0
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
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>
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>
genegr
force-pushed
the
feat/flasharray-nvme-tcp-support
branch
from
September 24, 2026 17:54
43a15b5 to
6df04e8
Compare
Resize of a volume on an NVMe-TCP pool previously failed. The provider half
worked -- FlashArrayAdapter.resize() PATCHes the array to grow the namespace --
but the KVM half was never wired up:
* LibvirtResizeVolumeCommandWrapper short-circuits MultipathSCSIPool via
handleMultipathSCSIResize(), but MultipathNVMeOFPool implements
KVMStoragePool directly rather than extending MultipathSCSIPool, so it
fell through.
* It then reached LibvirtComputingResource.getResizeScriptType(), which has
no NVMeTCP case, and threw
"Cannot determine resize type from pool type NVMeTCP".
* MultipathNVMeOFAdapterBase.resize() threw UnsupportedOperationException.
Wire up the host-side half, mirroring how the Fibre Channel path is handled
(FC shipped resize in its own initial PR, apache#7889):
* LibvirtResizeVolumeCommandWrapper: dispatch MultipathNVMeOFPool to a new
handleMultipathNVMeOFResize(), alongside the existing SCSI branch.
* MultipathNVMeOFPool.resize(): delegate to the adaptor, mirroring
MultipathSCSIPool.resize().
* MultipathNVMeOFAdapterBase.resize(): issue nvme ns-rescan so the kernel
observes the grown namespace without waiting for an asynchronous event
notification from the target, poll until the new size is visible, then
virsh blockresize so a running guest sees the capacity without a reboot.
Unlike the SCSI path there is no device-mapper map to grow, so no helper
script is needed; the existing rescanAllControllers() is reused.
If the namespace does not report the new size within the settle timeout we
log a warning rather than throwing: the array has already been grown at that
point, and failing here would make the management server roll back a resize
that actually happened.
The guest-notification lookup matches the domain disk source against the
/dev/disk/by-id symlink, its canonicalised /dev/nvmeXnY target, or the bare
EUI, because libvirt may report any of those forms.
Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
validateVolumeResizeWithSize() refuses to resize a volume on a managed pool while its VM is running, unless the pool type is on an allowlist. That list held PowerFlex and FiberChannel but not NVMeTCP, so resizing an attached NVMe-TCP volume on a running KVM instance failed at the API layer with "This kind of KVM disk cannot be resized while it is connected to a VM that's not in the Stopped state." before any host-side work was attempted. Add NVMeTCP alongside FiberChannel. The two transports are equivalent here: in both cases the provider grows the volume on the array and the KVM agent then makes the new capacity visible to the guest, which the preceding commit implements for NVMe-oF. Found by running the test matrix @abh1sar requested on apache#13061: offline resize (VM Stopped) already worked end to end -- CloudStack, the array and the guest all agreed on the new size -- and this guard was the only thing blocking the online case. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When Fibre Channel managed storage was added (apache#7889) the FiberChannel pool type was added to a number of allowlists in shared managed-storage code. The NVMe-TCP work added the pool type, the KVM adaptor and the lifecycle pivot but not those allowlists, so several managed-storage operations silently did not work on an NVMe-TCP pool even though the equivalent Fibre Channel operation did. StorageSystemDataMotionStrategy: * verifyFormatWithPoolType(): RAW was accepted only on PowerFlex and FiberChannel, so any data-motion of a RAW NVMe-TCP volume was rejected outright. Message updated to match. * handleCreateTemplateFromManagedVolume(): same format check, which made createTemplate from an NVMe-TCP volume fail with the (misleading) "When using managed storage, 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 for the copy. Both sites are updated together so access is still revoked in the finally block. * getSnapshotDetails(): PowerFlex and FiberChannel pass the snapshot path straight through as the device address, which is also correct for array-native NVMe-TCP snapshots. ApiDBUtils.getHypervisorTypeFromZone(): NVMeTCP is a KVM-only pool type, so its presence in a zone should imply KVM in the same way the other non-QCOW2 pool types already do. StatsCollector's volume-stats format check deliberately not touched: RAW is already in its accepted-format list, so the guard never fires for NVMe-TCP volumes and adding the pool type there would be dead weight. Found by working through the test matrix @abh1sar asked for on apache#13061, which surfaced these as a family rather than one at a time. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Access to a managed volume is granted and revoked one host at a time:
AdaptiveDataStoreDriverImpl.grantAccess() calls attach(volume, hostname) and
revokeAccess() calls detach(volume, hostname). Neither the adapter context nor
the data object carries the VM or its power state, so the adapter cannot tell
why access is being revoked.
attach() did not follow that model for NVMe-TCP. When a hostgroup was
configured it created a single host-group scoped connection shared by every
member host, while detach() still ran per host and deleted that shared
connection. The two are irreconcilable, and live migration shows why:
grantAccess(V, dst) -> POST /connections?host_group_names=cluster1 ("already exists")
... guest migrates and is 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 V is revoked from the whole
group including dst, the namespace disappears under the running guest, and with
werror=stop the VM freezes.
Group scoping was justified in a comment as giving "a consistent NSID visible to
every member host", but the NSID is not used to find the namespace: it is
located by its EUI-128 address (FlashArrayVolume.getAddress()), as the comment
20 lines further down already says. So the justification does not hold.
Connect and disconnect per host instead, which is what Fibre Channel has always
done in this same adapter and what the interface can express. The hostgroup
parameter remains accepted and is now used only where it was documented to be
used -- removing leftover group connections when a volume is deleted -- matching
the "retrieve for legacy purposes" comment on the field. In the "Connection
already exists" path a group-scoped match is demoted to a fallback so volumes
still carrying a group connection from an earlier release keep resolving.
Found while working through the test matrix @abh1sar asked for on
apache#13061, as the cause of the live-migration failure.
Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FlashArrayVolume defaults addressType to FIBERWWN in its constructor, so a
volume deserialised from the array reports an FC-style NAA WWN from getAddress()
until the pool's real address type is stamped onto it. Most accessors do that
via withAddressType(), but two paths returned an unstamped volume:
* revert() returned the private getVolume() result directly, so reverting a
snapshot on an NVMe-TCP pool produced a volume carrying an FC-style WWN.
* copy() returned the newly created volume unstamped. The adaptive driver
persists this one -- copyAsync() stores generatePathInfo(outVolume, ...) --
so the volume was recorded in the database with an address the host cannot
resolve, since NVMe namespaces are located by EUI-128.
Rather than add a third withAddressType() call at each site, stamp inside the
private getVolume() accessor so every path is correct by construction. That
matches the private getSnapshot() accessor, which already did this, and makes
the wrap at the public getVolume() call site redundant. copy() now reuses the
accessor for its source lookup instead of repeating the same GET inline, and
stamps the volume it hands back.
No behavioural change for Fibre Channel: FIBERWWN is what the constructor
already defaulted to, so stamping it is a no-op there.
Also drops a comment in getConnectionIdMap() describing connections as
host-group scoped, which is no longer how NVMe-TCP volumes are connected after
the preceding commit. The surrounding logic was already keyed on host name and
is unchanged; only nsid vs lun differs between the transports.
Found by auditing every NVMe-TCP divergence from the Fibre Channel path, on the
principle that NVMe-TCP is only a different carrier: the array operations are
identical and the host-side difference is an NQN instead of a WWN, which lives
on the array and is never touched by this adapter (hosts are resolved by name).
Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NVMe-TCP is a different carrier for the same array operations, so the NVMe-oF
storage adaptor should only diverge from MultipathSCSIAdapterBase where the
transport genuinely differs. Several unimplemented methods threw
UnsupportedOperationException where the Fibre Channel adaptor returns a benign
value, which means a caller that degrades gracefully on Fibre Channel aborts on
NVMe-TCP:
* deletePhysicalDisk(): FC returns false. Namespaces are created and destroyed
by the storage provider, so a host-side delete is simply not handled here;
throwing from a cleanup path is worse than reporting that.
* createTemplateFromDisk(), listPhysicalDisks(): FC returns null.
* createFolder(): FC logs and returns true. Block storage has no directory
structure, so this is a no-op rather than an error.
createPhysicalDisk(), createDiskFromTemplate() and createDiskFromTemplateBacking()
still throw, because the Fibre Channel adaptor throws for those too.
createTemplateFromDirectDownloadFile() also still throws, but the message now
states the limitation instead of reading as an oversight. The Fibre Channel
implementation resolves the downloaded file with destPool.getPhysicalDisk(),
which on this adaptor requires a "type=NVMETCP;address=..." path and therefore
cannot describe a plain local file. Supporting it needs a separate way to present
the local file as the copy source, which is left for follow-up rather than
guessed at here.
No functional change to the connect/disconnect paths. The comment on
disconnectPhysicalDisk() is updated because it described connections as
host-group scoped, which is no longer how NVMe-TCP volumes are connected, and to
record why the host-side no-op is correct: the kernel drops the namespace when
the target removes the connection, whereas Fibre Channel must flush its
device-mapper entry.
Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
createTemplateFromDirectDownloadFile() threw UnsupportedOperationException, so a
template registered with directdownload=true could not be deployed onto an
NVMe-TCP pool: the download itself succeeded and the deployment then failed on
the host.
The two path arguments are different kinds of thing, and conflating them is what
made this look harder than it is. templateFilePath is a plain local file produced
by the direct-download helper, while destTemplatePath is a managed volume path of
the form "type=NVMETCP;address=...". Only the destination may be resolved through
KVMStoragePool.getPhysicalDisk(); handing the local file to that would pass a file
name to parseAndValidatePath() and fail. KVMStorageProcessor has already issued
connectPhysicalDisk() for the destination by the time we are called, so the
namespace is present.
Worth recording that the Fibre Channel implementation does conflate them. It calls
destPool.getPhysicalDisk(templateFilePath), and FiberChannelAdapter's
parseAndValidatePath() treats any string without a ";" as a bare WWN, so a local
file path becomes "/dev/mapper/3" + that path and the lookup cannot succeed. 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. Three
reasons: it matches ScaleIO; it agrees with the QCOW2 format the template is
registered with, so stored content and declared format do not contradict each
other; and KVMStorageProcessor runs Qcow2Inspector.validateQcow2File() on the path
we return, which for a block device that exists would fail on RAW content and then
try to delete the device.
The disk returned to the caller is constructed rather than being the one
getPhysicalDisk() handed back, because its *name* matters and not just its path.
KVMStorageProcessor puts disk.getName() into the DirectDownloadAnswer, which
becomes the template's install path in template_spool_ref and later its external
name, and the provider interpolates that external name into array REST calls.
getPhysicalDisk() names a disk AddressInfo.toString(), e.g.
AddressInfo NVMETCP [address=006c..., connectionId=2172, path=/dev/disk/by-id/...]
Recording that as the install path makes the first deployment from a freshly
registered template fail: revokeAccess() -> detach() builds
"/connections?host_names=kvm01&volume_names=cloudstack2::<external name>" and
URI.create() rejects the spaces and brackets with "Illegal character in query".
The surfaced error is a misleading "Failed to update state", because the resulting
CloudRuntimeException is followed by applying OperationFailed to a template that
has already reached Ready. Naming the returned disk with the managed volume path
gives the same install path shape a volume records:
type=NVMETCP; address=006c...; providerName=cloudstack2::tpl-9-0-2-210; providerID=...
Also handles compressed templates via TemplateDownloaderUtil, since
direct-download templates are commonly published that way, and refuses up front if
the template will not fit the namespace rather than failing part way through the
convert.
Tested on a two-host KVM cluster against a FlashArray over NVMe-TCP: registered a
QCOW2 template with directdownload=true and deployed a VM from it, on the first
attempt, with a storage offering tagged for the NVMe-TCP pool:
ROOT-32 Everpure X90 NVMe-TCP pool2 Ready
type=NVMETCP; address=006c1b16ce1c034d24a9371c0623b125
Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.