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

Cloud ObjectStore #4487

Merged
merged 30 commits into from Dec 12, 2017

Conversation

Projects
None yet
9 participants
@VJalili
Member

VJalili commented Aug 23, 2017

This PR is a follow up from #4352 which we had to close it due to an issue with rebase. You may check that PR for some comments and reviews.

The Cloud ObjectStore is a provider-agnostic cloud-based storage. In other words, the Cloud ObjectStore can read/write from various cloud-based storages without a provider-specific implementation; it leverages on CloudBridge which provides a unified access to various providers.

This PR sets a base for the User-based ObjectStore we have been working on. Initially, the Cloud ObjectStore was part of the User-based ObjectStore (PR #4314), and it is being submitted as a separate PR following the comments on the PR #4314.

To simplify the comparison with S3ObjectStore, the Cloud ObjectStore provides the very same functionality as S3ObjectStore, i.e., read/write to AWS S3 buckets. The only difference is: S3ObjectStore leverages on boto while Cloud uses CloudBridge.

VJalili and others added some commits Jul 21, 2017

Removed boto import, and replaced all S3-specific exception catches with
a generic exception--a temporary solution till CloudBridge wraps the
exceptions properly.
Consolidated two upload methods, i.e., upload from file and string,
because CloudBridge internally maps to appropriate functions depending
on the input type.
Update CloudBridge to its current latest
Should use the wheel created at [this PR](galaxyproject/starforge#139).

@VJalili VJalili referenced this pull request Aug 23, 2017

Closed

Cloud ObjectStore #4352

@galaxybot galaxybot added the triage label Aug 23, 2017

@galaxybot galaxybot added this to the 17.09 milestone Aug 23, 2017

log.debug("Cache cleaning done. Total space freed: %s", convert_bytes(deleted_amount))
return
def _get_bucket(self, bucket_name):

This comment has been minimized.

@VJalili

VJalili Aug 28, 2017

Member

@nuwang; referring to your comment on #4352; the logic here is "get the bucket, if not available, then create it first, then return the bucket". However, the cloudbridge logic is "get the bucket, if not available, return none" (see this line). So, do you want to update cloudbridge and have this logic implemented internally?

This comment has been minimized.

@nuwang

nuwang Sep 1, 2017

Member

@VJalili @afgane I think that sequence is alright. I was thinking more about the outer loop - having to loop 5 times before getting a bucket. Calling cloudbridge.create() should guarantee that the bucket is available when it returns, or it should raise an exception. Having to loop suggests that the cloudbridge code is not portable across providers (e.g. this might work in one go on openstack, and a developer would think the code is working correctly, but then things will actually fall over on AWS). We could either document this as being necessary at the interface level, or handle this internally within cloudbridge. Do you know when specifically this occurs?

This comment has been minimized.

@nuwang

nuwang Sep 1, 2017

Member

I've created an issue here so we can continue the discussion: CloudVE/cloudbridge#64

This comment has been minimized.

@VJalili

VJalili Sep 5, 2017

Member

@nuwang I agree. I removed the retries here, and now we rely on the first try :)

This comment has been minimized.

@nuwang

nuwang Sep 5, 2017

Member

@VJalili Ok, sounds good :-) Will close the corresponding issue.

@jgoecks jgoecks removed the triage label Aug 30, 2017

@afgane

This comment has been minimized.

Contributor

afgane commented Aug 30, 2017

Looks good to me as an intermediate step that focuses on replacing AWS/boto code with the cloud-agnostic CloudBridge code. Much of this will get redone again with the move toward the user-based object store so even though there are a few AWS-specific terms, that's fine.

+1

Rework conditional dependency handling in galaxy.objectstore.cloud.
This file may be loaded (e.g. for testing) but not used, so we shouldn't log a generic error about cloudbridge being unavailable, if someone attempts to actually use the object store and it isn't available then raise an informative exception.
@jmchilton

This comment has been minimized.

Member

jmchilton commented Sep 5, 2017

I've opened a PR to rework the conditional dependencies here (VJalili#3) - see PR for more information.

I'd also like to see this line:

logging.getLogger('boto').setLevel(logging.INFO)  # Otherwise boto is quite noisy

Moved somewhere else - unconditionally applying it as a result of importing a random module in Galaxy seems wrong - but the same line appears in s3.py so I wouldn't hold up this PR on that I guess.

VJalili added some commits Sep 5, 2017

Merge pull request #3 from jmchilton/CloudObjectStore
Rework conditional dependency handling in galaxy.objectstore.cloud.
@VJalili

