Disable the cache updater when doing the encryption migration #14573

icewind1991 · 2015-02-27T13:17:40Z

Brings the time to do the migration in the unit tests down to <2s from ~11s on my local machine

cc @PVince81 @DeepDiver1975 @schiesbn

ghost · 2015-02-27T13:42:02Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10057/
Test PASSed.

karlitschek · 2015-02-27T13:47:44Z

very nice. Unfortunately @schiesbn is out the next few weeks. I don´t know who can test this.

PVince81 · 2015-02-27T13:57:19Z

I can. I did the benchmark 😄

PVince81 · 2015-02-27T13:59:21Z

apps/files_encryption/lib/migration.php

@@ -74,6 +75,7 @@ public function reorganizeSystemFolderStructure() {
 		$this->view->deleteAll('/owncloud_private_key');
 		$this->view->deleteAll('/files_encryption/share-keys');
 		$this->view->deleteAll('/files_encryption/keyfiles');
+		$this->view->getMount('')->getStorage()->getScanner()->scan('');


Would be good to only rescan the "files_encryption" folder, not everything.

Ah, I see it's done below.

Do we really need to rescan the root ?

This way we clean up owncloud_private_key and there isn't much more then encryption files in the root storage afaik

Won't this also automatically index all the home folders inside the "root" storage ?
Maybe better run the scan explicitly on "files_encryption" and "owncloud_private_key" (or tell the cache to directly remove the entry)

Won't this also automatically index all the home folders inside the "root" storage ?

It might, will change it

I was running the test before you change this, and can confirm that the scanner goes into the user folder, the backup folders, etc...

PVince81 · 2015-02-27T14:20:02Z

This with the benchmark diff with this fix: https://blackfire.io/profiles/compare/889f482d-7902-46c6-ac1b-fc2d64e9b139/graph (but without the last change that prevents rescanning root)

It's now faster.

PVince81 · 2015-02-27T14:33:05Z

Can you add a unit test that tests whether disabling the file cache really disables it ? (no effect)

PVince81 · 2015-02-27T14:46:04Z

Nice, this saves another minute. Here is the updated comparison: https://blackfire.io/profiles/compare/079d0fd6-fe33-447b-9869-4b1ff100d302/graph

icewind1991 · 2015-02-27T14:49:29Z

Can you add a unit test that tests whether disabling the file cache really disables it ? (no effect)

Done

PVince81 · 2015-02-27T14:49:44Z

@icewind1991 here is the report: https://blackfire.io/profiles/6fe2f304-dbc1-48b8-9113-9c2e98350a9c/graph

It seems the hooks code is triggered too. It would probably make sense to disable hooks too when working on encryption keys.

What do you think of that "is_dir()" that is called 1260992 times ?

PVince81 · 2015-02-27T14:53:36Z

Note that in my test case there is only one share key for each file, because I only have a single user.
There are 1600 files, not sure why is_dir would be called that often, seems to be this one here: https://github.com/owncloud/core/blob/master/apps/files_encryption/lib/migration.php#L258

I also see here that keys are copied instead of moved here: https://github.com/owncloud/core/blob/master/apps/files_encryption/lib/migration.php#L264

Not sure why @schiesbn decided to copy, especially that we already have a backup of all keys in the backup folder.

PVince81 · 2015-02-27T14:54:00Z

I'm ok to merge this PR first as it already provides an improvement 👍

ghost · 2015-02-27T14:58:06Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10061/
Test PASSed.

PVince81 · 2015-02-27T15:09:48Z

@icewind1991 I'll have a try changing "copy" to "rename" and see if it improves.
As for the is_dir call and hooks I'm not sure.,

PVince81 · 2015-02-27T15:26:30Z

Wow, replacing "copy" with "rename" reduced from 4 minutes to 2 minutes. (just pushed)
Here is the comparison of v8.0.0 against this branch: https://blackfire.io/profiles/compare/4fecf69f-b6fb-403e-aa8d-7ffc2513af1c/graph

PVince81 · 2015-02-27T16:15:08Z

Rebased.

@icewind1991 can you thumbs up my commit and show enthusiasm about the last performance report ? 😛

I'll do some more testing, we need to make sure trashbin keys also still work.

PVince81 · 2015-02-27T16:21:33Z

Tests

single user update
update with multiple users with several shares and reshares
update with stuff in trash, must be restorable afterwards
update with versions

DeepDiver1975 · 2015-02-27T16:31:53Z

Nice job @icewind1991 @PVince81 !!!

PVince81 · 2015-02-27T16:32:10Z

Something went wrong with the trashbin (it was a shared file that I deleted), but I suspect it's not related to this PR. Will check.

PVince81 · 2015-02-27T16:34:52Z

test: link share

PVince81 · 2015-02-27T16:36:00Z

Bääh... it's a bug in stable7, will raise separately.

PVince81 · 2015-02-27T16:41:54Z

The trashbin bug about sharing is only in stable7 but not master, will need to find the PR that fixes it.

But I can say that one user was able to restore the file from trashbin when not shared, so it works.

If we all agree on the code changes I can submit a Jenkins PR ?

PVince81 · 2015-02-27T16:42:13Z

@th3fallen can you review, too ?

PVince81 · 2015-02-27T17:11:45Z

Raised trashbin issue here, it's stable7 only: #14588

scrutinizer-notifier · 2015-02-27T17:14:46Z

The inspection completed: 1 new issues, 5 updated code elements

PVince81 · 2015-02-27T17:19:21Z

test: system mount external storage
test: personal mount external storage

DeepDiver1975 · 2015-03-02T08:14:55Z

👍

icewind1991 · 2015-03-02T12:46:09Z

👍 for @PVince81's changes

PVince81 · 2015-03-02T13:23:44Z

@owncloud-bot retest this please

ghost · 2015-03-02T13:52:19Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10119/
Test PASSed.

Disable the cache updater when doing the encryption migration

PVince81 · 2015-03-02T13:53:49Z

@karlitschek Backport to stable8 for 8.0.2 ?

PVince81 · 2015-03-02T13:54:30Z

Note, this is "only" a performance improvement, so the question is whether it's acceptable to have slow OC7->OC8.0.2 encryption migration.

DeepDiver1975 · 2015-03-02T19:34:06Z

Note, this is "only" a performance improvement, so the question is whether it's acceptable to have slow OC7->OC8.0.2 encryption migration.

Well - the user experience was pretty bad on this - we had user where the migration command was running over night. It would have been nice to have this fixed for 8.0.1 (especially because of EE release).

We at least need a PR against stable8 to allow everybody to patch their installations is necessary.

Long story short: please prepare a PR against stable8 - THX

icewind1991 · 2015-03-03T12:15:40Z

stable8: #14669

icewind1991 mentioned this pull request Feb 27, 2015

Problem with upgrading files_encryption from OC 7 => OC 8 #14012

Closed

PVince81 reviewed Feb 27, 2015
View reviewed changes

icewind1991 force-pushed the enc-migrate-disable-updater branch from 868d0f4 to ef91c0c Compare February 27, 2015 14:16

icewind1991 and others added 4 commits February 27, 2015 17:14

Allow disabling the cache updater

f6182aa

Disable the cache updater when doing the encryption migration

6f000ff

add some tests for disabled updater

89735ab

Move keys instead of copy during encryption key migration

22bc622

PVince81 force-pushed the enc-migrate-disable-updater branch from 43252aa to 22bc622 Compare February 27, 2015 16:14

DeepDiver1975 added this to the 8.1-current milestone Feb 27, 2015

DeepDiver1975 added the 3 - To Review label Feb 27, 2015

PVince81 pushed a commit that referenced this pull request Mar 2, 2015

Merge pull request #14573 from owncloud/enc-migrate-disable-updater

e61aa37

Disable the cache updater when doing the encryption migration

PVince81 merged commit e61aa37 into master Mar 2, 2015

PVince81 deleted the enc-migrate-disable-updater branch March 2, 2015 13:53

DeepDiver1975 added the Backport-Request label Mar 2, 2015

icewind1991 mentioned this pull request Mar 3, 2015

[stable8] Disable the cache updater when doing the encryption migration #14669

Merged

MorrisJobke added 8.0-current and removed Backport-Request labels Mar 18, 2015

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

Disable the cache updater when doing the encryption migration #14573

Disable the cache updater when doing the encryption migration #14573

Conversation

icewind1991 commented Feb 27, 2015

ghost commented Feb 27, 2015

karlitschek commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 Feb 27, 2015

Choose a reason for hiding this comment

PVince81 Feb 27, 2015

Choose a reason for hiding this comment

icewind1991 Feb 27, 2015

Choose a reason for hiding this comment

PVince81 Feb 27, 2015

Choose a reason for hiding this comment

icewind1991 Feb 27, 2015

Choose a reason for hiding this comment

icewind1991 Feb 27, 2015

Choose a reason for hiding this comment

PVince81 Feb 27, 2015

Choose a reason for hiding this comment

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

icewind1991 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

ghost commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

Tests

DeepDiver1975 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

PVince81 commented Feb 27, 2015

scrutinizer-notifier commented Feb 27, 2015

PVince81 commented Feb 27, 2015

DeepDiver1975 commented Mar 2, 2015

icewind1991 commented Mar 2, 2015

PVince81 commented Mar 2, 2015

ghost commented Mar 2, 2015

PVince81 commented Mar 2, 2015

PVince81 commented Mar 2, 2015

DeepDiver1975 commented Mar 2, 2015

icewind1991 commented Mar 3, 2015