Skip to content

Deal with Storage Manager tech debt #12321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
DaanHoogland wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Deal with Storage Manager tech debt #12321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions api/src/main/java/com/cloud/storage/StorageService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

package com.cloud.storage;

import java.net.UnknownHostException;
import java.util.Map;

import org.apache.cloudstack.api.command.admin.storage.CancelPrimaryStorageMaintenanceCmd;
Expand All @@ -35,10 +34,8 @@
import org.apache.cloudstack.api.command.admin.storage.UpdateStoragePoolCmd;

import com.cloud.exception.DiscoveryException;
import com.cloud.exception.InsufficientCapacityException;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.exception.PermissionDeniedException;
import com.cloud.exception.ResourceInUseException;
import com.cloud.exception.ResourceUnavailableException;
import org.apache.cloudstack.api.command.admin.storage.heuristics.CreateSecondaryStorageSelectorCmd;
import org.apache.cloudstack.api.command.admin.storage.heuristics.RemoveSecondaryStorageSelectorCmd;
Expand All @@ -55,12 +52,9 @@ public interface StorageService {
* storage pool.
* @return
* The StoragePool created.
* @throws ResourceInUseException
* @throws IllegalArgumentException
* @throws UnknownHostException
* @throws ResourceUnavailableException
*/
StoragePool createPool(CreateStoragePoolCmd cmd) throws ResourceInUseException, IllegalArgumentException, UnknownHostException, ResourceUnavailableException;
StoragePool createPool(CreateStoragePoolCmd cmd) throws IllegalArgumentException;

ImageStore createSecondaryStagingStore(CreateSecondaryStagingStoreCmd cmd);

Expand All @@ -79,10 +73,8 @@ public interface StorageService {
* @param primaryStorageId
* - the primaryStorageId
* @return the primary storage pool
* @throws ResourceUnavailableException
* @throws InsufficientCapacityException
*/
StoragePool preparePrimaryStorageForMaintenance(Long primaryStorageId) throws ResourceUnavailableException, InsufficientCapacityException;
StoragePool preparePrimaryStorageForMaintenance(Long primaryStorageId);

/**
* Complete maintenance for primary storage
Expand All @@ -108,7 +100,7 @@ public interface StorageService {

boolean deleteSecondaryStagingStore(DeleteSecondaryStagingStoreCmd cmd);

ImageStore discoverImageStore(String name, String url, String providerName, Long zoneId, Map details) throws IllegalArgumentException, DiscoveryException, InvalidParameterValueException;
ImageStore discoverImageStore(String name, String url, String providerName, Long zoneId, Map<String, String> details) throws IllegalArgumentException, InvalidParameterValueException;

/**
* Migrate existing NFS to use object store.
Expand All @@ -134,7 +126,7 @@ public interface StorageService {

void removeSecondaryStorageHeuristic(RemoveSecondaryStorageSelectorCmd cmd);

ObjectStore discoverObjectStore(String name, String url, Long size, String providerName, Map details) throws IllegalArgumentException, DiscoveryException, InvalidParameterValueException;
ObjectStore discoverObjectStore(String name, String url, Long size, String providerName, Map<String, String> details) throws IllegalArgumentException, DiscoveryException, InvalidParameterValueException;

boolean deleteObjectStore(DeleteObjectStoragePoolCmd cmd);

Expand Down
24 changes: 9 additions & 15 deletions api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddSecondaryStorageCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.apache.cloudstack.api.response.ImageStoreResponse;
import org.apache.cloudstack.api.response.ZoneResponse;

import com.cloud.exception.DiscoveryException;
import com.cloud.storage.ImageStore;
import com.cloud.user.Account;

Expand Down Expand Up @@ -67,20 +66,15 @@ public long getEntityOwnerId() {

@Override
public void execute(){
try{
ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), null);
ImageStoreResponse storeResponse = null;
if (result != null ) {
storeResponse = _responseGenerator.createImageStoreResponse(result);
storeResponse.setResponseName(getCommandName());
storeResponse.setObjectName("secondarystorage");
setResponseObject(storeResponse);
} else {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add secondary storage");
}
} catch (DiscoveryException ex) {
logger.warn("Exception: ", ex);
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), null);
ImageStoreResponse storeResponse = null;
if (result != null ) {
storeResponse = _responseGenerator.createImageStoreResponse(result);
storeResponse.setResponseName(getCommandName());
storeResponse.setObjectName("secondarystorage");
setResponseObject(storeResponse);
} else {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add secondary storage");
}
}
}
32 changes: 12 additions & 20 deletions api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import java.util.Collection;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;


Expand All @@ -31,7 +30,6 @@
import org.apache.cloudstack.api.response.ImageStoreResponse;
import org.apache.cloudstack.api.response.ZoneResponse;

import com.cloud.exception.DiscoveryException;
import com.cloud.storage.ImageStore;
import com.cloud.user.Account;

Expand Down Expand Up @@ -79,11 +77,10 @@ public Long getZoneId() {
public Map<String, String> getDetails() {
Map<String, String> detailsMap = null;
if (details != null && !details.isEmpty()) {
detailsMap = new HashMap<String, String>();
detailsMap = new HashMap<>();
Collection<?> props = details.values();
Iterator<?> iter = props.iterator();
while (iter.hasNext()) {
HashMap<String, String> detail = (HashMap<String, String>)iter.next();
for (Object prop : props) {
HashMap<String, String> detail = (HashMap<String, String>) prop;
String key = detail.get("key");
String value = detail.get("value");
detailsMap.put(key, value);
Expand Down Expand Up @@ -123,20 +120,15 @@ public long getEntityOwnerId() {

@Override
public void execute(){
try{
ImageStore result = _storageService.discoverImageStore(getName(), getUrl(), getProviderName(), getZoneId(), getDetails());
ImageStoreResponse storeResponse = null;
if (result != null) {
storeResponse = _responseGenerator.createImageStoreResponse(result);
storeResponse.setResponseName(getCommandName());
storeResponse.setObjectName("imagestore");
setResponseObject(storeResponse);
} else {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add secondary storage");
}
} catch (DiscoveryException ex) {
logger.warn("Exception: ", ex);
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
ImageStore result = _storageService.discoverImageStore(getName(), getUrl(), getProviderName(), getZoneId(), getDetails());
ImageStoreResponse storeResponse;
if (result != null) {
storeResponse = _responseGenerator.createImageStoreResponse(result);
storeResponse.setResponseName(getCommandName());
storeResponse.setObjectName("imagestore");
setResponseObject(storeResponse);
} else {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add secondary storage");
}
}
}
44 changes: 19 additions & 25 deletions api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreS3CMD.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,14 @@
import org.apache.cloudstack.api.response.ImageStoreResponse;

import com.cloud.exception.ConcurrentOperationException;
import com.cloud.exception.DiscoveryException;
import com.cloud.exception.InsufficientCapacityException;
import com.cloud.exception.NetworkRuleConflictException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.storage.ImageStore;

@APICommand(name = "addImageStoreS3", description = "Adds S3 Image Store", responseObject = ImageStoreResponse.class, since = "4.7.0",
requestHasSensitiveInfo = true, responseHasSensitiveInfo = false)
@APICommand(name = "addImageStoreS3", description = "Adds S3 Image Store", responseObject = ImageStoreResponse.class,
since = "4.7.0", responseHasSensitiveInfo = false)
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removed attribute "requestHasSensitiveInfo = true" from the APICommand annotation indicates that this command was previously marked as containing sensitive information (access keys, secret keys). Removing this flag could have security implications if there's any downstream processing that relies on this metadata to handle sensitive data appropriately. Verify that this removal is intentional and that sensitive parameter handling is maintained through other means.

Suggested change
since = "4.7.0", responseHasSensitiveInfo = false)
since = "4.7.0", requestHasSensitiveInfo = true, responseHasSensitiveInfo = false)

Copilot uses AI. Check for mistakes.
public final class AddImageStoreS3CMD extends BaseCmd implements ClientOptions {

private static final String s_name = "addImageStoreS3Response";
Expand All @@ -73,32 +72,32 @@ public final class AddImageStoreS3CMD extends BaseCmd implements ClientOptions {
@Parameter(name = S3_BUCKET_NAME, type = STRING, required = true, description = "Name of the storage bucket")
private String bucketName;

@Parameter(name = S3_SIGNER, type = STRING, required = false, description = "Signer Algorithm to use, either S3SignerType or AWSS3V4SignerType")
@Parameter(name = S3_SIGNER, type = STRING, description = "Signer Algorithm to use, either S3SignerType or AWSS3V4SignerType")
private String signer;

@Parameter(name = S3_HTTPS_FLAG, type = BOOLEAN, required = false, description = "Use HTTPS instead of HTTP")
@Parameter(name = S3_HTTPS_FLAG, type = BOOLEAN, description = "Use HTTPS instead of HTTP")
private Boolean httpsFlag;

@Parameter(name = S3_CONNECTION_TIMEOUT, type = INTEGER, required = false, description = "Connection timeout (milliseconds)")
@Parameter(name = S3_CONNECTION_TIMEOUT, type = INTEGER, description = "Connection timeout (milliseconds)")
private Integer connectionTimeout;

@Parameter(name = S3_MAX_ERROR_RETRY, type = INTEGER, required = false, description = "Maximum number of times to retry on error")
@Parameter(name = S3_MAX_ERROR_RETRY, type = INTEGER, description = "Maximum number of times to retry on error")
private Integer maxErrorRetry;

@Parameter(name = S3_SOCKET_TIMEOUT, type = INTEGER, required = false, description = "Socket timeout (milliseconds)")
@Parameter(name = S3_SOCKET_TIMEOUT, type = INTEGER, description = "Socket timeout (milliseconds)")
private Integer socketTimeout;

@Parameter(name = S3_CONNECTION_TTL, type = INTEGER, required = false, description = "Connection TTL (milliseconds)")
@Parameter(name = S3_CONNECTION_TTL, type = INTEGER, description = "Connection TTL (milliseconds)")
private Integer connectionTtl;

@Parameter(name = S3_USE_TCP_KEEPALIVE, type = BOOLEAN, required = false, description = "Whether TCP keep-alive is used")
@Parameter(name = S3_USE_TCP_KEEPALIVE, type = BOOLEAN, description = "Whether TCP keep-alive is used")
private Boolean useTCPKeepAlive;

@Override
public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException,
ResourceAllocationException, NetworkRuleConflictException {

Map<String, String> dm = new HashMap();
Map<String, String> dm = new HashMap<>();

dm.put(ApiConstants.S3_ACCESS_KEY, getAccessKey());
dm.put(ApiConstants.S3_SECRET_KEY, getSecretKey());
Expand Down Expand Up @@ -127,20 +126,15 @@ public void execute() throws ResourceUnavailableException, InsufficientCapacityE
dm.put(ApiConstants.S3_USE_TCP_KEEPALIVE, getUseTCPKeepAlive().toString());
}

try{
ImageStore result = _storageService.discoverImageStore(null, null, "S3", null, dm);
ImageStoreResponse storeResponse;
if (result != null) {
storeResponse = _responseGenerator.createImageStoreResponse(result);
storeResponse.setResponseName(getCommandName());
storeResponse.setObjectName("imagestore");
setResponseObject(storeResponse);
} else {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add S3 Image Store.");
}
} catch (DiscoveryException ex) {
logger.warn("Exception: ", ex);
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
ImageStore result = _storageService.discoverImageStore(null, null, "S3", null, dm);
ImageStoreResponse storeResponse;
if (result != null) {
storeResponse = _responseGenerator.createImageStoreResponse(result);
storeResponse.setResponseName(getCommandName());
storeResponse.setObjectName("imagestore");
setResponseObject(storeResponse);
} else {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add S3 Image Store.");
}
}

Expand Down
74 changes: 47 additions & 27 deletions api/src/main/java/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
// under the License.
package org.apache.cloudstack.api.command.admin.storage;

import java.net.UnknownHostException;
import java.util.Map;


Expand All @@ -31,8 +30,6 @@
import org.apache.cloudstack.api.response.StoragePoolResponse;
import org.apache.cloudstack.api.response.ZoneResponse;

import com.cloud.exception.ResourceInUseException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.storage.StoragePool;
import com.cloud.user.Account;

Expand All @@ -46,53 +43,85 @@ public class CreateStoragePoolCmd extends BaseCmd {
//////////////// API parameters /////////////////////
/////////////////////////////////////////////////////

@Parameter(name = ApiConstants.CLUSTER_ID, type = CommandType.UUID, entityType = ClusterResponse.class, description = "The cluster ID for the storage pool")
@Parameter(name = ApiConstants.CLUSTER_ID,
type = CommandType.UUID,
entityType = ClusterResponse.class,
description = "The cluster ID for the storage pool")
private Long clusterId;

@Parameter(name = ApiConstants.DETAILS, type = CommandType.MAP, description = "The details for the storage pool")
@Parameter(name = ApiConstants.DETAILS,
type = CommandType.MAP,
description = "The details for the storage pool")
private Map details;

@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "The name for the storage pool")
@Parameter(name = ApiConstants.NAME,
type = CommandType.STRING,
required = true,
description = "The name for the storage pool")
private String storagePoolName;

@Parameter(name = ApiConstants.POD_ID, type = CommandType.UUID, entityType = PodResponse.class, description = "The Pod ID for the storage pool")
@Parameter(name = ApiConstants.POD_ID,
type = CommandType.UUID,
entityType = PodResponse.class,
description = "The Pod ID for the storage pool")
private Long podId;

@Parameter(name = ApiConstants.TAGS, type = CommandType.STRING, description = "The tags for the storage pool")
@Parameter(name = ApiConstants.TAGS,
type = CommandType.STRING,
description = "The tags for the storage pool")
private String tags;

@Parameter(name = ApiConstants.STORAGE_ACCESS_GROUPS, type = CommandType.STRING,
@Parameter(name = ApiConstants.STORAGE_ACCESS_GROUPS,
type = CommandType.STRING,
description = "comma separated list of storage access groups for connecting to hosts having those specific groups", since = "4.21.0")
private String storageAccessGroups;

@Parameter(name = ApiConstants.URL, type = CommandType.STRING, required = true, description = "The URL of the storage pool")
@Parameter(name = ApiConstants.URL,
type = CommandType.STRING,
required = true,
description = "The URL of the storage pool")
private String url;

@Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, required = true, description = "The Zone ID for the storage pool")
@Parameter(name = ApiConstants.ZONE_ID,
type = CommandType.UUID,
entityType = ZoneResponse.class,
required = true,
description = "The Zone ID for the storage pool")
private Long zoneId;

@Parameter(name = ApiConstants.PROVIDER, type = CommandType.STRING, required = false, description = "The storage provider name")
@Parameter(name = ApiConstants.PROVIDER,
type = CommandType.STRING,
description = "The storage provider name")
private String storageProviderName;

@Parameter(name = ApiConstants.SCOPE, type = CommandType.STRING, required = false, description = "The scope of the storage: cluster or zone")
@Parameter(name = ApiConstants.SCOPE,
type = CommandType.STRING,
description = "The scope of the storage: cluster or zone")
private String scope;

@Parameter(name = ApiConstants.MANAGED, type = CommandType.BOOLEAN, required = false, description = "Whether the storage should be managed by CloudStack")
@Parameter(name = ApiConstants.MANAGED,
type = CommandType.BOOLEAN,
description = "Whether the storage should be managed by CloudStack")
private Boolean managed;

@Parameter(name = ApiConstants.CAPACITY_IOPS, type = CommandType.LONG, required = false, description = "IOPS CloudStack can provision from this storage pool")
@Parameter(name = ApiConstants.CAPACITY_IOPS,
type = CommandType.LONG,
description = "IOPS CloudStack can provision from this storage pool")
private Long capacityIops;

@Parameter(name = ApiConstants.CAPACITY_BYTES, type = CommandType.LONG, required = false, description = "Bytes CloudStack can provision from this storage pool")
@Parameter(name = ApiConstants.CAPACITY_BYTES,
type = CommandType.LONG,
description = "Bytes CloudStack can provision from this storage pool")
private Long capacityBytes;

@Parameter(name = ApiConstants.HYPERVISOR,
type = CommandType.STRING,
required = false,
description = "Hypervisor type of the hosts in zone that will be attached to this storage pool. KVM, VMware supported as of now.")
private String hypervisor;

@Parameter(name = ApiConstants.IS_TAG_A_RULE, type = CommandType.BOOLEAN, description = ApiConstants.PARAMETER_DESCRIPTION_IS_TAG_A_RULE)
@Parameter(name = ApiConstants.IS_TAG_A_RULE,
type = CommandType.BOOLEAN,
description = ApiConstants.PARAMETER_DESCRIPTION_IS_TAG_A_RULE)
private Boolean isTagARule;

/////////////////////////////////////////////////////
Expand Down Expand Up @@ -175,15 +204,6 @@ public void execute() {
} else {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to add storage pool");
}
} catch (ResourceUnavailableException ex1) {
logger.warn("Exception: ", ex1);
throw new ServerApiException(ApiErrorCode.RESOURCE_UNAVAILABLE_ERROR, ex1.getMessage());
} catch (ResourceInUseException ex2) {
logger.warn("Exception: ", ex2);
throw new ServerApiException(ApiErrorCode.RESOURCE_IN_USE_ERROR, ex2.getMessage());
} catch (UnknownHostException ex3) {
logger.warn("Exception: ", ex3);
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex3.getMessage());
} catch (Exception ex4) {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex4.getMessage());
}
Expand Down
2 changes: 1 addition & 1 deletion api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public ApiCommandResourceType getApiResourceType() {
return ApiCommandResourceType.StoragePool;
}

public Map<String,String> getDetails() {
public Map<String,Object> getDetails() {
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type of getDetails() has changed from Map<String,String> to Map<String,Object>. This is a breaking API change that could affect callers expecting Map<String,String>. Verify that all callers of this method have been updated to handle the new return type, particularly the extractApiParamAsMap method in StorageManagerImpl which now correctly expects Map<String,Object>.

Copilot uses AI. Check for mistakes.
return details;
}

Expand Down
Loading
Loading