Object storage #8383

DeepDiver1975 · 2014-04-28T13:26:38Z

don't merge before owncloud-archive/3rdparty#99 is in 3rdparty

add rackspace and update composer owncloud-archive/3rdparty#99 moves rackspace to 3rdparty, but files_external also provides it, causing tests to fail ...
swift test should create a new container on every test run and delete it afterwards

PVince81 · 2014-04-28T14:24:53Z

lib/private/util.php

@@ -57,7 +90,9 @@ public static function setupFS( $user = '' ) {
 				// set up quota for home storages, even for other users
 				// which can happen when using sharing

-				if ($storage instanceof \OC\Files\Storage\Home) {
+				if ($storage instanceof \OC\Files\Storage\Home
+					|| $storage instanceof \OCA\ObjectStorage\S3 // FIXME introduce interface \OC\Files\Storage\HomeStorage? or add method?


I suppose we might also want to support OpenStack ?

We started to implement this approach against a VM with ceph installed. Ceph supports S3 API as well as Swift.

Ah, nice 😄
I just saw in the files_external project that S3 and Swift have a slightly different implementation. It might make sense to consolidate them at some point.

I'm not to sure if we can merge the implementations - these primary storage backends will work on the basic assumption that ownCloud is the only system accessing the 'files'. We never scan and we never store folder structure information in the object store - in contrary to our existing files_external implementations.

Let's see ...

I see, makes sense.

butonic · 2014-06-06T22:17:28Z

@DeepDiver1975 @karlitschek @craigpg I managed to set up an openstack server in a virtual machine using http://devstack.org/ Objectstore on swift, s3 and rackspace are working. This PR adds the necessary configuration parameters to core. https://github.com/owncloud/objectstore contains the implementations for each backend as an app.

karlitschek · 2014-06-07T12:14:36Z

great. Let´s finish this next week. We need Swift and S3 as separate apps and the documentation should be updated about how to configure Swift.
Does this work for you?

butonic · 2014-06-07T20:11:28Z

@karlitschek sure, I got a http://devstack.org/ vm running and it provides a swift as well as an s3 api. will split the code next week and update the docs accordingly.

karlitschek · 2014-06-07T20:13:15Z

Sweeet

PVince81 · 2014-06-08T16:46:39Z

@butonic will those docs include instructions how to build that VM ?
Did you just run their cloud-init script or did the VM needs additional tweaks to provide a swift and s3 interface ? (would be nice to have such a VM for testing ext storage)

butonic · 2014-06-10T10:11:37Z

@PVince81 I downloaded the ubuntu server 14.04 x64 iso and installed that in a VM with virtualbox. i used stack as the username during the installation. In the vm I had to install memcached prior to get the devstack script running:

sudo apt-get install memcached git
git clone https://github.com/openstack-dev/devstack.git

my devstack/local.conf literally looks like this

[[local|localrc]]
ADMIN_PASSWORD=password
MYSQL_PASSWORD=password
RABBIT_PASSWORD=password
SERVICE_PASSWORD=password
SERVICE_TOKEN=tokentoken

SWIFT_HASH=66a3d6b56c1f479c8b4e70ab5c2000f5

enable_service s-proxy s-object s-container s-account

after booting the vm you have to start the services manually by running

cd devstack
./stack.sh

The first time you execute stack.sh will take quite a while, downloading and installing all packages.

You will end up in a screen session with a lot of openstack services started in individual tab so you can follow any log messages. Quite nice.

I'm trying to slim down the stack configuration and add s3 because that should speed up deployment and reduce ram usage.

butonic · 2014-06-12T10:13:36Z

@PVince81 @icewind1991 @schiesbn I need a review regarding the approach I took to replace the root storage as well as the users home storage with an object storage implementation. Currently, the objectstore is magically enabled by putting something like the following in the config.php:

  'home_storage' => array(
    'class' => 'OC\\Files\\ObjectStore\\Swift',
    'arguments' => array(
      'username' => 'demo',
      'password' => 'password',
      'container' => 'devobjectstore',
      'region' => 'RegionOne', //required, devstack defaults to 'RegionOne'
      'url' => 'http://192.168.42.114:5000/v2.0', // The Identity / Keystone endpoint
      'tenantName' => 'demo', // required on devstack
      'serviceName' => 'swift', //devstack uses 'swift' by default, the lib defaults to 'cloudFiles' if omitted
    ),
  ),
  'root_storage' => array(
    'class' => 'OC\\Files\\ObjectStore\\Swift',
    'arguments' => array (
      'username' => 'demo',
      'password' => 'password',
      'container' => 'devobjectstore',
      'region' => 'RegionOne', //required, devstack defaults to 'RegionOne'
      'url' => 'http://192.168.42.114:5000/v2.0', // The Identity / Keystone endpoint
      'tenantName' => 'demo', // required on devstack
      'serviceName' => 'swift', //devstack uses 'swift' by default, the lib defaults to 'cloudFiles' if omitted
    ),
  ),

Yes, there are some annoyances with this configuration, such as having to specify the configuration twice: once for the root storage and once for the users home storage. What I am more interested in is a review of the storage assembly in setupFS(). I deisbled the creation of the skeleton for objectstorage based home folders because it does not use the oc filesystem. Should we move skeleton creation to post filesystem setup, maybe an app? Should I unify the configuration for root and home storage or do you see a valid use case where they might be set up differently?

The AbstractObjectStorage uses a NoopScanner because the files don't reside on the local filesystem and we can pull the metadata from the filecache. The objectstorage is assumed to be used by owncloud exclusively.

butonic · 2014-06-13T14:01:48Z

Sharing and encryption work fine.
@schiesbn The problem with encryption is that it totally kills performance because we have to fetch the key from the object store as well as the file itself.

After this has been merged investigate improving performance by caching the encryption keys with memcached / apc?

@icewind1991 Did I understand correctly the I have to wrap the users home storage so we can apply quota? If so, that is the reason whyI had to mount the object store as '/' as well as ''. There is no real need to use a quota wrapper on the root storage, is there?

butonic · 2014-06-16T09:39:44Z

tests fail on mysql because it has problems storing characters like 💩 (Pile of poo ... or 💩) likely tracked in #7030

icewind1991 · 2014-06-16T11:57:29Z

lib/private/files/filesystem.php

+				//default home storage configuration:
+				'class' => '\OC\Files\Storage\Home',
+				'arguments' => array()
+			));


Why is the homestorage config option objectstore ?

I only wanted to have on config option to make it easier to set up an objectstorage based owncloud. It does not really make sense to have two separate configuration options like "root_storage" and "home_storage" which I removed with 547791b. But I need the configuration when setting up these storages, hence the IMHO more meaningful name of "objectstore". Does that make sense?

But the config option can be used to setup other home storages besides object store

icewind1991 · 2014-06-16T12:13:45Z

I think it would be better to split of the object store methods getObject(), createObject() (writeObject()?) and deleteObject() and probably getUrn() into a seperate interface/class which is then passed to the ObjectStoreStorage class to seperate the objectstore logic from the logic of maping an objectstore to a storage backend.

This would also allow to have a seperate HomeObjectStoreStorage to be used as home storage while sill having a more general ObjectStoreStorage which can be mounted everywhere by the user.

butonic · 2014-06-16T15:29:44Z

Hm, that will only add another indirection. Currently the "objectstore" config option configures which class should be used to provide the objectstore implementation. The AbstractObjectStorage class is meant to help implement the individual storage backends like swift or s3 ... depending on what capabilities the storage provides developers might want to provide completely different implementations of \OC\Files\Storage\Common is_dir() or any other method in that class. So, I think inheritance makes more sense than composition in this case.

I'll have to think on the matter of an implementation that users can mount themselves ...

ghost · 2014-06-16T17:58:18Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5633/

icewind1991 · 2014-06-16T21:55:27Z

The problem with the current inheritance approach is that you can't have multiple types of objectstorage storages, you can't have a seperate class for HomeObjectStoreStorage because you can't create a subclass of AbstractObjectStorage for that since all implementations extend AbstractObjectStorage and not AbstractHomeObjectStoreStorage.

Imo, composition makes sense here since the type of object storage used is not related to the type of storage backend.

The current implementation just doesn't provide the flexibility I would like to see from this

butonic · 2014-06-17T20:18:20Z

@icewind1991 ok, changed the architecture from inheritance to composition. For now I kept the single "objectstore" configuration and hardcoded using a plain ObjectStoreStorage for the root storage vs a HomeObjectStoreStorage for user homes. Currently the HomeObjectStoreStorage implementation is a subclass of ObjectStoreStorage. As a result the getId implementation is currently the same. For the HomeObjectStoreStorage I could require a user and then always use 'objstore::user:' as the id. But how should I calculate a unique storage id for the plain ObjectStoreStorage class which is intended to be mounted in other mount points? I will have a user name but I need another datum to make the id unique. Maybe the mount point with the slashes squased to underscores? Or the mountpoint as a md5? @icewind1991 got a recommendation? The files external implementation uses the bucket .... hm ... that would only work when the same bucket is used more than once because the fileid is unique.

ghost · 2014-06-17T20:28:22Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5655/

…ounted as root

copy skeleton only for local filesystems

… end user

butonic · 2014-06-26T12:56:44Z

With regard to folders being permanent: yes, they only need to be stored as metadata in the local filecache. The storage for it is an objectstore though. Originally when the constructor failed to connect to the object store it would throw an exception directing users to theri administrator. The connection is now only checked when an actual file up/download is tried due to the lazy initialization introduced by @icewind1991 with eb97eee
Not sure if we need to fail earlier.

butonic · 2014-06-26T12:58:12Z

BUG skeleton (creating the welcome.txt) does not work because it copies the files without the owncloud stream wrapper

PVince81 · 2014-06-26T13:01:50Z

PVince81 · 2014-06-26T13:02:49Z

BUG: SWIFT ext storage: WebDAV/sync client upload seem to have issue with mime type (ALSO on master)

To be raised/handled separately.

PVince81 · 2014-06-26T13:10:52Z

The "cannot detect remote changes" for now, this is an old issue that is discussed here: #8633
Since the library upgrade doesn't break nor improve that issue, I suggest to ignore it for now so we can merge this PR faster.

PVince81 · 2014-06-26T13:13:35Z

I quickly compared the SWIFT ext storage with master and it seems to be faster on this branch.

This PR doesn't break existing functionality and the issues related to object store can be fixed separately. Issues must be raised after this is merged.

👍 from me.

icewind1991 · 2014-06-26T14:31:54Z

@icewind1991 now the testStat() fails for both: objectstore and files_external. The objectstore uses the local database to store the mtime, so hasUpdated actually works correctly. Please revert 92f2a43 because the NoopScanner replaces the Scanner implementation and has the same effect. Or am I wrong?

Simply returning false from hasUpdated() prevents having to query the mtime from the database, since the mtime comparison will always return false it's a completely wasted query

craigpg · 2014-06-26T16:15:27Z

Keeping in oC 7.0 CE

butonic · 2014-06-26T16:32:05Z

On 26. Juni 2014 16:32:03 MESZ, icewind1991 [email protected] wrote:

Simply returning false from hasUpdated() prevents having to query
the mtime from the database, since the mtime comparison will always
return false it's a completely wasted query
That depends on the mtime parameter, but I admit that I don't know who is using this method to determine if a file has updated since a given time. And since we can provide that Implementation based on the filecache, I fail to see the reason for just always returning false when we are breaking the tests with it.

icewind1991 · 2014-06-26T22:40:02Z

The method is only used for updating the cache which is not needed in this case.

For the unit test, I think it's fine best to just overwrite the unit test since it's for a use case not supported

butonic · 2014-06-27T07:38:20Z

For the sake of performance I'll change the Test method. But shouldn't hasUpdated then be something like needsCacheUpdate()? Otherwise someone else might start relying on the functionality. Anyway, I'll update the PHPDoc comment to clarify a bit.

PVince81 · 2014-06-27T09:32:14Z

I suggest to ignore / disable the testStat() issue as it isn't directly related to this feature. The problem already existed before. Let's merge this as soon as possible.

Maybe you can just override and NOOP that test for now and follow up in #8633 for a proper solution later.

scrutinizer-notifier · 2014-06-27T10:14:44Z

A new inspection was created.

butonic · 2014-06-27T10:15:30Z

@PVince81 @icewind1991 @DeepDiver1975 overwrote the testStat and updated the hasUpdated descriptions. lets see what jenkins says and then merge it

scrutinizer-notifier · 2014-06-27T10:40:53Z

The inspection completed: 32 new issues, 54 updated code elements

ghost · 2014-06-27T11:28:11Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5799/

icewind1991 · 2014-06-27T12:01:58Z

lib/private/files/objectstore/objectstorestorage.php

+		$path = str_replace('//', '/', $path);
+
+		if (!$path) {
+			$path = '.';


What's the reasoning behind not just returning an empty string here?

dirname('/folder') returns .. So identify the root folder as . I assume you don't want to see '.' in the filecache table? I'll look into that.

scrutinizer-notifier · 2014-06-27T13:00:44Z

A new inspection was created.

butonic · 2014-06-27T13:04:03Z

@icewind1991 ok, now the root is stored as '' in the cache. Since I started with a copy of files_externals swift implementation I assume it also stores root as '.' in the cache, but thats a different issue.

good to merge?

icewind1991 · 2014-06-27T13:15:11Z

👍

butonic · 2014-06-27T13:24:50Z

@karlitschek only waiting for jenkins build before merging

ghost · 2014-06-27T14:46:27Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5802/

Object storage

PVince81 · 2014-06-27T14:53:18Z

Yay! Clicked the big fat green button 😄
Congrats, guys!

butonic · 2014-06-27T15:07:14Z

ok, next up fixing versions and text editor previews ...

PVince81 reviewed Apr 28, 2014
View reviewed changes

karlitschek added the Planned Feature label May 13, 2014

karlitschek added this to the ownCloud 7 milestone May 13, 2014

butonic mentioned this pull request Jun 11, 2014

add rackspace and update composer owncloud-archive/3rdparty#99

Merged

butonic changed the title ~~[WIP] Object storage~~ Object storage Jun 12, 2014

icewind1991 reviewed Jun 16, 2014
View reviewed changes

butonic added 7 commits June 18, 2014 12:53

allow configuring objectstore as home and root storage

3c66a8c

make objectstore setup more robust, don't pass user to object store m…

2bf7e5c

…ounted as root

change 'object_storage' config param to 'root_storage'

85ccfad

copy skeleton only for local filesystems

move swift and abstract code to core

ccb698a

require updated 3rdparty with rackspace libs

f1bf132

fix fallback config for default home storage

1374ba9

when root storage cannot be mounted throw an exception visible to the…

60a8419

… end user

update hasUpdated description, overwrtie testStat for objectstore test

ab93f19

icewind1991 reviewed Jun 27, 2014
View reviewed changes

use normalize path to convert '.' to '' in objectstorestorage

25dbbba

PVince81 pushed a commit that referenced this pull request Jun 27, 2014

Merge pull request #8383 from owncloud/object_storage

fd8b568

Object storage

PVince81 merged commit fd8b568 into master Jun 27, 2014

PVince81 deleted the object_storage branch June 27, 2014 14:53

lock bot locked as resolved and limited conversation to collaborators Aug 21, 2019

Object storage #8383

Object storage #8383

Conversation

DeepDiver1975 commented Apr 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic commented Jun 6, 2014

karlitschek commented Jun 7, 2014

butonic commented Jun 7, 2014

karlitschek commented Jun 7, 2014

PVince81 commented Jun 8, 2014

butonic commented Jun 10, 2014

butonic commented Jun 12, 2014

butonic commented Jun 13, 2014

butonic commented Jun 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icewind1991 commented Jun 16, 2014

butonic commented Jun 16, 2014

ghost commented Jun 16, 2014

icewind1991 commented Jun 16, 2014

butonic commented Jun 17, 2014

ghost commented Jun 17, 2014

butonic commented Jun 26, 2014

butonic commented Jun 26, 2014

PVince81 commented Jun 26, 2014

Manual test of the SWIFT external storage (against devstack)

PVince81 commented Jun 26, 2014

PVince81 commented Jun 26, 2014

PVince81 commented Jun 26, 2014

icewind1991 commented Jun 26, 2014

craigpg commented Jun 26, 2014

butonic commented Jun 26, 2014

icewind1991 commented Jun 26, 2014

butonic commented Jun 27, 2014

PVince81 commented Jun 27, 2014

scrutinizer-notifier commented Jun 27, 2014

butonic commented Jun 27, 2014

scrutinizer-notifier commented Jun 27, 2014

ghost commented Jun 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrutinizer-notifier commented Jun 27, 2014

butonic commented Jun 27, 2014

icewind1991 commented Jun 27, 2014

butonic commented Jun 27, 2014

ghost commented Jun 27, 2014

PVince81 commented Jun 27, 2014

butonic commented Jun 27, 2014