This comment has been minimized.

Member

VJalili commented Sep 5, 2017

logging.getLogger('boto').setLevel(logging.INFO)  # Otherwise boto is quite noisy

Moved somewhere else - unconditionally applying it as a result of importing a random module in Galaxy seems wrong - but the same line appears in s3.py so I wouldn't hold up this PR on that I guess.

@jmchilton Thanks for mentioning this. Actually, this line should be removed as we're not (explicitly) using boto; I removed it.

@natefoo

This comment has been minimized.

Member

natefoo commented Sep 5, 2017

I've had about 2 minutes to look at this and have about 1 minute to write this comment but I wanted to get something in here just so I'll get notifications as a participant. ;)

Without having looked myself, it seems? like there's a lot of duplication from S3? Can these not be refactored into a CachingObjectStore (which I could've sworn we already had as a parent of the S3ObjectStore)?

@VJalili

This comment has been minimized.

Member

VJalili commented Sep 5, 2017

@natefoo you're right, the Cloud class is very close to the S3ObjectStore ObjectStore, and that is intentional :) the reason is #4314.

The Cloud module introduced in this PR, will be updated in follow-up PRs to enable a per user-based ObjectStore (i.e., plug-and-play your own resource such as Amazon S3, Microsoft Azure, Google drive, and etc.).

@natefoo

This comment has been minimized.

Member

natefoo commented Sep 6, 2017

Ah, so the plan is to merge them in the future.

@VJalili

This comment has been minimized.

Member

VJalili commented Sep 6, 2017

Actually, I would also prefer to merge the overlapping modules in favor of orthogonal functionality. But as you may read in #4314, it seems it is more "galaxy-ish" to keep S3 and Azure (even-though you get the very same functionality with Cloud) for backward compatibility. Therefore, merging cloud with s3 and azure_blob is not in the plan :)

@natefoo

This comment has been minimized.

Member

natefoo commented Sep 6, 2017

In that case, can we aim for refactoring the duplication into a caching object store class and have S3, Cloud, and Azure subclass from that?

@dannon

This comment has been minimized.

Member

dannon commented Sep 6, 2017

@natefoo That was the plan AFAIK, we just wanted to keep the initial implementation of this simple and isolated, and make a refactoring pass after.

@VJalili Sorry for the delay getting to this, I'll test it for ya today and see how it goes.

@dannon

This comment has been minimized.

Member

dannon commented Sep 11, 2017

In testing, I'm finding that the path to the cached version of a file is actually what gets uploaded as the contents of the file, instead of the contents as expected.

I set up a new IAM user, uploaded a new dataset, and observed that in my S3 bucket the contents of the uploaded file were actually something like /Users/yoplait/work/galaxy/database/object_store_cache/000/dataset_183.dat, instead of the data expected.

The cached file on disk is fine, but when I delete that cached file, the file is replaced by a plain text file containing the string as mentioned above. That's a good news / bad news thing in that the good news is that fetching the file from S3 is working, but we definitely need to fix the contents that get uploaded.

Feel free to move it back to 17.09 if you feel inclined, but because this is a fairly critical issue for this PR (and I'm not seeing this as an urgent feature for 17.09), I'm going to bump this to 18.01.

@dannon dannon modified the milestones: 18.01, 17.09 Sep 11, 2017

Resolved a bug with uploading a dataset using Cloud (not differentiating
between uploading from a file vs. string).
@VJalili

This comment has been minimized.

Member

VJalili commented Sep 12, 2017

Interesting point, thanks @dannon
I fixed it.

It seems there was a piece of code in the original PR which was differentiating between uploading from a file and object. The same logic was moved to this PR, but it seems I removed it at this commit assuming it is handled internally by CloudBridge; maybe a misunderstanding. I reverted the commit.

@VJalili VJalili referenced this pull request Dec 7, 2017

Closed

The Roadmap #1928

13 of 110 tasks complete

@dannon dannon merged commit 45ac767 into galaxyproject:dev Dec 12, 2017

6 checks passed

api test Build finished. 292 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 45 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Member

dannon commented Dec 12, 2017

Merged locally resolving conflicts. Thanks for the fixes @VJalili, I confirmed that the dataset contents bug above was fixed and stuff seemed to be working pretty well to me.

@VJalili

This comment has been minimized.

Member

VJalili commented Dec 12, 2017

Thanks @dannon .

@VJalili VJalili deleted the VJalili:CloudObjectStore branch Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment