genegr commented on PR #13061: URL: https://github.com/apache/cloudstack/pull/13061#issuecomment-4305048423
Pushed `723bf1445f kvm/flasharray: address review feedback on NVMe-TCP PR` to address this round of feedback: ### Code fixes - **`NVMeTCPAdapter` logger rename** — `LOGGER_NVMETCP` → `LOGGER` to match the project convention (@sureshanaparti). - **`rescanAllControllers()` now honours `waitFor(...)`** and `destroyForcibly()`s the `nvme ns-rescan` process on timeout, so hung rescans cannot accumulate under load (@Copilot). - **`Integer.parseInt(waitTime)` is now guarded** in `connectPhysicalDisk()` — a non-integer `STORAGE_POOL_DISK_WAIT` detail no longer aborts the connect; we log a warning and fall back to `DEFAULT_DISK_WAIT` (@Copilot). - **Retry path recognises host-scoped NVMe-TCP connections** — the "Connection already exists" fallback was only matching host-group connections, so a `transport=nvme-tcp` pool without `hostgroup` would never hit its retry branch. Now checks both (@Copilot). - **Attach-failure error message is transport-agnostic** — the message used to say "did not return a LUN"; under NVMe-TCP we return an NSID. Reworded to "did not return connection information (lun/nsid)" so NVMe-TCP debugging isn't misleading (@Copilot). - **EUI-128 length validation in `getVolumeByAddress()`** — NVMe-TCP path no longer blind-slices `substring(2,16)/substring(22)`; an address that isn't a 32-hex EUI-128 is now rejected explicitly rather than surfacing as a mislabelled "not found" via `StringIndexOutOfBoundsException` (@Copilot). - **`FlashArrayVolume` serial length check** — NGUID construction now validates `serial.length() == 24` before slicing, and fails fast with a clear message instead of throwing `StringIndexOutOfBoundsException` deep in the address getter (@Copilot). - **`getSnapshot()` now applies `withAddressType(...)`** so the returned `FlashArrayVolume` carries the pool's transport type (NVMe-TCP vs FC) — without this, `ProviderSnapshot.getAddress()` on an NVMe-TCP pool emitted an FC-style WWN instead of the EUI-128, and adaptive's `takeSnapshot` / `revertSnapshot` paths persisted the wrong address (@Copilot). ### Extra fix in the same area While wiring `withAddressType(...)` into `getSnapshot()` I noticed the same pattern was **also missing in `snapshot()`** (the method that actually *creates* the snapshot), and in the other `getSnapshot()` overload. Both now apply `withAddressType(...)` as well, so the address stored at snapshot-create time is already the NVMe EUI-128 on an NVMe-TCP pool rather than the FC-WWN default set by `FlashArrayVolume`'s constructor. Verified end-to-end on an NVMe-TCP pool: snapshotting a volume now persists `install_path=006c1b16ce1c034d24a9371c05ab334a` (NVMe EUI-128) rather than the previous `624a93706c1b16ce1c034d1c05ab3346` (FC WWN), and subsequent `revertSnapshot` uses the correct transport-specific address. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
