[pve-devel] [PATCH container] Remove an unused volume from the config if it is pending to be re-added

[Fabian Grünbichler]

On November 27, 2019 9:39 am, Fabian Ebner wrote:
> On 11/26/19 4:18 PM, Oguz Bektas wrote:
>> hi,
>> 
>> found an issue here:
>> 1 - create a new mp on running container
>> 2 - mp gets added as pending, but disk is not created yet
>> 3 - revert the pending mp
>> 4 - this adds an unused disk, which doesn't exist and can't be removed via GUI
>> 
>> when i try to revert a new mp, 'guests:1'
>> 
>> this is what $mp is:
>> 
>> $VAR1 = {
>> 'type' => 'volume',
>> 'mp' => 'abc',
>> 'volume' => 'guests:1'
>> };
>> 
>> so it passes your checks since it's defined and type is 'volume'
> 
> Thank you for testing. Doing the same thing for a running VM (adding an 
> IDE disk and reverting the pending change) also produces an unused disk 
> that wasn't there before, but it already got allocated, so one can 
> remove it without problems.
> 
> Should we allocate mount points straight away as well?
> Or would it be better to simply mark an unused volume (cross it out with 
> a red line in the GUI) when it is pending to be re-added?

we discussed this when Oguz implemented pending changes for containers, 
and settled on the following approach:

1.) add to pending, without side-effects
2.a) if running, attempt to hot-plug pending changes
2.b) if not running, apply pending changes

vdisk allocation only happens in step 2, not 1. we actually would like 
to make the qemu/VM code path more similar to this, not the other way 
round - it is better to keep config-only and actual changes distinct, as 
it simplifies error-handling and reasoning about the code.

>> you could add another check to see if the disk is new, but then it might break the hotplug mount
>> 
> 
> That should work as well. Please tell me if I'm wrong, but isn't a 
> hotplug mount done straight away and hence doesn't land in the pending 
> section of the config? And so it can't be reverted either?

see above ;)

>> On Tue, Nov 26, 2019 at 12:51:38PM +0100, Fabian Ebner wrote:
>>> This makes the behavior more similar to what we do for VM configs.
>>> If we have a pending change re-adding an unused volume as a mount point, then
>>> the unused volume will not show up anymore. And if we revert such a change, the
>>> volume will re-appear as an unused volume.
>>> Like this a user with a running container won't be able to re-add an unused
>>> volume multiple times via the web GUI.
>>>
>>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>>> ---
>>>   src/PVE/LXC/Config.pm | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
>>> index ffc5911..c2ae166 100644
>>> --- a/src/PVE/LXC/Config.pm
>>> +++ b/src/PVE/LXC/Config.pm
>>> @@ -839,12 +839,14 @@ sub write_pct_config {
>>>       delete $conf->{snapstate}; # just to be sure
>>>   
>>>       my $volidlist = PVE::LXC::Config->get_vm_volumes($conf);
>>> +    push @$volidlist, @{PVE::LXC::Config->get_vm_volumes($conf->{pending})};
>>>       my $used_volids = {};
>>>       foreach my $vid (@$volidlist) {
>>>           $used_volids->{$vid} = 1;
>>>       }
>>>   
>>>       # remove 'unusedX' settings if the volume is still used
>>> +    # or if it's pending to be re-added
>>>       foreach my $key (keys %$conf) {
>>>           my $value = $conf->{$key};
>>>           if ($key =~ m/^unused/ && $used_volids->{$value}) {
>>> @@ -904,6 +906,15 @@ sub update_pct_config {
>>>       my $storage_cfg = PVE::Storage::config();
>>>   
>>>       foreach my $opt (@$revert) {
>>> +	# unused volume that was pending to become a mount point should show up again
>>> +	if ($opt =~ m/^mp(\d+)$/) {
>>> +	    my $mp = eval { $class->parse_ct_mountpoint($conf->{pending}->{$opt}); };
>>> +	    if (defined($mp) && $mp->{type} eq 'volume') {
>>> +		$class->add_unused_volume($conf, $mp->{volume})
>>> +		    if !$class->is_volume_in_use($conf, $conf->{pending}->{$opt}, 1, 0);
>>> +	    }
>>> +	}
>>> +
>>>   	delete $conf->{pending}->{$opt};
>>>   	$class->remove_from_pending_delete($conf, $opt); # also remove from deletion queue
>>>       }
>>> -- 
>>> 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
> 
>