From f4a3d8fc4339ccfd3cd45d1cbb37545ac88473ad Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 21:46:44 +0100 Subject: [PATCH 01/11] Add hostname column to cron_schedule table --- Setup/UpgradeSchema.php | 26 ++++++++++++++++++++++++++ etc/db_schema.xml | 1 + etc/db_schema_whitelist.json | 9 +++++++++ etc/module.xml | 2 +- 4 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 etc/db_schema_whitelist.json mode change 100755 => 100644 etc/module.xml diff --git a/Setup/UpgradeSchema.php b/Setup/UpgradeSchema.php index 53a7be4c..40b5b42b 100644 --- a/Setup/UpgradeSchema.php +++ b/Setup/UpgradeSchema.php @@ -46,6 +46,10 @@ public function upgrade( $this->addKillRequestToSchedule(); } + if (version_compare($context->getVersion(), '1.9.0') < 0) { + $this->addHostnameToSchedule(); + } + $this->setup->endSetup(); } @@ -73,6 +77,28 @@ public function addPidToSchedule() ); } + /** + * Add column to cron_schedule to keep track of which server is running each process + */ + public function addHostnameToSchedule() + { + if (version_compare($this->magentoMetaData->getVersion(), '2.3.0', '>=')) { + // For Magento 2.3+, db_schema.xml is used instead + return; + } + $this->setup->getConnection()->addColumn( + $this->setup->getTable('cron_schedule'), + 'hostname', + [ + 'type' => Table::TYPE_INTEGER, + 'comment' => 'Hostname of the server running this job', + 'nullable' => true, + 'default' => null, + 'after' => 'pid', + ] + ); + } + /** * Add column to cron_schedule to send kill requests */ diff --git a/etc/db_schema.xml b/etc/db_schema.xml index 509bc21f..20809cf4 100644 --- a/etc/db_schema.xml +++ b/etc/db_schema.xml @@ -2,6 +2,7 @@ +
diff --git a/etc/db_schema_whitelist.json b/etc/db_schema_whitelist.json new file mode 100644 index 00000000..69469470 --- /dev/null +++ b/etc/db_schema_whitelist.json @@ -0,0 +1,9 @@ +{ + "cron_schedule": { + "column": { + "hostname": true, + "pid": true, + "kill_request": true + } + } +} diff --git a/etc/module.xml b/etc/module.xml old mode 100755 new mode 100644 index a09a0787..62115fab --- a/etc/module.xml +++ b/etc/module.xml @@ -1,6 +1,6 @@ - + From 0b96132985ed086a2ec614ed938b036032f8e443 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 21:57:44 +0100 Subject: [PATCH 02/11] Show hostname in grid --- .../ui_component/cronjobmanager_manage_grid.xml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) mode change 100755 => 100644 view/adminhtml/ui_component/cronjobmanager_manage_grid.xml diff --git a/view/adminhtml/ui_component/cronjobmanager_manage_grid.xml b/view/adminhtml/ui_component/cronjobmanager_manage_grid.xml old mode 100755 new mode 100644 index 8d449951..b4e6f835 --- a/view/adminhtml/ui_component/cronjobmanager_manage_grid.xml +++ b/view/adminhtml/ui_component/cronjobmanager_manage_grid.xml @@ -102,7 +102,7 @@ Dispatch Now - WARNING: Depending on the actions you selected, (and the amount of them) this process + WARNING: Depending on the actions you selected, (and the amount of them) this process can consume a lot of time and memory. Are you sure you want to proceed? dispatch @@ -225,6 +225,16 @@ + + + + text + true + Hostname + false + + + From 60bd5efe07b9489de9b63e0c99f507b73c468f5f Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 21:58:54 +0100 Subject: [PATCH 03/11] Set hostname when running job --- Plugin/Cron/Model/SchedulePlugin.php | 1 + Plugin/Cron/Model/ScheduleResourcePlugin.php | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Plugin/Cron/Model/SchedulePlugin.php b/Plugin/Cron/Model/SchedulePlugin.php index 8ce2c2b0..0083d0c1 100644 --- a/Plugin/Cron/Model/SchedulePlugin.php +++ b/Plugin/Cron/Model/SchedulePlugin.php @@ -10,6 +10,7 @@ class SchedulePlugin public function afterTryLockJob(Schedule $subject, bool $result) { if ($result) { + $subject->setData('hostname', \gethostname()); $subject->setData('pid', \getmypid()); } return $result; diff --git a/Plugin/Cron/Model/ScheduleResourcePlugin.php b/Plugin/Cron/Model/ScheduleResourcePlugin.php index 23dd2e99..3345eca4 100644 --- a/Plugin/Cron/Model/ScheduleResourcePlugin.php +++ b/Plugin/Cron/Model/ScheduleResourcePlugin.php @@ -36,7 +36,7 @@ public function afterSave( } /** - * Replace method to update pid column together with status column + * Replace method to update pid and hostname columns together with status column * * @param \Magento\Cron\Model\ResourceModel\Schedule $subject * @param callable $proceed @@ -68,6 +68,7 @@ public function aroundTrySetJobUniqueStatusAtomic( ['existing' => $subject->getTable('cron_schedule')], $match, [ + 'hostname' => new \Zend_Db_Expr($connection->quote(\gethostname())), 'status' => new \Zend_Db_Expr($connection->quote($newStatus)), 'pid' => new \Zend_Db_Expr($connection->quote(\getmypid())) ] From cfd36ba396e09390362079418834435328b7328c Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 21:59:51 +0100 Subject: [PATCH 04/11] Add integration test to cover setting hostname --- Test/Integration/HostnameTest.php | 85 +++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 Test/Integration/HostnameTest.php diff --git a/Test/Integration/HostnameTest.php b/Test/Integration/HostnameTest.php new file mode 100644 index 00000000..545bacd2 --- /dev/null +++ b/Test/Integration/HostnameTest.php @@ -0,0 +1,85 @@ +objectManager = Bootstrap::getObjectManager(); + $this->scheduleResource = $this->objectManager->get(ScheduleResource::class); + } + + public function testProcessIdSavedOnStart(): void + { + $this->givenHostname($hostname); + $this->givenPendingSchedule($schedule); + $this->whenTryLockJob($schedule); + $this->thenScheduleIsSavedWithHostname($schedule, $hostname); + } + + public function testProcessIdMaintainedAfterSuccesfulRun(): void + { + $this->givenHostname($hostname); + $this->givenPendingSchedule($schedule); + $this->whenTryLockJob($schedule); + $this->andScheduleSavedWithSuccess($schedule); + $this->thenScheduleIsSavedWithHostname($schedule, $hostname); + } + + private function givenPendingSchedule(&$schedule): void + { + /** @var Schedule $newSchedule */ + $newSchedule = $this->objectManager->create(Schedule::class); + $newSchedule->setStatus(Schedule::STATUS_PENDING); + $newSchedule->setJobCode('test_job_code'); + $newSchedule->save(); + /** @var Schedule $schedule */ + $schedule = $this->objectManager->create(Schedule::class); + $schedule->load($newSchedule->getId()); + } + + private function whenTryLockJob(Schedule $schedule): void + { + $lock = $schedule->tryLockJob(); + $this->assertTrue($lock, 'Precondition: tryLockJob() should be successful'); + } + + private function andScheduleSavedWithSuccess(Schedule $schedule): void + { + $schedule->setStatus(Schedule::STATUS_SUCCESS); + $this->scheduleResource->save($schedule); + } + + private function thenScheduleIsSavedWithHostname(Schedule $schedule, string $hostname): void + { + $this->scheduleResource->load($schedule, $schedule->getId()); + $this->assertEquals($hostname, $schedule->getData('hostname'), 'Hostname should be saved in schedule'); + } + + private function givenHostname(&$hostname): void + { + $hostname = \gethostname(); + $this->assertNotFalse($hostname, 'Precondition: gethostname() should not return false'); + } +} From f8e62b4af9168b5d36d2533e3b4607f5e7e7f170 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 22:14:02 +0100 Subject: [PATCH 05/11] Allow retreival of hostname from data model --- Api/Data/ScheduleInterface.php | 5 +++++ Model/Data/Schedule.php | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/Api/Data/ScheduleInterface.php b/Api/Data/ScheduleInterface.php index e6308482..daf3eee1 100644 --- a/Api/Data/ScheduleInterface.php +++ b/Api/Data/ScheduleInterface.php @@ -33,6 +33,11 @@ public function getJobCode(): string; */ public function getStatus(): string; + /** + * @return string|null + */ + public function getHostname(); + /** * @return int|null */ diff --git a/Model/Data/Schedule.php b/Model/Data/Schedule.php index 67c0182e..72f8b128 100644 --- a/Model/Data/Schedule.php +++ b/Model/Data/Schedule.php @@ -13,6 +13,7 @@ class Schedule extends DataObject implements \EthanYehuda\CronjobManager\Api\Dat const KEY_SCHEDULE_ID = 'schedule_id'; const KEY_JOB_CODE = 'job_code'; const KEY_STATUS = 'status'; + const KEY_HOSTNAME = 'hostname'; const KEY_PID = 'pid'; const KEY_MESSAGES = 'messages'; const KEY_CREATED_AT = 'created_at'; @@ -41,6 +42,11 @@ public function getStatus(): string return $this->getData(self::KEY_STATUS); } + public function getHostname(): string + { + return (string) $this->getData(self::KEY_HOSTNAME); + } + public function getPid() { return (int) $this->getData(self::KEY_PID); From 4d21a956e28867791fcb28163a7f9f181fc4a593 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 22:21:00 +0100 Subject: [PATCH 06/11] Only mark jobs as 'gone' if on same host --- Model/CleanRunningJobs.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Model/CleanRunningJobs.php b/Model/CleanRunningJobs.php index 46f8f624..ef163c29 100644 --- a/Model/CleanRunningJobs.php +++ b/Model/CleanRunningJobs.php @@ -53,6 +53,10 @@ public function execute() $runningJobs = $this->scheduleRepository->getByStatus(ScheduleInterface::STATUS_RUNNING); foreach ($runningJobs as $schedule) { + if ($schedule->getHostname() !== \gethostname()) { + continue; + } + if ($this->processManagement->isPidAlive($schedule->getPid())) { continue; } From 23110acb15a424c149c5648de6e18ec9c2600c93 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 22:29:27 +0100 Subject: [PATCH 07/11] Only kill jobs on same host --- Console/Command/KillJob.php | 18 +++++++++--------- Model/ProcessKillRequests.php | 2 +- Model/ProcessManagement.php | 5 ++++- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/Console/Command/KillJob.php b/Console/Command/KillJob.php index 2857abd2..d6eda792 100644 --- a/Console/Command/KillJob.php +++ b/Console/Command/KillJob.php @@ -38,27 +38,27 @@ class KillJob extends Command private $scheduleRepository; /** - * @var ScheduleManagementInterface + * @var ScheduleManagementInterface */ private $scheduleManagement; /** - * @var SearchCriteriaBuilder + * @var SearchCriteriaBuilder */ private $searchCriteriaBuilder; - + /** - * @var FilterBuilder + * @var FilterBuilder */ private $filterBuilder; /** - * @var FilterGroupBuilder + * @var FilterGroupBuilder */ private $filterGroupBuilder; - + /** - * @var ProcessManagement + * @var ProcessManagement */ private $processManagement; @@ -137,7 +137,7 @@ protected function execute(InputInterface $input, OutputInterface $output) /** @var bool $killed */ $killed = false; if ($optionProcKill) { - $killed = $this->processManagement->killPid($pid); + $killed = $this->processManagement->killPid($pid, $job->getHostname()); } else { $killed = $this->scheduleManagement->kill($id, \time()); } @@ -190,7 +190,7 @@ private function loadRunningJobsByCode(string $jobCode): array $searchCriteria = $this->searchCriteriaBuilder->setFilterGroups( [$jobCodeFilterGroup, $statusFilterGroup] )->create(); - + /** @var \Magento\Framework\Api\SearchResultsInterface $result */ $result = $this->scheduleRepository->getList($searchCriteria); return $result->getItems(); diff --git a/Model/ProcessKillRequests.php b/Model/ProcessKillRequests.php index 48d88dee..ac355bd8 100644 --- a/Model/ProcessKillRequests.php +++ b/Model/ProcessKillRequests.php @@ -51,7 +51,7 @@ public function execute() private function killScheduleProcess(ScheduleInterface $schedule): void { - if ($this->processManagement->killPid($schedule->getPid())) { + if ($this->processManagement->killPid($schedule->getPid(), $schedule->getHostname())) { $messages = []; if ($schedule->getMessages()) { $messages[] = $schedule->getMessages(); diff --git a/Model/ProcessManagement.php b/Model/ProcessManagement.php index 1d2fbfb5..7b918f78 100644 --- a/Model/ProcessManagement.php +++ b/Model/ProcessManagement.php @@ -12,8 +12,11 @@ public function isPidAlive(int $pid): bool return \file_exists('/proc/' . $pid); } - public function killPid($pid): bool + public function killPid(int $pid, string $hostname): bool { + if ($hostname !== \gethostname()) { + return false; + } if (!$this->isPidAlive($pid)) { return false; } From 2f4b8b941b55b7b49d00358b0f1308b974192f27 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 22:29:48 +0100 Subject: [PATCH 08/11] Remove dependency on /proc filesystem (ie Linux) --- Model/ProcessManagement.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Model/ProcessManagement.php b/Model/ProcessManagement.php index 7b918f78..2077fd19 100644 --- a/Model/ProcessManagement.php +++ b/Model/ProcessManagement.php @@ -9,7 +9,7 @@ class ProcessManagement public function isPidAlive(int $pid): bool { - return \file_exists('/proc/' . $pid); + return \posix_kill($pid, 0); } public function killPid(int $pid, string $hostname): bool From 29ca0d1d88d0e5dde8ccfa23e1c267ce34c74203 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Sat, 16 May 2020 23:04:33 +0100 Subject: [PATCH 09/11] Update integration tests to cover new behaviour --- Test/Integration/CleanRunningJobsTest.php | 19 +++++++++++++++++- Test/Integration/ProcessKillRequestsTest.php | 21 ++++++++++++++++++-- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/Test/Integration/CleanRunningJobsTest.php b/Test/Integration/CleanRunningJobsTest.php index d1b34114..ba8e5653 100644 --- a/Test/Integration/CleanRunningJobsTest.php +++ b/Test/Integration/CleanRunningJobsTest.php @@ -19,6 +19,8 @@ class CleanRunningJobsTest extends TestCase { const NOW = '2019-02-09 18:33:00'; + const REMOTE_HOSTNAME = 'hostname.example.net'; + /** * @var ObjectManager */ @@ -34,7 +36,7 @@ class CleanRunningJobsTest extends TestCase const DEAD_PID = 99999999; - protected function setUp() + protected function setUp(): void { $this->objectManager = Bootstrap::getObjectManager(); $this->objectManager->configure(['preferences' => [Clock::class => FakeClock::class]]); @@ -47,11 +49,20 @@ protected function setUp() public function testDeadRunningJobsAreCleaned() { $this->givenRunningScheduleWithInactiveProcess($schedule); + $this->givenScheduleIsRunningOnHost($schedule, \gethostname()); $this->whenEventIsDispatched('process_cron_queue_before'); $this->thenScheduleHasStatus($schedule, Schedule::STATUS_ERROR); $this->andScheduleHasMessage($schedule, 'Process went away at ' . self::NOW); } + public function testDeadRunningJobsOnAnotherHostAreNotCleaned() + { + $this->givenRunningScheduleWithInactiveProcess($schedule); + $this->givenScheduleIsRunningOnHost($schedule, self::REMOTE_HOSTNAME); + $this->whenEventIsDispatched('process_cron_queue_before'); + $this->thenScheduleHasStatus($schedule, Schedule::STATUS_RUNNING); + } + public function testActiveRunningJobsAreNotCleaned() { $this->givenRunningScheduleWithActiveProcess($schedule); @@ -68,6 +79,12 @@ private function givenRunningScheduleWithInactiveProcess(&$schedule) $schedule->save(); } + private function givenScheduleIsRunningOnHost(Schedule &$schedule, string $hostname): void + { + $schedule->setData('hostname', $hostname); + $schedule->save(); + } + private function givenRunningScheduleWithActiveProcess(&$schedule) { /** @var Schedule $schedule */ diff --git a/Test/Integration/ProcessKillRequestsTest.php b/Test/Integration/ProcessKillRequestsTest.php index 873b5b75..9367176b 100644 --- a/Test/Integration/ProcessKillRequestsTest.php +++ b/Test/Integration/ProcessKillRequestsTest.php @@ -21,6 +21,8 @@ class ProcessKillRequestsTest extends TestCase { const NOW = '2019-02-09 18:33:00'; + const REMOTE_HOSTNAME = 'hostname.example.net'; + /** * @var int */ @@ -46,7 +48,7 @@ class ProcessKillRequestsTest extends TestCase */ private $clock; - protected function setUp() + protected function setUp(): void { $this->objectManager = Bootstrap::getObjectManager(); $this->objectManager->configure(['preferences' => [Clock::class => FakeClock::class]]); @@ -57,7 +59,7 @@ protected function setUp() $this->processManagement = $this->objectManager->get(ProcessManagement::class); } - protected function tearDown() + protected function tearDown(): void { /* * Take care of children that we failed to kill @@ -70,12 +72,21 @@ protected function tearDown() public function testDeadRunningJobsAreCleaned() { $this->givenRunningScheduleWithKillRequest($schedule, $this->timeStampInThePast()); + $this->givenScheduleIsRunningOnHost($schedule, \gethostname()); $this->whenEventIsDispatched('process_cron_queue_before'); $this->thenScheduleHasStatus($schedule, ScheduleInterface::STATUS_KILLED); $this->andScheduleHasMessage($schedule, 'Process was killed at ' . self::NOW); $this->andProcessIsKilled($schedule); } + public function testDeadRunningJobsOnAnotherHostAreNotCleaned() + { + $this->givenRunningScheduleWithKillRequest($schedule, $this->timeStampInThePast()); + $this->givenScheduleIsRunningOnHost($schedule, self::REMOTE_HOSTNAME); + $this->whenEventIsDispatched('process_cron_queue_before'); + $this->thenScheduleHasStatus($schedule, Schedule::STATUS_RUNNING); + } + private function givenRunningScheduleWithKillRequest(&$schedule, int $timestamp) { @@ -87,6 +98,12 @@ private function givenRunningScheduleWithKillRequest(&$schedule, int $timestamp) $this->scheduleManagement->kill((int)$schedule->getId(), $timestamp); } + private function givenScheduleIsRunningOnHost(Schedule &$schedule, string $hostname): void + { + $schedule->setData('hostname', $hostname); + $schedule->save(); + } + private function whenEventIsDispatched($eventName) { $this->eventManager->dispatch($eventName); From bfb94d53f0009bb3015397bfb2f229cafcd0084d Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Mon, 18 May 2020 22:31:01 +0100 Subject: [PATCH 10/11] Correct hostname column type --- Setup/UpgradeSchema.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Setup/UpgradeSchema.php b/Setup/UpgradeSchema.php index 40b5b42b..0badbb98 100644 --- a/Setup/UpgradeSchema.php +++ b/Setup/UpgradeSchema.php @@ -90,7 +90,8 @@ public function addHostnameToSchedule() $this->setup->getTable('cron_schedule'), 'hostname', [ - 'type' => Table::TYPE_INTEGER, + 'type' => Table::TYPE_TEXT, + 'length' => 255, 'comment' => 'Hostname of the server running this job', 'nullable' => true, 'default' => null, From 16e29b763df385db2b73d22030ca53035c4d975d Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Fri, 22 May 2020 22:08:35 +0100 Subject: [PATCH 11/11] Remove unused code The function this was wrapping was introduced in v2.2.0-RC1.2 and was called within Magento\Cron\Model\Schedule::tryLockJob(), which we also have an after plugin for. As we are setting (and now saving, see #142) pid & hostname in there, this around plugin is no longer required. Since v2.3.5, tryLockJob() no longer calls this function, further rendering this plugin obsolete. --- Plugin/Cron/Model/ScheduleResourcePlugin.php | 51 -------------------- 1 file changed, 51 deletions(-) diff --git a/Plugin/Cron/Model/ScheduleResourcePlugin.php b/Plugin/Cron/Model/ScheduleResourcePlugin.php index 3345eca4..3347c060 100644 --- a/Plugin/Cron/Model/ScheduleResourcePlugin.php +++ b/Plugin/Cron/Model/ScheduleResourcePlugin.php @@ -34,55 +34,4 @@ public function afterSave( } return $result; } - - /** - * Replace method to update pid and hostname columns together with status column - * - * @param \Magento\Cron\Model\ResourceModel\Schedule $subject - * @param callable $proceed - * @param $scheduleId - * @param $newStatus - * @param $currentStatus - * @return bool - * @throws \Zend_Db_Statement_Exception - */ - public function aroundTrySetJobUniqueStatusAtomic( - \Magento\Cron\Model\ResourceModel\Schedule $subject, - callable $proceed, - $scheduleId, - $newStatus, - $currentStatus - ) { - $connection = $subject->getConnection(); - - // this condition added to avoid cron jobs locking after incorrect termination of running job - $match = $connection->quoteInto( - 'existing.job_code = current.job_code ' . - 'AND (existing.executed_at > UTC_TIMESTAMP() - INTERVAL 1 DAY OR existing.executed_at IS NULL) ' . - 'AND existing.status = ?', - $newStatus - ); - - $selectIfUnlocked = $connection->select() - ->joinLeft( - ['existing' => $subject->getTable('cron_schedule')], - $match, - [ - 'hostname' => new \Zend_Db_Expr($connection->quote(\gethostname())), - 'status' => new \Zend_Db_Expr($connection->quote($newStatus)), - 'pid' => new \Zend_Db_Expr($connection->quote(\getmypid())) - ] - ) - ->where('current.schedule_id = ?', $scheduleId) - ->where('current.status = ?', $currentStatus) - ->where('existing.schedule_id IS NULL'); - - $update = $connection->updateFromSelect($selectIfUnlocked, ['current' => $subject->getTable('cron_schedule')]); - $result = $connection->query($update)->rowCount(); - - if ($result == 1) { - return true; - } - return false; - } }