Properly propagate changes between related storages

[RobinMcCorkell]

An attempt at fixing owncloud/enterprise#804.

The ETag propagator looks for related storage, which are detected using a new oc_storages field 'config_id', set by files_external to a unique value that groups together storage configs. When a cache entry is being updated in such a storage, the path gets written to a new table oc_storages_recheck along with the related storage IDs. When the other user(s) log in, this table is consulted and any paths listed get rescanned, and the entries deleted. Thus, changes get properly propagated across related storages that do not share a storage ID.

I encountered one problem: apps/files/ajax/scan.php will try to scan the storage, but for some reason marks everything as changed. Thus on login in the web interface oc_storages_recheck will be populated with every file the scanner finds, which isn't great, since the vast majority of them (if not all) are not changes at all. @icewind1991 can you shed some light on this? This results in vastly decreased performance, since it effectively triggers a DELETE query for every file in the storage (when oc_storages_recheck gets read and cleared). Not good.

cc @cmonteroluque @DeepDiver1975 @PVince81 @jvillafanez

[PVince81]

Is this fix the "propagate etag to all applicable users of the same storage entry" ?
Or does it do magic to bind together separate storage entries that might point to the same share ? 😕

[RobinMcCorkell]

@PVince81 The second one, the first problem is still unsolved.

[RobinMcCorkell]

To test (if you don't have an SMB domain handy):

1. Set up SFTP with two users
2. Create the same users in ownCloud, with the same passwords
3. Set up an SFTP storage with Session credentials in the admin settings, make applicable to both users
4. Set filesystem check to 'Never' for that storage.
5. Via WebDAV (to avoid ajax/scan.php), make a change as one user
6. Do the same as the other user
7. Notice that changes are propagated properly between the storages (magic!)

[PVince81]

The second one is dangerous, because in some cases different credentials to the same share might produce a different directory structure. (as pointed out before by @jvillafanez @jmaciasportela)

[RobinMcCorkell]

@PVince81 That's why a scan is triggered, instead of simply updating the filecache. If the other credentials do not give permission to the file, the scan will not add anything to the cache.

[DeepDiver1975]

💥

PHP Fatal error:  Call to a member function rescanUncleanPaths() on null in /var/jenkins/workspace/core-ci-linux/database/sqlite/label/SLAVE/lib/private/files/mount/manager.php on line 45
21:03:01 PHP Stack trace:
21:03:01 PHP   1. {main}() /usr/local/bin/phpunit:0
21:03:01 PHP   2. PHPUnit_TextUI_Command::main() /usr/local/bin/phpunit:537
21:03:01 PHP   3. PHPUnit_TextUI_Command->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:105
21:03:01 PHP   4. PHPUnit_TextUI_TestRunner->doRun() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:153
21:03:01 PHP   5. PHPUnit_Framework_TestSuite->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/TestRunner.php:406
21:03:01 PHP   6. PHPUnit_Framework_TestSuite->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:722
21:03:01 PHP   7. PHPUnit_Framework_TestCase->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:722
21:03:01 PHP   8. PHPUnit_Framework_TestResult->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:699
21:03:01 PHP   9. PHP_Invoker->invoke() phar:///usr/local/bin/phpunit/phpunit/Framework/TestResult.php:607
21:03:01 PHP  10. call_user_func_array:{phar:///usr/local/bin/phpunit/php-invoker/Invoker.php:93}() phar:///usr/local/bin/phpunit/php-invoker/Invoker.php:93
21:03:01 PHP  11. PHPUnit_Framework_TestCase->runBare() phar:///usr/local/bin/phpunit/php-invoker/Invoker.php:93
21:03:01 PHP  12. PHPUnit_Framework_TestCase->runTest() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:743
21:03:01 PHP  13. ReflectionMethod->invokeArgs() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:866
21:03:01 PHP  14. Test\Files\Node\Root->testGet() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:866
21:03:01 PHP  15. OC\Files\Node\Root->mount() /var/jenkins/workspace/core-ci-linux/database/sqlite/label/SLAVE/tests/lib/files/node/root.php:54
21:03:01 PHP  16. OC\Files\Mount\Manager->addMount() /var/jenkins/workspace/core-ci-linux/database/sqlite/label/SLAVE/lib/private/files/node/root.php:126

[DeepDiver1975]

I'm not willing to accept a database change after feature freeze -> 9.0

[RobinMcCorkell]

@cmonteroluque The linked issue is for 8.1.4, what do we do?

[RobinMcCorkell]

@DeepDiver1975 Tests fixed (grumble grumble why don't people use mocks...)

[DeepDiver1975]

@cmonteroluque The linked issue is for 8.1.4, what do we do?

@cmonteroluque database changes are not acceptable for minor releases - not much we can do - unless we find another solution ...

[DeepDiver1975]

table has no id, no pk -> bad design

[RobinMcCorkell]

Do I just need to add an auto-incrementing ID column and create an index for it as the primary key? (I'm still a newbie when it comes to DB stuff)

[karlitschek]

I agree with @DeepDiver1975 too risky for now.

[RobinMcCorkell]

Fixes #13456

[ghost]

@DeepDiver1975 @karlitschek We committed to fixing owncloud/enterprise#804 for a major customer in the 8.1.4 timeframe. I think we need to bring this back, if a safer patch is required I am open to that.

[icewind1991]

I'm not sure we should have this in core since it's not a very common use case (for community at least). Having it in an app would be better imo

[RobinMcCorkell]

@icewind1991 The code needs to interface closely with the cache propagator, so I don't think it would work to have this in a separate app. Also, the code doesn't do anything unless storages are configured with a config_id, which is set in files_external, so any core storages won't be affected.

[icewind1991]

From what I can tell the only place where there is coupling between this propagation and core is the change propagator where storages are marked as changed, that coupling can be replaced by a adding a new hook to the change propagator so apps can listen to changes

[RobinMcCorkell]

@icewind1991 OK, that would work, but splitting this into an app would mean that files_external would have an inter-app dependency, which I thought we wanted to avoid at all costs. I'd just like to stress that this code does absolutely nothing unless an app configures a storage in the right way...

[PVince81]

@icewind1991 has been working on moving etag propagation to the storages, see #20439

Not sure in what way this would affect this PR here ?

[RobinMcCorkell]

Since related storages (but not identical storages) are treated as separate storages in oC, #20439 probably won't fix the issue at hand.

[PVince81]

Would be good to revive this.

My main concern is about SMB storages that deliver different trees for the same shares when the user name is different (like the "homes" share). We might need a way to find out whether the remote share/folder is actually the same. Maybe set some extended attributes there ?

[RobinMcCorkell]

@PVince81 We are trying to fix sooo many edge cases with this approach... unless it is absolutely required soon I'd be more in favour of killing this PR and starting over with a better approach, namely a daemon that listens for changes on the remote storage and notifies ownCloud. With this approach the DB will get needless writes, requests will become very slow, and we'll hit all sorts of weird edge cases

[PVince81]

Another idea then: a new mount checkbox option "Use the same storage for all users" that internaly uses the same storage id for all users.

[icewind1991]

Another idea then: a new mount checkbox option "Use the same storage for all users" that internaly uses the same storage id for all users.

That will break everything if the storage contents are different (also with only different permissions or something)

[PVince81]

That will break everything if the storage contents are different (also with only different permissions or something)

Agreed, but that would be the admin's responsibility to make the right choice. So we might have proper warnings for that option. I'd disable it by default.

If the admin does it properly, would this qualify as a good solution ?

[bboule]

@cmonteroluque @PVince81 This seems stalled is there anything we can do? I went ahead and assigned a milestone sure it is wrong but this seems like something that needs a little love

[ghost]

@bboule no argument here

[MorrisJobke]

@bboule @cmonteroluque I think there is nothing we could do in the scope of 9.0 and 8.1.6 - I will adjust the milestone to 9.1 (same as the enterprise ticket)

[MorrisJobke]

I think there is nothing we could do in the scope of 9.0 and 8.1.6

because it involves a restructuring of the underlying concepts how stuff is propagated.

[PVince81]

Do we still want this somewhat risky approach ?

Needs rebase

[DeepDiver1975]

Do we still want this somewhat risky approach ?

@icewind1991 we basically need to rely on your expertise here - do you accept this change or not? THX

[PVince81]

I vote against this risky change 👎 unless there is a compelling reason.
Waiting for @icewind1991 to comment

[icewind1991]

I don't think the current approach is something we want, with smb notify we've cover the majority of the use case already and adding a "Use the same storage for all users" option that was discussed above should be a better way to cover the rest.

[PVince81]

Thanks, closing this.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.