[pve-devel] [PATCH manager] ui: mpedit: activate backup on MP creation

[Aaron Lauterer]

On 11/27/19 4:50 PM, Thomas Lamprecht wrote:
> On 11/27/19 4:05 PM, Aaron Lauterer wrote:
>>
>>
>> On 11/27/19 12:13 PM, Oguz Bektas wrote:
>>> hi,
>>>
>>> tested and works as expected, although one thing i'd change is the
>>> variable name 'isBackedUp'. it sounds a bit like there's already a
>>> backup present for the mp.
>>>
>>> maybe something like 'enableBackup' or similar?
>>
>> What about 'isBackupIncluded'? 'enableBackup' might also be a bit too ambiguous. At that step we do not enable or activate any backup, just set the flag to include this mount point IF a backup job is created.
> 
> maybe "getsBackedUp"
> 
> but not sure about this, it changes consistency with VMs, and we have this
> since multiple years as is. Do you have people actively running into such
> an situation? I mean the "Backup" checkbox is exactly below the required
> "path", so IMO not something to hidden.

AFAIU this will kinda make it consistent with VMs. In VMs right now if I 
  add a new disk in a hurry and only enter the most necessary 
information to be able to click OK it will be included in a backup.

While for LXC MPs the disk of the MP will not be included unless I 
checked the backup checkbox.

For VM disks to not be included in a backup a user needs to activate the 
advanced part of the panel and manually check the "No backup" checkbox 
(a weird pattern by itself).

I did have a case recently where this probably would have helped as the 
container was backed up, but not the mount point.

I would also argue that it is better to have them included in the backup 
by default (when added via the GUI). If the backup gets too big it will 
be noticed and can be acted upon. If the MP is not being backed up it 
will fly under the radar in most situations until it is too late -> 
people realizing that the backup id only partial when they want to restore.

> 
> But I understand where you come from, maybe we could display it more
> visually, e.g. by rendering the backup off setting explicitly as
> 
> +-------+-------------------------------------------------------------+
> | MP... | local:122/vm-122-disk-0.raw,size=4G  (Excluded from Backup) |
> +-------+-------------------------------------------------------------+
> 
> ? not to sure though.

nice but will the user see this if they enable backups later on and 
never look in the ressource panel? Or if they add the container to a 
pool that is backed up?

The thing is, that that little checkbox is easily overlooked when in a 
hurry because there will be no error because of missing data when 
clicking OK like it happen when not having entered a mount path.

> 
>   
>>>
>>> On Wed, Nov 27, 2019 at 11:49:38AM +0100, Aaron Lauterer wrote:
>>>> This patch enables the backup checkbox by default for newly created
>>>> LXC mount points, aligning it with the behaviour when adding disks to
>>>> VMs. There new disks are automatically part of backups. If it is not
>>>> wanted it needs to be actively disabled in the advanced section.
>>>>
>>>> Hopefully this will help to avoid situations in the future where people
>>>> realize too late that the mount point has not been backed up when they
>>>> expected it to because they missed the checkbox.
>>>>
>>>> The reason why `view.isCreate` is not passed directly is because
>>>> AFAIU the 'view.isCreate' can have one of three values:
>>>> * null - editing an existing mount point
>>>> * true - creating a new mp
>>>> * array('unusedX') - adding an unused disk again
>>>>
>>>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>>>> ---
>>>>
>>>> I went with the more verbose `if` because I wasn't sure if a
>>>> vm.set ('isBackedUp', !!view.isCreate);
>>>> would be okay to use.
>>>>
>>>>    www/manager6/lxc/MPEdit.js | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/www/manager6/lxc/MPEdit.js b/www/manager6/lxc/MPEdit.js
>>>> index 780bef0f..954fa44f 100644
>>>> --- a/www/manager6/lxc/MPEdit.js
>>>> +++ b/www/manager6/lxc/MPEdit.js
>>>> @@ -134,6 +134,11 @@ Ext.define('PVE.lxc.MountPointInputPanel', {
>>>>            vm.set('node', view.nodename);
>>>>            vm.set('unpriv', view.unprivileged);
>>>>            vm.set('hideStorSelector', view.unused || !view.isCreate);
>>>> +
>>>> +        // can be array if created from unused disk
>>>> +        if (view.isCreate) {
>>>> +        vm.set('isBackedUp', true);
>>>> +        }
>>>>        }
>>>>        },
>>>>    @@ -243,7 +248,8 @@ Ext.define('PVE.lxc.MountPointInputPanel', {
>>>>            fieldLabel: gettext('Backup'),
>>>>            bind: {
>>>>            hidden: '{isRoot}',
>>>> -        disabled: '{isBindOrRoot}'
>>>> +        disabled: '{isBindOrRoot}',
>>>> +        value: '{isBackedUp}'
>>>>            }
>>>>        }
>>>>        ],
>>>> -- 
>>>> 2.20.1
>>>>
>>>>
>>>> _______________________________________________
>>>> pve-devel mailing list
>>>> pve-devel at pve.proxmox.com
>>>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel at pve.proxmox.com
>>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
> 
>