[pve-devel] [PATCH v2 manager 1/3] fix #1594: add "Run now" button to cluster backup page

[Stefan Reiter]

On 8/7/19 10:31 AM, Dominik Csapak wrote:
> looks mostly ok (did not test it), but some comments inline
> 
> On 8/5/19 4:58 PM, Stefan Reiter wrote:
>> To allow masking to be correctly handled, we use a counter variable to
>> keep the mask visible until all tasks have been completed. Any errors
>> that occurred will be displayed in a consolidated message box.
>>
>> Includes a "confirm" dialog to not accidentally run a potentially 
>> large backup
>> job.
>>
>> Curiously, the "pve-cluster-backup" data model seems to have been broken
>> entirely.  I'm not sure how it was working before, but the changes in
>> this patch need it fixed, so include that as well (use "mode" instead of
>> individual flags + "compress" is not a boolean).
>>
>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>> ---
>>   www/manager6/dc/Backup.js | 92 ++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 85 insertions(+), 7 deletions(-)
>>
>> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
>> index 79e9cace..a26ea63d 100644
>> --- a/www/manager6/dc/Backup.js
>> +++ b/www/manager6/dc/Backup.js
>> @@ -419,6 +419,62 @@ Ext.define('PVE.dc.BackupView', {
>>               win.show();
>>       };
>> +    var run_now_finished = function(errors) {
>> +        me.unmask();
>> +
>> +        if (errors !== undefined && errors.length > 0) {
>> +        Ext.Msg.alert('Error', 'Some errors have been encountered:<br 
>> />---<br />'
>> +            + errors.join('<br />---<br />'));
>> +        }
>> +    }
>> +
>> +    var run_backup_now = function(job) {
>> +        job = Ext.clone(job);
>> +
>> +        var allNodes = PVE.data.ResourceStore.getNodes();
>> +        var jobNode = job.node;
>> +
>> +        // Remove properties related to scheduling
>> +        delete job.enabled;
>> +        delete job.starttime;
>> +        delete job.dow;
>> +        delete job.id;
>> +        delete job.node;
>> +        job.all = job.all === true ? 1 : 0;
>> +
>> +        var errors = [];
>> +        var inProgress = 0;
>> +
>> +        me.mask(gettext('Please wait...'), 'x-mask-loading');
>> +
>> +        allNodes.forEach(node => {
>> +        if (node.status !== 'online' ||
>> +            (jobNode !== undefined && jobNode !== node.node)) {
>> +            return;
>> +        }
>> +
>> +        inProgress++;
>> +        Proxmox.Utils.API2Request({
>> +            url: '/nodes/' + node.node + '/vzdump',
>> +            method: 'POST',
>> +            params: job,
>> +            failure: function (response, opts) {
>> +            errors.push(node.node + ': ' + response.htmlStatus);
>> +            inProgress--;
>> +            if (inProgress == 0) {
>> +                run_now_finished(errors);
> 
> at this point, the user may have switched the panel already and it might 
> seem weird to get an error 'out of nowhere'...
> 
> you could check 'me.destroyed' (see extjs docs) if the current panel 
> still is valid and only show the errors if it is.
> also, if it is not, the 'unmask' in run_now_finished
> will probably fail anyway, so in that case the error will not get shown...
> 

In my testing, the call always returned fast, as long as nothing went 
bad with the connection, so the time span in which the user can click 
away is minimal to begin with.

Adding a check for 'me.destroyed' is a good idea, but the issue remains 
that the user will not be informed of any errors that might have occured 
if they click away.

Any other suggestions? Mask the entire window?

> i really wish the api2request would return a promise, then
> we could simply do async/await here...
> 
>> +            }
>> +            },
>> +            success: function (response, opts) {
>> +            inProgress--;
>> +            if (inProgress == 0) {
>> +                run_now_finished(errors);
>> +            }
>> +            }
>> +        });
>> +        });
>> +    }
>> +
>>       var edit_btn = new Proxmox.button.Button({
>>           text: gettext('Edit'),
>>           disabled: true,
>> @@ -426,6 +482,31 @@ Ext.define('PVE.dc.BackupView', {
>>           handler: run_editor
>>       });
>> +    var run_btn = new Proxmox.button.Button({
>> +        text: gettext('Run now'),
>> +        disabled: true,
>> +        selModel: sm,
>> +        handler: function() {
>> +        var rec = sm.getSelection()[0];
>> +        if (!rec) {
>> +            return;
>> +        }
>> +
>> +        Ext.Msg.show({
>> +            title: gettext('Confirm'),
>> +            icon: Ext.Msg.QUESTION,
>> +            msg: gettext('Start the selected backup job now?'),
>> +            buttons: Ext.Msg.YESNO,
>> +            callback: function(btn) {
>> +            if (btn !== 'yes') {
>> +                return;
>> +            }
>> +            run_backup_now(rec.data);
>> +            }
>> +        });
>> +        }
>> +    });
>> +
>>       var remove_btn = Ext.create('Proxmox.button.StdRemoveButton', {
>>           selModel: sm,
>>           baseurl: '/cluster/backup',
>> @@ -454,7 +535,8 @@ Ext.define('PVE.dc.BackupView', {
>>               }
>>           },
>>           remove_btn,
>> -        edit_btn
>> +        edit_btn,
>> +        run_btn
>>           ],
>>           columns: [
>>           {
>> @@ -576,13 +658,9 @@ Ext.define('PVE.dc.BackupView', {
>>       fields: [
>>           'id', 'starttime', 'dow',
>>           'storage', 'node', 'vmid', 'exclude',
>> -        'mailto', 'pool',
>> +        'mailto', 'pool', 'compress', 'mode',
>>           { name: 'enabled', type: 'boolean' },
>> -        { name: 'all', type: 'boolean' },
>> -        { name: 'snapshot', type: 'boolean' },
>> -        { name: 'stop', type: 'boolean' },
>> -        { name: 'suspend', type: 'boolean' },
>> -        { name: 'compress', type: 'boolean' } > +        { name: 
>> 'all', type: 'boolean' }
>>       ]
>>       });
>>   });
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>