Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instantly apply configuration changes in the cron schedule #9957

Merged
merged 15 commits into from Jun 21, 2017

Conversation

@AntonEvers
Copy link
Contributor

AntonEvers commented Jun 15, 2017

Description

This pull request:

  1. fixes a number of errors in the unit tests that came with the cron process
  2. deletes cron jobs that have been disabled in the config, so already scheduled jobs will not execute although they have been deleted
  3. deletes scheduled cron jobs that do not match their cron expression anymore, because it has been altered in the configuration.
  4. changes the order of execution from running, generating, cleaning to cleaning, generating, running so that clean jobs will not run and generated jobs will run immediately.

This fixes #3380 (moved to forums)

Fixed Issues (if relevant)

  1. #3380: Remove scheduled jobs after changing cron settings

Manual testing scenarios

  1. Truncate the cron_schedule table
  2. Run the bin/magento cron:run command
  3. Look inside the cron_schedule table to see that catalog_product_outdated_price_values_cleanup is scheduled every minute
  4. Open app/code/Magento/Catalog/etc/crontab.xml and change the cron expression from * * * * * to */10 * * * *
  5. Flush cache
  6. Run the bin/magento cron:run command
  7. See that all pending entries not matching the new expression have been removed, and nothing else
  8. Change catalog_product_outdated_price_values_cleanup back to * * * * * and truncate the cron_schedule table
  9. Run the bin/magento cron:run command
  10. Look inside the cron_schedule table to see that catalog_product_outdated_price_values_cleanup is scheduled every minute
  11. Open app/code/Magento/Catalog/etc/crontab.xml and remove the cron expression tag
  12. Flush cache
  13. Run the bin/magento cron:run command
  14. See that all pending entries of catalog_product_outdated_price_values_cleanup have been removed, and nothing else

You can also play around with enabling/disabling the sitemap generator or another cron job that has configuration for enable/disable and schedules.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)
Anton Evers added 12 commits Jun 14, 2017
This is in preparation of listening for changes in the configuration
to check if a scheduled cron job is still enabled. When you disable
a cron job in te config and flush the config cache, the cron observer
should remove any scheduled cron jobs that are pending. If the cron
observer runs the jobs before cleaning up, cron jobs that have been
disabled will still run 1 time before they are removed from the
schedule after disabling them.

The extra advantage of this setup is that cron jobs will run
directly after they have been scheduled, in the same run. So you
don't have to run the cron job twice to see jobs executing. One for
scheduling and one for running. If there is a perfectly good reason
not to generate the schedule and clean jobs before running them I
would very much like to know.
As I heard from @ishakhsuvarov Magento does not encourage usage of
`protected` methods, because they do not encourage inheritance-based
API. So composition in favor of inheritance. It's easier to maintain
and reuse.

I myself have already encountered scenarios where I wanted to use
methods in this observer elsewhere in custom code, but I had to
write duplicate code because these methods were `protected`.
When you change a cron expression in the config, you do not
want jobs that were scheduled ahead to be executed on the old
config cron expression.

The cleanup makes the config change take immediate effect.
Some unit tests did not flag as failed even though the thing
they were testing did not occur. I have rewritten these tests
to be more responsive when things go wrong.

There is however a big step to be made to make these tests
more readable and more consistent.
of this class. The public functions are either useful outside of
this class or useful to disable or extend using plugins
…and if disabled jobs will be deleted
Anton Evers
@okorshenko

This comment has been minimized.

Copy link
Contributor

okorshenko commented Jun 15, 2017

Hi @ajpevers Thank you for continuous contribution to Magento 2 project. Great job!
One thing that I'd like to highlight is Backward compatibility. Please, review this document and adjust your Pull Request according to this policy: http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/

This comment is less about this particular PR, but more about overall contribution.

Please, let me know if you have any questions.
Thank you

Anton Evers added 3 commits Jun 18, 2017
@AntonEvers

This comment has been minimized.

Copy link
Contributor Author

AntonEvers commented Jun 18, 2017

@okorshenko you're right, @vkublytskyi and @ishakhsuvarov have pointed out to me what I should take into account and how I should work around backward incompatibility in specific cases. Thanks for the heads-up. From now on I'll keep it in mind. I've rolled back backward incompatible rafactoring that I've done.

If there is a need to refactor these files I'll leave it to you and I'll just add the functional changes.

... and I see that I've made a typo in the commit messages just now... I meant backward of course.

@okorshenko okorshenko self-assigned this Jun 20, 2017
@okorshenko okorshenko added this to the June 2017 milestone Jun 20, 2017
@vherasymenko

