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

[Fabian Grünbichler]

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

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

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

> 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});
> >  
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel