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

[Fabian Ebner]

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?

> 
> 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?

> 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
>