fix(CardDAV): only run upgrade sync if 1000 users or less by miaulalala · Pull Request #39911 · nextcloud/server

miaulalala · 2023-08-16T14:45:16Z

Resolves: [Bug]: <dav> upgrade step takes hours when server connected to large LDAP directory, or has hundreds of users #39744
Resolves: [Bug]: Updating <dav> ... very slow with a lot of users #39769

Summary

Limit the upgrade sync to instances with 1000 users or less and show a warning in the admin section if the sync hasn't been run:

TODO

Adjust tests

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

ChristophWurst

nit: append 27 to upgrade_dav_sync_run to allow future dav upgrade syncs to have a unique key too

kesselb · 2023-08-17T09:39:59Z

nit: append 27 to upgrade_dav_sync_run to allow future dav upgrade syncs to have a unique key too

I don't understand 😕 Let's say we need another sync from 28 to 29. If we use the app config key without a prefix/suffix, we can reuse the same logic without changing the command or setup check.

ChristophWurst · 2023-08-17T09:56:31Z

Fair point if we reset upgrade_dav_sync_run. Sorry for the misdirection.

kesselb · 2023-08-17T13:38:51Z

How to test:

Patch countUsers in lib/private/User/Manager.php to return 1001
php occ migrations:execute dav 1027Date2023050412294
Should show the warning: "Could not sync system address books during update - too many user records have been found. Please call occ dav:sync-system-addressbook manually."
Login as admin and head to overview.
Should see a warning: "The DAV system address book sync has not run yet as your instance has more than 1000 users or because an error occured. Please run it manually by calling occ dav:sync-system-addressbook."
php occ dav:sync-system-addressbook
Warning in overview should be gone.

kesselb

👍

Tested the process by manually executing the migration.

Your call if you want to align the app config key or not ;)

kesselb · 2023-08-17T13:42:37Z

/backport to stable27

kesselb · 2023-08-25T11:21:43Z

 */
 	public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
+ if($this->userManager->countUsers() > 1000) {
+ $this->config->setAppValue('dav', 'needs_system_address_book_sync', 'no');


Suggested change
$this->config->setAppValue('dav', 'needs_system_address_book_sync', 'no');
$this->config->setAppValue('dav', 'needs_system_address_book_sync', 'yes');

Signed-off-by: Anna Larch <anna@nextcloud.com>

miaulalala added bug 2. developing Work in progress feature: carddav Related to CardDAV internals labels Aug 16, 2023

miaulalala added this to the Nextcloud 28 milestone Aug 16, 2023

miaulalala requested review from ChristophWurst, kesselb and st3iny August 16, 2023 14:45

miaulalala self-assigned this Aug 16, 2023

tcitworld reviewed Aug 16, 2023
View reviewed changes

Comment threadapps/settings/lib/SetupChecks/HasDavSyncRun.php Outdated

miaulalala marked this pull request as ready for review August 17, 2023 06:10

miaulalala added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 17, 2023

ChristophWurst reviewed Aug 17, 2023
View reviewed changes

Comment threadapps/dav/lib/Migration/Version1027Date20230504122946.php Outdated
Comment threadapps/dav/lib/Migration/Version1027Date20230504122946.php Outdated
Comment threadapps/settings/composer/composer/ClassLoader.php

miaulalala force-pushed the fix/carddav/limit-upgrade-sync-to-1000-users branch from 6a7a28e to 0074408Compare August 17, 2023 08:19

kesselb reviewed Aug 17, 2023
View reviewed changes

Comment threadapps/settings/lib/SetupChecks/HasDavSyncRun.php Outdated

miaulalala requested review from ChristophWurst, kesselb and tcitworld August 17, 2023 13:21

kesselb reviewed Aug 17, 2023
View reviewed changes

kesselb approved these changes Aug 17, 2023
View reviewed changes

backportbot-nextcloudBot added the backport-request label Aug 17, 2023

st3iny approved these changes Aug 21, 2023
View reviewed changes

miaulalala force-pushed the fix/carddav/limit-upgrade-sync-to-1000-users branch 2 times, most recently from d6a3589 to b02c2e5Compare August 25, 2023 06:18

kesselb reviewed Aug 25, 2023
View reviewed changes

Comment threadapps/settings/lib/SetupChecks/NeedsSystemAddressBookSync.php Outdated

kesselb reviewed Aug 25, 2023
View reviewed changes

miaulalala force-pushed the fix/carddav/limit-upgrade-sync-to-1000-users branch from abec672 to b8840c6Compare August 25, 2023 11:31

kesselb force-pushed the fix/carddav/limit-upgrade-sync-to-1000-users branch from b8840c6 to 7406027Compare August 25, 2023 15:23

fix(CardDAV): only run upgrade sync if 1000 users or less
7f11ee8
Signed-off-by: Anna Larch <anna@nextcloud.com>

kesselb force-pushed the fix/carddav/limit-upgrade-sync-to-1000-users branch from 7406027 to 7f11ee8Compare August 26, 2023 15:21

kesselb merged commit 8f4614b into masterAug 27, 2023

kesselb deleted the fix/carddav/limit-upgrade-sync-to-1000-users branch August 27, 2023 13:54

backportbot-nextcloudBot mentioned this pull request Aug 27, 2023
[stable27] fix(CardDAV): only run upgrade sync if 1000 users or less #40066
Merged

backportbot-nextcloudBot removed the backport-request label Aug 27, 2023

	$this->config->setAppValue('dav', 'needs_system_address_book_sync', 'no');
	$this->config->setAppValue('dav', 'needs_system_address_book_sync', 'yes');

Uh oh!

Uh oh!

Conversation

miaulalala commented Aug 16, 2023

Summary

TODO

Checklist

Uh oh!

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kesselb commented Aug 17, 2023

Uh oh!

ChristophWurst commented Aug 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kesselb commented Aug 17, 2023

Uh oh!

kesselb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kesselb commented Aug 17, 2023

Uh oh!

Uh oh!

kesselbAug 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kesselb left a comment •
edited
Loading