Moodle
  1. Moodle
  2. MDL-29470

Automated backup executing 2 days in a row

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Test #1

      1. Go to Settings > Site administration > Courses > Backup > Automated backup setup
      2. Enable it, and schedule it for the same weekday than today only, at 17:00 (if today's Wednesday, only tick Wednesday)
      3. Run the cron and look for 'complete - next execution: ...'
      4. Make sure the date is not set for tomorrow, but for a week later

      (If cron does not output 'complete - next execution...' set 'nextstarttime' to 0 for one row in mdl_backup_courses)

      Test #2

      1. Go to Settings > Site administration > Courses > Backup > Automated backup setup
      2. Enable it. Set weekdays to (Mon, Tue, Wed, Thu, Fri) at 17:00
      3. Enable your PHPUnit environment and run `phpunit lib/tests/backup_test.php`
      4. Make sure the test passes
      5. Go back to the automated backup settings and make sure they didn't change
      Show
      Test #1 Go to Settings > Site administration > Courses > Backup > Automated backup setup Enable it, and schedule it for the same weekday than today only, at 17:00 (if today's Wednesday, only tick Wednesday) Run the cron and look for 'complete - next execution: ...' Make sure the date is not set for tomorrow, but for a week later (If cron does not output 'complete - next execution...' set 'nextstarttime' to 0 for one row in mdl_backup_courses) Test #2 Go to Settings > Site administration > Courses > Backup > Automated backup setup Enable it. Set weekdays to (Mon, Tue, Wed, Thu, Fri) at 17:00 Enable your PHPUnit environment and run `phpunit lib/tests/backup_test.php` Make sure the test passes Go back to the automated backup settings and make sure they didn't change
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29470-master
    • Rank:
      18966

      Description

      We've selected so that automated backup executes every Tuesday and Friday at 11pm. For some reason it executes on Tuesday, Wednesday, Friday, and Saturday at 11pm, or basically it's executing on the next day where it's scheduled as well.

      The config in the database appears to be correct. Meaning the backup_auto_weekdays is set to 0010010 and te time is set to 23.

      I will post what I suspect to be the problem below.

        Issue Links

          Activity

          Hide
          Dimas Ariawan added a comment - - edited

          I suspect it may be cause by the code in /backup/util/helper/backup_cron_helper.class.php

          public static function calculate_next_automated_backup($timezone, $now) {
          .........
          .........
          //Get number of days (from today) to execute backups
          1 $automateddays = substr($config->backup_auto_weekdays,$date['wday']) . $config->backup_auto_weekdays;
          2 $daysfromtoday = strpos($automateddays, "1");
          3 if (empty($daysfromtoday))

          { $daysfromtoday = 1; }

          Let's assume today is Tuesday, the execution day.
          From my example case (0010010), line 1 will give us 100100010010. This comes from 10010 (substr(0010010, 2)) concatenated with 0010010.
          line 2 will then give us 0 (the position of the first "1")
          line 3 will be true because 0 is treated as empty, so daysfromtoday = 1, so it will execute the next day

          Can fellow developers confirm this?

          Show
          Dimas Ariawan added a comment - - edited I suspect it may be cause by the code in /backup/util/helper/backup_cron_helper.class.php public static function calculate_next_automated_backup($timezone, $now) { ......... ......... //Get number of days (from today) to execute backups 1 $automateddays = substr($config->backup_auto_weekdays,$date ['wday'] ) . $config->backup_auto_weekdays; 2 $daysfromtoday = strpos($automateddays, "1"); 3 if (empty($daysfromtoday)) { $daysfromtoday = 1; } Let's assume today is Tuesday, the execution day. From my example case (0010010), line 1 will give us 100100010010. This comes from 10010 (substr(0010010, 2)) concatenated with 0010010. line 2 will then give us 0 (the position of the first "1") line 3 will be true because 0 is treated as empty, so daysfromtoday = 1, so it will execute the next day Can fellow developers confirm this?
          Hide
          Marcello Sette added a comment - - edited

          I experiment the same bug (also in the last version 2.1.3+). This is my manual workaround:

          I suggest to change the line

          if (empty($daysfromtoday))

          { $daysfromtoday = 1; }

          with this line:

          if (empty($daysfromtoday))

          { $daysfromtoday = 7; }

          Bye

          Show
          Marcello Sette added a comment - - edited I experiment the same bug (also in the last version 2.1.3+). This is my manual workaround: I suggest to change the line if (empty($daysfromtoday)) { $daysfromtoday = 1; } with this line: if (empty($daysfromtoday)) { $daysfromtoday = 7; } Bye
          Hide
          Chris Follin added a comment -

          Dimas correctly identified the problem.

          Marcello's suggestion of setting $daysfromtoday = 7 would be a problem for sites that run backups more than once a week. If backups are supposed to run Tuesday and Friday, they will run on Tuesday and then be scheduled for the following Tuesday (7 days later), completely skipping Friday.

          The strpos() call can be changed slightly so that it ignores the first character ("today").

          $daysfromtoday = strpos($automateddays, "1", 1);
          

          That starts the search from the second character (position 1 in the haystack) and ignores today.

          Show
          Chris Follin added a comment - Dimas correctly identified the problem. Marcello's suggestion of setting $daysfromtoday = 7 would be a problem for sites that run backups more than once a week. If backups are supposed to run Tuesday and Friday, they will run on Tuesday and then be scheduled for the following Tuesday (7 days later), completely skipping Friday. The strpos() call can be changed slightly so that it ignores the first character ("today"). $daysfromtoday = strpos($automateddays, "1" , 1); That starts the search from the second character (position 1 in the haystack) and ignores today.
          Hide
          Chris Follin added a comment -

          Attaching patch of suggested modification.

          Show
          Chris Follin added a comment - Attaching patch of suggested modification.
          Hide
          Michael de Raadt added a comment -

          Giving this a nudge.

          Show
          Michael de Raadt added a comment - Giving this a nudge.
          Hide
          Dimas Ariawan added a comment -

          My apologies for not updating this ticket. Our workaround/ fix is the same as what Chris suggested, and our backups have been running as suggested in the past months after applying this change.
          Cheers!

          Show
          Dimas Ariawan added a comment - My apologies for not updating this ticket. Our workaround/ fix is the same as what Chris suggested, and our backups have been running as suggested in the past months after applying this change. Cheers!
          Hide
          Sebastian Tennant added a comment -

          Giving this a second nudge. It seems it's a very simple fix.

          Show
          Sebastian Tennant added a comment - Giving this a second nudge. It seems it's a very simple fix.
          Hide
          Sebastian Tennant added a comment - - edited

          I applied MDL29470.patch yesterday and I can confirm that it HASN'T solved the problem.

          I have automatic backups scheduled for once a week (04:00 on Sunday) and a few hours ago (04:00 on Monday) the backup routine did it's usual thing and backed eveything up for a second time in 24 hours, despite the patch, so it seems it's not a simple fix after all

          Moodle 2.1.6+ (Build: 20120615)

          Show
          Sebastian Tennant added a comment - - edited I applied MDL29470.patch yesterday and I can confirm that it HASN'T solved the problem. I have automatic backups scheduled for once a week (04:00 on Sunday) and a few hours ago (04:00 on Monday) the backup routine did it's usual thing and backed eveything up for a second time in 24 hours, despite the patch, so it seems it's not a simple fix after all Moodle 2.1.6+ (Build: 20120615)
          Hide
          Frédéric Massart added a comment -

          Estimated time: 5 hours

          Show
          Frédéric Massart added a comment - Estimated time: 5 hours
          Hide
          Frédéric Massart added a comment -

          There is my branch including the patch sent by Chris. The diff looks massive because I fixed several white spaces issues.

          Not knowing the naming conventions, or guide lines, I have included some Unit Tests as a second commit.

          I am waiting for your review to backport this fix to 2.2 and 2.3.

          Show
          Frédéric Massart added a comment - There is my branch including the patch sent by Chris. The diff looks massive because I fixed several white spaces issues. Not knowing the naming conventions, or guide lines, I have included some Unit Tests as a second commit. I am waiting for your review to backport this fix to 2.2 and 2.3.
          Hide
          Andrew Davis added a comment -

          This needs some testing instructions. Although this will probably be awkward to test, the testing instructions should NOT simply be to run the new unit tests

          Make double extra sure that the set_config() calls in the unit tests are not altering the actual site config. I'd go so far as to set my site config to different values, run the unit tests then verify that my site settings are unchanged.

          Consider moving "$timezone = $admin->timezone;" up to somewhere near the top of test_next_automated_backup(). Unless I'm misunderstanding what is going on, that doesn't need to be done over and over.

          Looks good though

          Show
          Andrew Davis added a comment - This needs some testing instructions. Although this will probably be awkward to test, the testing instructions should NOT simply be to run the new unit tests Make double extra sure that the set_config() calls in the unit tests are not altering the actual site config. I'd go so far as to set my site config to different values, run the unit tests then verify that my site settings are unchanged. Consider moving "$timezone = $admin->timezone;" up to somewhere near the top of test_next_automated_backup(). Unless I'm misunderstanding what is going on, that doesn't need to be done over and over. Looks good though
          Hide
          Frédéric Massart added a comment -

          Thanks for your feedback Andrew. I have updated the patch and added some testing instructions. I am now pushing for integration. Cheers!

          Show
          Frédéric Massart added a comment - Thanks for your feedback Andrew. I have updated the patch and added some testing instructions. I am now pushing for integration. Cheers!
          Hide
          Sebastian Tennant added a comment - - edited

          Roughly two weeks ago I reported that the patch hadn't fixed the problem. I was mistaken. It has. Since then the patch has been updated and I would like to test the updated patch by pulling from your git repository Frédéric, but you don't have a branch from MOODLE_21_STABLE, which is what I need.

          Could you add a MOODLE_21_STABLE...MDL-29470-22 branch please?

          Could you also confirm that when this fix is accepted upstream, it will make it into MOODLE21_STABLE?

          Show
          Sebastian Tennant added a comment - - edited Roughly two weeks ago I reported that the patch hadn't fixed the problem. I was mistaken. It has. Since then the patch has been updated and I would like to test the updated patch by pulling from your git repository Frédéric, but you don't have a branch from MOODLE_21_STABLE, which is what I need. Could you add a MOODLE_21_STABLE... MDL-29470 -22 branch please? Could you also confirm that when this fix is accepted upstream, it will make it into MOODLE21_STABLE?
          Hide
          Frédéric Massart added a comment -

          Hi Sebastian,

          According to http://docs.moodle.org/dev/Releases#Moodle_2.1 we are not supporting 2.1 any more, except for security issues. But, for your convenience, I have create a branch for 2.1. Keep an eye on this issue to see if the integrators pulled those changes in 2.1 Stable or not.

          Thank you!

          Show
          Frédéric Massart added a comment - Hi Sebastian, According to http://docs.moodle.org/dev/Releases#Moodle_2.1 we are not supporting 2.1 any more, except for security issues. But, for your convenience, I have create a branch for 2.1. Keep an eye on this issue to see if the integrators pulled those changes in 2.1 Stable or not. Thank you!
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          You need to tidy up your <2.3 branches, phpunit wasn't present then, we had simpletest (don't expect you to convert the tests to simpletest, just don't include them).

          Show
          Dan Poltawski added a comment - Hi Fred, You need to tidy up your <2.3 branches, phpunit wasn't present then, we had simpletest (don't expect you to convert the tests to simpletest, just don't include them).
          Hide
          Frédéric Massart added a comment -

          Good point Dan, there you go!

          Show
          Frédéric Massart added a comment - Good point Dan, there you go!
          Hide
          Sebastian Tennant added a comment -

          Thank you Frédéric. I have merged your MDL-29470-21 into my production tree. I will watch to see whether the integrators add your changes to MOODLE_21_STABLE or not. Presumably, if/when they do, I will experience a merge conflict when I try to pull from MOODLE_21_STABLE as part of my regular update routine?

          Show
          Sebastian Tennant added a comment - Thank you Frédéric. I have merged your MDL-29470 -21 into my production tree. I will watch to see whether the integrators add your changes to MOODLE_21_STABLE or not. Presumably, if/when they do, I will experience a merge conflict when I try to pull from MOODLE_21_STABLE as part of my regular update routine?
          Hide
          Frédéric Massart added a comment -

          Yes you will have conflicts. The easier is probably to revert the patch before updating and trying to apply it afterwards. (Especially if I update the patch before they're integrated)

          Show
          Frédéric Massart added a comment - Yes you will have conflicts. The easier is probably to revert the patch before updating and trying to apply it afterwards. (Especially if I update the patch before they're integrated)
          Hide
          Sebastian Tennant added a comment -

          Thanks for the tip Frédéric. I'm still in the process of learning about git so can you confirm that'git reset --hard origin/MOODLE_21_STABLE' is the best way to revert the patch, i.e., 'unmerge' your patch branch?

          Show
          Sebastian Tennant added a comment - Thanks for the tip Frédéric. I'm still in the process of learning about git so can you confirm that'git reset --hard origin/MOODLE_21_STABLE' is the best way to revert the patch, i.e., 'unmerge' your patch branch?
          Hide
          Sam Hemelryk added a comment -

          Thanks Fred, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Fred, this has been integrated now
          Hide
          Frédéric Massart added a comment -

          @Sam Thanks!
          @Sebastian The command `git reset --hard origin/MOODLE_21_STABLE` will revert all your changes to the latest stable we released. This will definitely revert the patch, but this could also revert any changes you made locally. I would recommend you to have a look at `git reflog` which tracks all your actions. The patch you pulled contained two commits, you should see them in `git reflog`. Note down the commit code of the latest action which is not one of my commits (most probably next to HEAD@

          {2}

          , and then use `git reset --hard HereTheCommitReferenceSuchAs61aabeb`. Again, please be careful while using revert --hard!

          Show
          Frédéric Massart added a comment - @Sam Thanks! @Sebastian The command `git reset --hard origin/MOODLE_21_STABLE` will revert all your changes to the latest stable we released. This will definitely revert the patch, but this could also revert any changes you made locally. I would recommend you to have a look at `git reflog` which tracks all your actions. The patch you pulled contained two commits, you should see them in `git reflog`. Note down the commit code of the latest action which is not one of my commits (most probably next to HEAD@ {2} , and then use `git reset --hard HereTheCommitReferenceSuchAs61aabeb`. Again, please be careful while using revert --hard!
          Hide
          Adrian Greeve added a comment -

          I tested this in:

          • 2.1
          • 2.2
          • master

          And I ran the unit test in master.
          The unit test passed with the settings remaining the same, and the cron had the next backup set to the next week.
          No problems encountered.
          Test passed

          Show
          Adrian Greeve added a comment - I tested this in: 2.1 2.2 master And I ran the unit test in master. The unit test passed with the settings remaining the same, and the cron had the next backup set to the next week. No problems encountered. Test passed
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          You've made it into the weekly release!

          Thanks for your contribution - here are some random drummers to keep you inspired for the next week!
          http://www.youtube.com/watch?v=_QhpHUmVCmY

          Show
          Dan Poltawski added a comment - Congratulations! You've made it into the weekly release! Thanks for your contribution - here are some random drummers to keep you inspired for the next week! http://www.youtube.com/watch?v=_QhpHUmVCmY
          Hide
          Sebastian Tennant added a comment -

          @Frédéric Thanks very much for the git tips. git reflog is definitely the way to go.

          Show
          Sebastian Tennant added a comment - @Frédéric Thanks very much for the git tips. git reflog is definitely the way to go.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: