Add repair step to load missing apps before upgrade #27711

VicDeo · 2017-04-20T18:35:42Z

Description

Before update repair step

Related Issue

owncloud/market#39
#26227

Motivation and Context

Allow seamless app unbundling for the next releases

How Has This Been Tested?

Testing in progress

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81 · 2017-04-20T19:40:59Z

lib/private/Repair/Apps.php

+			if ($isMarketEnabled){
+				$this->loadApp('market');
+				foreach ($missingApps as $app) {
+					$output->info("Repairing missing app: $app");


I wouldn't call it "repairing" because it sounds like it's broken.

Migrating from 9.1 to 10.0 it is an expected step to migrate the app.
Maybe call this "Fetching app from market"

PVince81 · 2017-04-20T19:41:34Z

lib/private/Repair/Apps.php

+						IRepairStep::class . '::repairAppStoreApps',
+						new GenericEvent($app)
+					);
+					$this->appManager->enableApp($app);


Not sure if this is safe, I'd suggest to first fetch all apps and only enable them all at the end ?

Just in case one app in the middle breaks everything...

PVince81 · 2017-04-20T19:42:09Z

Code makes sense so far

PVince81 · 2017-04-20T19:45:08Z

lib/private/Repair/Apps.php

+				foreach ($missingApps as $app) {
+					$output->info("Repairing missing app: $app");
+					$this->eventDispatcher->dispatch(
+						IRepairStep::class . '::repairAppStoreApps',


Hmmm, what if it fails ? Does it abort correctly ?

Also need to display the error to the admin so they can find if it's a connectivity issue or maybe they're unlucky and the app doesn't exist on the market.

In case the app doesn't exist on the market then it's the situation from #27047 where someone deleted the code of a third party app and is trying to upgrade.

VicDeo · 2017-04-20T23:08:18Z

@PVince81 Regarding your question "do you pull the app from the market before the DB update ?"
Works for clean install too:

install 10rc1
Enable user_ldap
Replace with daily
Apply core/market patches
Kill user_ldap
Update
App is alive again

Detecting an exact error requires additional exception subclassing in the market app

PVince81 · 2017-04-21T06:19:43Z

lib/private/Repair/Apps.php

+					IRepairStep::class . '::repairAppStoreApps',
+					new GenericEvent($app)
+				);
+				$this->appManager->enableApp($app);


I'm not sure whether it's the right time to enable the app here. From what I remember the update process has a specific moment where it enables all apps, maybe as part of upgrading apps, not sure. Can you verify that this fits in the workflow ? Ideal is to have the exact same workflow as before and not enable apps too early or too late which has a risk of influencing / modifying update behavior.

@PVince81 Update tries to enable only apps that were disabled by update (3rd party or incompatible)
I haven't managed to find the reason but the app code is restored as disabled otherwise.

VicDeo · 2017-04-21T20:12:54Z

PVince81 · 2017-04-24T06:50:07Z

PVince81 · 2017-04-24T06:54:18Z

add specific exception for marketplace connection not available ?

PVince81 · 2017-04-24T06:56:44Z

lib/private/Repair/Apps.php

+
+			throw new RepairException('Upgrade is not possible');
+		} elseif ($hasNotUpdatedCompatibleApps) {
+			// warn?


I'd say yes, warn. Then the admin should be able to upgrade these apps inside the market app later on (or update these manually in case of offline mode).

PVince81 · 2017-04-24T07:07:13Z

log info message in console when processing apps (especially if market is slow, so admin knows what the update process is currently doing)

PVince81 · 2017-04-24T07:20:13Z

BUG: update with outdated app that has no new version must fail

My steps:

occ market:install contacts
occ app:enable contacts
Edit contacts app and make it incompatible by changing max-version
Increase version in version.php
occ app upgrade

Expected result:

Update aborts.

Actual result:

Update goes through.

@VicDeo not sure if my testing is wrong.
The market does not provide a new version since I left the app version unchanged.

Edit: I tested this wrong. Need to set version.php version to 10.1 for better results.

PVince81 · 2017-04-24T07:51:05Z

lib/private/Repair/Apps.php

+				);
+
+				foreach ($appsToEnable as $app){
+					$this->appManager->enableApp($app);


I suggest we don't enable anything here. Why ? Because these apps were already enabled before, else we wouldn't do any updating of them.

I think this loop is anyway a no-op, it will see the app is already enabled and skip. So better remove it.

@PVince81 in case of missing code app is going into a disabled state after the code is restored. I haven't found the reason, this is a fixup.

Can you add a code comment then to prevent future devs to wonder about the same question ?

PVince81 · 2017-04-24T07:54:34Z

Looks good so far apart from the one bug.

I'll continue testing with the planned user_ldap case:

user_ldap 10.0 case

Setup LDAP in 9.1 then upgrade to 10.0:

TEST: with user_ldap app missing: abort
TEST: admin installs beta user_ldap manually: upgrade goes through and finds users

PVince81 · 2017-04-24T08:11:47Z

I did all tests with occ upgrade. Will check how the web UI behaves.
For now showing "sorry, need to disable app" is enough.

butonic

I think the logic can be improved to prevent the output not reflecting that apps have been updated.

butonic · 2017-04-24T08:05:31Z

lib/private/Repair/Apps.php

+					'reinstallAppStoreApp'
+				);
+				$hasNotUpdatedCompatibleApps = count($failedCompatibleApps);
+


Hm, so the current implementation will try to enable apps that have been updated.

core/lib/private/App/AppManager.php

Line 218 in 2d000d9

throw new \Exception("$appId can't be enabled since it is not installed.");

currently might throw an \Exception, casing a full stop of this process, and then the rest of this function will not be executed.

I think it should throw an AppNotInstalledException instead.

That still allows the foreach to be escaped if the exception is thrown. I'd rather move 98-105 out of the try catch. If we try to enable an app that wasn't installed in the first place something has gone wrong and I think it is ok to fail early in that case.

For clarification: if an app's code is missing but the DB contains an entry about an enabled app, do we call this "app is installed but missing code" in the logic ? @VicDeo

From my understanding this code only runs for apps that either are pending updates or where the code has disappeared (so it was installed at some point, like our future "user_ldap" case).

@PVince81 this PR works with enabled apps only
there are three kinds of them:

enabled, but no code

enabled, but incompatible

enabled and compatible

these lines (reenabling the apps) can be safely removed once #27730 is merged.

butonic · 2017-04-24T08:18:08Z

lib/private/Repair/Apps.php

+		}
+
+		// Do not put this in else
+		if (!$isMarketEnabled) {


The current code will overwite the $failedIncompatibleApps if 90 throws an AppManagerException exception. Incompatible apps might actually have been updated. If we move the initialization to the top this should work correctly. How about initializing $hasNotUpdatedCompatibleApps, $failedMissingApps and $failedIncompatibleApps in 73-75 instead of checking $isMarketEnabled here. Then you can introduce a new function that does all the error handling in 120-134 and call it from the catch as well.

@butonic This catch means that marketplace is not available. Code starting from this line is executed both for disabled market app and when market app had any issue with connection to marketplace

yup, and $this->getAppsFromMarket() for failedIncompatibleApps and failedMissingApps might have succeeded ... than internet dies and $this->getAppsFromMarket() for failedCompatibleApps throws the Exception ... this would overwrite the failedIncompatibleApps and failedMissingApps arrays, with the initial value. Thats wrong, isn't it?

PVince81 · 2017-04-24T08:30:24Z

TEST: web UI also aborts properly

PVince81 · 2017-04-24T08:31:16Z

@VicDeo please fix the bugs and address/clarify @butonic's concerns.

Other than that I think we should merge this. Further polishing (better error messages) to be done separately.

PVince81 · 2017-04-24T08:51:19Z

BUG: web UI still says "These incompatible apps will be disabled" even though we don't. Need to remove this message.

VicDeo · 2017-04-24T09:04:43Z

@PVince81 regaring your testcase #27711 (comment)
the app is being pulled from market and becomes compatible (appinfo.xml is updated)

But it is going to disabled state just as an app with missing code. :(
I'll check whether any app pulled from market needs enabling even if it was enabled before.

VicDeo · 2017-04-24T09:12:21Z

@PVince81 regrading output #27711 (comment)

I'm limited with info and warn at the moment. And warn is not appropriate while info is not shown with a default verbocity level:

php occ upgrade -v
...
2017-04-24T09:09:26+00:00 Repair info: removing duplicate entries from lucene_status
2017-04-24T09:09:26+00:00 Repair step: Put back missing apps
2017-04-24T09:09:26+00:00 Repair info: Fetching app from market: admin_audit
2017-04-24T09:09:26+00:00 Repair info: Fetching app from market: comments
2017-04-24T09:09:26+00:00 Repair info: Fetching app from market: contacts
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: dav
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: enterprise_key
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: federatedfilesharing
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: federation
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: files
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: files_antivirus
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: files_external
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: files_sharing
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: files_texteditor
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: files_trashbin
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: files_versions
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: gallery
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: market
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: provisioning_api
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: systemtags
2017-04-24T09:09:27+00:00 Repair info: Fetching app from market: testing
2017-04-24T09:09:28+00:00 Repair info: Fetching app from market: updatenotification
2017-04-24T09:09:28+00:00 Repair info: Fetching app from market: user_ldap
2017-04-24T09:09:28+00:00 Repair info: Fetching app from market: user_persona
2017-04-24T09:09:28+00:00 Updating database schema
2017-04-24T09:09:28+00:00 Updated database

PVince81 · 2017-04-24T09:20:45Z

the app is being pulled from market and becomes compatible

If the version from the market is the same it shouldn't try and pull it.

PVince81 · 2017-04-24T11:11:33Z

Rebased to get the "don't disable third party app" improvement.

Talked with @VicDeo and he said there are only cosmetic changes left to do.

So I'm going to redo the test plan and hope all goes well now.

PVince81 · 2017-04-24T11:21:32Z

@VicDeo I removed the obsolete message here: e4cee4d

PVince81 · 2017-04-24T11:27:40Z

PVince81 · 2017-04-24T11:29:23Z

BUG: only fail once (I pointed the market URL to localhost)

Repair warning: GuzzleHttp\Exception\ConnectException
Repair warning: cURL error 7: Failed to connect to marketplace.int.owncloud.com port 443: Connection refused
Repair warning: GuzzleHttp\Exception\ConnectException
Repair warning: cURL error 7: Failed to connect to marketplace.int.owncloud.com port 443: Connection refused

To be raised separately.

PVince81 · 2017-04-24T11:36:03Z

BUG: ⚠️ occ upgrade for apps should not try updating apps

Basically when only a single app has a new version in info.xml, one needs to run occ upgrade to proceed with the DB upgrade. When this happens we must not attempt to update the app codes.

Need to add a check for "is core upgrade needed", if yes then also update app codes
If only apps need DB update, proceed without market.

Edit: fixed in 446cb08

PVince81 · 2017-04-24T11:52:22Z

@VicDeo #27711 (comment) still doesn't work correctly.

It seems it decides to disable the outdated app automatically and moves forward. This is very bad because it could disable an important app. This is the case where the app exists in app store but the platform was updated to a more recent incompatible version.

You need to only accept the market version if it is strictly bigger than the current version from info.xml.
However if the app code is missing, it is fine to take the same version.

PVince81 · 2017-04-24T12:13:34Z

BUG: Is it loading the same apps list repeatedly ???

Repair warning: Client error response [url] localhost/api/v1/platform/10.0.0/apps.json [status code] 404 [reason phrase] Not Found
Repair warning: GuzzleHttp\Exception\ClientException
Repair warning: Client error response [url] localhost/api/v1/platform/10.0.0/apps.json [status code] 404 [reason phrase] Not Found
Repair warning: GuzzleHttp\Exception\ClientException
Repair warning: Client error response [url] localhost/api/v1/platform/10.0.0/apps.json [status code] 404 [reason phrase] Not Found
Repair warning: GuzzleHttp\Exception\ClientException
Repair warning: Client error response [url] localhost/api/v1/platform/10.0.0/apps.json [status code] 404 [reason phrase] Not Found
Repair warning: GuzzleHttp\Exception\ClientException
Repair warning: Client error response [url] localhost/api/v1/platform/10.0.0/apps.json [status code] 404 [reason phrase] Not Found
Repair warning: GuzzleHttp\Exception\ClientException
Repair warning: Client error response [url] localhost/api/v1/platform/10.0.0/apps.json [status code] 404 [reason phrase] Not Found
Repair warning: GuzzleHttp\Exception\ClientException
Repair warning: Client error response [url] localhost/api/v1/platform/10.0.0/apps.json [status code] 404 [reason phrase] Not Found
Repair warning: GuzzleHttp\Exception\ClientException
Repair warning: Client error response [url] localhost/api/v1/platform/10.0.0/apps.json [status code] 404 [reason phrase] Not Found

If doing an app-only upgrade then it's not about updating the code but about updating the database. Still keep the logic about missing app code and incompatible apps.

PVince81 · 2017-04-24T12:29:45Z

@VicDeo ok you were right. The fact that the old app got reinstalled even if it had the same version was a side-effect of my testing. Now when testing this case I increase the core version to "10.1" and then it refetches the market app and sees that it's incompatible.

The sad part is that it refetches the app needlessly, but we can live with this for now and optimize this separately.

PVince81 · 2017-04-24T12:36:32Z

BUG: something is still disabling the "contacts" app after an upgrade and even after the code has been replaced.

It is likely that the app versions are cached somewhere and used by the old code that disabled incompatible apps.

Edit: fix is here f3607c0

PVince81 · 2017-04-24T12:37:30Z

To raise: add OCC command "market:upgrade" to upgrade all apps like we did here
To raise: argument to force disabling apps occ upgrade --disable-incompatible instead of aborting

PVince81 · 2017-04-24T12:47:09Z

Fixed issue with incompatible app getting autodisabled even after updating to a compatible one: f3607c0
(hint: caching...)

PVince81 · 2017-04-24T12:59:35Z

I fixed the repeated Guzzle issues by properly catching ClientException here: owncloud/market@ad482cf

PVince81 · 2017-04-24T13:00:19Z

@VicDeo @butonic can you guys check my latest code changes ? Thanks

VicDeo · 2017-04-24T15:38:04Z

@PVince81 Additionally fixed: update to the same version.
It was even capable to downgrade the app so I added an additional check into the listener.

lock · 2019-08-03T12:02:21Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

VicDeo added the 2 - Developing label Apr 20, 2017

VicDeo added this to the 10.0 milestone Apr 20, 2017

PVince81 reviewed Apr 20, 2017

View reviewed changes

PVince81 mentioned this pull request Apr 20, 2017

Listen to repair app event owncloud/market#33

Merged

PVince81 reviewed Apr 20, 2017

View reviewed changes

PVince81 reviewed Apr 21, 2017

View reviewed changes

PVince81 mentioned this pull request Apr 21, 2017

Adjust update process to work with market owncloud/market#39

Closed

VicDeo force-pushed the repair-apps branch from d32d8bd to 71eb28e Compare April 21, 2017 16:29

VicDeo force-pushed the repair-apps branch from ff85b1c to 1e6b4f5 Compare April 21, 2017 21:52

PVince81 mentioned this pull request Apr 24, 2017

Revert "Dispatch upgradeAppStoreApps event" #27729

Merged

PVince81 reviewed Apr 24, 2017

View reviewed changes

butonic requested changes Apr 24, 2017

View reviewed changes

VicDeo force-pushed the repair-apps branch from 9ad67d0 to 0928868 Compare April 24, 2017 09:17

VicDeo added 2 commits April 24, 2017 13:10

Remove enabling workaround

30b4438

Put compatible apps at the very end

33ef309

PVince81 force-pushed the repair-apps branch from e514c1b to 33ef309 Compare April 24, 2017 11:11

Remove pre-update message about disabled incompatible apps

e4cee4d

PVince81 mentioned this pull request Apr 24, 2017

Check for missing app code before upgrade #27047

Closed

9 tasks

Only update market apps when doing a core upgrade

446cb08

If doing an app-only upgrade then it's not about updating the code but about updating the database. Still keep the logic about missing app code and incompatible apps.

Clear $appInfo cache after updating app

f3607c0

Vincent Petry and others added 2 commits April 24, 2017 15:36

Output reason when marketplace not accessible during upgrade

631ddce

Catch AppUpdateNotFoundException

5f307c0

VicDeo added 3 - To Review and removed 2 - Developing labels Apr 24, 2017

PVince81 merged commit 4ea017a into master Apr 24, 2017

PVince81 deleted the repair-apps branch April 24, 2017 18:42

VicDeo mentioned this pull request Oct 13, 2017

[Stable10] revert 27648 update from market #29249

Merged

6 tasks

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

Add repair step to load missing apps before upgrade #27711

Add repair step to load missing apps before upgrade #27711

Conversation

VicDeo commented Apr 20, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Apr 20, 2017

Choose a reason for hiding this comment

VicDeo commented Apr 20, 2017

Choose a reason for hiding this comment

VicDeo Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

VicDeo commented Apr 21, 2017 • edited by PVince81 Loading

PVince81 commented Apr 24, 2017 • edited by davitol Loading

Marketplace unavailable / offline

Marketplace available but app does not exist there

Marketplace available, app exists but no new version

Marketplace available, app exists and new version available

Other

PVince81 commented Apr 24, 2017

Choose a reason for hiding this comment

PVince81 commented Apr 24, 2017 • edited Loading

PVince81 commented Apr 24, 2017 • edited Loading

My steps:

Expected result:

Actual result:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Apr 24, 2017 • edited Loading

user_ldap 10.0 case

PVince81 commented Apr 24, 2017

butonic left a comment

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

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

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017 • edited Loading

VicDeo commented Apr 24, 2017 • edited Loading

VicDeo commented Apr 24, 2017

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017 • edited by davitol Loading

Marketplace unavailable / offline

Marketplace available but app does not exist there

Marketplace available, app exists but no new version

Marketplace available, app exists and new version available

PVince81 commented Apr 24, 2017 • edited Loading

PVince81 commented Apr 24, 2017 • edited Loading

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017 • edited Loading

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017 • edited Loading

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017

PVince81 commented Apr 24, 2017

VicDeo commented Apr 24, 2017

lock bot commented Aug 3, 2019

VicDeo commented Apr 20, 2017 •

edited

Loading

VicDeo Apr 21, 2017 •

edited

Loading

VicDeo commented Apr 21, 2017 •

edited by PVince81

Loading

PVince81 commented Apr 24, 2017 •

edited by davitol

Loading

PVince81 commented Apr 24, 2017 •

edited

Loading

PVince81 commented Apr 24, 2017 •

edited

Loading

PVince81 commented Apr 24, 2017 •

edited

Loading

PVince81 commented Apr 24, 2017 •

edited

Loading

VicDeo commented Apr 24, 2017 •

edited

Loading

PVince81 commented Apr 24, 2017 •

edited by davitol

Loading

PVince81 commented Apr 24, 2017 •

edited

Loading

PVince81 commented Apr 24, 2017 •

edited

Loading

PVince81 commented Apr 24, 2017 •

edited

Loading

PVince81 commented Apr 24, 2017 •

edited

Loading