virtualmin / virtualmin-gpl

Virtualmin web hosting control panel for Webmin
https://www.virtualmin.com
GNU General Public License v3.0
376 stars 118 forks source link

Additional post backup status partial #1001

Closed eugenefvdm closed 2 months ago

eugenefvdm commented 2 months ago

This is a proposal set set an additional environment variable to indicate if a backup was a partial success as opposed to a hard success or fail. We are using centralised software to keep track of all backups and it would be useful for our operators to know when backups were partial.

jcameron commented 2 months ago

Thanks for this PR! But wouldn't this change the BACKUP_STATUS environment variable from a number (0 or 1) to a string (ok, failed, partial) ?

eugenefvdm commented 2 months ago

The new string status is set in backup.pl because that's the place where one determines if it's partial.

Then set_backup_envs in backup-libs.pl is called with the string value, where a new environment variable is set

$ENV{'BACKUP_STATUS_CODE'} = $status eq 'ok' ? 1 :
                                $status eq 'partial' ? 2 : 0;

Ideally I would have liked just BACKUP_STATUS instead of a new value.

I might have done something wrong because I don't know the code well enough.

I'll see if I can test this, but I'm not sure how to make a backup fail partially but will try to kill the network during backup.

jcameron commented 2 months ago

So it looks like in the old code, BACKUP_STATUS was set to either 0 or 1 , and BACKUP_STATUS_CODE was not set at all.

Unfortunately the comment on set_backup_envs makes it seem like the status parameter is either "ok" or "failed", when actually it's just a number : https://github.com/virtualmin/virtualmin-gpl/blob/master/backups-lib.pl#L6940

eugenefvdm commented 2 months ago

when actually it's just a number

That is a big clue that the code I submitted will break stuff.

My original idea was in addition to BACKUP_STATUS returning 1 for success, 0 for fail, there is a 2 for partial. I have this on many backups:

image

if [ $BACKUP_STATUS = 1 ]; then r="success"; else r="error"; fi; wget -q "https://backupdashboard.com/api/v1/job?api_token=secret&frequency=daily&type=incremental&status=$r"

I didn't quite find the right place where 0 or 1 is set, and added a 3rd option BACKUP_STATUS_CODE.

Thanks for the input so far! I'm just not experienced enough to get this pull request done, but will give it another try.

In the interest of not wasting your time I'l going to close this pull request so long until I have a better grasp on things.

jcameron commented 2 months ago

Check out this patch, which adds a new environment variable for the names of failed domains : https://github.com/virtualmin/virtualmin-gpl/commit/d1635f0b8bb00697e22969981dd8fa70e374a6e2

eugenefvdm commented 2 months ago

Wow @jcameron this is really great work. I checked it out and your solution is absolutely perfect. Thanks a million!