[pve-devel] [PATCH container 1/2] Fix #1544: add skiplock to lxc api path

[Thomas Lamprecht]

On 3/21/18 9:00 AM, Fabian Grünbichler wrote:
> On Tue, Mar 20, 2018 at 04:00:51PM +0100, Thomas Lamprecht wrote:
>> a bit strange to be honest, there's already:
>> # pct unlock VMID
>>
>> doing the same thing in a completely other matter,
>> using LXC::Config->remove_lock (inherited from AbstractConfig)
> 
> yes, but that one is a single-use convenience command.
> 
>> Ideally, both the API and the CLI helper share their code, after your patch.
> 
> they do, but you need to look at the right API / CLI :-P

I did, just because we track the lock in the config it doesn't means
it should be seen as arbitrary config option which should be removable
the same way a, e.g., disk is.

> its 'pct set' and 'update_vm', not 'pct unlock' and 'update_vm'. we
> could of course (also?) introduce a new 'unlock_vm' API call - but that
> seems kind of redundant to me.
> 

I'd rather remove the set one, tbh.

>> I'd not allow changing params on a locked VM, to much strange side effects..
> 
> it's only possible for root anyhow, and I don't see how the side effects
> get stranger than if the user does 'pct unlock XX; pct set XX -foo bar'?
> 

Difference:

One gets first unlocked then you can do stuff with it.
An operation using the lock gets hold of the changes.

This allows to change/modify operations and the lock is
still set, an ongoing locked operation isn't able to see
this change, this completely defeats the purpose of a lock!

> we are already in manual error recovery territory here anyway..
> 

Yes, that's exactly my point. No need to allow for potential more
problems...

>> Allow to remove the lock as its own operation, then a user can do everything again.
>>
>> What about VMs?
> 
> the patch is modeled after the qemu API, which has the exact same
> behaviour (including all of the negative points you mention above).
> 
> we already have
> - qm unlock / pct unlock (consistent)
> - qm set / pct set (inconsistent)
> - update_vm / update_vm (inconsistent)
> 
> this patch makes the latter two variants consistent again (and also
> consistent with the automatically generated API documentation, see the
> bug report).
> 

Doing something just for the sake of consistency does not
makes anything right.

I do not feel good with the "set arbitrary options with locked
configs without removing the locks" approach...

>> On 3/16/18 5:30 PM, Alwin Antreich wrote:
>>> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
>>> ---
>>>  src/PVE/API2/LXC/Config.pm | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
>>> index 2b622b3..2d69049 100644
>>> --- a/src/PVE/API2/LXC/Config.pm
>>> +++ b/src/PVE/API2/LXC/Config.pm
>>> @@ -80,6 +80,7 @@ __PACKAGE__->register_method({
>>>  	    {
>>>  		node => get_standard_option('pve-node'),
>>>  		vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
>>> +		skiplock => get_standard_option('skiplock'),
>>>  		delete => {
>>>  		    type => 'string', format => 'pve-configid-list',
>>>  		    description => "A list of settings you want to delete.",
>>> @@ -107,6 +108,10 @@ __PACKAGE__->register_method({
>>>  
>>>  	my $digest = extract_param($param, 'digest');
>>>  
>>> +	my $skiplock = extract_param($param, 'skiplock');
>>> +	raise_param_exc({ skiplock => "Only root may use this option." })
>>> +	    if $skiplock && $authuser ne 'root at pam';
>>> +
>>>  	die "no options specified\n" if !scalar(keys %$param);
>>>  
>>>  	my $delete_str = extract_param($param, 'delete');
>>> @@ -155,7 +160,7 @@ __PACKAGE__->register_method({
>>>  	my $code = sub {
>>>  
>>>  	    my $conf = PVE::LXC::Config->load_config($vmid);
>>> -	    PVE::LXC::Config->check_lock($conf);
>>> +	    PVE::LXC::Config->check_lock($conf) if !$skiplock;
>>>  
>>>  	    PVE::Tools::assert_if_modified($digest, $conf->{digest});
>>>  
>>>