-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Adding new Dell EMC ECS Object Storage Plugin for CloudStack #12124
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
jbampton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have failing tests for pre-commit and license checks.
Have a quick read up on pre-commit it really is a series of basic checks we run:
https://github.com/apache/cloudstack/blob/main/PRE-COMMIT.md
Also we normally add the ASF license header to all files or add an exclude
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12124 +/- ##
============================================
- Coverage 17.56% 17.53% -0.04%
- Complexity 15538 15548 +10
============================================
Files 5912 5916 +4
Lines 529383 530514 +1131
Branches 64660 64933 +273
============================================
+ Hits 92984 93002 +18
- Misses 425941 427052 +1111
- Partials 10458 10460 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mhkadhum can you check the build errors? |
...S/src/main/java/org/apache/cloudstack/storage/datastore/driver/EcsObjectStoreDriverImpl.java
Outdated
Show resolved
Hide resolved
.../main/resources/META-INF/cloudstack/storage-object-ecs/spring-storage-object-ecs-context.xml
Show resolved
Hide resolved
.../main/resources/META-INF/cloudstack/storage-object-ecs/spring-storage-object-ecs-context.xml
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback! ive added the missing license headers and ran |
tools/marvin/setup.py
Outdated
|
|
||
|
|
||
| VERSION = "4.23.0.0-SNAPSHOT" | ||
| VERSION = "4.23.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be in the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DaanHoogland
This was updated after running the pre-commit, should i keep it as
VERSION = "4.23.0.0-SNAPSHOT"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it should only be updated by building (not by pre-commit) somebody introduced that behaviour for testing I think and it forces us to be more vigilent at checking in, so I don’t mind. We need to keep snapshot though untill releasing .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok,
I've just added it back.
|
looks generally good @mhkadhum , thanks for the contribution. Will you be maintaining this? As it is a 3rd party component and it may require hardware or licenses not available to the project. |
Yes, I will be the maintainer for the plugin. I have made a new commit to fix the previous build. Could you kindly review it |
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found any oddities in the code, but one remark (in two parts ;) ) Feel free to ignore but I think it will help in the future.
...main/java/org/apache/cloudstack/storage/datastore/lifecycle/EcsObjectStoreLifeCycleImpl.java
Outdated
Show resolved
Hide resolved
...S/src/main/java/org/apache/cloudstack/storage/datastore/driver/EcsObjectStoreDriverImpl.java
Show resolved
Hide resolved
…rom lifecycle , introduce caching to ECS token
|
Hi, X-SDS-AUTH-MAX-AGE
|
|
Hello @abh1sar @DaanHoogland |
sorry @mhkadhum , this #12124 (comment) you mean right? I didn’t read it as a question and though this is ready for testing. What issues are you having right now? |
No worries @DaanHoogland |
|
Hi @mhkadhum, I am planning to finish my review by early next week. |
No, but we will regard this as 3rd party extension and trust you with specific feature testing, only doing regression testing. (sponsored donation in the sense that you make an instance available for testing is welcome, but you maintaining it as even more ;) |
abh1sar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed EcsObjectStoreDriverImpl
Overall functionality looks good but there are some comments on code maintainability and organisation. Please check.
Overall, I think use of reflections can be avoided in most cases.
| final String name = bucket.getName(); | ||
|
|
||
| if (objectLock) { | ||
| throw new CloudRuntimeException("Dell ECS doesn't support this feature: object locking"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think InvalidParameterValueException will be more suitable here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive changed this to InvalidParameterValueException
| private static final long EXPIRY_SKEW_SEC = 30; // refresh early | ||
| private static final ConcurrentHashMap<TokenKey, TokenEntry> TOKEN_CACHE = new ConcurrentHashMap<>(); | ||
| private static final ConcurrentHashMap<TokenKey, Object> TOKEN_LOCKS = new ConcurrentHashMap<>(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could I suggest creating separate classes for Token Management and XML parsing for better maintainability and reusability?
The EcsObjectStoreDriverImpl is currently doing a lot of different things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
Token management and XML parsing have been extracted into separate classes
(EcsMgmtTokenManager and EcsXmlParser) to improve maintainability and reuse.
| private static final String MGMT_URL = "mgmt_url"; // e.g. https://ecs-api.example.com | ||
| private static final String SA_USER = "sa_user"; // service account user | ||
| private static final String SA_PASS = "sa_password"; // service account password | ||
| private static final String NAMESPACE = "namespace"; // e.g. cloudstack | ||
| private static final String INSECURE = "insecure"; // "true" to ignore TLS cert/host | ||
| private static final String S3_HOST = "s3_host"; // S3 endpoint host (or URL if UI provides it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider moving all the constants to a new EcsConstants class.
EcsObjectStoreDriverImpl and EcsObjectStoreLifeCyclImpl both are using these.
| private static final long DEFAULT_TOKEN_MAX_AGE_SEC = 300; // fallback if header missing | ||
| private static final long EXPIRY_SKEW_SEC = 30; // refresh early |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this something that should be configurable during runtime (using global settings)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ECS returns the token expiry via a response header at creation time. The default value is only used as a fallback in cases where the header is missing or unavailable. Since the expiry is determined by ECS and not intended to be tuned at runtime, adding this to global configuration does not seem necessary at this point.
| import com.cloud.utils.exception.CloudRuntimeException; | ||
|
|
||
| public class EcsObjectStoreDriverImpl extends BaseObjectStoreDriverImpl { | ||
| private static final Logger logger = LogManager.getLogger(EcsObjectStoreDriverImpl.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseObjectStoreDriverImpl already has a LogManager. No need to define it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
| private BucketVO resolveBucketVO(final BucketTO bucket, final long storeId) { | ||
| long id = getLongFromGetter(bucket, "getId", -1L); | ||
| if (id <= 0) id = getLongFromGetter(bucket, "getBucketId", -1L); | ||
| if (id > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bucket's id is defined as NOT NULL AUTO_INCREMENT in the schema. So, it can never be <= 0.
The code below line 1274 is not required. I don't think it will ever happen that bucketVO is not found using the id , but it is found using uuid or name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have minimized this method
private BucketVO resolveBucketVO(final BucketTO bucket) {
if (bucket == null) return null;
long id = getLongFromGetter(bucket, "getId", -1L);
if (id > 0) {
return bucketDao.findById(id);
}
return null;
}
| } | ||
|
|
||
| private BucketVO resolveBucketVO(final BucketTO bucket, final long storeId) { | ||
| long id = getLongFromGetter(bucket, "getId", -1L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just do long id = bucket.getId() as a bucket's id can never be null.
| } | ||
| } | ||
|
|
||
| String accessKey = vo != null ? safeString(vo, "getAccessKey") : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need reflections for getters anywhere in this file.
The interfaces are clearly defined.
this can be written as
String accessKey = vo.getAccessKey()
| @SuppressWarnings("unchecked") | ||
| private BucketVO findBucketVOByStoreAndName(final long storeId, final String name) { | ||
| try { | ||
| java.lang.reflect.Method m = bucketDao.getClass().getMethod("listByStoreId", long.class); | ||
| Object res = m.invoke(bucketDao, storeId); | ||
| if (res instanceof List<?>) { | ||
| for (Object o : (List<?>) res) { | ||
| if (o instanceof BucketVO) { | ||
| BucketVO vo = (BucketVO) o; | ||
| try { if (name.equals(vo.getName())) return vo; } catch (Throwable ignore) { } | ||
| } | ||
| } | ||
| } | ||
| } catch (NoSuchMethodException ignored) { | ||
| } catch (Throwable t) { | ||
| logger.debug("ECS: listByStoreId fallback failed: {}", t.getMessage()); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private BucketVO findBucketVOAnyByName(final String name) { | ||
| try { | ||
| java.lang.reflect.Method m = bucketDao.getClass().getMethod("listAll"); | ||
| Object res = m.invoke(bucketDao); | ||
| if (res instanceof List<?>) { | ||
| for (Object o : (List<?>) res) { | ||
| if (o instanceof BucketVO) { | ||
| BucketVO vo = (BucketVO) o; | ||
| try { if (name.equals(vo.getName())) return vo; } catch (Throwable ignore) { } | ||
| } | ||
| } | ||
| } | ||
| } catch (NoSuchMethodException ignored) { | ||
| } catch (Throwable t) { | ||
| logger.debug("ECS: listAll scan failed: {}", t.getMessage()); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If bucketDao.findById(id) has returned null that means the bucket doesn't exist in cloudstack db for some reason.
I don't think it will be found by any traversing any list as well.
So, if bucketDao.findById(id) returns null, we can just move on.
| final int maxTries = 45; | ||
| for (int attempt = 1; attempt <= maxTries; attempt++) { | ||
| BucketVO vo = resolveBucketVO(bucket, storeId); | ||
| if (vo == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we go ahead with this operation if BucketVO is null i.e, it is not in the acs db, should we still manage it?
Hello @abh1sar |

Description
This PR adds support for a Dell EMC ECS S3 object storage plugin for Apache CloudStack. ECS is a software-defined object storage platform that supports both object and file-system protocols, with a focus on scalable and reliable object storage. We have been using ECS in production for three years, and this work extends CloudStack’s existing MinIO plugin to provide similar functionality for ECS.
The implementation supports the full lifecycle of S3-compatible buckets on ECS, including user provisioning, bucket creation, policy management, versioning, creation-time encryption, and integration with CloudStack’s S3 Browser. All functionality has been tested in a lab environment on Ubuntu using a CloudStack development setup based on the official installation guidelines.
Key architectural differences from MinIO:
Management API Integration
ECS requires use of the ECS Management API (port 4443, or 443 when fronted by HAProxy). CloudStack authenticates with management-user credentials to perform bucket and user operations.
Namespace Requirements
A dedicated ECS namespace is required for CloudStack-managed buckets. Multiple namespaces allow different CloudStack environments to share the same ECS cluster.
S3 Endpoints (Public and Private)
ECS exposes S3 services on ports 9020/9021 (non-TLS/TLS). In our deployment, these are routed through HAProxy and exposed externally on port 443. The Public URL is displayed to CloudStack users, while the Private URL is used internally.
TLS Handling
The “Allow Insecure HTTPS” option controls whether CloudStack accepts untrusted certificates when communicating with the ECS Management API.
User Provisioning Workflow
When a CloudStack user creates their first bucket, CloudStack provisions a corresponding ECS object user using the CloudStack UUID with a
cs-prefix. ECS generates access and secret keys once, which CloudStack securely stores and reuses for subsequent bucket operations.Bucket features:
Bucket modification supports quota changes, versioning updates, and policy changes. Encryption is excluded because ECS does not allow changing it after creation.
CloudStack’s S3 Browser supports upload, download, delete, listing, and prefix filtering through the ECS S3 endpoint. ECS prevents deletion of non-empty buckets, and CloudStack surfaces these errors accordingly.
We welcome review and feedback. The development fork is available here:
https://github.com/mhkadhum/cloudstack
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
The plugin was tested in an Ubuntu-based CloudStack development environment created by cloning the CloudStack source and following the official installation instructions. Testing included: