Being able to set backup intervals is great and all, but if backups get queued up and run at a peak time, they can be awful for site performance. I suggest adding backup window settings, and only queuing up backups within that window. It shouldn't be too complicated, as it should probably only be a global setting, and use the hostmaster site's timezone setting. I think we'd just need fields for beginning- and end-times for our window setting.

CommentFileSizeAuthor
#5 allow_setting_backup_window-2154813-5.patch7.66 KBgboudrias
#3 allow_setting_backup_window-2154813-3.patch5.65 KBgboudrias
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

helmo’s picture

helmo CreditAttribution: helmo commented

Yes, sounds reasonable. A global setting should suffice.

I guess the best place would be hosting_backup_queue_queue(), unless the queue API could support a concept of time constraints.

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
CreditAttribution: ergonlogic commented

Agreed, wrapping everything in hosting_backup_queue_queue() in a check against our window parameters should do it. To begin with, a window beginning- and end-time should be sufficient, but it'd eventually be nice to be able to also specify the days of the week (in a multi-value select list, or something) that backups are allowed. That way weekly backups could be scheduled with quite a bit of granularity. The UI could get pretty confusing though...

I think enhancing the queue API in core would probably better be served by making it really pluggable, so something like Jenkins could be used for this. But that'll have to wait for some more significant re-factoring of Aegir core.

gboudrias’s picture

gboudrias CreditAttribution: gboudrias commented
Status: Active » Needs review
FileSize
allow_setting_backup_window-2154813-3.patch5.65 KB

I tried my hand at the proposed implementation. See attached patch.

I planned for this patch to be simple but it got rather long. Maybe it should go into a submodule? Not sure.

The backup manager admin interface would now give an "Illegal string offset" warning when the time variables are set, but it seems to be a problem with the Date API module, so I'm not sure how much hassle it's worth.

Since I used the Date module, I added a dependency to Date API. I'm also not entirely certain it's worth it, but dates are hard enough as it is.

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
CreditAttribution: ergonlogic commented

I think it'd be simpler to just save the time as text and validate that it falls between 00:00 and 23:59. Adding date.module as a dependency seems like real overkill.

Also, I think having this as a sub-module would be best.

gboudrias’s picture

gboudrias CreditAttribution: gboudrias commented
FileSize
allow_setting_backup_window-2154813-5.patch7.66 KB

Okay, changed the date-text to normal textfield with just a little validation (it's so much simpler now!).

Also I put all that in a submodule and made it its own feature.

gboudrias’s picture

gboudrias CreditAttribution: gboudrias commented
gboudrias’s picture

gboudrias CreditAttribution: gboudrias commented

FYI, deployed the patch to a production server, will report if it worked tomorrow.

gboudrias’s picture

gboudrias CreditAttribution: gboudrias commented

Seems to work, please review :)

gboudrias’s picture

gboudrias CreditAttribution: gboudrias commented

This is tested on at least 4 production servers now, I think it's safe to commit it. I can do it myself if you give me access.

helmo’s picture

helmo CreditAttribution: helmo commented
Status: Needs review » Reviewed & tested by the community

Go ahead, you have access.

  • gboudrias committed 8e1cd18 on 6.x-2.x 
    Issue #2154813 by ergonlogic, helmo: Add hosting_backup_window module
gboudrias’s picture

gboudrias CreditAttribution: gboudrias commented
Status: Reviewed & tested by the community » Fixed

Thanks :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.