Copilot commented on code in PR #13061:
URL: https://github.com/apache/cloudstack/pull/13061#discussion_r3711135887
##########
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:
For NVMETCP, `getVolumeByAddress` accepts any 32-character string and
deterministically maps it into a serial without validating the expected
FlashArray EUI-128 layout (e.g., leading `00` and the embedded Pure OUI at the
expected offset). This makes it easier for a malformed/tampered address to
resolve to an unintended volume serial. Recommend validating the EUI structure
(prefix + expected OUI segment, and hex-only characters) before deriving the
serial.
##########
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:
The code expects a 24-hex-character serial (per the exception
message/comments) but only rejects serials shorter than 24. If a longer serial
ever appears, the EUI will be derived from only the first 24 chars, which can
cause collisions (two different serials with the same first 24 chars map to the
same EUI) and break volume identity. Consider enforcing `serial.length() == 24`
(and optionally validating hex) when building the EUI-128 address.
##########
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:
This method currently returns `true` unconditionally, which tells callers
the disk was deleted even though no delete occurred. That can lead to incorrect
orchestration state and leaked namespaces on the provider. Instead, either
delegate to the adaptor (and let it throw/handle unsupported deletion) or throw
an `UnsupportedOperationException`/return `false` to avoid reporting a
successful deletion.
--
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]
