Re: [pve-devel] [PATCH container] fix #2820: block adding new volume with same id if it's pending delete
fabian's variant can be done like this:
---
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0a28380..ba5e548 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1248,6 +1248,9 @@ sub vmconfig_hotplug_pending {
die "skip\n";
}
+ if (exists($conf->{$opt})) {
+ die "skip\n";
+ }
$class->apply_pending_mountpoint($vmid, $conf, $opt, $storecfg,
1);
# apply_pending_mountpoint modifies the value if it creates a
new disk
$value = $conf->{pending}->{$opt};
---
we just check if the mpX is already in the config, if yes then the hotplug is
skipped, adding it as a pending change for the next reboot.
the "replaced" disk becomes unused.
On Wed, Jul 01, 2020 at 02:50:06PM +0200, Thomas Lamprecht wrote:
> On 01.07.20 14:43, Fabian Grünbichler wrote:
> > On July 1, 2020 2:05 pm, Thomas Lamprecht wrote:
> >> On 01.07.20 09:11, Fabian Grünbichler wrote:
> >>> - we can actually just put the new mpX into the pending queue, and
> >>> remove the entry from the pending deletion queue? (it's hotplugging
> >>> that is the problem, not queuing the pending change)
> >>
> >> Even if we could I'm not sure I want to be able to add a new mpX as pending
> >> if the old is still pending its deletion. But, tbh, I did not looked at
> >> details
> >> so I may missing something..
> >
> > well, the sequence is
> >
> > - delete mp0 (queued)
> > - set a new mp0 (queued)
> >
> > just like a general
> >
> > - delete foo (queued)
> > - set foo (queued)
> >
> > where the set removes the queued deletion. in the case of mp, applying
> > that pending change should then add the old volume ID as unused, but
> > that IMHO does not change the semantics of '(queuing a) set overrides
> > earlier queued delete'.
>
> IMO the set mpX isn't your general option setting, and I'd just not allow
> re-setting it with a delete still pending, to dangerous IMO.
> Maybe better make it clear for the user that they either need to apply the
> pending change (e.g., CT reboot), revert it or just use another mpX id.
if this is too dangerous, then i'll instead make a v3, changing the match logic
to work with the parse_pending_delete helper.
which is better?
>
> >
> > but this is broken for regular hotplug without deletion as well, setting
> > mpX with a new volume ID if the slot is already used does not queue it
> > as pending change, but
> > - mounts the new volume ID in addition to the old one
> > - adds the old volume ID as unused, even though it is still mounted in
> > the container
>
> gosh.. yeah that needs to fail too.
>
> >
> > so this is broken in more ways than just what I initially found..
> >
>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] fix #2820: block adding new volume with same id if it's pending delete
On 01.07.20 14:43, Fabian Grünbichler wrote: > On July 1, 2020 2:05 pm, Thomas Lamprecht wrote: >> On 01.07.20 09:11, Fabian Grünbichler wrote: >>> - we can actually just put the new mpX into the pending queue, and >>> remove the entry from the pending deletion queue? (it's hotplugging >>> that is the problem, not queuing the pending change) >> >> Even if we could I'm not sure I want to be able to add a new mpX as pending >> if the old is still pending its deletion. But, tbh, I did not looked at >> details >> so I may missing something.. > > well, the sequence is > > - delete mp0 (queued) > - set a new mp0 (queued) > > just like a general > > - delete foo (queued) > - set foo (queued) > > where the set removes the queued deletion. in the case of mp, applying > that pending change should then add the old volume ID as unused, but > that IMHO does not change the semantics of '(queuing a) set overrides > earlier queued delete'. IMO the set mpX isn't your general option setting, and I'd just not allow re-setting it with a delete still pending, to dangerous IMO. Maybe better make it clear for the user that they either need to apply the pending change (e.g., CT reboot), revert it or just use another mpX id. > > but this is broken for regular hotplug without deletion as well, setting > mpX with a new volume ID if the slot is already used does not queue it > as pending change, but > - mounts the new volume ID in addition to the old one > - adds the old volume ID as unused, even though it is still mounted in > the container gosh.. yeah that needs to fail too. > > so this is broken in more ways than just what I initially found.. > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] fix #2820: block adding new volume with same id if it's pending delete
On July 1, 2020 2:05 pm, Thomas Lamprecht wrote: > On 01.07.20 09:11, Fabian Grünbichler wrote: >> - we can actually just put the new mpX into the pending queue, and >> remove the entry from the pending deletion queue? (it's hotplugging >> that is the problem, not queuing the pending change) > > Even if we could I'm not sure I want to be able to add a new mpX as pending > if the old is still pending its deletion. But, tbh, I did not looked at > details > so I may missing something.. well, the sequence is - delete mp0 (queued) - set a new mp0 (queued) just like a general - delete foo (queued) - set foo (queued) where the set removes the queued deletion. in the case of mp, applying that pending change should then add the old volume ID as unused, but that IMHO does not change the semantics of '(queuing a) set overrides earlier queued delete'. but this is broken for regular hotplug without deletion as well, setting mpX with a new volume ID if the slot is already used does not queue it as pending change, but - mounts the new volume ID in addition to the old one - adds the old volume ID as unused, even though it is still mounted in the container so this is broken in more ways than just what I initially found.. ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] fix #2820: block adding new volume with same id if it's pending delete
On 01.07.20 09:11, Fabian Grünbichler wrote: > - we can actually just put the new mpX into the pending queue, and > remove the entry from the pending deletion queue? (it's hotplugging > that is the problem, not queuing the pending change) Even if we could I'm not sure I want to be able to add a new mpX as pending if the old is still pending its deletion. But, tbh, I did not looked at details so I may missing something.. ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] fix #2820: block adding new volume with same id if it's pending delete
On June 30, 2020 3:56 pm, Oguz Bektas wrote:
> do a simple check to see if our $opt is already in the delete section.
>
> Signed-off-by: Oguz Bektas
> ---
> src/PVE/LXC/Config.pm | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 0a28380..237e2e5 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -974,6 +974,9 @@ sub update_pct_config {
> my $value = $param->{$opt};
> if ($opt =~ m/^mp(\d+)$/ || $opt eq 'rootfs') {
> $class->check_protection($conf, "can't update CT $vmid drive
> '$opt'");
> + if ($conf->{pending}->{delete} =~ m/$opt/) {
this is incomplete:
- $conf->{pending} or $conf->{pending}->{delete} might be undef
- the matching is fuzzy (e.g., there might be a pending deletion of
mp10, and we are currently hotplugging mp1)
- we can actually just put the new mpX into the pending queue, and
remove the entry from the pending deletion queue? (it's hotplugging
that is the problem, not queuing the pending change)
> + die "${opt} is in pending delete queue. please select another
> mountpoint ID\n";
> + }
> my $mp = $class->parse_volume($opt, $value);
> $check_content_type->($mp) if ($mp->{type} eq 'volume');
> } elsif ($opt eq 'hookscript') {
> --
> 2.20.1
>
> ___
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] fix #2820: block adding new volume with same id if it's pending delete
On 30.06.20 15:56, Oguz Bektas wrote:
> do a simple check to see if our $opt is already in the delete section.
>
When can this even happen? If a delete is pending the real property is still
in the config?
> Signed-off-by: Oguz Bektas
> ---
> src/PVE/LXC/Config.pm | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 0a28380..237e2e5 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -974,6 +974,9 @@ sub update_pct_config {
> my $value = $param->{$opt};
> if ($opt =~ m/^mp(\d+)$/ || $opt eq 'rootfs') {
> $class->check_protection($conf, "can't update CT $vmid drive
> '$opt'");
> + if ($conf->{pending}->{delete} =~ m/$opt/) {
so adding 'mp1' matches a pending delete for unrelated 'mp10 ??
> + die "${opt} is in pending delete queue. please select another
> mountpoint ID\n";
Sentences start upper case after a full stop.
> + }
> my $mp = $class->parse_volume($opt, $value);
> $check_content_type->($mp) if ($mp->{type} eq 'volume');
> } elsif ($opt eq 'hookscript') {
>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH container] fix #2820: block adding new volume with same id if it's pending delete
do a simple check to see if our $opt is already in the delete section.
Signed-off-by: Oguz Bektas
---
src/PVE/LXC/Config.pm | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0a28380..237e2e5 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -974,6 +974,9 @@ sub update_pct_config {
my $value = $param->{$opt};
if ($opt =~ m/^mp(\d+)$/ || $opt eq 'rootfs') {
$class->check_protection($conf, "can't update CT $vmid drive
'$opt'");
+ if ($conf->{pending}->{delete} =~ m/$opt/) {
+ die "${opt} is in pending delete queue. please select another
mountpoint ID\n";
+ }
my $mp = $class->parse_volume($opt, $value);
$check_content_type->($mp) if ($mp->{type} eq 'volume');
} elsif ($opt eq 'hookscript') {
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
