Skip to content
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

Add S3_RGW storage driver #786

Closed
wants to merge 1 commit into from
Closed

Conversation

@jmunhoz
Copy link
Contributor

jmunhoz commented May 13, 2016

Add S3_RGW storage driver

Signed-off-by: Javier M. Mellid jmunhoz@igalia.com

@@ -69,6 +69,8 @@

S3_RGW_OUTSCALE_DEFAULT_REGION = 'eu-west-2'

S3_RGW_DEFAULT_REGION = 'default'

This comment has been minimized.

@Kami

Kami May 13, 2016 Member

I would prefer to put non-s3 related stuff and RGW driver in a separate file / module (e.g. drivers/rgw.py or similar).

This module should only contain base classes and S3 driver related code.

This comment has been minimized.

@jmunhoz

jmunhoz May 13, 2016 Author Contributor

@Kami makes sense! thanks

This comment has been minimized.

@jmunhoz

jmunhoz May 17, 2016 Author Contributor

@Kami done. All RGW stuff lives in a separated module (drivers/rgw.py) now.

@jmunhoz jmunhoz force-pushed the jmunhoz:s3-rgw-storage-driver branch from ee119bc to 3cc79d9 May 17, 2016
@Kami
Copy link
Member

Kami commented May 17, 2016

@jmunhoz Thanks, pylint step is failing, can you please have a look - https://travis-ci.org/apache/libcloud/builds/130791984

Add Ceph S3 RGW drivers and related stuff in the drivers/rgw.py module:

- S3 RGW driver
- S3 RGW Outscale driver

Signed-off-by: Javier M. Mellid <jmunhoz@igalia.com>
@jmunhoz jmunhoz force-pushed the jmunhoz:s3-rgw-storage-driver branch from 3cc79d9 to c9cedd0 May 17, 2016
@jmunhoz
Copy link
Contributor Author

jmunhoz commented May 17, 2016

@Kami I updated the PR to adjust the hierarchy although pylint is expected to fail with this new update. With this PR I am suggesting the following hierarchy:

S3RGWOutscaleStorageDriver -> S3RGWStorageDriver -> S3StorageDriver

The Outscale code is looking for reusing all methods in the RGW driver (_ex_connection_class_kwargs, etc) but the constructor. That's the reason to see pylint failing. In the case of S3RGWOutscaleStorageDriver the code calls the S3RGWStorageDriver's parent instead of the S3RGWOutscaleStorageDriver's parent as expected by pylint. It is also passing 2.x and 3.x series together with lint checks so perhaps the approach could be considered good enough.

@jmunhoz
Copy link
Contributor Author

jmunhoz commented May 19, 2016

@Kami would it be possible merging this PR as it is and then iterating over this pylint error if needed?

Libcloud is using the PUT + UNSIGNED-PAYLOAD + Auth header combination to sign uploads via S3. This combo is not available in the Ceph RGW's s3-tests in this moment, so having this RGW driver in place would be useful to add the Libcloud coverage in Ceph. This way everybody (s3-tests, developers and users) could use the same upstream code without modifications.

The Libcloud support in Ceph (PUT + UNSIGNED-PAYLOAD + Auth header) was upstream several weeks ago. I commented on this issue. It is available at:

ceph/ceph#8601

Thanks!

@Kami
Copy link
Member

Kami commented May 19, 2016

@Kami I updated the PR to adjust the hierarchy although pylint is expected to fail with this new update. With this PR I am suggesting the following hierarchy: S3RGWOutscaleStorageDriver -> S3RGWStorageDriver -> S3StorageDriver

@jmunhoz I think the correct class-hierarchy should be S3StorageDriver -> S3RGWStorageDriver -> S3RGWOutscaleStorageDriver.

The reason for that is that S3 is the generic S3 implementation and other S3 based providers should inherit from it (and not vice-versa).

If needed, we could also refactor base S3 functionality into BaseS3Driver and then have the following class hierarchy - BaseS3Driver -> S3Driver, BaseS3Driver -> S3RGWStorageDriver, S3RGWStorageDriver -> S3RGWOutscaleStorageDriver (that's similar to what we do with EC2 driver right now).


host = S3_RGW_OUTSCALE_HOSTS_BY_REGION[region]
self.connectionCls.host = host
super(S3RGWStorageDriver, self).__init__(key, secret,

This comment has been minimized.

@Kami

Kami May 19, 2016 Member

You should pass S3RGWOutscaleStorageDriver as the first argument to super(). This will fix pylint issue.

@Kami
Copy link
Member

Kami commented May 19, 2016

Also, the lint issue is actually a very minor one, please see https://github.com/apache/libcloud/pull/786/files#r63909503.

@Kami
Copy link
Member

Kami commented May 20, 2016

I fixed the lint issue and went ahead and merged PR into trunk - thanks!

When you get a chance, please also submit a corresponding PR with tests.

@asfgit asfgit closed this in f81ab67 May 20, 2016
@jmunhoz
Copy link
Contributor Author

jmunhoz commented May 20, 2016

@Kami Thanks for taking care of this review. I was working on your review comments the last hours but I was not fast enough to update the PR before merging and closing this PR.

I think I have a PR addressing your previous comments now. It has a better design and it is not forced by the kwargs issue.

I reviewed the class-hierarchy stuff and I think we are on the same page. I was using UML notation (maybe not the best choice to communicate my idea in plain text, sorry!) with the following meaning subclass -> superclass. As mentioned I think we are on the same page. The code follows this class hierarchy.

By the way, I think the lint fix could be breaking the driver cause it will call on the parent constructor without the signature_version parameter (it was pop'ed in the subclass) so the parent will set the default signature (v2). The previous code was jumping over the parent's constructor. It was the reason to lint complaining.

As commented, I will test the link fix and I will open a new PR to fix it if needed together with the improvements suggested by you on the previous revision.

@Kami
Copy link
Member

Kami commented May 20, 2016

@jmunhoz Sounds good & thanks :)

@jmunhoz
Copy link
Contributor Author

jmunhoz commented May 20, 2016

@Kami I opened a new PR #792 It fixes the default signature issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.