Wait for cron to finish before running upgrade command #9900

MorrisJobke · 2018-06-18T15:27:22Z

fixes Check before running "upgrade" if cronjob is still running and wait for it to finish #9562

To test this, add this:

diff --git a/lib/private/Authentication/Token/DefaultTokenProvider.php b/lib/private/Authentication/Token/DefaultTokenProvider.php
index 7a43dbb23e..fb2b33f564 100644
--- a/lib/private/Authentication/Token/DefaultTokenProvider.php
+++ b/lib/private/Authentication/Token/DefaultTokenProvider.php
@@ -281,9 +281,11 @@ class DefaultTokenProvider implements IProvider {
 	public function invalidateOldTokens() {
 		$olderThan = $this->time->getTime() - (int) $this->config->getSystemValue('session_lifetime', 60 * 60 * 24);
 		$this->logger->debug('Invalidating session tokens older than ' . date('c', $olderThan), ['app' => 'cron']);
+		sleep(15);
 		$this->mapper->invalidateOld($olderThan, IToken::DO_NOT_REMEMBER);
 		$rememberThreshold = $this->time->getTime() - (int) $this->config->getSystemValue('remember_login_cookie_lifetime', 60 * 60 * 24 * 15);
 		$this->logger->debug('Invalidating remembered session tokens older than ' . date('c', $rememberThreshold), ['app' => 'cron']);
+		sleep(15);
 		$this->mapper->invalidateOld($rememberThreshold, IToken::REMEMBER);
 	}

Then start a cron job cron.php, raise the version number in version.php or lower it in config/config.php and then trigger the upgrade by calling occ upgrade.

It then looks like this:

$ occ upgrade
Nextcloud or one of the apps require upgrade - only a limited number of commands are available
You may use your browser or the occ upgrade command to do the upgrade
Set log level to debug
Turned on maintenance mode
Waiting for cron to finish (checks again in 5 seconds)...
Waiting for cron to finish (checks again in 5 seconds)...
Waiting for cron to finish (checks again in 5 seconds)...
Waiting for cron to finish (checks again in 5 seconds)...
Updating database schema
Updated database
Checking for update of app activity in appstore
Checked for update of app "activity" in appstore 
Checking for update of app bruteforcesettings in appstore
Checked for update of app "bruteforcesettings" in appstore 
Checking for update of app comments in appstore
Checked for update of app "comments" in appstore 
...

When no cron job runs, then it looks like this:

$ occ upgrade
Nextcloud or one of the apps require upgrade - only a limited number of commands are available
You may use your browser or the occ upgrade command to do the upgrade
Set log level to debug
Turned on maintenance mode
Updating database schema
Updated database
Checking for update of app activity in appstore
Checked for update of app "activity" in appstore 
Checking for update of app bruteforcesettings in appstore
Checked for update of app "bruteforcesettings" in appstore 
Checking for update of app comments in appstore
Checked for update of app "comments" in appstore 
...

MorrisJobke · 2018-06-18T15:28:58Z

The web UI looks like this:

MorrisJobke · 2018-06-18T15:30:27Z

I'm tempted to backport this, because we think that this could fix the issue #4978

codecov · 2018-06-18T15:57:51Z

Codecov Report

Merging #9900 into master will decrease coverage by 0.16%.
The diff coverage is 3.33%.

@@             Coverage Diff             @@
##             master   #9900      +/-   ##
===========================================
- Coverage     52.07%   51.9%   -0.17%     
+ Complexity    25901   25813      -88     
===========================================
  Files          1642    1637       -5     
  Lines         95884   95539     -345     
  Branches       1318    1318              
===========================================
- Hits          49929   49593     -336     
+ Misses        45955   45946       -9

Impacted Files	Coverage Δ	Complexity Δ
lib/private/BackgroundJob/JobList.php	`72.84% <0%> (-6.29%)`	`31 <2> (+2)`
core/Command/Upgrade.php	`0% <0%> (ø)`	`68 <1> (+1)`	⬆️
core/ajax/update.php	`0% <0%> (ø)`	`11 <0> (ø)`	⬇️
lib/private/Updater.php	`5.31% <11.11%> (+0.2%)`	`120 <2> (+2)`	⬆️
lib/private/Authentication/Token/DefaultToken.php	`85.24% <0%> (-6.69%)`	`20% <0%> (ø)`
...B/QueryBuilder/FunctionBuilder/FunctionBuilder.php	`77.77% <0%> (-2.23%)`	`9% <0%> (-1%)`
lib/private/User/Database.php	`79.47% <0%> (-1.78%)`	`43% <0%> (+1%)`
apps/files_trashbin/lib/Expiration.php	`90.32% <0%> (-1.62%)`	`29% <0%> (ø)`
settings/Controller/AuthSettingsController.php	`77.5% <0%> (-0.88%)`	`20% <0%> (+2%)`
...vate/Authentication/Token/DefaultTokenProvider.php	`98.03% <0%> (-0.02%)`	`32% <0%> (ø)`
... and 9 more

skjnldsv

Makes a lot of sense!

blizzz · 2018-06-19T08:32:25Z

lib/private/BackgroundJob/JobList.php

+		$query = $this->connection->getQueryBuilder();
+		$query->select('*')
+			->from('jobs')
+			->where($query->expr()->gt('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - 12 * 3600, IQueryBuilder::PARAM_INT)))


Let's document why this is supposed to be working. The code is clear, but the thought behinds should be expressed.

Strictly speaking, it does only report whether a job should run (it might be running, or it might be due with the next job run). Correct me if I am wrong. It could perhaps lead to an infinite loop when a job reincarnates and registers to run next time again. Though, cron should not be triggered when maintenance mode is enabled, which in the whole scenario is the case, so it would block for 12 hours instead.

Are the 12 hours based on something?

The 12 hours is the same timeout that is also used to re-schedule an non-terminated job (see a bit above in this file - getNextJob(). The idea here is: give a job enough time to run very long and then after a timeout re-schedule it nevertheless. It's more likely to be crashed than to run that long.

In theory it could lead to an endless loop (as in - at most 12 hours), because the cron will not start when maintenance mode is active and this method is only executed in maintenance mode (see where it is added in the upgrader class. I could add this as comments of course.

Comment added.

From a UX perspective it still is problematic if it will take that long, as it'll look that nothing is happening. Of course stating "it might take up to 12 hours" in the GUI would scare people away. OK, let's get it in and collect some experience.

* fixes #9562 Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke · 2018-06-26T14:36:42Z

This will only trigger for 14.0.0.7+, because it would cause #9992 otherwise. See #10010

MorrisJobke · 2018-07-09T13:19:13Z

Let's not backport this.

MorrisJobke · 2018-11-01T11:53:52Z

Reverted in #12188

MorrisJobke added 3. to review Waiting for reviews feature: install and update labels Jun 18, 2018

MorrisJobke added this to the Nextcloud 14 milestone Jun 18, 2018

MorrisJobke requested review from rullzer, nickvergessen, ChristophWurst, juliusknorr and skjnldsv June 18, 2018 15:27

MorrisJobke added the backport-request label Jun 18, 2018

This was referenced Jun 18, 2018

/data/files_external/rootcerts.crt empty after update #4978

Closed

Check before running "upgrade" if cronjob is still running and wait for it to finish #9562

Closed

skjnldsv approved these changes Jun 18, 2018

View reviewed changes

blizzz reviewed Jun 19, 2018

View reviewed changes

Wait for cron to finish before running upgrade command

18e9631

* fixes #9562 Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke force-pushed the feature/noid/wait-for-cron-to-finish branch from b0c929d to 18e9631 Compare June 19, 2018 12:22

blizzz approved these changes Jun 20, 2018

View reviewed changes

MorrisJobke merged commit 50df29d into master Jun 20, 2018

MorrisJobke deleted the feature/noid/wait-for-cron-to-finish branch June 20, 2018 12:14

MorrisJobke mentioned this pull request Jun 25, 2018

Cronjob blocking update (forever) #9992

Closed

MorrisJobke removed the backport-request label Jul 9, 2018

MorrisJobke mentioned this pull request Nov 1, 2018

Revert "Wait for cron to finish before running upgrade command" #12188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for cron to finish before running upgrade command #9900

Wait for cron to finish before running upgrade command #9900

MorrisJobke commented Jun 18, 2018

MorrisJobke commented Jun 18, 2018

MorrisJobke commented Jun 18, 2018

codecov bot commented Jun 18, 2018 •

edited

Loading

skjnldsv left a comment

blizzz Jun 19, 2018

MorrisJobke Jun 19, 2018

MorrisJobke Jun 19, 2018

blizzz Jun 20, 2018

MorrisJobke commented Jun 26, 2018

MorrisJobke commented Jul 9, 2018

MorrisJobke commented Nov 1, 2018

Wait for cron to finish before running upgrade command #9900

Wait for cron to finish before running upgrade command #9900

Conversation

MorrisJobke commented Jun 18, 2018

MorrisJobke commented Jun 18, 2018

MorrisJobke commented Jun 18, 2018

codecov bot commented Jun 18, 2018 • edited Loading

Codecov Report

skjnldsv left a comment

Choose a reason for hiding this comment

blizzz Jun 19, 2018

Choose a reason for hiding this comment

MorrisJobke Jun 19, 2018

Choose a reason for hiding this comment

MorrisJobke Jun 19, 2018

Choose a reason for hiding this comment

blizzz Jun 20, 2018

Choose a reason for hiding this comment

MorrisJobke commented Jun 26, 2018

MorrisJobke commented Jul 9, 2018

MorrisJobke commented Nov 1, 2018

codecov bot commented Jun 18, 2018 •

edited

Loading