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 <[email protected]>
---
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
[email protected]
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
[email protected]
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
[email protected]
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
[email protected]
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
