Copilot commented on code in PR #13053:
URL: https://github.com/apache/cloudstack/pull/13053#discussion_r3122476006
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:
##########
@@ -19,58 +19,137 @@
package org.apache.cloudstack.storage.utils;
-import com.cloud.storage.ScopeType;
+import com.cloud.exception.InvalidParameterValueException;
import com.cloud.utils.StringUtils;
import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.cloudstack.storage.feign.model.Lun;
+import org.apache.cloudstack.storage.feign.model.LunSpace;
import org.apache.cloudstack.storage.feign.model.OntapStorage;
+import org.apache.cloudstack.storage.feign.model.Svm;
import org.apache.cloudstack.storage.provider.StorageProviderFactory;
import org.apache.cloudstack.storage.service.StorageStrategy;
+import org.apache.cloudstack.storage.service.model.CloudStackVolume;
import org.apache.cloudstack.storage.service.model.ProtocolType;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.springframework.util.Base64Utils;
-
-import java.nio.charset.StandardCharsets;
import java.util.Map;
public class OntapStorageUtils {
private static final Logger logger =
LogManager.getLogger(OntapStorageUtils.class);
-
private static final String BASIC = "Basic";
private static final String AUTH_HEADER_COLON = ":";
+ /**
+ * Method generates authentication headers using storage backend
credentials passed as normal string
+ *
+ * @param username -->> username of the storage backend
+ * @param password -->> normal decoded password of the storage backend
+ * @return
+ */
public static String generateAuthHeader (String username, String password)
{
- byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON
+ password).getBytes(StandardCharsets.UTF_8));
+ byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON
+ password).getBytes());
return BASIC + StringUtils.SPACE + new String(encodedBytes);
Review Comment:
`generateAuthHeader` uses `String.getBytes()` and `new String(byte[])`
without an explicit charset. This makes auth header generation platform-default
dependent and can break for non-ASCII credentials on systems with non-UTF-8
defaults. Please use `StandardCharsets.UTF_8` explicitly for both conversions.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/provider/StorageProviderFactory.java:
##########
@@ -36,23 +39,25 @@ public class StorageProviderFactory {
public static StorageStrategy getStrategy(OntapStorage ontapStorage) {
ProtocolType protocol = ontapStorage.getProtocol();
logger.info("Initializing StorageProviderFactory with protocol: " +
protocol);
+ String decodedPassword = new
String(java.util.Base64.getDecoder().decode(ontapStorage.getPassword()),
StandardCharsets.UTF_8);
+ ontapStorage = new OntapStorage(
+ ontapStorage.getUsername(),
+ decodedPassword,
+ ontapStorage.getStorageIP(),
+ ontapStorage.getSvmName(),
+ ontapStorage.getSize(),
+ protocol);
Review Comment:
`StorageProviderFactory.getStrategy` unconditionally Base64-decodes
`ontapStorage.getPassword()`. This will throw an IllegalArgumentException if
the password is provided in plain text (e.g., via API clients or existing DB
data), making pool creation/attachment fail. Consider either storing a clear
contract for password encoding (and enforcing it at input), or decoding
conditionally (try/catch with fallback to the original string).
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -490,16 +504,19 @@ public boolean deleteDataStore(DataStore store) {
storagePoolId, e.getMessage(), e);
}
AccessGroup accessGroup = new AccessGroup();
- accessGroup.setPrimaryDataStoreInfo(primaryDataStoreInfo);
+ accessGroup.setStoragePoolId(storagePoolId);
+ // Delete access groups associated with this storage pool
storageStrategy.deleteAccessGroup(accessGroup);
logger.info("deleteDataStore: Successfully deleted access groups
for storage pool '{}'", storagePool.getName());
Review Comment:
`deleteDataStore` calls `storageStrategy.deleteAccessGroup(accessGroup)` but
the `AccessGroup` instance only has `storagePoolId` set; `hostsToConnect` is
never populated. The SAN implementation
(`UnifiedSANStrategy.deleteAccessGroup`) iterates
`accessGroup.getHostsToConnect()` and will throw at runtime for iSCSI pools.
Either populate `hostsToConnect` (e.g., from the cluster/zone scope or
storage_pool_host_ref) before calling deleteAccessGroup, or make SAN
deleteAccessGroup handle a null/empty host list safely (and/or skip SAN
access-group deletion here if it cannot be determined).
```suggestion
// Delete access groups associated with this storage pool only
when host information is available.
// Some SAN strategy implementations iterate hostsToConnect and
can fail at runtime if it is null.
if (accessGroup.getHostsToConnect() == null ||
accessGroup.getHostsToConnect().isEmpty()) {
logger.warn("deleteDataStore: Skipping access-group deletion
for storage pool '{}' because hostsToConnect is not populated",
storagePool.getName());
} else {
storageStrategy.deleteAccessGroup(accessGroup);
logger.info("deleteDataStore: Successfully deleted access
groups for storage pool '{}'", storagePool.getName());
}
```
##########
server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java:
##########
@@ -390,6 +390,15 @@ public VMSnapshot allocVMSnapshot(Long vmId, String
vsDisplayName, String vsDesc
//Other Storage volume plugins could integrate this with their own
functionality for group snapshots
VMSnapshotStrategy snapshotStrategy =
storageStrategyFactory.getVmSnapshotStrategy(userVmVo.getId(),
rootVolumePool.getId(), snapshotMemory);
if (snapshotStrategy == null) {
+ // Check if this is ONTAP managed storage with memory snapshot
request - provide specific error message
+ if (snapshotMemory && rootVolumePool.isManaged() &&
+
"ONTAP".equals(rootVolumePool.getStorageProviderName())) {
+ String message = String.format("Memory snapshots
(snapshotmemory=true) are not supported for VMs on ONTAP managed storage. " +
+ "Instance [%s] uses ONTAP storage which only
supports disk-only (crash-consistent) snapshots. " +
+ "Please use snapshotmemory=false for disk-only
snapshots.", userVmVo.getUuid());
+ logger.error(message);
+ throw new CloudRuntimeException(message);
Review Comment:
The provider-name check uses the literal string "ONTAP", but elsewhere in
this PR the provider is identified as "NetApp ONTAP" (UI,
KvmFileBasedStorageVmSnapshotStrategy, OntapStorageConstants). As-is, this
branch will never trigger and the error message for unsupported memory
snapshots on ONTAP managed storage will be missed. Consider comparing against a
shared constant (e.g., OntapStorageConstants.ONTAP_PLUGIN_NAME) or updating the
literal to the actual provider name used by storage pools.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java:
##########
@@ -87,11 +99,7 @@ public boolean hostConnect(long hostId, long poolId) {
"Unable to establish a connection from agent to
storage pool %s due to %s", pool, answer.getDetails()));
}
- if (!(answer instanceof ModifyStoragePoolAnswer)) {
- logger.error("Received unexpected answer type {} for storage
pool {}", answer.getClass().getName(), pool.getName());
- throw new CloudRuntimeException("Failed to connect to storage
pool. Please check agent logs for details.");
- }
-
+ // Get the mount path from the answer
Review Comment:
`hostConnect` casts `answer` to `ModifyStoragePoolAnswer` without checking
`instanceof`. If the agent returns a different `Answer` type, this will throw
`ClassCastException` and be caught by the broad catch, losing the real failure
reason. Please restore an explicit type check (and return a clear error) before
casting.
```suggestion
// Get the mount path from the answer
if (!(answer instanceof ModifyStoragePoolAnswer)) {
throw new CloudRuntimeException(String.format(
"Unexpected answer type %s returned for modify
storage pool command for pool %s on host %d",
answer.getClass().getName(), pool, hostId));
}
```
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -411,18 +420,18 @@ private boolean
validateProtocolSupportAndFetchHostsIdentifier(List<HostVO> host
for (HostVO host : hosts) {
if (host != null) {
ip = host.getStorageIpAddress() != null ?
host.getStorageIpAddress().trim() : "";
- if (ip.isEmpty()) {
- if (host.getPrivateIpAddress() == null ||
host.getPrivateIpAddress().trim().isEmpty()) {
- return false;
- }
- ip = host.getPrivateIpAddress().trim();
+ if (ip.isEmpty() && host.getPrivateIpAddress() != null
|| host.getPrivateIpAddress().trim().isEmpty()) {
+ // TODO we will inform customer through alert for
excluded host because of protocol enabled on host
+ continue;
+ } else {
+ ip = ip.isEmpty() ?
host.getPrivateIpAddress().trim() : ip;
}
Review Comment:
In NFS3 host identifier validation, the condition `if (ip.isEmpty() &&
host.getPrivateIpAddress() != null ||
host.getPrivateIpAddress().trim().isEmpty())` has incorrect operator precedence
and can throw a NullPointerException when privateIpAddress is null. It also
allows `hostIdentifiers.add(ip)` to reuse the previous host's IP when `host` is
null. Please restructure this block to (a) short-circuit correctly when private
IP is null/blank, and (b) only add an IP for the current host (skip null hosts
and reset `ip` per iteration).
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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 org.apache.cloudstack.storage.feign.model;
+
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class VolumeConcise {
+ @JsonProperty("uuid")
+ private String uuid;
+ @JsonProperty("name")
+ private String name;
+ public String getUuid() {
+ return uuid;
+ }
+ public void setUuid(String uuid) {
+ this.uuid = uuid;
+ }
+ public String getName() {
+ return name;
+ }
+ public void setName(String name) {}
Review Comment:
`VolumeConcise.setName` is a no-op, so JSON deserialization will never
populate `name`. This can lead to null names in `FlexVolSnapshot.volume` and
any logic relying on it. Implement the setter (or remove it and make the field
immutable with constructor-based binding).
```suggestion
public void setName(String name) {
this.name = name;
}
```
##########
plugins/storage/volume/ontap/pom.xml:
##########
@@ -121,12 +122,24 @@
<version>${mockito.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>net.bytebuddy</groupId>
+ <artifactId>byte-buddy-agent</artifactId>
+ <version>${byte-buddy-agent.version}</version>
+ <scope>test</scope>
+ </dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<version>${assertj.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.cloudstack</groupId>
+ <artifactId>cloud-engine-storage-snapshot</artifactId>
+ <version>4.23.0.0-SNAPSHOT</version>
Review Comment:
The dependency on `cloud-engine-storage-snapshot` is pinned to
`4.23.0.0-SNAPSHOT` while the rest of the module uses `${project.version}`.
This makes backports/version bumps harder and can break builds if the parent
version changes. Prefer `${project.version}` (or rely on dependencyManagement)
for intra-repo artifacts.
```suggestion
<version>${project.version}</version>
```
--
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]
