* [pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers @ 2021-10-13 12:31 Oguz Bektas 2021-10-13 12:31 ` [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties Oguz Bektas 2021-10-13 12:31 ` [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one Oguz Bektas 0 siblings, 2 replies; 5+ messages in thread From: Oguz Bektas @ 2021-10-13 12:31 UTC (permalink / raw) To: pve-devel v1->v2: * remove other snapshot-related properties during clone * add check when creating new snapshots and abort if new name already a parent for existing snapshot pve-container: Oguz Bektas (1): api: clone_vm: don't include snapshot properties src/PVE/API2/LXC.pm | 5 +++++ 1 file changed, 5 insertions(+) pve-guest-common: Oguz Bektas (1): snapshots: abort if new snapshot name is already parent to existing one src/PVE/AbstractConfig.pm | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties 2021-10-13 12:31 [pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers Oguz Bektas @ 2021-10-13 12:31 ` Oguz Bektas 2021-10-14 10:01 ` [pve-devel] applied: " Thomas Lamprecht 2021-10-13 12:31 ` [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one Oguz Bektas 1 sibling, 1 reply; 5+ messages in thread From: Oguz Bektas @ 2021-10-13 12:31 UTC (permalink / raw) To: pve-devel apparently this caused a weird[0] bug... when a container with a snapshot was cloned, it would take 'parent: foo' from the original container. if you add a new snapshot 'bar', and then another one 'foo', this causes the snapshots to become parents of each other (thus not parsed correctly in the tree view of GUI nor with 'pct listsnapshot CTID') we also drop these properties for VMs, so it makes sense to do the same here as well. [0]: https://forum.proxmox.com/threads/snapshots-of-one-lxc-disappeared.97711/ Signed-off-by: Oguz Bektas <o.bektas@proxmox.com> --- src/PVE/API2/LXC.pm | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm index 1f2f1f0..05cfbad 100644 --- a/src/PVE/API2/LXC.pm +++ b/src/PVE/API2/LXC.pm @@ -1512,7 +1512,12 @@ __PACKAGE__->register_method({ # Replace the 'disk' lock with a 'create' lock. $newconf->{lock} = 'create'; + # delete all snapshot related config options delete $newconf->{snapshots}; + delete $newconf->{parent}; + delete $newconf->{snaptime}; + delete $newconf->{snapstate}; + delete $newconf->{pending}; delete $newconf->{template}; if ($param->{hostname}) { -- 2.30.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] applied: [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties 2021-10-13 12:31 ` [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties Oguz Bektas @ 2021-10-14 10:01 ` Thomas Lamprecht 0 siblings, 0 replies; 5+ messages in thread From: Thomas Lamprecht @ 2021-10-14 10:01 UTC (permalink / raw) To: Proxmox VE development discussion, Oguz Bektas On 13.10.21 14:31, Oguz Bektas wrote: > apparently this caused a weird[0] bug... when a container with a snapshot was > cloned, it would take 'parent: foo' from the original container. if you > add a new snapshot 'bar', and then another one 'foo', this causes the > snapshots to become parents of each other (thus not parsed correctly in > the tree view of GUI nor with 'pct listsnapshot CTID') > > we also drop these properties for VMs, so it makes sense to do the same > here as well. > > [0]: https://forum.proxmox.com/threads/snapshots-of-one-lxc-disappeared.97711/ > > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com> > --- > src/PVE/API2/LXC.pm | 5 +++++ > 1 file changed, 5 insertions(+) > > applied this one already, thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one 2021-10-13 12:31 [pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers Oguz Bektas 2021-10-13 12:31 ` [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties Oguz Bektas @ 2021-10-13 12:31 ` Oguz Bektas 2021-10-14 7:34 ` Fabian Ebner 1 sibling, 1 reply; 5+ messages in thread From: Oguz Bektas @ 2021-10-13 12:31 UTC (permalink / raw) To: pve-devel Signed-off-by: Oguz Bektas <o.bektas@proxmox.com> --- src/PVE/AbstractConfig.pm | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm index 3348d8a..6849664 100644 --- a/src/PVE/AbstractConfig.pm +++ b/src/PVE/AbstractConfig.pm @@ -721,14 +721,22 @@ sub __snapshot_prepare { $conf->{lock} = 'snapshot'; + my $snapshots = $conf->{snapshots}; + die "snapshot name '$snapname' already used\n" - if defined($conf->{snapshots}->{$snapname}); + if defined($snapshots->{$snapname}); my $storecfg = PVE::Storage::config(); die "snapshot feature is not available\n" if !$class->has_feature('snapshot', $conf, $storecfg, undef, undef, $snapname eq 'vzdump'); - $snap = $conf->{snapshots}->{$snapname} = {}; + foreach my $existing_snapshot (keys %$snapshots) { + my $parent_name = $snapshots->{$existing_snapshot}->{parent} // ''; + die "snapshot '$snapname' cannot be used, $snapname already a parent for $existing_snapshot\n" + if $snapname eq $parent_name; + } + + $snap = $snapshots->{$snapname} = {}; if ($save_vmstate && $class->__snapshot_check_running($vmid)) { $class->__snapshot_save_vmstate($vmid, $conf, $snapname, $storecfg); -- 2.30.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one 2021-10-13 12:31 ` [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one Oguz Bektas @ 2021-10-14 7:34 ` Fabian Ebner 0 siblings, 0 replies; 5+ messages in thread From: Fabian Ebner @ 2021-10-14 7:34 UTC (permalink / raw) To: pve-devel, Oguz Bektas Am 13.10.21 um 14:31 schrieb Oguz Bektas: > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com> > --- > src/PVE/AbstractConfig.pm | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm > index 3348d8a..6849664 100644 > --- a/src/PVE/AbstractConfig.pm > +++ b/src/PVE/AbstractConfig.pm > @@ -721,14 +721,22 @@ sub __snapshot_prepare { > > $conf->{lock} = 'snapshot'; > > + my $snapshots = $conf->{snapshots}; > + > die "snapshot name '$snapname' already used\n" > - if defined($conf->{snapshots}->{$snapname}); > + if defined($snapshots->{$snapname}); > > my $storecfg = PVE::Storage::config(); > die "snapshot feature is not available\n" > if !$class->has_feature('snapshot', $conf, $storecfg, undef, undef, $snapname eq 'vzdump'); > > - $snap = $conf->{snapshots}->{$snapname} = {}; > + foreach my $existing_snapshot (keys %$snapshots) { > + my $parent_name = $snapshots->{$existing_snapshot}->{parent} // ''; > + die "snapshot '$snapname' cannot be used, $snapname already a parent for $existing_snapshot\n" > + if $snapname eq $parent_name; > + } Personally, I'd prefer the "automatically fix it up"-approach, because we we're the ones introducing the invalid property in the first place. But if we go for the "error out"-approach, the error message should be different. You're telling the user that the snapshot name cannot be used, but they'll just wonder why. Instead, the error should rather tell the user that manual fix-up is required and that the parent property is invalid, because the parent doesn't exist. > + > + $snap = $snapshots->{$snapname} = {}; > > if ($save_vmstate && $class->__snapshot_check_running($vmid)) { > $class->__snapshot_save_vmstate($vmid, $conf, $snapname, $storecfg); > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-14 10:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-13 12:31 [pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers Oguz Bektas 2021-10-13 12:31 ` [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties Oguz Bektas 2021-10-14 10:01 ` [pve-devel] applied: " Thomas Lamprecht 2021-10-13 12:31 ` [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one Oguz Bektas 2021-10-14 7:34 ` Fabian Ebner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox