Moodle
  1. Moodle
  2. MDL-37761

Improve backup/restore within Moodle (e.g. course and activity duplication)

    Details

    • Testing Instructions:
      Hide

      Note: For easiest testing, start with a clean install of Moodle (highly recommended).
      This is because we need to test restoration of de-duplicated files and want to 100% ensure that the files aren't already in use elsewhere in the interface.

      These testing instructions try to test:

      • file-less course import
      • file-less activity duplication
      • that there are no backup regressions
      • that the file existence checks are suitable, specifically:
        • changes to restore from a contenthash work
        • that we don't blow up when a file no longer exists and we can't find it anywhere
      1. Create a new course
      2. Back it up as is (download the backup file)
        • Confirm that the backup log states that the file inclusion setting has been set to '1'
        • Confirm that the backup completed as expected
      3. Turn editing on and add:
        • an image as a label
        • a folder with some files
      4. Back up the course again (download the backup file)
        • Confirm that the backup log states that the file inclusion setting has been set to '1'
      5. Unzip the backup
        • Confirm the files are present in the files directory
        • Confirm that the files.xml file contains appropriate metadata
        • Confirm that moodle_backup.xml incudes a include_files setting of 1
      6. Duplicate the image label
        • Confirm that the backup log states that the file inclusion setting has been set to '0'
        • Confirm that the duplicate label appears correctly
      7. Duplicate the folder
        • Confirm that the backup log states that the file inclusion setting has been set to '0'
        • Confirm that the duplicate folder appears correctly with all files present
      8. Edit backup/backup.php to change the mode from MODE_GENERAL to MODE_SAMESITE
      9. Back up the course again (download the backup file)
        • Confirm that the backup log states that the file inclusion setting has been set to '0'
      10. Unzip the backup
        • Confirm the files are NOT present in the files directory
        • Confirm that the files.xml file contains appropriate metadata
        • Confirm that moodle_backup.xml incudes a include_files setting of 0
      11. Delete the course in it's entirety
      12. Run the attached script (37761-trashpurge.php)
        • WARNING: This removes your trash directory immediately instead of after 4 days
        • Confirm that the files are now gone from the files table (if you started with a fresh site, the table should be mostly empty)
      13. Create a new course
      14. Restore from the most recent backup (the one generated by a hacked backup.php)
        • Confirm that you were shown a warning about missing files
      15. View the course again
        • Confirm that the files are missing
      16. Put all of the files into the course again (just drag/drop them into the course somewhere)
      17. Delete the course again
      18. Create a new course
      19. Restore from the most recent backup (the one generated by a hacked backup.php)
        • Confirm that you were NOT shown a warning about missing files
      20. View the course again
        • Confirm that the files are all present now (they should be restored based upon the contenthash)
      21. Create a new course
      22. Use the Import option to import from the other course
        • Confirm that the backup log states that the file inclusion setting has been set to '0'
      23. View the new course
        • Confirm that all of the files were displayed correctly
      24. Make a backup from an older version of Moodle and a course including some files
      25. Create a new course
      26. Restore from the older backup
        • Confirm that you were NOT shown a warning about missing files
        • Confirm that you were NOT shown any errors (at least none you don't expect)
        • Confirm that the files imported correctly from the backup
      Show
      Note: For easiest testing, start with a clean install of Moodle (highly recommended). This is because we need to test restoration of de-duplicated files and want to 100% ensure that the files aren't already in use elsewhere in the interface. These testing instructions try to test: file-less course import file-less activity duplication that there are no backup regressions that the file existence checks are suitable, specifically: changes to restore from a contenthash work that we don't blow up when a file no longer exists and we can't find it anywhere Create a new course Back it up as is (download the backup file) Confirm that the backup log states that the file inclusion setting has been set to '1' Confirm that the backup completed as expected Turn editing on and add: an image as a label a folder with some files Back up the course again (download the backup file) Confirm that the backup log states that the file inclusion setting has been set to '1' Unzip the backup Confirm the files are present in the files directory Confirm that the files.xml file contains appropriate metadata Confirm that moodle_backup.xml incudes a include_files setting of 1 Duplicate the image label Confirm that the backup log states that the file inclusion setting has been set to ' 0 ' Confirm that the duplicate label appears correctly Duplicate the folder Confirm that the backup log states that the file inclusion setting has been set to ' 0 ' Confirm that the duplicate folder appears correctly with all files present Edit backup/backup.php to change the mode from MODE_GENERAL to MODE_SAMESITE Back up the course again (download the backup file) Confirm that the backup log states that the file inclusion setting has been set to ' 0 ' Unzip the backup Confirm the files are NOT present in the files directory Confirm that the files.xml file contains appropriate metadata Confirm that moodle_backup.xml incudes a include_files setting of 0 Delete the course in it's entirety Run the attached script (37761-trashpurge.php) WARNING: This removes your trash directory immediately instead of after 4 days Confirm that the files are now gone from the files table (if you started with a fresh site, the table should be mostly empty) Create a new course Restore from the most recent backup (the one generated by a hacked backup.php) Confirm that you were shown a warning about missing files View the course again Confirm that the files are missing Put all of the files into the course again (just drag/drop them into the course somewhere) Delete the course again Create a new course Restore from the most recent backup (the one generated by a hacked backup.php) Confirm that you were NOT shown a warning about missing files View the course again Confirm that the files are all present now (they should be restored based upon the contenthash) Create a new course Use the Import option to import from the other course Confirm that the backup log states that the file inclusion setting has been set to ' 0 ' View the new course Confirm that all of the files were displayed correctly Make a backup from an older version of Moodle and a course including some files Create a new course Restore from the older backup Confirm that you were NOT shown a warning about missing files Confirm that you were NOT shown any errors (at least none you don't expect) Confirm that the files imported correctly from the backup
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
    • Rank:
      47488

      Description

      When copying a course (e.g. create + import), and possibly when duplicating an activity, the backup system currently backs up all files to temp and then restores that backup.

      Given we have data deduplication handled by the database, there's no need to actually back the files up and restore them - this just leads to increased disk utilisation for the backup process and the restore process (and possibly additional CPU time to recalculate the checksums).

      We should add a flag to the backup system to indicate when it should not backup files, and when it should not zip backups.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Adding Sam and Eloy as this has come up a couple of times in discussion with them.

          Show
          Andrew Nicols added a comment - Adding Sam and Eloy as this has come up a couple of times in discussion with them.
          Hide
          Andrew Nicols added a comment -

          Summary of discussion from today.

          (17:09:57) sam marshall: the summary is - currently we use some crappy 'export' flag to stop it making the actual zip (even though it's not an export), but it still copies the files into the temp directory. Ideally there should be extra/different flags like 'donotzip' and 'donotcopyfiles'. And this needs to work for restore as well because restore fails if you don't have the files even if they already exist.
          (17:10:47) Andrew Nicols: Have you got that working at the OU, or is this theoretical atm?
          (17:11:16) sam marshall: All theory, we didn't want to do core change at that point.
          (17:11:35) sam marshall: I think it would be relatively easy to change the backup system to support some extra flags like that, but we haven't looked at it in detail.
          (17:12:02) sam marshall: and 'relatively easy' is 'modulo the fact that backup/restore is really, really hard to understand and doesn't have proper phpdoc for most functions' :)
          (17:12:10) Andrew Nicols: heh ;)
          (17:12:55) Andrew Nicols: It's just in the case of importing from course to course really isn't it. I guess theoretically there may be times you want to do it for an actual backup, but chances are that would be a CLI only option
          (17:13:02) Andrew Nicols: with more ? somewhere in there
          (17:13:26) Andrew Nicols: It doesn't happen in the case of duplicating activities does it..?
          (17:13:27) ***Andrew Nicols hopes not
          (17:13:34) sam marshall: We used it for our 'roll forward' report so it's custom code calling the backup/restore controller to do it. (Which is actually surprisingly nice and easy to do, except for not having options.)
          (17:13:37) sam marshall: I think it does happen probably
          (17:13:40) sam marshall: but have not checked
          
          Show
          Andrew Nicols added a comment - Summary of discussion from today. (17:09:57) sam marshall: the summary is - currently we use some crappy 'export' flag to stop it making the actual zip (even though it's not an export), but it still copies the files into the temp directory. Ideally there should be extra/different flags like 'donotzip' and 'donotcopyfiles'. And this needs to work for restore as well because restore fails if you don't have the files even if they already exist. (17:10:47) Andrew Nicols: Have you got that working at the OU, or is this theoretical atm? (17:11:16) sam marshall: All theory, we didn't want to do core change at that point. (17:11:35) sam marshall: I think it would be relatively easy to change the backup system to support some extra flags like that, but we haven't looked at it in detail. (17:12:02) sam marshall: and 'relatively easy' is 'modulo the fact that backup/restore is really, really hard to understand and doesn't have proper phpdoc for most functions' :) (17:12:10) Andrew Nicols: heh ;) (17:12:55) Andrew Nicols: It's just in the case of importing from course to course really isn't it. I guess theoretically there may be times you want to do it for an actual backup, but chances are that would be a CLI only option (17:13:02) Andrew Nicols: with more ? somewhere in there (17:13:26) Andrew Nicols: It doesn't happen in the case of duplicating activities does it..? (17:13:27) ***Andrew Nicols hopes not (17:13:34) sam marshall: We used it for our 'roll forward' report so it's custom code calling the backup/restore controller to do it. (Which is actually surprisingly nice and easy to do, except for not having options.) (17:13:37) sam marshall: I think it does happen probably (17:13:40) sam marshall: but have not checked
          Hide
          Michael de Raadt added a comment -

          This would help overcome a large number of backup/restore problems.

          Show
          Michael de Raadt added a comment - This would help overcome a large number of backup/restore problems.
          Hide
          Michael de Raadt added a comment -

          Coying some content from the duplicate issue:

          Jonathan Moore said:

          Many mission critical installations of Moodle utilize storage file systems that support snapshots. Many larger Moodle installations report they are unable to use the scheduled course backup system because of the load created by generating course backups files. Both IOPs and CPU are used heavily in the process of moving a courses files to the course zip file. Because the zip files are binary blobs backup systems cannot determine that only minor changes have been made to the course. This increase load on the backup system as well and also greatly increases the amount of storage used on both the Moodle production system and the backup system. Scheduled course backups are convenient to provide a self-service means for teachers to restore accidentally deleted / modified courses.

          Solution
          Implement a version of course backup that doesn't copy the course files. Restoration of these "file-less" course backups would use the snapshot copy of the data root from the correct time period, so that file I/O is only generated when restores happen which is much less often.

          Many SANs now implement online access to their snapshots via a .snapshot folder. Some mechanism is needed to define the master path and rules for snapshots so this can work with majority of SANs. An example would be ./snapshot-21022013 fora snapshot taken on February 21st. I can provide more detail on the exact format used for both NetApp and GPFS based snapshots.

          Zipless backup
          Another variation of this concept for large sites not using a SAN would be to use zipless course backups. Then rsync style differential backups could be used to reduce I/O and CPU usage. There would be an increase in file storage used, but many large courses are primarily video and graphic content that doesn't compress well anyway. Hardlink replacement methods could be use to provide multiple points in time. Course backup files would be generated in realtime only when a user attempted to download the file.

          Mike Churchward said:
          We should probably consider the ideas in here as well: http://docs.moodle.org/dev/Perth_Hackfest_October_2012/Asynchronous_Backup_System_spec

          Show
          Michael de Raadt added a comment - Coying some content from the duplicate issue: Jonathan Moore said: Many mission critical installations of Moodle utilize storage file systems that support snapshots. Many larger Moodle installations report they are unable to use the scheduled course backup system because of the load created by generating course backups files. Both IOPs and CPU are used heavily in the process of moving a courses files to the course zip file. Because the zip files are binary blobs backup systems cannot determine that only minor changes have been made to the course. This increase load on the backup system as well and also greatly increases the amount of storage used on both the Moodle production system and the backup system. Scheduled course backups are convenient to provide a self-service means for teachers to restore accidentally deleted / modified courses. Solution Implement a version of course backup that doesn't copy the course files. Restoration of these "file-less" course backups would use the snapshot copy of the data root from the correct time period, so that file I/O is only generated when restores happen which is much less often. Many SANs now implement online access to their snapshots via a .snapshot folder. Some mechanism is needed to define the master path and rules for snapshots so this can work with majority of SANs. An example would be ./snapshot-21022013 fora snapshot taken on February 21st. I can provide more detail on the exact format used for both NetApp and GPFS based snapshots. Zipless backup Another variation of this concept for large sites not using a SAN would be to use zipless course backups. Then rsync style differential backups could be used to reduce I/O and CPU usage. There would be an increase in file storage used, but many large courses are primarily video and graphic content that doesn't compress well anyway. Hardlink replacement methods could be use to provide multiple points in time. Course backup files would be generated in realtime only when a user attempted to download the file. Mike Churchward said: We should probably consider the ideas in here as well: http://docs.moodle.org/dev/Perth_Hackfest_October_2012/Asynchronous_Backup_System_spec
          Hide
          Michael de Raadt added a comment -

          The first of Jonathan's options appears to be preferable, but would be a longer step. In relation to the File API, we should be able to link to duplicated files without copying them; that is what the File API was designed to do.

          Show
          Michael de Raadt added a comment - The first of Jonathan's options appears to be preferable, but would be a longer step. In relation to the File API, we should be able to link to duplicated files without copying them; that is what the File API was designed to do.
          Hide
          Jonathan Moore added a comment -

          I think Petr's suggestion of changing the time that files are kept in the trash was also a good one related to this topic. If the files are kept long enough in the trash then the SAN snapshot feature doesn't need to be relied on for the restores. This would make the restore code easier to implement since there wouldn't have to be any logic to map between various paths that different SAN manufacturers use.

          Show
          Jonathan Moore added a comment - I think Petr's suggestion of changing the time that files are kept in the trash was also a good one related to this topic. If the files are kept long enough in the trash then the SAN snapshot feature doesn't need to be relied on for the restores. This would make the restore code easier to implement since there wouldn't have to be any logic to map between various paths that different SAN manufacturers use.
          Hide
          Sam Marshall added a comment -

          This is related to an earlier request for an option in the backup API to not bother saving files, although not sure it is actually exactly the same.

          Show
          Sam Marshall added a comment - This is related to an earlier request for an option in the backup API to not bother saving files, although not sure it is actually exactly the same.
          Hide
          Andrew Nicols added a comment -

          I've written a patch which skips files at the backup stage when the backup mode is MODE_IMPORT (when importing courses, or duplicating activities).
          It adds a flag to the backup XML file so that it can be correctly handled during the restore.

          At present, this does not remove files from any other type of backup.

          Show
          Andrew Nicols added a comment - I've written a patch which skips files at the backup stage when the backup mode is MODE_IMPORT (when importing courses, or duplicating activities). It adds a flag to the backup XML file so that it can be correctly handled during the restore. At present, this does not remove files from any other type of backup.
          Hide
          Sam Marshall added a comment -

          Not sure I am qualified to do peer review, however I've done one; I looked at the patch and it looks good to me. Trivia:

          1. phpdoc for backup_includes_files is kind of wrong:
          a. Should be @param int $backupid
          b. Should be @return int, or probably @return bool (and should have a description, like @return bool True if backup includes files)

          2. Info variable includes_files; I notice that other variable below it is 'include_...' - this really should be consistent, but maybe it's better to do this one 'right', so not sure. Just flagging it for consideration.

          Regarding the use of MODE_IMPORT, I don't know where is this flag documented - if it is not documented in suitable phpdoc (the execute method?) I suggest adding documentation at least just for that MODE, with TODO for erst. If it is documented already I suggest updating it to make clear that MODE_IMPORT can be used for import, duplicate, and anything else where we are sure the backup is going to be used immediately in the same system, and that (among possibly other things) it has the effect of not including files.

          Show
          Sam Marshall added a comment - Not sure I am qualified to do peer review, however I've done one; I looked at the patch and it looks good to me. Trivia: 1. phpdoc for backup_includes_files is kind of wrong: a. Should be @param int $backupid b. Should be @return int, or probably @return bool (and should have a description, like @return bool True if backup includes files) 2. Info variable includes_files; I notice that other variable below it is 'include_...' - this really should be consistent, but maybe it's better to do this one 'right', so not sure. Just flagging it for consideration. Regarding the use of MODE_IMPORT, I don't know where is this flag documented - if it is not documented in suitable phpdoc (the execute method?) I suggest adding documentation at least just for that MODE, with TODO for erst. If it is documented already I suggest updating it to make clear that MODE_IMPORT can be used for import, duplicate, and anything else where we are sure the backup is going to be used immediately in the same system, and that (among possibly other things) it has the effect of not including files.
          Hide
          Michael de Raadt added a comment -

          Wow! This is really significant, Andrew.

          Thanks for the peer review, Sam. You are certainly qualified, but if you'd like to do this formally, you could follow http://docs.moodle.org/dev/Peer_reviewing_checklist

          Show
          Michael de Raadt added a comment - Wow! This is really significant, Andrew. Thanks for the peer review, Sam. You are certainly qualified, but if you'd like to do this formally, you could follow http://docs.moodle.org/dev/Peer_reviewing_checklist
          Hide
          Sam Marshall added a comment -

          OK, let me try the official checklist.

          Show
          Sam Marshall added a comment - OK, let me try the official checklist.
          Hide
          Andrew Nicols added a comment -

          Cheers, Sam.

          I've corrected the phpdoc as you suggest. The backup is actually a String (the controller is a hash) so I've corrected the docs accordingly.
          WRT the variable name, I was in two minds, but I think you're right. I've changed these to include_files. I've left the backup_includes_files() function because all of the other functions of this nature pluralise the includes for some reason.

          The MODE_IMPORT flag is relatively undocumented - it's defined in backup/backup.class.php. Is that where you'd suggest adding appropriate documentation? I notice that there's also a MODE_SAMESITE flag, which only seems to be used when duplicating a course. I don't know how this differs to an IMPORT, but either it should change to an IMPORT, or we should set include_files to false for SAMESITE. I feel that we could potentially use SAMESITE in the future for other purposes - e.g. automated backups as discussed above, in which case we may want to add a warning on the restore if the backup time was longer ago than the recycle timer.

          Does anyone with deeper backup knowledge (Eloy Lafuente (stronk7)) have a feeling on the SAMESITE/IMPORT query?

          Andrew

          Show
          Andrew Nicols added a comment - Cheers, Sam. I've corrected the phpdoc as you suggest. The backup is actually a String (the controller is a hash) so I've corrected the docs accordingly. WRT the variable name, I was in two minds, but I think you're right. I've changed these to include_files. I've left the backup_includes_files() function because all of the other functions of this nature pluralise the includes for some reason. The MODE_IMPORT flag is relatively undocumented - it's defined in backup/backup.class.php. Is that where you'd suggest adding appropriate documentation? I notice that there's also a MODE_SAMESITE flag, which only seems to be used when duplicating a course. I don't know how this differs to an IMPORT, but either it should change to an IMPORT, or we should set include_files to false for SAMESITE. I feel that we could potentially use SAMESITE in the future for other purposes - e.g. automated backups as discussed above, in which case we may want to add a warning on the restore if the backup time was longer ago than the recycle timer. Does anyone with deeper backup knowledge ( Eloy Lafuente (stronk7) ) have a feeling on the SAMESITE/IMPORT query? Andrew
          Hide
          Sam Marshall added a comment -

          [N] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Re syntax this is just the phpdoc syntax issues above. ALSO NOTE: Not sure I mentioned it last time but I think there should be phpdoc for the newly-added get/set _include_files functions as well as the other function.

          Re testing there are no unit tests but there don't seem to be unit tests anywhere in backup. There is a manual test which seems adequate although not very specific (maybe that increases the chance to fail though...).

          Show
          Sam Marshall added a comment - [N] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Re syntax this is just the phpdoc syntax issues above. ALSO NOTE: Not sure I mentioned it last time but I think there should be phpdoc for the newly-added get/set _include_files functions as well as the other function. Re testing there are no unit tests but there don't seem to be unit tests anywhere in backup. There is a manual test which seems adequate although not very specific (maybe that increases the chance to fail though...).
          Hide
          Sam Marshall added a comment -

          thanks andrew, changes look good. Teensy note - re string, isn't 'string' normally lowercase in phpdoc? (it is entirely possible that I have been living a lie all these years, though...)

          Show
          Sam Marshall added a comment - thanks andrew, changes look good. Teensy note - re string, isn't 'string' normally lowercase in phpdoc? (it is entirely possible that I have been living a lie all these years, though...)
          Hide
          Andrew Nicols added a comment - - edited

          Cheers for that Sam - I've corrected those and added additionald docs for the new getters and setters.

          I'm still looking at the MODE_SAMESITE options - I'm currently looking at and have implemented:

          • allowing SAMESITE backups to not include files
          • adding a precheck to warn users if the backup was made before the last time that filelastcleanup was run
          • improve the restore phase to warn if the file being restored does not exist in the file table

          My understanding of the file_storage code is that:

          • files in the trashdir should still exist in the file table, but won't have a context against them.
          • once a day on cron, the trash delete job is undertaken and:
            • each record without a context is deleted from the file table;
            • a fulldelete is performed on the trashdir; and finally
            • filelastcleanup is set.

          Therefore, for a file to be recovered from trash, it must still exist in the file table.

          The caveat here is that we don't know when the last cleanup actually was. The recommended method for disabling trash cleanups is to put $CFG->fileslastcleanup=time(); into config.php. Therefore for those sites disabling trash purging, filelastcleanup will always be newer than the time of the backup and the warning will always be shown.

          We could just remove the precheck:

          • it is likely to produce false positives; and
          • we warn if an individual file does not exist.

          Again, thoughts on the above appreciated.

          Show
          Andrew Nicols added a comment - - edited Cheers for that Sam - I've corrected those and added additionald docs for the new getters and setters. I'm still looking at the MODE_SAMESITE options - I'm currently looking at and have implemented: allowing SAMESITE backups to not include files adding a precheck to warn users if the backup was made before the last time that filelastcleanup was run improve the restore phase to warn if the file being restored does not exist in the file table My understanding of the file_storage code is that: files in the trashdir should still exist in the file table, but won't have a context against them. once a day on cron, the trash delete job is undertaken and: each record without a context is deleted from the file table; a fulldelete is performed on the trashdir; and finally filelastcleanup is set. Therefore, for a file to be recovered from trash, it must still exist in the file table. The caveat here is that we don't know when the last cleanup actually was . The recommended method for disabling trash cleanups is to put $CFG->fileslastcleanup=time(); into config.php. Therefore for those sites disabling trash purging, filelastcleanup will always be newer than the time of the backup and the warning will always be shown. We could just remove the precheck: it is likely to produce false positives; and we warn if an individual file does not exist. Again, thoughts on the above appreciated.
          Hide
          Andrew Nicols added a comment -

          I also see that there's a MODE_AUTOMATED - I'm not going to exclude files from backups using this mode for the moment.

          Show
          Andrew Nicols added a comment - I also see that there's a MODE_AUTOMATED - I'm not going to exclude files from backups using this mode for the moment.
          Hide
          Andrew Nicols added a comment -

          I've removed the warning that I was originally mooting and updated with all of Sam's suggestions.

          Just trying to write a test plan which adequately tests all cases.

          Show
          Andrew Nicols added a comment - I've removed the warning that I was originally mooting and updated with all of Sam's suggestions. Just trying to write a test plan which adequately tests all cases.
          Hide
          Andrew Nicols added a comment -

          Updated the testing instructions to try and test most (if not all) cases.

          The course duplication has no interface at present to test, but changing the backup to a SAMESITE backup should have the same effect.

          I'm going to go for lunch now, and the run through the testing instructions again from scratch to make sure I've not missed anything.

          Show
          Andrew Nicols added a comment - Updated the testing instructions to try and test most (if not all) cases. The course duplication has no interface at present to test, but changing the backup to a SAMESITE backup should have the same effect. I'm going to go for lunch now, and the run through the testing instructions again from scratch to make sure I've not missed anything.
          Hide
          Sam Marshall added a comment -

          The added documentation is great! If only somebody would do the same for the entire backup system...

          Only one comment, about the send_files_to_pool changes: I think it would be better to use ONLY contenthash and not the file id when searching for the file record. contenthash reliably indicates the precise same file in all cases, but fileid might be different on different sites.

          (I know these restore modes are never supposed to be used on different sites - but it seems like a bad idea to make the code more complicated by allowing both methods, one of which is 100% reliable and the other could be unreliable if something weird happens. Why not just use the 100% one, since we have it?

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y with above comment] Sanity check

          Show
          Sam Marshall added a comment - The added documentation is great! If only somebody would do the same for the entire backup system... Only one comment, about the send_files_to_pool changes: I think it would be better to use ONLY contenthash and not the file id when searching for the file record. contenthash reliably indicates the precise same file in all cases, but fileid might be different on different sites. (I know these restore modes are never supposed to be used on different sites - but it seems like a bad idea to make the code more complicated by allowing both methods, one of which is 100% reliable and the other could be unreliable if something weird happens. Why not just use the 100% one, since we have it? [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y with above comment] Sanity check
          Hide
          Andrew Nicols added a comment -

          I wrote a patch to make filepurgeage configurable for 15 months ago for 2.2. Details in MDL-31645.

          Show
          Andrew Nicols added a comment - I wrote a patch to make filepurgeage configurable for 15 months ago for 2.2. Details in MDL-31645 .
          Hide
          Andrew Nicols added a comment -

          Thanks for the idea Sam - I'll implement that. I'm inclined to agree.

          Show
          Andrew Nicols added a comment - Thanks for the idea Sam - I'll implement that. I'm inclined to agree.
          Hide
          Andrew Nicols added a comment -

          Pushed a change to switch to contenthash-only.

          I think that this is pretty much ready for integration.

          Show
          Andrew Nicols added a comment - Pushed a change to switch to contenthash-only. I think that this is pretty much ready for integration.
          Hide
          Sam Marshall added a comment -

          I agree this is ready for integration!

          Show
          Sam Marshall added a comment - I agree this is ready for integration!
          Hide
          Sam Marshall added a comment -

          Oh, one thing - I submitted for integration since you said it was ready and it's been reviewed - but, is the intent (as per bug options) to make this change for 2.4 and 2.5 as well as master? If so it would be a good idea to create the extra branches so as to check there aren't conflicts etc.

          Show
          Sam Marshall added a comment - Oh, one thing - I submitted for integration since you said it was ready and it's been reviewed - but, is the intent (as per bug options) to make this change for 2.4 and 2.5 as well as master? If so it would be a good idea to create the extra branches so as to check there aren't conflicts etc.
          Hide
          Andrew Nicols added a comment -

          Thanks Sam,

          IMO, if the desire is to backport this, that should be done as a backport issue. This is an improvement rather than a bugfix.

          Andrew

          Show
          Andrew Nicols added a comment - Thanks Sam, IMO, if the desire is to backport this, that should be done as a backport issue. This is an improvement rather than a bugfix. Andrew
          Hide
          Sam Marshall added a comment -

          Andrew - OK, should the 'affects version' (& corresponding branch) for this issue be changed to 2.6 then?

          Show
          Sam Marshall added a comment - Andrew - OK, should the 'affects version' (& corresponding branch) for this issue be changed to 2.6 then?
          Hide
          Andrew Nicols added a comment -

          It affects older versions, but it won't be fixed until 2.6.
          The branch is correct (it's on master already).

          Show
          Andrew Nicols added a comment - It affects older versions, but it won't be fixed until 2.6. The branch is correct (it's on master already).
          Hide
          Andrew Nicols added a comment -

          Rebased on latest latest master.

          Show
          Andrew Nicols added a comment - Rebased on latest latest master.
          Hide
          Dan Poltawski added a comment -

          1. Sam wrote:

          Re testing there are no unit tests but there don't seem to be unit tests anywhere in backup. There is a manual test which seems adequate although not very specific (maybe that increases the chance to fail though...).

          Urm, what about:

          backup/controller/tests/controller_test.php
          backup/converter/moodle1/tests/lib_test.php
          backup/util/checks/tests/checks_test.php
          backup/util/dbops/tests/dbops_test.php
          backup/util/destinations/tests/destinations_test.php
          backup/util/factories/tests/factories_test.php
          backup/util/helper/tests/converterhelper_test.php
          backup/util/helper/tests/cronhelper_test.php
          backup/util/helper/tests/decode_test.php
          backup/util/helper/tests/helper_test.php
          backup/util/loggers/tests/logger_test.php
          backup/util/plan/tests/plan_test.php
          backup/util/plan/tests/step_test.php
          backup/util/plan/tests/task_test.php
          backup/util/settings/tests/settings_test.php
          backup/util/structure/tests/baseatom_test.php
          backup/util/structure/tests/baseattribute_test.php
          backup/util/structure/tests/basefinalelement_test.php
          backup/util/structure/tests/basenestedelement_test.php
          backup/util/structure/tests/baseoptiogroup_test.php
          backup/util/structure/tests/structure_test.php
          backup/util/ui/tests/ui_test.php
          backup/util/xml/output/tests/output_test.php
          backup/util/xml/parser/tests/parser_test.php
          backup/util/xml/tests/writer_test.php
          

          (I don't know if there are suitable places to add unit tests, but please could you investigate)

          2. I think you mean bool in multiple places with this comment: @return int Indicates whether files should be included in backups.

          3. This comment seems to be misleading and not to match the code:

          // In an import, we don't need to include files.
                  if ($this->get_mode() === backup::MODE_SAMESITE) 
          

          4. Your comemnt in the code "Files should still exist in the file table until they are removed from the trash directory.", worries me, it suggests that there might be a possibility of files being cleaned up. But from what I can see of the code you are only enabling this for duplicating live courses. Am I missing something?

          5. MODE_SAMESITE is intriguing, I don't quite follow why the webservce is using this mode.

          I'm reopening for now, for those relatively trivial issues. I see you have tried to get Eloy onto this a few times and I will just acknowledge that i'd like to discuss this change with him before integrating (and i'll corner him)

          Thanks a lot for your work on this issue!

          Dan

          Show
          Dan Poltawski added a comment - 1. Sam wrote: Re testing there are no unit tests but there don't seem to be unit tests anywhere in backup. There is a manual test which seems adequate although not very specific (maybe that increases the chance to fail though...). Urm, what about: backup/controller/tests/controller_test.php backup/converter/moodle1/tests/lib_test.php backup/util/checks/tests/checks_test.php backup/util/dbops/tests/dbops_test.php backup/util/destinations/tests/destinations_test.php backup/util/factories/tests/factories_test.php backup/util/helper/tests/converterhelper_test.php backup/util/helper/tests/cronhelper_test.php backup/util/helper/tests/decode_test.php backup/util/helper/tests/helper_test.php backup/util/loggers/tests/logger_test.php backup/util/plan/tests/plan_test.php backup/util/plan/tests/step_test.php backup/util/plan/tests/task_test.php backup/util/settings/tests/settings_test.php backup/util/structure/tests/baseatom_test.php backup/util/structure/tests/baseattribute_test.php backup/util/structure/tests/basefinalelement_test.php backup/util/structure/tests/basenestedelement_test.php backup/util/structure/tests/baseoptiogroup_test.php backup/util/structure/tests/structure_test.php backup/util/ui/tests/ui_test.php backup/util/xml/output/tests/output_test.php backup/util/xml/parser/tests/parser_test.php backup/util/xml/tests/writer_test.php (I don't know if there are suitable places to add unit tests, but please could you investigate) 2. I think you mean bool in multiple places with this comment: @return int Indicates whether files should be included in backups. 3. This comment seems to be misleading and not to match the code: // In an import , we don't need to include files. if ($ this ->get_mode() === backup::MODE_SAMESITE) 4. Your comemnt in the code "Files should still exist in the file table until they are removed from the trash directory.", worries me, it suggests that there might be a possibility of files being cleaned up. But from what I can see of the code you are only enabling this for duplicating live courses. Am I missing something? 5. MODE_SAMESITE is intriguing, I don't quite follow why the webservce is using this mode. I'm reopening for now, for those relatively trivial issues. I see you have tried to get Eloy onto this a few times and I will just acknowledge that i'd like to discuss this change with him before integrating (and i'll corner him) Thanks a lot for your work on this issue! Dan
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Nicols added a comment -

          Cheers Dan,

          2: nope - this is an integer. That fits with other parts of the backup code where other boolean-like states are instead stored as integers. I suspect that this could be to make parsing the XML file less confused somehow.

          3: thanks - have updated that comment

          4: There shouldn't be any case where a file is removed from the trash directory before the file is removed from the files table, but it is possible for a file to be removed from the files table and have the trash directory preserved. That's what the message is trying to encapsulate. I'll see if I can re-word it to clarify further.

          5: I don't see anywhere that the webservice is using MODE_SAMESITE - where are you seeing it? I see it used twice in course/externallib.php::duplicate_course() but nowhere else.

          Show
          Andrew Nicols added a comment - Cheers Dan, 2: nope - this is an integer. That fits with other parts of the backup code where other boolean-like states are instead stored as integers. I suspect that this could be to make parsing the XML file less confused somehow. 3: thanks - have updated that comment 4: There shouldn't be any case where a file is removed from the trash directory before the file is removed from the files table, but it is possible for a file to be removed from the files table and have the trash directory preserved. That's what the message is trying to encapsulate. I'll see if I can re-word it to clarify further. 5: I don't see anywhere that the webservice is using MODE_SAMESITE - where are you seeing it? I see it used twice in course/externallib.php::duplicate_course() but nowhere else.
          Hide
          Dan Poltawski added a comment -

          2: nope - this is an integer. That fits with other parts of the backup code where other boolean-like states are instead stored as integers. I suspect that this could be to make parsing the XML file less confused somehow.

          Ah, sorry my mistake, I missed the:

          $this->includefiles = (int) $includefiles;
          

          Makes sense to copy exist style, though for the record I think thats ugly and if that is the case so we should do it at the xml writing stage

          5: I don't see anywhere that the webservice is using MODE_SAMESITE - where are you seeing it? I see it used twice in course/externallib.php::duplicate_course() but nowhere else.

          externalib.php is the file we use for web service functions, so that is what you see http://docs.moodle.org/dev/External_services_description#externallib.php

          Show
          Dan Poltawski added a comment - 2: nope - this is an integer. That fits with other parts of the backup code where other boolean-like states are instead stored as integers. I suspect that this could be to make parsing the XML file less confused somehow. Ah, sorry my mistake, I missed the: $ this ->includefiles = ( int ) $includefiles; Makes sense to copy exist style, though for the record I think thats ugly and if that is the case so we should do it at the xml writing stage 5: I don't see anywhere that the webservice is using MODE_SAMESITE - where are you seeing it? I see it used twice in course/externallib.php::duplicate_course() but nowhere else. externalib.php is the file we use for web service functions, so that is what you see http://docs.moodle.org/dev/External_services_description#externallib.php
          Hide
          Andrew Nicols added a comment -

          5: I'm not sure why the duplicate_course code uses MODE_SAMESITE instead of MODE_IMPORT. Sam Marshall and I were discussing it in private chat. Perhaps this should be changed to MODE_IMPORT instead, but that is probably for another issue really and is outside the scope of this one.

          Show
          Andrew Nicols added a comment - 5: I'm not sure why the duplicate_course code uses MODE_SAMESITE instead of MODE_IMPORT. Sam Marshall and I were discussing it in private chat. Perhaps this should be changed to MODE_IMPORT instead, but that is probably for another issue really and is outside the scope of this one.
          Hide
          Andrew Nicols added a comment -

          I've updated the comments for the parts of the patch you mentioned, and I've removed a little bit of duplicated code that I came across (the $file_record was duplicated three times with identical contents).

          Show
          Andrew Nicols added a comment - I've updated the comments for the parts of the patch you mentioned, and I've removed a little bit of duplicated code that I came across (the $file_record was duplicated three times with identical contents).
          Hide
          Andrew Nicols added a comment -

          I've updated the branch, adding some unit tests too.

          Show
          Andrew Nicols added a comment - I've updated the branch, adding some unit tests too.
          Hide
          Andrew Nicols added a comment -

          I've run through all of the tests again just to make certain all is still good.

          Show
          Andrew Nicols added a comment - I've run through all of the tests again just to make certain all is still good.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi, some background about MODE_SAMESITE:

          1) Initially, the "samesite" thingy (that is used here and there on restore to decide multiple things), was planned as a mode (MODE_SAMESITE). But later it was reconsidered and restore_controller::is_samesite() was created to address that and the mode was abandoned. Unluckily I missed to erase the constant.

          2) Later, when some backup&restore WSs were being implemented (MDL-32233...) there were some problems both with permissions (MODE_IMPORT requires extra perms) and functionalities (MODE_GENERAL does "too much") so I suggested to Jerome/Juan Leyva to use that "legacy" mode that was free from those problems.

          So, surely... we should consider, for 2.6 to:

          1) For this issue, ignore completely that mode.
          2) Delete that mode and change current uses (WS) to something else.
          3) Surely, consider splitting the MODE and some features like (fileless y/n and zip y/n). By default each mode can have those features set to a value, but we should be able to handle them separately. That or create new MODES, to cover all the combinations. Although I think I prefer the split and keep the MODEs list simple.

          All this said without looking to the patch here... just for reference... going to look now.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi, some background about MODE_SAMESITE: 1) Initially, the "samesite" thingy (that is used here and there on restore to decide multiple things), was planned as a mode (MODE_SAMESITE). But later it was reconsidered and restore_controller::is_samesite() was created to address that and the mode was abandoned. Unluckily I missed to erase the constant. 2) Later, when some backup&restore WSs were being implemented ( MDL-32233 ...) there were some problems both with permissions (MODE_IMPORT requires extra perms) and functionalities (MODE_GENERAL does "too much") so I suggested to Jerome/Juan Leyva to use that "legacy" mode that was free from those problems. So, surely... we should consider, for 2.6 to: 1) For this issue, ignore completely that mode. 2) Delete that mode and change current uses (WS) to something else. 3) Surely, consider splitting the MODE and some features like (fileless y/n and zip y/n). By default each mode can have those features set to a value, but we should be able to handle them separately. That or create new MODES, to cover all the combinations. Although I think I prefer the split and keep the MODEs list simple. All this said without looking to the patch here... just for reference... going to look now. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, some more extra background about why we ended using the horrible MODE_SAMESITE is that we wanted, in the duplicate_course WS, to be able to decide if the operation should include user information or no (aka, duplicate posts, grades and so on...).

          Initially, based on that setting, the WS was using MODE_IMPORT (no user info ever) or MODE_SAMESITE (user info included), but we decided to always use the same mode (SAMESITE).

          As said, it's a mode that surely we could take rid of (if we decouple the user/files/zip bits). But it does not need to be done here. So it's correct to assume, like the patch does, that SAMESITE is also an "immediate" backup&restore (like IMPORT is) and it's safe to be done without files&zip.

          About the patch... surely the only tiny bit I don't like... is the call to load_controller() happening within send_files_to_pool(). It should not be loaded there coz it causes loggers to be _wakeup() and other bits that may lead to problems. Alternatively I'd try one of this:

          1) Or we use backup_general_helper::get_backup_information($tempdir) => I think $basepath is $tempdir.
          2) Or we add a new param to the function.

          Everything else looks correct, new tests are welcome and the testing instructions seem to cover all combinations (perhaps restoring an old course with all debugs enabled is missing, just to verify no notices/warn happen and the include_files is set for them).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, some more extra background about why we ended using the horrible MODE_SAMESITE is that we wanted, in the duplicate_course WS, to be able to decide if the operation should include user information or no (aka, duplicate posts, grades and so on...). Initially, based on that setting, the WS was using MODE_IMPORT (no user info ever) or MODE_SAMESITE (user info included), but we decided to always use the same mode (SAMESITE). As said, it's a mode that surely we could take rid of (if we decouple the user/files/zip bits). But it does not need to be done here. So it's correct to assume, like the patch does, that SAMESITE is also an "immediate" backup&restore (like IMPORT is) and it's safe to be done without files&zip. About the patch... surely the only tiny bit I don't like... is the call to load_controller() happening within send_files_to_pool(). It should not be loaded there coz it causes loggers to be _wakeup() and other bits that may lead to problems. Alternatively I'd try one of this: 1) Or we use backup_general_helper::get_backup_information($tempdir) => I think $basepath is $tempdir. 2) Or we add a new param to the function. Everything else looks correct, new tests are welcome and the testing instructions seem to cover all combinations (perhaps restoring an old course with all debugs enabled is missing, just to verify no notices/warn happen and the include_files is set for them). Thanks!
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          Please could you address Eloys comments? If you can do it quick, I will be able to get this integrated for this weeks testing - thanks!

          Show
          Dan Poltawski added a comment - Hi Andrew, Please could you address Eloys comments? If you can do it quick, I will be able to get this integrated for this weeks testing - thanks!
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Nicols added a comment -

          Thanks Eloy Lafuente (stronk7), looking at backup_general_helper, I think it probably makes sense to go with using get_backup_information, though that does reparse the XML (which isn't a problem as such really).

          I'll run through the tests again shortly and will add a test to cover restoration of an old backup file (sorry, I had meant to add this); then I'll resubmit for integration.

          Thanks both!

          Show
          Andrew Nicols added a comment - Thanks Eloy Lafuente (stronk7) , looking at backup_general_helper, I think it probably makes sense to go with using get_backup_information, though that does reparse the XML (which isn't a problem as such really). I'll run through the tests again shortly and will add a test to cover restoration of an old backup file (sorry, I had meant to add this); then I'll resubmit for integration. Thanks both!
          Hide
          Andrew Nicols added a comment -

          I've made the changes as discussed by Eloy. The tempdir to be passed is actually the basename of $basepath rather than $basepath itself.

          I've run through all of the test instructions again, and I've added a note about testing a restoration from an older version which does not include the include_files setting (thanks Eloy Lafuente (stronk7)).

          I've also taken the liberty of rebasing on this week's master.

          Show
          Andrew Nicols added a comment - I've made the changes as discussed by Eloy. The tempdir to be passed is actually the basename of $basepath rather than $basepath itself. I've run through all of the test instructions again, and I've added a note about testing a restoration from an older version which does not include the include_files setting (thanks Eloy Lafuente (stronk7) ). I've also taken the liberty of rebasing on this week's master.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks Andrew

          Show
          Dan Poltawski added a comment - Integrated to master, thanks Andrew
          Hide
          David Monllaó added a comment -

          It passes, tested using a 2.4 backup file.

          The only thing I've notices is than when I duplicate the label and the folder I see two new logs in moodledata/temp/backup/ with different contents; setting file inclusion is 0 in both label and folder. Copying the file contents for the record.

          [Wed 19 Jun 2013 13:05:14 WST] [info] instantiating restore controller db5f2a78269c498524d534cbaae332b5
          [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 100
          [Wed 19 Jun 2013 13:05:14 WST] [debug] loading backup info
          [Wed 19 Jun 2013 13:05:14 WST] [debug] loading controller plan
          [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 300
          [Wed 19 Jun 2013 13:05:14 WST] [info] checking plan security
          [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 600
          [Wed 19 Jun 2013 13:05:14 WST] [debug] saving controller to db
          [Wed 19 Jun 2013 13:05:14 WST] [debug] calculating controller checksum fc8bb2d0f959360dcbbfa341cd1c042d
          [Wed 19 Jun 2013 13:05:14 WST] [debug] loading controller from db
          [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 700
          [Wed 19 Jun 2013 13:05:14 WST] [debug] saving controller to db
          [Wed 19 Jun 2013 13:05:14 WST] [debug] calculating controller checksum fa205d114248e9182e36b68875754a21
          [Wed 19 Jun 2013 13:05:14 WST] [debug] loading controller from db
          [Wed 19 Jun 2013 13:05:14 WST] [debug] notifying plan about excluded activities by type
          [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 800
          [Wed 19 Jun 2013 13:05:14 WST] [info] processing file aliases queue
          [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 1000
          [Wed 19 Jun 2013 13:05:14 WST] [debug] saving controller to db
          
          [Wed 19 Jun 2013 13:05:13 WST] [info] instantiating backup controller 2056ac1dcab923f9315590da07198ff6
          [Wed 19 Jun 2013 13:05:13 WST] [debug] setting controller status to 100
          [Wed 19 Jun 2013 13:05:13 WST] [debug] loading controller plan
          [Wed 19 Jun 2013 13:05:13 WST] [debug] setting controller status to 300
          [Wed 19 Jun 2013 13:05:13 WST] [debug] applying plan defaults
          [Wed 19 Jun 2013 13:05:13 WST] [debug] setting controller status to 400
          [Wed 19 Jun 2013 13:05:13 WST] [debug] setting file inclusion to 0
          [Wed 19 Jun 2013 13:05:13 WST] [info] checking plan security
          [Wed 19 Jun 2013 13:05:13 WST] [debug] setting controller status to 700
          [Wed 19 Jun 2013 13:05:13 WST] [debug] saving controller to db
          [Wed 19 Jun 2013 13:05:13 WST] [debug] calculating controller checksum 0a0369408e18410cb7fa70048bf40d3b
          [Wed 19 Jun 2013 13:05:14 WST] [debug] loading controller from db
          [Wed 19 Jun 2013 13:05:14 WST] [debug] notifying plan about excluded activities by type
          [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 800
          [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 1000
          [Wed 19 Jun 2013 13:05:14 WST] [debug] saving controller to db
          
          Show
          David Monllaó added a comment - It passes, tested using a 2.4 backup file. The only thing I've notices is than when I duplicate the label and the folder I see two new logs in moodledata/temp/backup/ with different contents; setting file inclusion is 0 in both label and folder. Copying the file contents for the record. [Wed 19 Jun 2013 13:05:14 WST] [info] instantiating restore controller db5f2a78269c498524d534cbaae332b5 [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 100 [Wed 19 Jun 2013 13:05:14 WST] [debug] loading backup info [Wed 19 Jun 2013 13:05:14 WST] [debug] loading controller plan [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 300 [Wed 19 Jun 2013 13:05:14 WST] [info] checking plan security [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 600 [Wed 19 Jun 2013 13:05:14 WST] [debug] saving controller to db [Wed 19 Jun 2013 13:05:14 WST] [debug] calculating controller checksum fc8bb2d0f959360dcbbfa341cd1c042d [Wed 19 Jun 2013 13:05:14 WST] [debug] loading controller from db [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 700 [Wed 19 Jun 2013 13:05:14 WST] [debug] saving controller to db [Wed 19 Jun 2013 13:05:14 WST] [debug] calculating controller checksum fa205d114248e9182e36b68875754a21 [Wed 19 Jun 2013 13:05:14 WST] [debug] loading controller from db [Wed 19 Jun 2013 13:05:14 WST] [debug] notifying plan about excluded activities by type [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 800 [Wed 19 Jun 2013 13:05:14 WST] [info] processing file aliases queue [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 1000 [Wed 19 Jun 2013 13:05:14 WST] [debug] saving controller to db [Wed 19 Jun 2013 13:05:13 WST] [info] instantiating backup controller 2056ac1dcab923f9315590da07198ff6 [Wed 19 Jun 2013 13:05:13 WST] [debug] setting controller status to 100 [Wed 19 Jun 2013 13:05:13 WST] [debug] loading controller plan [Wed 19 Jun 2013 13:05:13 WST] [debug] setting controller status to 300 [Wed 19 Jun 2013 13:05:13 WST] [debug] applying plan defaults [Wed 19 Jun 2013 13:05:13 WST] [debug] setting controller status to 400 [Wed 19 Jun 2013 13:05:13 WST] [debug] setting file inclusion to 0 [Wed 19 Jun 2013 13:05:13 WST] [info] checking plan security [Wed 19 Jun 2013 13:05:13 WST] [debug] setting controller status to 700 [Wed 19 Jun 2013 13:05:13 WST] [debug] saving controller to db [Wed 19 Jun 2013 13:05:13 WST] [debug] calculating controller checksum 0a0369408e18410cb7fa70048bf40d3b [Wed 19 Jun 2013 13:05:14 WST] [debug] loading controller from db [Wed 19 Jun 2013 13:05:14 WST] [debug] notifying plan about excluded activities by type [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 800 [Wed 19 Jun 2013 13:05:14 WST] [debug] setting controller status to 1000 [Wed 19 Jun 2013 13:05:14 WST] [debug] saving controller to db
          Hide
          Andrew Nicols added a comment -

          Thanks David Monllaó. When you duplicate an activity, you're effectively backing it up, and immediately restoring it. The second log is the backup log, and the first one is the restore log, so I think that this is the expected behaviour.

          Cheers.

          Show
          Andrew Nicols added a comment - Thanks David Monllaó . When you duplicate an activity, you're effectively backing it up, and immediately restoring it. The second log is the backup log, and the first one is the restore log, so I think that this is the expected behaviour. Cheers.
          Hide
          Dan Poltawski added a comment -

          Thanks for your contributions!

          _main:
          @ BB#0:
                  push    {r7, lr}
                  mov     r7, sp
                  sub     sp, #4
                  movw    r0, :lower16:(L_.str-(LPC0_0+4))
                  movt    r0, :upper16:(L_.str-(LPC0_0+4))
          LPC0_0:
                  add     r0, pc
                  bl      _printf
                  movs    r1, #0
                  movt    r1, #0
                  str     r0, [sp]                @ 4-byte Spill
                  mov     r0, r1
                  add     sp, #4
                  pop     {r7, pc}
          
                  .section        __TEXT,__cstring,cstring_literals
          L_.str:                                 @ @.str
                  .asciz   "This code is now upstream!"
          
          Show
          Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4- byte Spill mov r0, r1 add sp, #4 pop {r7, pc} .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: