Store storage availability in database #13641

RobinMcCorkell · 2015-01-23T22:02:51Z

Storage status is saved in the database. Failed storages are rechecked every 10 minutes, while working storages are rechecked every request. Using the files_external app will recheck all external storages when the settings page is viewed, or whenever an external storage is saved.

The advantage of this is that failed external storages will not impede page loading, apart from every 10 minutes when a recheck is performed. To test, simply create several external storages will valid details other than the credentials (SFTP is nice and slow for this), then load the files page. On master, it will load slowly as timeouts occur. With this PR, it will load slowly once, then be fast for 10 minutes.

The counterpart to this is #13515, which eliminates the slowdown on the settings pages.

Fixes #13531

cc @PVince81 @icewind1991 @DeepDiver1975

P.S. This is my first work involving database stuff. It's likely that I got something wrong, so review those bits in particular! The unit tests should help though 😄

LukasReschke · 2015-01-23T22:06:06Z

lib/private/files/cache/storage.php

@@ -91,6 +99,36 @@ public static function getNumericStorageId($storageId) {
 	}

 	/**
+	 * @return bool


May also return null

Specifically cannot return null - see database schema

What is L108 then for?

Ah, sorry, it was late and I wasn't thinking properly 😄

LukasReschke · 2015-01-23T22:08:00Z

17:06:08 .....................................PHP Fatal error:  Call to undefined method Mock_Storage_77aba13b::test() in /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/files/mount/mountpoint.php on line 207
17:06:11 PHP Stack trace:
17:06:11 PHP   1. {main}() /usr/local/bin/phpunit:0
17:06:11 PHP   2. PHPUnit_TextUI_Command::main() /usr/local/bin/phpunit:612
17:06:11 PHP   3. PHPUnit_TextUI_Command->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:138
17:06:11 PHP   4. PHPUnit_TextUI_TestRunner->doRun() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:186
17:06:11 PHP   5. PHPUnit_Framework_TestSuite->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/TestRunner.php:423
17:06:11 PHP   6. PHPUnit_Framework_TestSuite->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:751
17:06:11 PHP   7. PHPUnit_Framework_TestCase->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:751
17:06:11 PHP   8. PHPUnit_Framework_TestResult->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:722
17:06:11 PHP   9. PHP_Invoker->invoke() phar:///usr/local/bin/phpunit/phpunit/Framework/TestResult.php:641
17:06:11 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
17:06:11 PHP  11. PHPUnit_Framework_TestCase->runBare() phar:///usr/local/bin/phpunit/php-invoker/Invoker.php:93
17:06:11 PHP  12. PHPUnit_Framework_TestCase->runTest() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:766
17:06:11 PHP  13. ReflectionMethod->invokeArgs() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:881
17:06:11 PHP  14. Test\Files\Node\Root->testGet() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:881
17:06:11 PHP  15. OC\Files\Node\Root->mount() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/tests/lib/files/node/root.php:54
17:06:11 PHP  16. OC\Files\Mount\Manager->addMount() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/files/node/root.php:112
17:06:11 PHP  17. OC\Files\Mount\MountPoint->isAvailable() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/files/mount/manager.php:23

RobinMcCorkell · 2015-01-24T13:06:48Z

Hmm, I'm trying to fix these test failures, and it seems there is a cyclic dependency. This PR introduces the isAvailable() test before allowing a storage to be mounted, which checks if the storage can be stat()'d. However, during login, the filesystem is set up before the home storage is created (for a new user where it doesn't already exist), which fails because the home storage is unreachable at that stage.

There are a couple of solutions that I can see: either move the home storage creation so that it occurs before the filesystem is set up, or create the home storage while the storage is being mounted (so trigger that from test() or isAvailable()). This is going deep into things-may-break-horribly territory though, perhaps @icewind1991 can help?

PVince81 · 2015-01-26T10:04:28Z

Nice work. I wonder if it's too early for this, as we wanted to revamp the DB anyway here: #11261 (comment)

But that might fit with the new scheme, too.
I'll review this later for 8.1.

DeepDiver1975 · 2015-01-26T10:12:48Z

lib/private/files/cache/storage.php

@@ -91,6 +99,36 @@ public static function getNumericStorageId($storageId) {
 	}

 	/**
+	 * @return bool
+	 */
+	public function getAvailability() {


I'd merge this into one function where all information is returned. With the current design two queries will be fired to get availability and last checked

RobinMcCorkell · 2015-01-27T14:55:40Z

Right, I think I've got it now. The availability checker is implemented as a storage wrapper, and uses the following algorithm for every storage call:

If marked as available in storage cache, continue with normal call.
If recheck TTL reached, perform storage test, update storage cache and go to 1. with new availability information
Otherwise, throw a StorageNotAvailableException

If the call to the storage itself throws a StorageNotAvailableException, the storage will be marked as not available in the storage cache, then the exception is rethrown up the call stack.

One issue noticed with this new code: getDirectoryContent() in a view will break if an exception is thrown at any point, such as when external storages visible in the root directory are probed. I added a try catch block to stop this, so that StorageNotAvailableExceptions are silently ignored.

For this PR to be useful, storages need to throw StorageNotAvailableException when they fail. At the moment, very few actually do, but for testing purposes the SFTP changes I made in #13390 work well. As mentioned earlier, SFTP is very good for noticing the timeouts, since an authentication failure typically has a long timeout.

@icewind1991 Tell me if I'm doing anything wrong 😄

TODO:

Write unit tests

icewind1991 · 2015-01-27T15:15:26Z

This approach seems fine by me

RobinMcCorkell · 2015-02-09T16:36:17Z

Rebase conflict was the version.php update. No other code is changed.

RobinMcCorkell · 2015-02-24T13:18:23Z

Rebase conflict was license headers and version.php.

icewind1991 · 2015-03-24T15:19:36Z

👍 nice work

MorrisJobke · 2015-03-24T15:38:05Z

I added a SMB backend in the admin panel
green dot appears
I shut down the SMB service
I modified the last_accessed column to value-1000
accessed the mounted folder to retrigger the check
database still holds the available value 1

RobinMcCorkell · 2015-03-30T11:38:15Z

@MorrisJobke This only works if the storage cooperates and throws StorageNotAvailableException. Try merging sftp_exceptions (#13390) with this branch and set up an SFTP storage. Then it will work as you expect. This PR just lays the groundwork for storage availability

scrutinizer-notifier · 2015-07-20T15:06:15Z

A new inspection was created.

Storage status is saved in the database. Failed storages are rechecked every 10 minutes, while working storages are rechecked every request. Using the files_external app will recheck all external storages when the settings page is viewed, or whenever an external storage is saved.

This usually doesn't cause issues, but in unit tests sometimes a wrapped storage is passed to Filesystem::mount() and gets rewrapped, hitting the XDebug function nesting level limit when used.

scrutinizer-notifier · 2015-07-20T15:27:40Z

A new inspection was created.

ghost · 2015-07-20T17:29:31Z

🚀 Test PASSed.🚀

RobinMcCorkell · 2015-07-28T15:25:08Z

@MorrisJobke Could you re-review this please?

RobinMcCorkell · 2015-07-28T15:25:23Z

Also cc @PVince81

PVince81 · 2015-08-05T14:10:53Z

lib/private/files/mount/mountpoint.php

+			$this->storage = $storage;
+			// only wrap if not already wrapped
+			if (!($this->storage instanceof Wrapper)) {
+				$this->storage = $this->loader->wrap($this, $this->storage);


Is this required by this PR or is an additional fix ?

Without this fix, unit tests will fail, since our unit tests attempt to re-wrap storages sometimes causing function nesting limit failures. In the actual codebase it makes no difference, since we never pass a wrapped storage to be re-wrapped.

Actually, this is a poor fix. There may be usecases where instead of a Storage being passed in, a Wrapper is (aka pre-wrapped storage) but still needs to be wrapped with the other storage wrappers too.

PVince81 · 2015-08-07T15:30:53Z

Didn't destroy the known universe 👍 (ran a few smashbox tests and encryption + ext storage)

Store storage availability in database

PVince81 · 2015-08-07T15:53:55Z

Regression, cannot create user: #18129

RobinMcCorkell added app:files_external performance labels Jan 23, 2015

RobinMcCorkell added this to the 8.1-next milestone Jan 23, 2015

LukasReschke reviewed Jan 23, 2015
View reviewed changes

DeepDiver1975 reviewed Jan 26, 2015
View reviewed changes

PVince81 mentioned this pull request Jan 26, 2015

Not all remote shares removed #13569

Closed

RobinMcCorkell force-pushed the cache-storage-status branch 2 times, most recently from 040cde0 to 1eb24d9 Compare January 26, 2015 16:30

RobinMcCorkell changed the title ~~Store storage availability in database~~ [WIP]Store storage availability in database Jan 26, 2015

RobinMcCorkell changed the title ~~[WIP]Store storage availability in database~~ [WIP] Store storage availability in database Jan 26, 2015

RobinMcCorkell force-pushed the cache-storage-status branch 2 times, most recently from 4f89a52 to 3a6c0a8 Compare January 27, 2015 14:44

RobinMcCorkell force-pushed the cache-storage-status branch from 3a6c0a8 to c784d7e Compare January 28, 2015 09:35

RobinMcCorkell changed the title ~~[WIP] Store storage availability in database~~ Store storage availability in database Jan 28, 2015

This was referenced Jan 28, 2015

[CI] Storage availability #13723

Closed

[CI] Storage availability 2 #13724

Closed

RobinMcCorkell force-pushed the cache-storage-status branch from c784d7e to 796b4f6 Compare February 9, 2015 16:35

LukasReschke added the 2 - Developing label Feb 24, 2015

LukasReschke changed the title ~~Store storage availability in database~~ [WIP] Store storage availability in database Feb 24, 2015

RobinMcCorkell force-pushed the cache-storage-status branch from 796b4f6 to 2ba1019 Compare February 24, 2015 13:16

RobinMcCorkell force-pushed the cache-storage-status branch from 2ba1019 to 6ce67af Compare February 26, 2015 11:55

RobinMcCorkell force-pushed the cache-storage-status branch from b96796c to a69b752 Compare March 30, 2015 10:54

RobinMcCorkell mentioned this pull request Apr 8, 2015

Master unit tests are broken #15326

Closed

MorrisJobke added 2 - Developing and removed 3 - To Review labels Jul 2, 2015

RobinMcCorkell force-pushed the cache-storage-status branch from a69b752 to 79e131f Compare July 20, 2015 15:06

Robin McCorkell added 2 commits July 20, 2015 16:27

Fix mounting wrapped storages resulting in many-layered wrapping

75a5e6e

This usually doesn't cause issues, but in unit tests sometimes a wrapped storage is passed to Filesystem::mount() and gets rewrapped, hitting the XDebug function nesting level limit when used.

RobinMcCorkell force-pushed the cache-storage-status branch from 79e131f to 75a5e6e Compare July 20, 2015 15:27

MorrisJobke added 3 - To Review and removed 2 - Developing labels Jul 20, 2015

RobinMcCorkell mentioned this pull request Aug 3, 2015

Properly handle unavailable external storages #11792

Closed

9 tasks

PVince81 reviewed Aug 5, 2015
View reviewed changes

PVince81 pushed a commit that referenced this pull request Aug 7, 2015

Merge pull request #13641 from owncloud/cache-storage-status

b3a1aef

Store storage availability in database

PVince81 merged commit b3a1aef into master Aug 7, 2015

PVince81 deleted the cache-storage-status branch August 7, 2015 15:31

PVince81 mentioned this pull request Aug 7, 2015

Cannot create user on master (1054 Unknown column 'available' in 'where clause) #18129

Closed

RobinMcCorkell mentioned this pull request Aug 7, 2015

Replace 'don't wrap storage if already wrapped' fix attempt #18130

Closed

PVince81 mentioned this pull request Aug 20, 2015

Bogus shared::$path storage on master #18455

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store storage availability in database #13641

Store storage availability in database #13641

RobinMcCorkell commented Jan 23, 2015

LukasReschke Jan 23, 2015

RobinMcCorkell Jan 23, 2015

LukasReschke Jan 24, 2015

RobinMcCorkell Jan 24, 2015

LukasReschke commented Jan 23, 2015

RobinMcCorkell commented Jan 24, 2015

PVince81 commented Jan 26, 2015

DeepDiver1975 Jan 26, 2015

RobinMcCorkell commented Jan 27, 2015

icewind1991 commented Jan 27, 2015

RobinMcCorkell commented Feb 9, 2015

RobinMcCorkell commented Feb 24, 2015

icewind1991 commented Mar 24, 2015

MorrisJobke commented Mar 24, 2015

RobinMcCorkell commented Mar 30, 2015

scrutinizer-notifier commented Jul 20, 2015

scrutinizer-notifier commented Jul 20, 2015

ghost commented Jul 20, 2015

RobinMcCorkell commented Jul 28, 2015

RobinMcCorkell commented Jul 28, 2015

PVince81 Aug 5, 2015

RobinMcCorkell Aug 5, 2015

RobinMcCorkell Aug 7, 2015

PVince81 commented Aug 7, 2015

PVince81 commented Aug 7, 2015

Store storage availability in database #13641

Store storage availability in database #13641

Conversation

RobinMcCorkell commented Jan 23, 2015

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

LukasReschke commented Jan 23, 2015

RobinMcCorkell commented Jan 24, 2015

PVince81 commented Jan 26, 2015

Choose a reason for hiding this comment

RobinMcCorkell commented Jan 27, 2015

icewind1991 commented Jan 27, 2015

RobinMcCorkell commented Feb 9, 2015

RobinMcCorkell commented Feb 24, 2015

icewind1991 commented Mar 24, 2015

MorrisJobke commented Mar 24, 2015

RobinMcCorkell commented Mar 30, 2015

scrutinizer-notifier commented Jul 20, 2015

scrutinizer-notifier commented Jul 20, 2015

ghost commented Jul 20, 2015

RobinMcCorkell commented Jul 28, 2015

RobinMcCorkell commented Jul 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Aug 7, 2015

PVince81 commented Aug 7, 2015