Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle owncloud migration to latest release #23044

Merged
merged 21 commits into from
Dec 10, 2020
Merged

Handle owncloud migration to latest release #23044

merged 21 commits into from
Dec 10, 2020

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Sep 25, 2020

Steps to migrate from 10.5 to this branch

  • make sure to have activity/files_pdfviewer/notifications/viewer/firstrunwizard installed
  • occ upgrade
  • occ db:convert-filecache-bigint
  • occ db:add-missing-columns
  • occ db:add-missing-indices
  • occ db:add-missing-primary-keys (edit by PVince81, had to do this on my setup)

Adjustments

  • Avoid exception in bruteforce protection capabilities on the upgrade page (as the table doesn't exist yet)
  • db:add-missing-indices
    In SchemaException.php line 85: There is no column with name 'userid' on table 'oc_properties'.
  • Check for removed migrations/repair in server steps between 12 and master
  • Revert repair steps that might be relevant when migrating

Missing from the database diff between 10.5 and 20

With this set of patches the migration succeed however there is some difference in the database schema that should be addressed:

  • oc_authtoken -> loginname -> 255, add scope field
  • oc_calendar components 20 -> 64
  • oc_calendarsubscriptions lastmodified should be default null
  • oc_cards -> add KEY addressbookid_uri_index (addressbookid,uri),
  • oc_external_config -> value default null
  • oc_files_trash auto_id bigint
  • oc_jobs execution duration default -1/0
  • oc_mounts -> add KEY mounts_mount_id_index (mount_id)
  • oc_privatedata (can be ingored, moved to https://github.com/nextcloud/privatedata)
  • oc_dav_properties -> migrate to oc_properties (see Handle owncloud migration to latest release #23044 (comment)) The table is kept for backup reasons
  • oc_properties -> droped file id and contents
  • oc_trusted_servers (only comments added

Removing data from database (core/Migrations/Version21000Date20201120141228.php)

Pulled out

@PVince81
Copy link
Member

oc_activity -> app varchar 32 -> 255, add type to activity_filter key

In a fresh install of NC 20 the app column still has 32 characters, same in activity master.
Not sure where that comes from.

@PVince81
Copy link
Member

ok, got it, it's the other way around...

@PVince81
Copy link
Member

regarding bigints I think we should add these to NC (and not convert bigint to int in NC)
the reason is most likely that the number of fileids/entries can become very high on huge instances

not sure if such change would be backported to NC 20 (if DB changes are allowed in minor releases), so might only arrive with NC 21 then ?

@PVince81
Copy link
Member

oc_dav_job_status is for pending asynchronous/lazy operations: https://github.com/owncloud/core/blob/master/apps/dav/lib/DAV/LazyOpsPlugin.php#L65

we can delete it as it shouldn't contain any active entries during a migration

@PVince81
Copy link
Member

PVince81 commented Oct 29, 2020

oc_dav_properties is using fileid to map DAV properties so that they can survive renames

Given that oc_properties only contains the path, DAV properties will likely get lost when renaming through some code paths.
see owncloud/core#25563

Maybe to keep oc_properties we could put a file id into the "propertiespath" column or add a new fileid (or objectid) column ?

Or in theory we could also discard (or keep for later) that table, in case whatever properties exist in there is anyway irrelevant for NC apps. Note that sometimes when Webdav is mounted as a Windows drive it will add a lot of Windows-specific properties. Deleting these shouldn't cause any issue as Windows would re-populate the next time.

@skjnldsv skjnldsv added this to the Nextcloud 21 milestone Oct 31, 2020
@juliusknorr
Copy link
Member Author

Thanks a lot for digging into this already :)

regarding bigints I think we should add these to NC (and not convert bigint to int in NC)

Yes, definitely. At least bigint conversions are fine for minor releases if we let the user do the migration through occ db:convert-filecache-bigint

not sure if such change would be backported to NC 20 (if DB changes are allowed in minor releases), so might only arrive with NC 21 then ?

@rullzer what do you think?

@juliusknorr
Copy link
Member Author

juliusknorr commented Nov 27, 2020

Or in theory we could also discard (or keep for later) that table, in case whatever properties exist in there is anyway irrelevant for NC apps. Note that sometimes when Webdav is mounted as a Windows drive it will add a lot of Windows-specific properties. Deleting these shouldn't cause any issue as Windows would re-populate the next time.

I'd actually say we can go that way. We just recreate the table then with our format and mainly need to migrate oc_dav_properties from that to ours. For that migration I'm not entirely sure if there is any other relevant properties than calendar ones (cc @skjnldsv does store contacts some in there as well) we could just extract the user id from the propertypath, but my DAV internal knowledge is a bit limited in that regard. So any input welcome. Maybe @icewind1991 or @rullzer can also have a look if that makes sense.

For the ownCloud oc_dav_properties migration to oc_properties i would just try to match against /calendar/userid/ and extract the userid for our table format from that.

+----+------------------------+------------------------------------------+---------------+--------------+
| id | propertypath           | propertyname                             | propertyvalue | propertytype |
+----+------------------------+------------------------------------------+---------------+--------------+
|  1 | calendars/admin/sdfsdf | {http://owncloud.org/ns}calendar-enabled | 0             |            1 |
+----+------------------------+------------------------------------------+---------------+--------------+

becomes

+----+--------+--------------------------+------------------------------------------+---------------+
| id | userid | propertypath             | propertyname                             | propertyvalue |
+----+--------+--------------------------+------------------------------------------+---------------+
|  1 | admin  | calendars/admin/personal | {http://owncloud.org/ns}calendar-enabled | 0             |
+----+--------+--------------------------+------------------------------------------+---------------+

The propertytype introduced in owncloud/core#37314 is also most likely irrelevant for us for now.

@nextcloud nextcloud deleted a comment from faily-bot bot Nov 27, 2020
@rullzer
Copy link
Member

rullzer commented Dec 3, 2020

How did you get to that userid? shouldn't it be admin then?

@juliusknorr
Copy link
Member Author

Woops, must have been a copy past error when trying to align copied dumps from the the two instances i had. Yes that would of course be admin.

@juliusknorr juliusknorr force-pushed the migration-10.5 branch 2 times, most recently from d24ec24 to 7520da6 Compare December 7, 2020 14:46
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 7, 2020
@juliusknorr juliusknorr marked this pull request as ready for review December 7, 2020 15:14
@juliusknorr juliusknorr requested review from MorrisJobke, nickvergessen, rullzer and PVince81 and removed request for MorrisJobke and nickvergessen December 7, 2020 15:14
core/Migrations/Version13000Date20170718121200.php Outdated Show resolved Hide resolved
core/Migrations/Version13000Date20170718121200.php Outdated Show resolved Hide resolved
$column->setUnsigned(true);
} else {
$table->addColumn('remember', 'smallint', [
'notnull' => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invalid default

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, there were actually some more columns affected by that in the authtoken table

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
This reverts commit d9b1492.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@nextcloud nextcloud deleted a comment from faily-bot bot Dec 9, 2020
Comment on lines 27 to 34
use OC\BackgroundJob\QueuedJob;
use OCP\Files\IRootFolder;
use OCP\Files\NotFoundException;
use OCP\Files\Storage;
use OCP\IAvatarManager;
use OCP\ILogger;
use OCP\IUser;
use OCP\IUserManager;
use Psr\Log\LoggerInterface;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something is unused

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

as the files are not scanned we cannot use the OCP\Files api

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>

Revert "Make sure the migrations table schema is always checked"

This reverts commit 258955e.

Set current vendor during upgrade and perform migrations table change if needed

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@rullzer rullzer merged commit ccd5ca5 into master Dec 10, 2020
@rullzer rullzer deleted the migration-10.5 branch December 10, 2020 13:32
@rullzer
Copy link
Member

rullzer commented Dec 10, 2020

/backport to stable20

@PVince81
Copy link
Member

seems we missed oc_share_external: #24813


if ($schema->hasTable('systemtag')) {
$table = $schema->getTable('systemtag');
if ($table->hasColumn('systemtag')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong check, this should be "hasColumn('assignable')"

@dossantg
Copy link

Is it possible to backport this PR for 19.0.7? I think the commit (c77e259) for indice on oc_cards can fix #23980

But maybe i am wrong!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants