genegr commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3712514453
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayVolume.java:
##########
@@ -107,6 +111,22 @@ public AddressType getAddressType() {
@JsonIgnore
public String getAddress() {
if (serial == null) return null;
+ if (AddressType.NVMETCP.equals(addressType)) {
+ // EUI-128 layout for FlashArray NVMe namespaces:
+ // 00 + serial[0:14] + <Pure OUI (24a937)> + serial[14:24]
+ // This is the value the Linux kernel exposes as
+ // /dev/disk/by-id/nvme-eui.<result>
+ if (serial.length() < 24) {
+ throw new RuntimeException("FlashArray serial [" + serial
+ + "] is too short to build an NVMe EUI-128 address "
+ + "(expected 24 hex characters, got "
+ + serial.length() + ")");
+ }
+ // Slice exact ranges rather than substring(14) so a serial with
unexpected trailing
+ // characters cannot produce an EUI longer than 32 hex chars
(which would not match
+ // /dev/disk/by-id/nvme-eui.<eui> on Linux).
+ return ("00" + serial.substring(0, 14) + PURE_OUI_EUI +
serial.substring(14, 24)).toLowerCase();
Review Comment:
Good catch — you're right that my previous change traded one problem for
another. Rejecting only `length() < 24` while slicing `serial[0:24]` meant two
distinct serials sharing a 24-character prefix would collapse onto the same
EUI-128, which is a volume-identity bug rather than just a cosmetic one.
Fixed in `43a15b5857`: the serial must now match `[0-9a-fA-F]{24}` exactly,
so over-long and non-hex serials are both rejected up front.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -307,14 +345,24 @@ public ProviderVolume
getVolumeByAddress(ProviderAdapterContext context, Address
throw new RuntimeException("Invalid search criteria provided for
getVolumeByAddress");
}
- // only support WWN type addresses at this time.
- if (!ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ String serial;
+ if (ProviderVolume.AddressType.FIBERWWN.equals(addressType)) {
+ // Strip the NAA prefix (1 char) + Pure OUI to recover the volume
serial.
+ serial = address.substring(FlashArrayVolume.PURE_OUI.length() +
1).toUpperCase();
+ } else if (ProviderVolume.AddressType.NVMETCP.equals(addressType)) {
+ // Reverse the EUI-128 layout: serial = eui[2:16] + eui[22:32],
after
+ // stripping the optional "eui." prefix that appears in udev paths.
+ String eui = address.startsWith("eui.") ? address.substring(4) :
address;
+ if (eui == null || eui.length() != 32) {
+ throw new RuntimeException("Invalid NVMe-TCP EUI-128 address ["
+ + address + "]: expected 32 hex characters, got "
+ + (eui == null ? "null" :
String.valueOf(eui.length())));
+ }
+ serial = (eui.substring(2, 16) + eui.substring(22)).toUpperCase();
Review Comment:
Fixed in `43a15b5857`. `getVolumeByAddress` now validates the full
FlashArray EUI-128 layout before reversing it into a serial:
- exactly 32 hexadecimal characters (was: any 32-character string)
- a `00` prefix
- the Pure Storage OUI (`24a937`) at offset 16, which is where
`getAddress()` places it
Sanity-checked the offsets against a real namespace from my lab:
`006c1b16ce1c034d24a9371c05ab334a` passes all three checks and round-trips to
serial `6C1B16CE1C034D1C05AB334A` and back unchanged.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathNVMeOFPool.java:
##########
@@ -0,0 +1,157 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.storage;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cloudstack.utils.qemu.QemuImg;
+import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
+import org.joda.time.Duration;
+
+import com.cloud.agent.api.to.HostTO;
+import com.cloud.hypervisor.kvm.resource.KVMHABase.HAStoragePool;
+import com.cloud.storage.Storage;
+import com.cloud.storage.Storage.ProvisioningType;
+
+/**
+ * KVMStoragePool for NVMe-over-Fabrics pools. Mirror of
+ * {@link MultipathSCSIPool} for adapters based on
+ * {@link MultipathNVMeOFAdapterBase}. Every data operation is delegated
+ * back to the adapter; the pool itself only tracks addressing/identity.
+ */
+public class MultipathNVMeOFPool implements KVMStoragePool {
+ private final String uuid;
+ private final String sourceHost;
+ private final int sourcePort;
+ private final String sourceDir;
+ private final Storage.StoragePoolType storagePoolType;
+ private final StorageAdaptor storageAdaptor;
+ private final Map<String, String> details;
+ private long capacity;
+ private long used;
+ private long available;
+
+ public MultipathNVMeOFPool(String uuid, String host, int port, String path,
+ Storage.StoragePoolType poolType, Map<String, String> poolDetails,
StorageAdaptor adaptor) {
+ this.uuid = uuid;
+ this.sourceHost = host;
+ this.sourcePort = port;
+ this.sourceDir = path;
+ this.storagePoolType = poolType;
+ this.storageAdaptor = adaptor;
+ this.details = poolDetails;
+ this.capacity = 0;
+ this.used = 0;
+ this.available = 0;
+ }
+
+ public MultipathNVMeOFPool(String uuid, StorageAdaptor adaptor) {
+ this.uuid = uuid;
+ this.sourceHost = null;
+ this.sourcePort = -1;
+ this.sourceDir = null;
+ this.storagePoolType = Storage.StoragePoolType.NVMeTCP;
+ this.storageAdaptor = adaptor;
+ this.details = new HashMap<>();
+ this.capacity = 0;
+ this.used = 0;
+ this.available = 0;
+ }
+
+ @Override
+ public KVMPhysicalDisk createPhysicalDisk(String volumeUuid,
ProvisioningType provisioningType, long size, byte[] passphrase) {
+ return null;
+ }
+
+ @Override
+ public KVMPhysicalDisk createPhysicalDisk(String volumeUuid,
PhysicalDiskFormat format, ProvisioningType provisioningType, long size, byte[]
passphrase) {
+ return null;
+ }
+
+ @Override
+ public boolean connectPhysicalDisk(String volumeUuid, Map<String, String>
details) {
+ return storageAdaptor.connectPhysicalDisk(volumeUuid, this, details,
false);
+ }
+
+ @Override
+ public KVMPhysicalDisk getPhysicalDisk(String volumeId) {
+ return storageAdaptor.getPhysicalDisk(volumeId, this);
+ }
+
+ @Override
+ public boolean disconnectPhysicalDisk(String volumeUuid) {
+ return storageAdaptor.disconnectPhysicalDisk(volumeUuid, this);
+ }
+
+ @Override
+ public boolean deletePhysicalDisk(String volumeUuid, Storage.ImageFormat
format) {
+ return true;
+ }
Review Comment:
I'd like to keep this one as `return true`, for two reasons.
**It matches the sibling adapter.** `MultipathSCSIPool.deletePhysicalDisk`
(the FC/iSCSI class this one is modelled on) is byte-identical — `return true;`
— as is its `listPhysicalDisks() { return null; }`. Changing only the NVMe
class would leave the two out of step.
**Nothing is actually leaked here.** For managed storage the namespace is
deleted on the *provider* side: the management server drives
`FlashArrayAdapter.delete()`, which is what issues the destroy against the
array. The KVM agent's pool-level `deletePhysicalDisk` exists for
hypervisor-local disk *files* (a qcow2 on NFS, an LV on CLVM); an NVMe-TCP
namespace is not a host-local object, so there is genuinely nothing for this
method to do. It isn't "reporting success for work it skipped" so much as "no
host-side artifact to remove".
Worth noting too that all three in-tree callers ignore the return value —
`KVMStorageProcessor.deleteVolume` (twice) and `deleteBackup` call it for side
effects only and catch just `CloudRuntimeException`. So `true` vs `false`
changes nothing observable, while throwing `UnsupportedOperationException`
*would* change behaviour: it would turn a harmless no-op into a hard failure on
the volume-delete and backup-delete paths.
Happy to switch both classes together if a maintainer would rather have the
stricter contract — I just don't want to introduce a new failure mode in this
PR for a path I can't exercise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
