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.eb...@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@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
>>
>
> _______________________________________________
> 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