This comment has been minimized.

Copy link

vherasymenko commented Jun 21, 2017

@ajpevers Hi. After applying your PR nothing changed in my Magento behavior.

My Steps to check issue:

  1. Install Magento
  2. Truncate table cron_schedule
  3. Run php bin/magento cron:run
  4. Look inside in table cron_schedule
    • Jobs catalog_product_outdated_price_values_cleanup is scheduled every minute.
  5. Then change in app/code/Magento/Catalog/etc/crontab.xml cron expression from * * * * * to */10 * * * *
  6. Run php bin/magento cache:flush
  7. Run php bin/magento cron:run
  8. Look inside in table cron_schedule and pay attention on catalog_product_outdated_price_values_cleanup with pending status.

Actual Result: jobs with status pending is still exist in table.
Expected Result: There are should be removed.

Note: And in step №7 in your scenario new jobs with new expression should be displayed ? If yes then in my case there are is missing. Displayed only jobs with old expression with status success and pending.

Could you help me with checking your PR ??? Thanks.

@vherasymenko

This comment has been minimized.

Copy link

vherasymenko commented Jun 21, 2017

@ajpevers Sorry for disturbing I had problem on my side. Your PR works fine.

@magento-team magento-team merged commit 7327a83 into magento:develop Jun 21, 2017
3 checks passed
3 checks passed
codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
magento-team pushed a commit that referenced this pull request Jun 21, 2017
…dule #9957

 - cron observer must have only 1 public method
@maqlec

This comment has been minimized.

Copy link
Contributor

maqlec commented Jan 26, 2018

Hi guys, I've installed Magento 2.2 with this change in it and with my schedules strange things are happened. There are often duplicated schedules, created exactly at the same time and scheduled exactly at the same minute. There are not regular. Could you check if this is happening with you? In 2.1 everythig works fine and this commit is a bridge between old and new

@AntonEvers

This comment has been minimized.

Copy link
Contributor Author

AntonEvers commented Feb 4, 2018

@maqlec it is still possible for Magento to run multiple overlapping instances of bin/magento cron:run. Even if you only schedule the cron once a minute, if a cron instance runs for more than a minute they can start overlapping. This could explain the behaviour you're describing.

You could try to apply this patch to eliminate that scenario if you are running linux and have pgrep installed:

diff --git a/Observer/ProcessCronQueueObserver.php b/Observer/ProcessCronQueueObserver.php
index f772a6c0c8..3e3d0ccdce 100644
--- a/Observer/ProcessCronQueueObserver.php
+++ b/Observer/ProcessCronQueueObserver.php
@@ -201,6 +201,13 @@ class ProcessCronQueueObserver implements ObserverInterface
                     ) == 1
                 )
             ) {
+                if (shell_exec(
+                    'pgrep -f -x \'' . $phpPath . ' ' . BP . '/bin/magento cron:run --group=' . $groupId . ' --' . Cli::INPUT_KEY_BOOTSTRAP . '='
+                    . self::STANDALONE_PROCESS_STARTED . '=1\''
+                )) {
+                    continue;
+                }
+
                 $this->_shell->execute(
                     $phpPath . ' %s cron:run --group=' . $groupId . ' --' . Cli::INPUT_KEY_BOOTSTRAP . '='
                     . self::STANDALONE_PROCESS_STARTED . '=1',
@maqlec

This comment has been minimized.

Copy link
Contributor

maqlec commented Feb 4, 2018

I've followed your change and I resolved my problem by moving one line at original place.
I did not inquire why you moved them both above but when the tasks are generated after cron execution, duplicates do not appear.

diff --git a/Observer/ProcessCronQueueObserver.php b/Observer/ProcessCronQueueObserver.php
index f772a6c..bf99dc0 100644
--- a/Observer/ProcessCronQueueObserver.php
+++ b/Observer/ProcessCronQueueObserver.php
@@ -187,7 +187,6 @@ class ProcessCronQueueObserver implements ObserverInterface
 
         foreach ($jobGroupsRoot as $groupId => $jobsRoot) {
             $this->_cleanup($groupId);
-            $this->_generate($groupId);
             if ($this->_request->getParam('group') !== null
                 && $this->_request->getParam('group') !== '\'' . ($groupId) . '\''
                 && $this->_request->getParam('group') !== $groupId
@@ -247,6 +246,7 @@ class ProcessCronQueueObserver implements ObserverInterface
                 }
                 $schedule->save();
             }
+            $this->_generate($groupId);
         }
     }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.