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

[Aaron Lauterer]

On 11/28/19 12:00 PM, Thomas Lamprecht wrote:
> On 11/27/19 5:59 PM, Aaron Lauterer wrote:
>> On 11/27/19 4:50 PM, Thomas Lamprecht wrote:
>>> 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).
> 
> 
> Oh, you're right, I checked the wrong thing - sorry.
> 
>>
>> 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?
>>
> 
> No, but that's also not really solved by the default on check box, IMO.
> If you changed that once you may not remember about that the next week (or day ^^)
> 
> So, additionally to the default on (where we could see if we ensure that both
> VM and CT have the same checkbox name ("Include in Backup"?) and same behavior.
> 
> Then, we could additionally add a mechanism to backup jobs to see what gets backed
> up, i.e.,  the ui could be a window with the VM/CT list as tree with the disks as
> tree-root children - with a symbol/text for if one disk is backed up or not, e.g.
> 
> + VM 101 ...
> `- disk scis1 - included in backup
> `- disk scsi2 - excluded from backup
> 
> + CT 102 ...
> `- rootfs - included in backup
> `- mp0 - included in backup
> `- mp1 - excluded from backup
> 
> (as basic example for what I mean).
> This way one could easily verify the VM/CTs affected by a backup job and further
> see at a glance which disks are skipped or not.
> 

I do like the idea of seeing in the backup jobs panel which parts get 
backed up :)

So my proposal on how to proceed is as follows:

* Send a v2 of this patch with better naming. Unless we come up with 
something better "Include in backup".
* align the VM disk panel to the LXC MP panel:
   - rename checkbox and behavior to include, not exclude from backup
   - enable by default on new disks
   - move from advanced area to normal area?
* enhance backup job view to include a detailed list of disks per guest 
which shows the status, included, not included, not possible to back up 
(bind mounts)