From b292f919c646f2cc3f10c220c5c72a5b132b6467 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Thu, 1 Nov 2018 12:52:09 +0100 Subject: [PATCH] Revert "Wait for cron to finish before running upgrade command" This reverts commit 18e9631810ad1d3d72c2b4bbee330169808108ad. 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 | 27 ++++++------------ tests/lib/UpdaterTest.php | 9 +----- 5 files changed, 12 insertions(+), 76 deletions(-) diff --git a/core/Command/Upgrade.php b/core/Command/Upgrade.php index 86f049d9f2a..5a2deea0b6c 100644 --- a/core/Command/Upgrade.php +++ b/core/Command/Upgrade.php @@ -99,8 +99,7 @@ class Upgrade extends Command { $this->config, \OC::$server->getIntegrityCodeChecker(), $this->logger, - $this->installer, - \OC::$server->getJobList() + $this->installer ); $dispatcher = \OC::$server->getEventDispatcher(); @@ -192,9 +191,6 @@ class Upgrade extends Command { $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 377da746100..6ec6b71731d 100644 --- a/core/ajax/update.php +++ b/core/ajax/update.php @@ -119,8 +119,7 @@ if (\OCP\Util::needUpgrade()) { $config, \OC::$server->getIntegrityCodeChecker(), $logger, - \OC::$server->query(\OC\Installer::class), - \OC::$server->getJobList() + \OC::$server->query(\OC\Installer::class) ); $incompatibleApps = []; @@ -153,9 +152,6 @@ if (\OCP\Util::needUpgrade()) { $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 fab608cf164..e890c35868b 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -48,9 +48,6 @@ class JobList implements IJobList { /**@var ITimeFactory */ protected $timeFactory; - /** @var int - 12 hours * 3600 seconds*/ - private $jobTimeOut = 43200; - /** * @param IDBConnection $connection * @param IConfig $config @@ -186,7 +183,7 @@ class JobList implements IJobList { $query = $this->connection->getQueryBuilder(); $query->select('*') ->from('jobs') - ->where($query->expr()->lte('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - $this->jobTimeOut, IQueryBuilder::PARAM_INT))) + ->where($query->expr()->lte('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - 12 * 3600, IQueryBuilder::PARAM_INT))) ->andWhere($query->expr()->lte('last_checked', $query->createNamedParameter($this->timeFactory->getTime(), IQueryBuilder::PARAM_INT))) ->orderBy('last_checked', 'ASC') ->setMaxResults(1); @@ -346,39 +343,4 @@ class JobList implements IJobList { ->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 8a2b1cd188c..4b4723be94f 100644 --- a/lib/private/Updater.php +++ b/lib/private/Updater.php @@ -38,7 +38,6 @@ use OC\DB\MigrationService; use OC\Hooks\BasicEmitter; use OC\IntegrityCheck\Checker; use OC_App; -use OCP\BackgroundJob\IJobList; use OCP\IConfig; use OCP\ILogger; use OCP\Util; @@ -67,9 +66,6 @@ class Updater extends BasicEmitter { /** @var Installer */ private $installer; - /** @var IJobList */ - private $jobList; - private $logLevelNames = [ 0 => 'Debug', 1 => 'Info', @@ -78,16 +74,20 @@ 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, - Installer $installer, - IJobList $jobList) { + ILogger $log = null, + Installer $installer) { $this->log = $log; $this->config = $config; $this->checker = $checker; $this->installer = $installer; - $this->jobList = $jobList; } /** @@ -114,11 +114,6 @@ class Updater extends BasicEmitter { $installedVersion = $this->config->getSystemValue('version', '0.0.0'); $currentVersion = implode('.', \OCP\Util::getVersion()); - // see https://github.com/nextcloud/server/issues/9992 for potential problem - if (version_compare($installedVersion, '14.0.0.9', '>=')) { - $this->waitForCronToFinish(); - } - $this->log->debug('starting upgrade from ' . $installedVersion . ' to ' . $currentVersion, array('app' => 'core')); $success = true; @@ -617,12 +612,6 @@ class Updater extends BasicEmitter { }); } - 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 02fea9c3c0a..afc9f9b1f86 100644 --- a/tests/lib/UpdaterTest.php +++ b/tests/lib/UpdaterTest.php @@ -24,7 +24,6 @@ namespace Test; use OC\Installer; use OC\Updater; -use OCP\BackgroundJob\IJobList; use OCP\IConfig; use OCP\ILogger; use OC\IntegrityCheck\Checker; @@ -40,8 +39,6 @@ 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(); @@ -57,16 +54,12 @@ class UpdaterTest extends TestCase { $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->jobList + $this->installer ); } -- 2.39.5