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

Alexandre DERUMIER aderumier at odiso.com
Wed Sep 26 10:26:54 CEST 2018


>>There is absolutely no guarantee that the diff contains the iface line if the interface config contains more than 3 lines. 

>>Instead, you need to read/parse both files. Then compare parsed data. 
>>
>>> + } elsif ($line =~ m/ovs_type/) { 
>>
>>same as above. 

Ok, no problem, I'll take a little be more time to implement ;)



>>> + #if we still have error, recopy failed interface old config in current config 
>>
>>not sure rollback is a good idea, because all other commands before 
>>assumed the new configuration. Maybe this can result in an inconsistent network config?? 

I don't rollback all config, only config of failed interfaces.
So it should be safe. (and ifdown/ifup shouldn't failed, until we have a syntax config error, but previous reload should fail before)

----- Mail original -----
De: "dietmar" <dietmar at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>, "aderumier" <aderumier at odiso.com>
Envoyé: Mercredi 26 Septembre 2018 08:05:33
Objet: Re: [pve-devel] [PATCH v3 pve-manager] API2 : Network : add network config reload

# git am yourpatch.eml 
Applying: API2 : Network : add network config reload 
.git/rebase-apply/patch:33: trailing whitespace. 
name => 'reload_network_config', 
.git/rebase-apply/patch:34: trailing whitespace. 
path => '', 
.git/rebase-apply/patch:43: space before tab in indent. 
additionalProperties => 0, 
.git/rebase-apply/patch:75: trailing whitespace. 
} 
.git/rebase-apply/patch:78: trailing whitespace. 
raise_param_exc({ config => "reloading config with ovs changes is not possible currently\n" }) 
warning: squelched 2 whitespace errors 
warning: 7 lines add whitespace errors. 


more comments inline 

> On September 25, 2018 at 10:03 AM Alexandre Derumier <aderumier at odiso.com> 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. 
> 
> Some specific interfaces options can't be reloaded online 
> (because kernel don't implement it), it this case, we ifdown/ifup 
> theses interfaces. (mainly vxlan interfaces options) 
> --- 
> changelog v3: 
> - catch errors on reloading, and try to ifdown/ifup interfaces 
> - if an interfaces really can't be reloaded, of ifdown/ifup, 
> revert current config of theses interfaces and keep interfaces.new file 
> 
> changelog v2: 
> 
> - remove restart option 
> - check if vm|ct are running on a bridge delete 
> - run the networking service reload in a task 
> 
> PVE/API2/Network.pm | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++- 
> 1 file changed, 113 insertions(+), 1 deletion(-) 
> 
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm 
> index 92256863..6d24ae24 100644 
> --- a/PVE/API2/Network.pm 
> +++ b/PVE/API2/Network.pm 
> @@ -4,7 +4,7 @@ use strict; 
> use warnings; 
> 
> use Net::IP qw(:PROC); 
> -use PVE::Tools qw(extract_param); 
> +use PVE::Tools qw(extract_param dir_glob_regex); 
> use PVE::SafeSyslog; 
> use PVE::INotify; 
> use PVE::Exception qw(raise_param_exc); 
> @@ -471,6 +471,118 @@ __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'), 
> + }, 
> + }, 
> + returns => { type => 'string' }, 
> + code => sub { 
> + 
> + my ($param) = @_; 
> + 
> + my $rpcenv = PVE::RPCEnvironment::get(); 
> + 
> + my $authuser = $rpcenv->get_user(); 
> + 
> + my $current_config_file = "/etc/network/interfaces"; 
> + my $new_config_file = "/etc/network/interfaces.new"; 
> + 
> + raise_param_exc({ config => "you need ifupdown2 to reload 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}; 
> + 
> + 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; 

There is absolutely no guarantee that the diff contains the iface line if the interface config contains more than 3 lines. 

Instead, you need to read/parse both files. Then compare parsed data. 

> + } elsif ($line =~ m/ovs_type/) { 

same as above. 

Also, what if someone changes the type from OVS to normal bridge? 

> + $ovs_changes = 1; 
> + } 
> + } 
> + 
> + raise_param_exc({ config => "reloading config with ovs changes is not possible currently\n" }) 
> + if $ovs_changes && !$param->{restart}; 
> + 
> + foreach my $bridge (keys %$bridges_delete) { 
> + 
> + my (undef, $interface) = dir_glob_regex("/sys/class/net/$bridge/brif", '(tap|veth|fwpr).*'); 
> + raise_param_exc({ config => "bridge deletion is not possible currently if vm or ct are running on this bridge\n" }) 
> + if defined($interface); 
> + } 
> + 
> + my $worker = sub { 
> + 
> + #clean-me 
> + my $fh = IO::File->new("<$current_config_file"); 
> + my $running_config = PVE::INotify::read_etc_network_interfaces(1,$fh); 
> + $fh->close(); 
> + 
> + PVE::Tools::file_copy($new_config_file, $current_config_file); 
> + my $new_config = PVE::INotify::read_file('interfaces'); 
> + 
> + my $cmd = ['ifreload', '-a']; 
> + my $ifaces_errors = {}; 
> + my $ifaces_errors_final = {}; 
> + 
> + my $err = sub { 
> + my $line = shift; 
> + if ($line =~ /(warning|error): (\S+):/) { 
> + $ifaces_errors->{$2} = 1; 
> + print "$2 : error reloading configuration online : try to ifdown/ifdown later : $line \n"; 
> + } 
> + }; 
> + 
> + PVE::Tools::run_command($cmd,errfunc => $err); 
> + 
> + my $err2 = sub { 
> + my $line = shift; 
> + if ($line =~ /(warning|error): (\S+):/) { 
> + $ifaces_errors_final->{$2} = 1; 
> + print "$2 : error restart: $line \n"; 
> + } 
> + }; 
> + 
> + #try ifdown/up for non online change options 
> + foreach my $iface (keys %$ifaces_errors) { 
> + eval { PVE::Tools::run_command(['ifdown',$iface]) }; 
> + PVE::Tools::run_command(['ifup',$iface],errfunc => $err2); 
> + } 
> + 
> + #if we still have error, recopy failed interface old config in current config 

not sure rollback is a good idea, because all other commands before 
assumed the new configuration. Maybe this can result in an inconsistent network config?? 


> + if(keys %$ifaces_errors_final > 0 ) { 
> + foreach my $iface (keys %$ifaces_errors_final) { 
> + print "error: $iface config has not been applied\n"; 
> + delete $new_config->{ifaces}->{$iface}; 
> + $new_config->{ifaces}->{$iface} = $running_config->{ifaces}->{$iface}; 
> + } 
> + #clean-me 
> + my $fh = IO::File->new(">$current_config_file"); 
> + PVE::INotify::write_etc_network_interfaces(1, $fh, $new_config); 
> + $fh->close(); 
> + } else { 
> + unlink $new_config_file; 
> + } 
> + }; 
> + return $rpcenv->fork_worker('srvreload', 'networking', $authuser, $worker); 
> + }}); 
> + 
> +__PACKAGE__->register_method({ 
> name => 'delete_network', 
> path => '{iface}', 
> method => 'DELETE', 
> -- 
> 2.11.0 
> 
> _______________________________________________ 
> pve-devel mailing list 
> pve-devel at pve.proxmox.com 
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 



More information about the pve-devel mailing list