Adding new Dell EMC ECS Object Storage Plugin for CloudStack #12124
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
jbampton
left a comment
There was a problem hiding this comment.
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.60% 17.57% -0.04%
- Complexity 15668 15669 +1
============================================
Files 5915 5920 +5
Lines 529963 531034 +1071
Branches 64734 64952 +218
============================================
+ Hits 93300 93309 +9
- Misses 426194 427257 +1063
+ Partials 10469 10468 -1
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.
this should not be in the PR
There was a problem hiding this comment.
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.
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 .
|
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
DaanHoogland
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
methods createBucket, listBuckets, listBucketObjects, deleteBucket, setBucketPolicy, getBucketPolicy, setOrSuspendVersioning, setBucketQuota, ensureAccountUserAndSecretandecsBucketExists` are rather great. Again not my code to maintain, but extracting smaller methods will make it more readible and may help find some code deduplication possibilities.
There was a problem hiding this comment.
Hello @DaanHoogland ,
I've been working the comments you provided in order to optimize the code. While these changes may not reduce the lines in the driver file :) but they will enhance the readability of the methods. Additionally, I am working on implementing token caching for the communication between CloudStack and ECS. Currently, the token is being generated for each request, but due to rate limits on token generation, this approach is inefficient. I will update the code soon.
There was a problem hiding this comment.
He @mhkadhum , this file still contains a few very large methods and also methods that contain nested try blocks. Will you address those? As discussed earlier, this will remain your responsibility so I am not demanding any change but I highly recommend you address these conditions of the code.
Hello @abh1sar |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16184 |
|
Hello @blueorangutan from my server |
|
sorry @mhkadhum , still compile errors (not lint anymore): https://github.com/apache/cloudstack/actions/runs/20570652663/job/59082786785?pr=12124#step:7:18860 |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16193 |
...S/src/main/java/org/apache/cloudstack/storage/datastore/driver/EcsObjectStoreDriverImpl.java
Outdated
Show resolved
Hide resolved
| <!-- ECS Object Store Configuration --> | ||
| <div v-else-if="form.provider === 'ECS'"> | ||
| <!-- S3 URL (for UI: this becomes addObjectStoragePool url=...) --> | ||
| <a-form-item name="url" ref="url"> |
There was a problem hiding this comment.
isn't url a required field?
There was a problem hiding this comment.
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16248 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15110)
|
| } | ||
| } | ||
|
|
||
| private TokenEntry loginAndGetTokenFresh(final String mgmtUrl, |
There was a problem hiding this comment.
this method contains three levels of try blocks. the second level has no catch or final blocks. @mhkadhum , are you sure about this? I’d try to dissect it in smaller methods (no 👎 !)
There was a problem hiding this comment.
He @mhkadhum , this file still contains a few very large methods and also methods that contain nested try blocks. Will you address those? As discussed earlier, this will remain your responsibility so I am not demanding any change but I highly recommend you address these conditions of the code.
6 participants
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: