[pve-devel] [PATCH container] create_vm: fix order of config creation/reading/locking

[Fabian Ebner]

On 30.04.20 08:59, Fabian Grünbichler wrote:
> On April 29, 2020 11:58 am, Fabian Ebner wrote:
>> The update_pct_config call leads to a write_config call and so the
>> configuration file was created before it was intended to be created.
>>
>> When the CFS is updated in between the write_config call and the
>> PVE::Cluster::check_vmid_unused call in create_and_lock_config,
>> the container file would already exist and so creation would
>> fail after writing out a basically empty config.
>> (Meaning this is necessary for the proposed "update CFS after
>> obtaining a configuration file lock" changes)
>>
>> Even worse, a race was possible for two containers created with the
>> same ID at the same time:
>> Assuming the initial PVE::Cluster::check_vmid_unused check in the
>> parameter verification passes for both create_vm calls, the later one
>> would potentially overwrite the earlier configuration file with its
>> update_pct_config call.
>>
>> Additionally, the file read for $old_config was always the one written
>> by update_pct_config. Meaning that for a create_vm call with force=1,
>> already existing old volumes were not removed.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> 
> hmm, update_pct_config there was not intended to write out the config,
> but just to fill the $conf hash.
> 
> three alternative approaches that would return to the original semantics:
> 1 skip hotplug/apply pending if pending section is empty (should always
>    be the case in this code path)
> 2 add explicit parameter to skip
> 3 drop the hotplug/apply pending calls in update_pct_config, add them to
> the update_vm API endpoint instead.
> 
> I'd prefer 3 - 1 - 2 in that order ;)
> 
> 3 would be called with the same semantics in both call sites (create_vm
> and update_vm): here are add/delete/revert parameters, here's a conf
> hash; merge those operating solely on the conf hash.
> 

Ok. I'll send a v2 with approach number 3.

>> ---
>>
>> Note that this should be applied before [0] to avoid temporary
>> (further ;)) breakage of container creation.
>>
>> [0]: https://pve.proxmox.com/pipermail/pve-devel/2020-April/043155.html
>>
>>   src/PVE/API2/LXC.pm | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>> index 148ba6a..46d2fd7 100644
>> --- a/src/PVE/API2/LXC.pm
>> +++ b/src/PVE/API2/LXC.pm
>> @@ -334,10 +334,6 @@ __PACKAGE__->register_method({
>>   	# check/activate default storage
>>   	&$check_and_activate_storage($storage) if !defined($mp_param->{rootfs});
>>   
>> -	PVE::LXC::Config->update_pct_config($vmid, $conf, 0, $no_disk_param);
>> -
>> -	$conf->{unprivileged} = 1 if $unprivileged;
>> -
>>   	my $emsg = $restore ? "unable to restore CT $vmid -" : "unable to create CT $vmid -";
>>   
>>   	eval { PVE::LXC::Config->create_and_lock_config($vmid, $force) };
>> @@ -346,8 +342,11 @@ __PACKAGE__->register_method({
>>   	my $code = sub {
>>   	    my $old_conf = PVE::LXC::Config->load_config($vmid);
>>   	    my $was_template;
>> -
>>   	    my $vollist = [];
>> +
>> +	    PVE::LXC::Config->update_pct_config($vmid, $conf, 0, $no_disk_param);
>> +	    $conf->{unprivileged} = 1 if $unprivileged;
>> +
>>   	    eval {
>>   		my $orig_mp_param; # only used if $restore
>>   		if ($restore) {
>> -- 
>> 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
>