[pve-devel] [PATCH pve-manager] API2 : Network : add network config reload

[Alexandre DERUMIER]

>>I'm not sure that's a good idea. In theory `restart` would have worked 
>>before as well, but the consequence in this case is *always* that the 
>>API call fails due to a connection loss. 
>>IMO we should only ever do 'reloads' - they too can kill the connection 
>>of course, but restart *always* does

Ok no problem. It was only mainly for ovs, as I can't manage reload until a true ifupdown2 addon
is done for ovs. (to manage graph relationship).
I have plan to do it later if I have time.
I'll keep reload only for now.


>>And it would probably look better for the caller (UI) if it returned a 
>>task UPID and runs in the background. That way the call can always 
>>finish first (the actual execution of the reload command should probably 
>>be delayed by a few seconds to be sure), and we can later see the task 
>>in the tasklog - if it works. This would probably make it easier to 
>>handle on the web UI side. 

yes, you are righr. i'll rework my patch.



----- Mail original -----
De: "Wolfgang Bumiller" <w.bumiller at proxmox.com>
À: "aderumier" <aderumier at odiso.com>
Cc: "pve-devel" <pve-devel at pve.proxmox.com>, "Dominik Csapak" <d.csapak at proxmox.com>
Envoyé: Mardi 26 Juin 2018 13:45:36
Objet: Re: [pve-devel] [PATCH pve-manager] API2 : Network : add network config reload

On Fri, Jun 22, 2018 at 03:06:26AM +0200, Alexandre Derumier wrote: 
> This add a new api to online reload networking configuration 
> with ifupdown2. 
> 
> This work with native ifupdown2 modules, as ifupdown2 have 
> interface dependency relationships. 
> 
> an optional "restart" param is available, for non native modules 
> like openvswitch. 

I'm not sure that's a good idea. In theory `restart` would have worked 
before as well, but the consequence in this case is *always* that the 
API call fails due to a connection loss. 

IMO we should only ever do 'reloads' - they too can kill the connection 
of course, but restart *always* does. 
And it would probably look better for the caller (UI) if it returned a 
task UPID and runs in the background. That way the call can always 
finish first (the actual execution of the reload command should probably 
be delayed by a few seconds to be sure), and we can later see the task 
in the tasklog - if it works. This would probably make it easier to 
handle on the web UI side. 

> --- 
> PVE/API2/Network.pm | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> 1 file changed, 65 insertions(+) 
> 
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm 
> index 92256863..3fb71617 100644 
> --- a/PVE/API2/Network.pm 
> +++ b/PVE/API2/Network.pm 
> @@ -471,6 +471,71 @@ __PACKAGE__->register_method({ 
> }}); 
> 
> __PACKAGE__->register_method({ 
> + name => 'reload_network_config', 
> + path => '', 
> + method => 'PUT', 
> + permissions => { 
> + check => ['perm', '/nodes/{node}', [ 'Sys.Modify' ]], 
> + }, 
> + description => "Reload network configuration", 
> + protected => 1, 
> + proxyto => 'node', 
> + parameters => { 
> + additionalProperties => 0, 
> + properties => { 
> + node => get_standard_option('pve-node'), 
> + restart => { 
> + type => 'boolean', 
> + description => "restart networking.", 
> + optional => 1, 
> + } 
> + }, 
> + }, 
> + returns => { type => "null" }, 
> + code => sub { 
> + 
> + my ($param) = @_; 
> + 
> + my $action = $param->{restart} ? "restart" : "reload"; 
> + my $current_config_file = "/etc/network/interfaces"; 
> + my $new_config_file = "/etc/network/interfaces.new"; 
> + 
> + raise_param_exc({ config => "you need ifupdown2 to $action networking" }) if !-e '/usr/share/ifupdown2'; 
> + raise_param_exc({ config => "no new network config to apply" }) if !-e $new_config_file; 
> + 
> + my $tmp = PVE::INotify::read_file('interfaces', 1); 
> + my $config = $tmp->{data}; 
> + my $changes = $tmp->{changes}; 
> + 
> + raise_param_exc({ config => "no changes detected" }) if !$changes; 
> + 
> + my $ovs_changes = undef; 
> + my $bridges_delete = {}; 
> + my @lines = split(/\n/, $changes); 
> + foreach my $line (@lines) { 
> + if($line =~ m/^\-iface\s(vmbr(\S+))/) { 
> + $bridges_delete->{$1} = 1; 
> + } elsif ($line =~ m/ovs_type/) { 
> + $ovs_changes = 1; 
> + } 
> + } 
> + 
> + raise_param_exc({ config => "reloading config with ovs changes is not possible currently, please use restart" }) 
> + if $ovs_changes && !$param->{restart}; 
> + 
> + #fixme : restart : check if vm are running on bridge, and try to reattach them ? 
> + raise_param_exc({ config => "restart config with bridge delete is not possible currently, please use reload" }) 
> + if keys %$bridges_delete && $param->{restart}; 
> + 
> + PVE::Tools::file_copy($new_config_file, $current_config_file); 
> + unlink $new_config_file; 
> + 
> + PVE::Tools::run_command(['systemctl', $action, 'networking']); 
> + 
> + return undef; 
> + }}); 
> + 
> +__PACKAGE__->register_method({ 
> name => 'delete_network', 
> path => '{iface}', 
> method => 'DELETE', 
> -- 
> 2.11.0