Don't do app upgrades in the background #9866

icewind1991 · 2014-07-24T15:56:08Z

This removes the logic for transparently upgrading apps and moves it to the same system we use for core upgrades.
Transparent app upgrades can leave the instance in a broken state when requests are being processed in parallel.

Only doing the upgrades in the foreground prevents this and provide a way for users to know when an upgrade failed.

cc @DeepDiver1975 @PVince81

PVince81 · 2014-07-24T15:58:01Z

lib/private/app.php

 	 */
-	public static function loadApp($app) {
+	public static function loadApp($app, $checkUpgrade = true) {


Should we default this to false instead or do you think there's a risk to break other things ?

imo defaulting to false provides a bigger risk (an accidental app being loaded without properly being upgraded)

PVince81 · 2014-07-24T16:12:05Z

To test:

Main update

Regular upgrade
CLI upgrade (:warning: only works with Do not load apps in console.php when an upgrade is due #9861 applied)
Upgrade with broken app (throw exception in update.php)
CLI upgrade with broken app
upgrade with unsupported app (must disable itself)

Single app update

Single app update: blocks all connections until accessed through web UI
Single broken app update: blocks connections, stays blocked after upgrade

PVince81 · 2014-07-24T16:26:57Z

Ok from what I see, whenever an app needs upgrade, the "Upgrade to OC 8" page appears again.
We might want to change the text later to make sure people know it's just about updating apps.
CC @owncloud/designers

PVince81 · 2014-07-24T16:27:36Z

Bonus point for the fact that you're now showing what app is being updated 😄

PVince81 · 2014-07-24T16:37:24Z

test with third party apps that need update
test what happens when updating app from app store

PVince81 · 2014-07-24T16:38:17Z

@georgehrke do you see any conflict with this and the app store approach of updating ?
Short version: this PR will display the update page as soon as there is at least one app that need updating (when code_version > db_version)

PVince81 · 2014-07-24T16:41:45Z

TODO: move the core version setting to the end

PVince81 · 2014-07-24T17:04:56Z

Just tested the following:

Install mozilla_sync (with the code checker off...)
Set version to 1.3 in appinfo/version
Set version to 1.3 in the database.
Update mozilla_sync from the apps page
Refresh the page

At step 4) it looks like only the code is updated, but the actual DB upgrade occurs after a page refresh, where the "upgrade to OC8" page appears.
This also means that as soon as an app is updated, no further ajax calls will be allowed.
So either the app store update should be changed automatically refresh the page.

We need to make sure that this worklow is acceptable, or tweak it.

PVince81 · 2014-07-24T18:24:27Z

properly integrate with app store: maybe make app store refresh the page, but not sure about the UX, needs to be discussed

Also I'm not sure about the impact of this.
Would be definitely nice to sort out in OC 8 but not sure about 7.0.1.

PVince81 · 2014-07-24T18:24:46Z

Apart from the the code looks good 😄
Thanks @icewind1991

jancborchardt · 2014-07-24T18:47:58Z

Hm, does that mean that whenever an app needs a upgrade, the blocking page will be shown? Should only be invokable by admins, no?

georgehrke · 2014-07-24T18:51:22Z

code looks good, don't see any direct conflicts with app updates for shipped apps, didn't test though. Will try to find time for testing this tomorrow

PVince81 · 2014-07-24T19:18:57Z

@jancborchardt, yes only admins. Other users would get "Service not available" or something until the admin runs the upgrade procedure.
I like that approach for the core update but think it's a bit cumbersome for individual apps.

jancborchardt · 2014-07-24T23:22:25Z

@PVince81 wait – but that »Service not available« page in the first place only appears once the admin dedicatedly pushes the »update app« switch, right? Otherwise it sounds like app updates will halt the installation until an admin is there.

MorrisJobke · 2014-07-25T07:15:38Z

@jancborchardt App updates are invoked by admins ;) There is no auto-updater

schiessle · 2014-07-25T08:08:59Z

I'm not sure if I like the idea of the update screen for every app update. This can be quite irritating especially
if a different user visits the side exactly at this moment. What happens if multiple apps needs to be updated, will I get a separate update screen for each app?

Why not just activate the maintenance mode before we call the update script? This way we make sure that the script gets triggered only once and we can still perform the updates in the background on-demand.

icewind1991 · 2014-07-25T10:53:06Z

There are 2 cases for upgrading apps

an app is updated from the web ui, in that case we could enable maintance mode in ajax and perform the upgrade transparently
an app is updated by the admin replacing the code, that case is when we need to show to upgrade page

I'll look into "fixing" updating apps from the app store

schiessle · 2014-07-25T11:31:59Z

an app is updated by the admin replacing the code, that case is
when we need to show to upgrade page

can you elaborate on it? I don't know the code, so maybe I miss
something obvious, but why can't we just switch to maintenance mode if
we detect a version change right before calling the update.php?

icewind1991 · 2014-07-25T11:45:07Z

The app store now properly set the app version in the database after updating, when updating an app trough the app store you no longer need to go trough the upgrade page

icewind1991 · 2014-07-25T11:48:12Z

can you elaborate on it? I don't know the code, so maybe I miss
something obvious, but why can't we just switch to maintenance mode if
we detect a version change right before calling the update.php?

The problem is that the upgrade would be triggered by whatever request happens to be next, neither we nor the admin updating the app has any control when the upgrade would be triggered. By going trough the update page we not only ensure parallel requests dont try to both update the app but also ensure there is someone to respond to any error that might happen

ghost · 2014-07-25T12:56:17Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6406/

schiessle · 2014-08-01T10:01:29Z

What's the status? Isn't it something we would like to have for OC7.0.1?

icewind1991 · 2014-08-04T11:46:07Z

Rebased, I think all raised issues are taken care of now

ghost · 2014-08-04T12:48:04Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6526/

PVince81 · 2014-08-12T10:38:13Z

lib/private/updater.php

+		foreach ($apps as $appId) {
+			if (\OC_App::shouldUpgrade($appId)) {
+				\OC_App::updateApp($appId);
+				$this->emit('\OC\Updater', 'appUpgrade', array($appId, $version));


Undefined variable $version

PVince81 · 2014-08-12T10:54:08Z

I retested with the steps from #9866 (comment) and everything works fine now.

Upgrading a single third party app by clicking on "Update now" simply sets the maintenance mode during upgrade, which provides a better UX than before.

However there is one last small issue:

BUG: app version does not appear in upgrade status message due to "Undefined variable $version" (see inline comment)

@icewind1991 please fix that then this is good to go.

PVince81 · 2014-08-12T10:57:22Z

Fixes #9856
Fixes #9817

PVince81 · 2014-08-13T16:35:09Z

@karlitschek @craigpg I have the feeling this would be useful to have in 7.0.2.
People have been reporting concurrency issues in 7.0.1 even with the patches that fixed them.

@schiesbn please also help reviewing

PVince81 · 2014-08-18T07:52:18Z

@icewind1991 any update ?

icewind1991 · 2014-08-18T13:31:21Z

Fixed the undefined variable

scrutinizer-notifier · 2014-08-18T13:31:43Z

A new inspection was created.

PVince81 · 2014-08-18T13:41:22Z

Works 👍

PVince81 · 2014-08-18T13:41:47Z

This needs another reviewer @schiesbn @DeepDiver1975 @th3fallen

ghost · 2014-08-18T14:58:20Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6768/

th3fallen · 2014-08-18T15:00:31Z

Works for me as well 👍 some unit tests wouldn't hurt though....

PVince81 · 2014-08-19T08:14:10Z

@icewind1991 add unit tests separately ? From what I remember it's quite difficult with the current structure and might require some refactoring...

icewind1991 · 2014-08-19T11:33:35Z

@icewind1991 add unit tests separately ? From what I remember it's quite difficult with the current structure and might require some refactoring...

Yes, I'm not sure how to add unit tests atm

Don't do app upgrades in the background

MorrisJobke · 2014-08-19T11:36:11Z

@icewind1991 @karlitschek Backport needed and possible?

karlitschek · 2014-08-19T12:01:52Z

please backport

icewind1991 · 2014-08-19T12:02:41Z

Backport would be useful to increase upgrade stability

icewind1991 · 2014-08-19T12:08:15Z

Stable7: c69215c...7842014

icewind1991 mentioned this pull request Jul 24, 2014

Do not load apps in console.php when an upgrade is due #9861

Merged

PVince81 reviewed Jul 24, 2014
View reviewed changes

PVince81 mentioned this pull request Jul 24, 2014

Prevent apps upgrade through cron or remote calls #9817

Closed

PVince81 mentioned this pull request Jul 24, 2014

Set version AFTER a successful update #9819

Merged

icewind1991 added 4 commits August 4, 2014 13:41

Send feedback from upgrading apps to the upgrade ui

3184983

Also set the app version when updating from app store

ae263da

Set maintaince mode when updating an app from the app store

5c1f572

only set core version at the end

8b4d3c0

MorrisJobke added the 5 - To review label Aug 5, 2014

PVince81 mentioned this pull request Aug 12, 2014

Apps update in loadApps issue for concurrent requests #9856

Closed

PVince81 reviewed Aug 12, 2014
View reviewed changes

fix undefined variable

ab590ce

icewind1991 added a commit that referenced this pull request Aug 19, 2014

Merge pull request #9866 from owncloud/app-upgrade

fdfc5c6

Don't do app upgrades in the background

icewind1991 merged commit fdfc5c6 into master Aug 19, 2014

icewind1991 deleted the app-upgrade branch August 19, 2014 11:33

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

Don't do app upgrades in the background #9866

Don't do app upgrades in the background #9866

Conversation

icewind1991 commented Jul 24, 2014

PVince81 Jul 24, 2014

Choose a reason for hiding this comment

icewind1991 Jul 24, 2014

Choose a reason for hiding this comment

PVince81 commented Jul 24, 2014

Main update

Single app update

PVince81 commented Jul 24, 2014

PVince81 commented Jul 24, 2014

PVince81 commented Jul 24, 2014

PVince81 commented Jul 24, 2014

PVince81 commented Jul 24, 2014

PVince81 commented Jul 24, 2014

PVince81 commented Jul 24, 2014

PVince81 commented Jul 24, 2014

jancborchardt commented Jul 24, 2014

georgehrke commented Jul 24, 2014

PVince81 commented Jul 24, 2014

jancborchardt commented Jul 24, 2014

MorrisJobke commented Jul 25, 2014

schiessle commented Jul 25, 2014

icewind1991 commented Jul 25, 2014

schiessle commented Jul 25, 2014

icewind1991 commented Jul 25, 2014

icewind1991 commented Jul 25, 2014

ghost commented Jul 25, 2014

schiessle commented Aug 1, 2014

icewind1991 commented Aug 4, 2014

ghost commented Aug 4, 2014

PVince81 Aug 12, 2014

Choose a reason for hiding this comment

PVince81 commented Aug 12, 2014

PVince81 commented Aug 12, 2014

PVince81 commented Aug 13, 2014

PVince81 commented Aug 18, 2014

icewind1991 commented Aug 18, 2014

scrutinizer-notifier commented Aug 18, 2014

PVince81 commented Aug 18, 2014

PVince81 commented Aug 18, 2014

ghost commented Aug 18, 2014

th3fallen commented Aug 18, 2014

PVince81 commented Aug 19, 2014

icewind1991 commented Aug 19, 2014

MorrisJobke commented Aug 19, 2014

karlitschek commented Aug 19, 2014

icewind1991 commented Aug 19, 2014

icewind1991 commented Aug 19, 2014