From 18e9631810ad1d3d72c2b4bbee330169808108ad Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Mon, 18 Jun 2018 17:23:54 +0200 Subject: [PATCH] Wait for cron to finish before running upgrade command * fixes #9562 Signed-off-by: Morris Jobke --- core/Command/Upgrade.php | 6 +++- core/ajax/update.php | 6 +++- lib/private/BackgroundJob/JobList.php | 40 ++++++++++++++++++++++++++- lib/private/Updater.php | 24 ++++++++++------ tests/lib/UpdaterTest.php | 9 +++++- 5 files changed, 73 insertions(+), 12 deletions(-) diff --git a/core/Command/Upgrade.php b/core/Command/Upgrade.php index 5a2deea0b6cbb..f6f33783684a0 100644 --- a/core/Command/Upgrade.php +++ b/core/Command/Upgrade.php @@ -99,7 +99,8 @@ protected function execute(InputInterface $input, OutputInterface $output) { $this->config, \OC::$server->getIntegrityCodeChecker(), $this->logger, - $this->installer + $this->installer, + \OC::$server->getJobList() ); $dispatcher = \OC::$server->getEventDispatcher(); @@ -191,6 +192,9 @@ protected function execute(InputInterface $input, OutputInterface $output) { $updater->listen('\OC\Updater', 'maintenanceActive', function () use($output) { $output->writeln('Maintenance mode is kept active'); }); + $updater->listen('\OC\Updater', 'waitForCronToFinish', function () use($output) { + $output->writeln('Waiting for cron to finish (checks again in 5 seconds)...'); + }); $updater->listen('\OC\Updater', 'updateEnd', function ($success) use($output, $self) { if ($success) { diff --git a/core/ajax/update.php b/core/ajax/update.php index 6ec6b71731d9c..c1210eaa5b74b 100644 --- a/core/ajax/update.php +++ b/core/ajax/update.php @@ -119,7 +119,8 @@ public function handleRepairFeedback($event) { $config, \OC::$server->getIntegrityCodeChecker(), $logger, - \OC::$server->query(\OC\Installer::class) + \OC::$server->query(\OC\Installer::class), + \OC::$server->getJobList() ); $incompatibleApps = []; @@ -152,6 +153,9 @@ public function handleRepairFeedback($event) { $updater->listen('\OC\Updater', 'maintenanceActive', function () use ($eventSource, $l) { $eventSource->send('success', (string)$l->t('Maintenance mode is kept active')); }); + $updater->listen('\OC\Updater', 'waitForCronToFinish', function () use ($eventSource, $l) { + $eventSource->send('success', (string)$l->t('Waiting for cron to finish (checks again in 5 seconds)...')); + }); $updater->listen('\OC\Updater', 'dbUpgradeBefore', function () use($eventSource, $l) { $eventSource->send('success', (string)$l->t('Updating database schema')); }); diff --git a/lib/private/BackgroundJob/JobList.php b/lib/private/BackgroundJob/JobList.php index 1399683dc9b49..a926194e9dff5 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -48,6 +48,9 @@ class JobList implements IJobList { /**@var ITimeFactory */ protected $timeFactory; + /** @var int - 12 hours * 3600 seconds*/ + private $jobTimeOut = 43200; + /** * @param IDBConnection $connection * @param IConfig $config @@ -182,7 +185,7 @@ public function getNext() { $query = $this->connection->getQueryBuilder(); $query->select('*') ->from('jobs') - ->where($query->expr()->lte('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - 12 * 3600, IQueryBuilder::PARAM_INT))) + ->where($query->expr()->lte('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - $this->jobTimeOut, IQueryBuilder::PARAM_INT))) ->orderBy('last_checked', 'ASC') ->setMaxResults(1); @@ -333,4 +336,39 @@ public function setExecutionTime(IJob $job, $timeTaken) { ->where($query->expr()->eq('id', $query->createNamedParameter($job->getId(), IQueryBuilder::PARAM_INT))); $query->execute(); } + + /** + * checks if a job is still running (reserved_at time is smaller than 12 hours ago) + * + * Background information: + * + * The 12 hours is the same timeout that is also used to re-schedule an non-terminated + * job (see getNext()). The idea here is to give a job enough time to run very + * long but still be able to recognize that it maybe crashed and re-schedule it + * after the timeout. It's more likely to be crashed at that time than it ran + * that long. + * + * In theory it could lead to an nearly endless loop (as in - at most 12 hours). + * The cron command will not start new jobs when maintenance mode is active and + * this method is only executed in maintenance mode (see where it is called in + * the upgrader class. So this means in the worst case we wait 12 hours when a + * job has crashed. On the other hand: then the instance should be fixed anyways. + * + * @return bool + */ + public function isAnyJobRunning(): bool { + $query = $this->connection->getQueryBuilder(); + $query->select('*') + ->from('jobs') + ->where($query->expr()->gt('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - $this->jobTimeOut, IQueryBuilder::PARAM_INT))) + ->setMaxResults(1); + $result = $query->execute(); + $row = $result->fetch(); + $result->closeCursor(); + + if ($row) { + return true; + } + return false; + } } diff --git a/lib/private/Updater.php b/lib/private/Updater.php index e6e38798142bd..02b3138f30fe0 100644 --- a/lib/private/Updater.php +++ b/lib/private/Updater.php @@ -38,6 +38,7 @@ use OC\Hooks\BasicEmitter; use OC\IntegrityCheck\Checker; use OC_App; +use OCP\BackgroundJob\IJobList; use OCP\IConfig; use OCP\ILogger; use OCP\Util; @@ -66,6 +67,9 @@ class Updater extends BasicEmitter { /** @var Installer */ private $installer; + /** @var IJobList */ + private $jobList; + private $logLevelNames = [ 0 => 'Debug', 1 => 'Info', @@ -74,20 +78,16 @@ class Updater extends BasicEmitter { 4 => 'Fatal', ]; - /** - * @param IConfig $config - * @param Checker $checker - * @param ILogger $log - * @param Installer $installer - */ public function __construct(IConfig $config, Checker $checker, - ILogger $log = null, - Installer $installer) { + ILogger $log, + Installer $installer, + IJobList $jobList) { $this->log = $log; $this->config = $config; $this->checker = $checker; $this->installer = $installer; + $this->jobList = $jobList; } /** @@ -111,6 +111,8 @@ public function upgrade() { $this->emit('\OC\Updater', 'maintenanceEnabled'); } + $this->waitForCronToFinish(); + $installedVersion = $this->config->getSystemValue('version', '0.0.0'); $currentVersion = implode('.', \OCP\Util::getVersion()); $this->log->debug('starting upgrade from ' . $installedVersion . ' to ' . $currentVersion, array('app' => 'core')); @@ -604,6 +606,12 @@ private function logAllEvents() { }); } + private function waitForCronToFinish() { + while ($this->jobList->isAnyJobRunning()) { + $this->emit('\OC\Updater', 'waitForCronToFinish'); + sleep(5); + } + } } diff --git a/tests/lib/UpdaterTest.php b/tests/lib/UpdaterTest.php index afc9f9b1f862e..02fea9c3c0ad9 100644 --- a/tests/lib/UpdaterTest.php +++ b/tests/lib/UpdaterTest.php @@ -24,6 +24,7 @@ use OC\Installer; use OC\Updater; +use OCP\BackgroundJob\IJobList; use OCP\IConfig; use OCP\ILogger; use OC\IntegrityCheck\Checker; @@ -39,6 +40,8 @@ class UpdaterTest extends TestCase { private $checker; /** @var Installer|\PHPUnit_Framework_MockObject_MockObject */ private $installer; + /** @var IJobList|\PHPUnit_Framework_MockObject_MockObject */ + private $jobList; public function setUp() { parent::setUp(); @@ -54,12 +57,16 @@ public function setUp() { $this->installer = $this->getMockBuilder(Installer::class) ->disableOriginalConstructor() ->getMock(); + $this->jobList = $this->getMockBuilder(IJobList::class) + ->disableOriginalConstructor() + ->getMock(); $this->updater = new Updater( $this->config, $this->checker, $this->logger, - $this->installer + $this->installer, + $this->jobList ); }