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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
