[pve-devel] [PATCH v3] LXC: more compact network configuration

[Wolfgang Bumiller]

Deduplicated network setup code.
Using 'ip route replace' to replace or add the route.
This strategy can be rolled back safely:
1) add new ip (no harm done, old ip still exists)
2) replace route
  on error:
    Delete the new ip, old one is still in place.
    If deleting the new ip fails, it was already modified
    from within the container, so we only warn about it.
  on success do (3):
3) delete old ip
  New IP + gateway are in place, old IP can be deleted.
  If deletion fails, we only warn like in the error case.
---
 src/PVE/LXC.pm | 143 +++++++++++++++++++++++++--------------------------------
 1 file changed, 62 insertions(+), 81 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 36c3995..4bea2f6 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1246,99 +1246,80 @@ sub update_ipconfig {
 
     my $lxc_setup = PVE::LXCSetup->new($conf, $rootdir);
 
-    my $update_gateway;
-    if (&$safe_string_ne($conf->{$opt}->{gw}, $newnet->{gw})) {
-
-	$update_gateway = 1;
-	if ($conf->{$opt}->{gw}) {
-	    my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'route', 'del', 'default', 'via', $conf->{$opt}->{gw} ];
-	    eval { PVE::Tools::run_command($cmd); };
-	    warn $@ if $@; # ignore errors here
-	    delete $conf->{$opt}->{gw};
-	    PVE::LXC::write_config($vmid, $conf);
-	    $lxc_setup->setup_network($conf);
-	}
-    }
+    my $optdata = $conf->{$opt};
+    my $deleted = [];
+    my $added = [];
+    my $netcmd = sub {
+	my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', @_];
+	PVE::Tools::run_command($cmd);
+    };
 
-    if (&$safe_string_ne($conf->{$opt}->{ip}, $newnet->{ip})) {
+    my $change_ip_config = sub {
+	my ($family_opt, $suffix) = @_;
+	$suffix = '' if !$suffix;
+	my $gw= "gw$suffix";
+	my $ip= "ip$suffix";
 
-	if ($conf->{$opt}->{ip}) {
-	    my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'addr', 'del', $conf->{$opt}->{ip}, 'dev', $eth  ];
-	    eval { PVE::Tools::run_command($cmd); };
-	    warn $@ if $@; # ignore errors here
-	    delete $conf->{$opt}->{ip};
-	    PVE::LXC::write_config($vmid, $conf);
-	    $lxc_setup->setup_network($conf);
-	}
+	my $change_ip = &$safe_string_ne($optdata->{$ip}, $newnet->{$ip});
+	my $change_gw = &$safe_string_ne($optdata->{$gw}, $newnet->{$gw});
 
-	if ($newnet->{ip}) {
-	    my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'addr', 'add', $newnet->{ip}, 'dev', $eth  ];
-	    PVE::Tools::run_command($cmd);
+	return if !$change_ip && !$change_gw;
 
-	    $conf->{$opt}->{ip} = $newnet->{ip};
-	    PVE::LXC::write_config($vmid, $conf);
-	    $lxc_setup->setup_network($conf);
+	# step 1: add new IP, if this fails we cancel
+	if ($change_ip && $newnet->{$ip}) {
+	    eval { &$netcmd($family_opt, 'addr', 'add', $newnet->{$ip}, 'dev', $eth); };
+	    if (my $err = $@) {
+		warn $err;
+		return;
+	    }
 	}
-    }
-
-    if ($update_gateway) {
-
-	if ($newnet->{gw}) {
-	    my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'route', 'add', 'default', 'via', $newnet->{gw} ];
-	    PVE::Tools::run_command($cmd);
 
-	    $conf->{$opt}->{gw} = $newnet->{gw};
-	    PVE::LXC::write_config($vmid, $conf);
-	    $lxc_setup->setup_network($conf);
-        }
-    }
-
-    my $update_gateway6;
-    if (&$safe_string_ne($conf->{$opt}->{gw6}, $newnet->{gw6})) {
-	
-	$update_gateway6 = 1;
-	if ($conf->{$opt}->{gw6}) {
-	    my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'route', 'del', 'default', 'via', $conf->{$opt}->{gw6} ];
-	    eval { PVE::Tools::run_command($cmd); };
-	    warn $@ if $@; # ignore errors here
-	    delete $conf->{$opt}->{gw6};
-	    PVE::LXC::write_config($vmid, $conf);
-	    $lxc_setup->setup_network($conf);
+	# step 2: replace gateway
+	#   If this fails we delete the added IP and cancel.
+	#   If it succeeds we save the config and delete the old IP, ignoring
+	#   errors. The config is then saved.
+	# Note: 'ip route replace' can add
+	if ($change_gw) {
+	    if ($newnet->{$gw}) {
+		eval { &$netcmd($family_opt, 'route', 'replace', 'default', 'via', $newnet->{$gw}); };
+		if (my $err = $@) {
+		    warn $err;
+		    # the route was not replaced, the old IP is still available
+		    # rollback (delete new IP) and cancel
+		    if ($change_ip) {
+			eval { &$netcmd($family_opt, 'addr', 'del', $newnet->{$ip}, 'dev', $eth); };
+			warn $@ if $@; # no need to die here
+		    }
+		    return;
+		}
+	    } else {
+		eval { &$netcmd($family_opt, 'route', 'del', 'default'); };
+		# if the route was not deleted, the guest might have deleted it manually
+		# warn and continue
+		warn $@ if $@;
+	    }
 	}
-    }
 
-    if (&$safe_string_ne($conf->{$opt}->{ip6}, $newnet->{ip6})) {
-
-	if ($conf->{$opt}->{ip6}) {
-	    my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'addr', 'del', $conf->{$opt}->{ip6}, 'dev', $eth  ];
-	    eval { PVE::Tools::run_command($cmd); };
-	    warn $@ if $@; # ignore errors here
-	    delete $conf->{$opt}->{ip6};
-	    PVE::LXC::write_config($vmid, $conf);
-	    $lxc_setup->setup_network($conf);
+	# from this point on we safe the configuration
+	# step 3: delete old IP ignoring errors
+	if ($change_ip && $optdata->{$ip}) {
+	    eval { &$netcmd($family_opt, 'addr', 'del', $optdata->{$ip}, 'dev', $eth); };
+	    warn $@ if $@; # no need to die here
 	}
 
-	if ($newnet->{ip6}) {
-	    my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'addr', 'add', $newnet->{ip6}, 'dev', $eth  ];
-	    PVE::Tools::run_command($cmd);
-
-	    $conf->{$opt}->{ip6} = $newnet->{ip6};
-	    PVE::LXC::write_config($vmid, $conf);
-	    $lxc_setup->setup_network($conf);
+	foreach my $property ($ip, $gw) {
+	    if ($newnet->{$property}) {
+		$optdata->{$property} = $newnet->{$property};
+	    } else {
+		delete $optdata->{$property};
+	    }
 	}
-    }
-
-    if ($update_gateway6) {
-
-	if ($newnet->{gw6}) {
-	    my $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'route', 'add', 'default', 'via', $newnet->{gw6} ];
-	    PVE::Tools::run_command($cmd);
+	PVE::LXC::write_config($vmid, $conf);
+	$lxc_setup->setup_network($conf);
+    };
 
-	    $conf->{$opt}->{gw6} = $newnet->{gw6};
-	    PVE::LXC::write_config($vmid, $conf);
-	    $lxc_setup->setup_network($conf);
-        }
-    }
+    &$change_ip_config('-4');
+    &$change_ip_config('-6', '6');
 }
 
 1;
-- 
2.1.4