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

[Dietmar Maurer]

# 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