[pve-devel] [PATCH v2 container 16/18] adapt config PUT method for the new update_pct_config

[Oguz Bektas]

On Wed, Oct 02, 2019 at 01:52:58PM +0200, Fabian Grünbichler wrote:
> On September 30, 2019 2:44 pm, Oguz Bektas wrote:
> > we don't need to extract 'delete' here, instead we pass it all as $param
> > and extract 'delete', 'revert' and most other things in
> > update_pct_config
> 
> I already asked that in v1 - wouldn't it make sense to keep the 
> parameter checks in the API call, and pass the already "verified" 
> $delete and $revert to update_pct_config? this would avoid the need of 
> adding new module dependencies from PVE::LXC::Config to 
> PVE::RPCEnvironment and PVE::Exception in #17..
> 
> if you see a good reason for moving all that to update_pct_config, 
> please say so (either in the changelog of the patch, or as reply to the 
> original review) and don't silently ignore open questions ;)

i actually wanted to move even more. in qemu api it's like:

-----------------------------------

-----------------------------------
__PACKAGE__->register_method({
    name => 'update_vm_async',
    path => '{vmid}/config',
    method => 'POST',
...

    returns => {
        type => 'string',
        optional => 1,
    },
    code => $update_vm_api,
});
-----------------------------------

and

-----------------------------------
__PACKAGE__->register_method({
    name => 'update_vm',
    path => '{vmid}/config',
    method => 'PUT',
    protected => 1,
    proxyto => 'node',
    description => "Set virtual machine options (synchrounous API) - You should consider using the POST method instead for any actions involving hotplug or storage allocation.",
...
    returns => { type => 'null' },
    code => sub {
        my ($param) = @_;
        &$update_vm_api($param, 1);
        return undef;
    }
});
-----------------------------------

which i find more readable than the current situation in container code, and the checks are in the $update_vm_api sub.

now i don't know which is better in terms of implementation, so if you believe that the checks should stay in api, then we can do it like that.

> 
> > 
> > Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> > ---
> >  src/PVE/API2/LXC/Config.pm | 55 ++++----------------------------------
> >  1 file changed, 5 insertions(+), 50 deletions(-)
> > 
> > diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
> > index 2c036f5..46d8e2f 100644
> > --- a/src/PVE/API2/LXC/Config.pm
> > +++ b/src/PVE/API2/LXC/Config.pm
> > @@ -119,58 +119,10 @@ __PACKAGE__->register_method({
> >      code => sub {
> >  	my ($param) = @_;
> >  
> > -	my $rpcenv = PVE::RPCEnvironment::get();
> > -	my $authuser = $rpcenv->get_user();
> > -
> >  	my $node = extract_param($param, 'node');
> >  	my $vmid = extract_param($param, 'vmid');
> > -
> >  	my $digest = extract_param($param, 'digest');
> >  
> > -	die "no options specified\n" if !scalar(keys %$param);
> > -
> > -	my $delete_str = extract_param($param, 'delete');
> > -	my @delete = PVE::Tools::split_list($delete_str);
> > -
> > -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, {}, [@delete]);
> > -
> > -	foreach my $opt (@delete) {
> > -	    raise_param_exc({ delete => "you can't use '-$opt' and -delete $opt' at the same time" })
> > -		if defined($param->{$opt});
> > -
> > -	    if (!PVE::LXC::Config->option_exists($opt)) {
> > -		raise_param_exc({ delete => "unknown option '$opt'" });
> > -	    }
> > -	}
> > -
> > -	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $param, []);
> > -
> > -	my $storage_cfg = cfs_read_file("storage.cfg");
> > -
> > -	my $repl_conf = PVE::ReplicationConfig->new();
> > -	my $is_replicated = $repl_conf->check_for_existing_jobs($vmid, 1);
> > -	if ($is_replicated) {
> > -	    PVE::LXC::Config->foreach_mountpoint_full($param, 0, sub {
> > -		my ($opt, $mountpoint) = @_;
> > -		my $volid = $mountpoint->{volume};
> > -		return if !$volid || !($mountpoint->{replicate}//1);
> > -		if ($mountpoint->{type} eq 'volume') {
> > -		    my ($storeid, $format);
> > -		    if ($volid =~ $PVE::LXC::NEW_DISK_RE) {
> > -			$storeid = $1;
> > -			$format = $mountpoint->{format} || PVE::Storage::storage_default_format($storage_cfg, $storeid);
> > -		    } else {
> > -			($storeid, undef) = PVE::Storage::parse_volume_id($volid, 1);
> > -			$format = (PVE::Storage::parse_volname($storage_cfg, $volid))[6];
> > -		    }
> > -		    return if PVE::Storage::storage_can_replicate($storage_cfg, $storeid, $format);
> > -		    my $scfg = PVE::Storage::storage_config($storage_cfg, $storeid);
> > -		    return if $scfg->{shared};
> > -		}
> > -		die "cannot add non-replicatable volume to a replicated VM\n";
> > -	    });
> > -	}
> > -
> >  	my $code = sub {
> >  
> >  	    my $conf = PVE::LXC::Config->load_config($vmid);
> > @@ -180,10 +132,13 @@ __PACKAGE__->register_method({
> >  
> >  	    my $running = PVE::LXC::check_running($vmid);
> >  
> > -	    PVE::LXC::Config->update_pct_config($vmid, $conf, $running, $param, \@delete);
> > +	    die "no options specified\n" if !scalar(keys %$param);
> > +
> > +	    PVE::LXC::Config->update_pct_config($vmid, $conf, $running, $param);
> > +	    $conf = PVE::LXC::Config->load_config($vmid);
> >  
> > -	    PVE::LXC::Config->write_config($vmid, $conf);
> >  	    PVE::LXC::update_lxc_config($vmid, $conf);
> > +
> >  	};
> >  
> >  	PVE::LXC::Config->lock_config($vmid, $code);
> > -- 
> > 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
>