[pve-devel] [PATCH pve-common 2/3] Inotify : add vxlan interface support

[Wolfgang Bumiller]

Comments inline.  Also, I would like a test case for this btw. (Which
reminds me, I should probably explain the functions used for t.*.pl
files a bit better in the comments...
Short summary: The t.create_network.pl should be a good place to copy
from, r() and w() call read/write_etc_network_interfaces() from/to
strings. load() accesses the other files in the test dir and for
convenience save() can be used to replace them temporarily.
$config globally accesses the last r()'d config in order to test
modifying the hash directly.
And finally `expect($string)` compares $string to the output of w() and
fails if they differ.

On Mon, Jun 25, 2018 at 05:41:34AM +0200, Alexandre Derumier wrote:
> ---
>  src/PVE/INotify.pm | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index 25ab868..2c1af3c 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -916,6 +916,11 @@ sub __read_etc_network_interfaces {
>  			    }
>  			}
>  			$d->{$id} = $value;
> +		    } elsif ($id eq 'vxlan-id' || $id eq 'vxlan-svcnodeip' || 
> +			     $id eq 'vxlan-physdev' || $id eq 'vxlan-local-tunnelip') {
> +			$d->{$id} = $value;
> +		    } elsif ($id eq 'vxlan-remoteip') {
> +			push @{$d->{$id}}, $value;
>  		    } else {
>  			push @{$f->{options}}, $option;
>  		    }
> @@ -1012,7 +1017,9 @@ sub __read_etc_network_interfaces {
>  	} elsif ($iface =~ m/^lo$/) {
>  	    $d->{type} = 'loopback';
>  	} else {
> -	    if (!$d->{ovs_type}) {
> +	    if ($d->{'vxlan-id'}) {
> +		$d->{type} = 'vxlan';
> +	    } elsif (!$d->{ovs_type}) {
>  		$d->{type} = 'unknown';
>  	    } elsif ($d->{ovs_type} eq 'OVSIntPort') {
>  		$d->{type} = $d->{ovs_type};
> @@ -1121,6 +1128,28 @@ sub __interface_to_string {
>  	    $raw .= "\tbond-xmit-hash-policy $d->{'bond_xmit_hash_policy'}\n";
>  	}
>  	$done->{'bond_xmit_hash_policy'} = 1;
> +    } elsif ($d->{type} eq 'vxlan') {
> +
> +	$raw .= "\tvxlan-id $d->{'vxlan-id'}\n" if $d->{'vxlan-id'};
> +	$done->{'vxlan-id'} = 1;
> +	if ($d->{'vxlan-svcnodeip'}) {

I think this should use defined(), even though they're equivalent given
the contents...
And with svcnodeip, physdev and tunnelip being written out without any
processing the if() { $raw..., $done... } pattern could probably be
written out once with a loop over the 3 keys. With all the inline text
this is starting to look like quite a cryptic block. (not that the rest
of the code isn't the same already anyway ;-) )

> +	    $raw .= "\tvxlan-svcnodeip $d->{'vxlan-svcnodeip'}\n";
> +	    $done->{'vxlan-svcnodeip'} = 1;
> +	}
> +	if ($d->{'vxlan-physdev'}) {
> +	    $raw .= "\tvxlan-physdev $d->{'vxlan-physdev'}\n";
> +	    $done->{'vxlan-physdev'} = 1;
> +	}
> +	if ($d->{'vxlan-local-tunnelip'}) {
> +	    $raw .= "\tvxlan-local-tunnelip $d->{'vxlan-local-tunnelip'}\n";
> +	    $done->{'vxlan-local-tunnelip'} = 1;
> +	}
> +	if ($d->{'vxlan-remoteip'}) {
> +	    foreach my $remoteip (@{$d->{'vxlan-remoteip'}}) {
> +		$raw .= "\tvxlan-remoteip $remoteip\n";
> +	    }
> +	    $done->{'vxlan-remoteip'} = 1;
> +	}
>  
>      } elsif ($d->{type} eq 'OVSBridge') {
>  
> @@ -1293,6 +1322,28 @@ sub __write_etc_network_interfaces {
>  	}
>      }
>  
> +    # check vxlan
> +    my $vxlans = {};
> +    foreach my $iface (keys %$ifaces) {
> +	my $d = $ifaces->{$iface};
> +
> +	if ($d->{type} eq 'vxlan' && $d->{'vxlan-id'}) {
> +	    my $vxlanid = $d->{'vxlan-id'};
> +	    die "iface $iface  : duplicate vxlan-id $vxlanid already used in $vxlans->{$vxlanid}" if $vxlans->{$vxlanid};
> +	    $vxlans->{$vxlanid} = $iface;
> +	}
> +
> +	if (($d->{'vxlan-svcnodeip'} && $d->{'vxlan-remoteip'}) ||
> +	    ($d->{'vxlan-svcnodeip'} && $d->{'vxlan-local-tunnelip'}) ||
> +	    ($d->{'vxlan-local-tunnelip'} && $d->{'vxlan-remoteip'})) {
> +	    die "iface $iface : you can't use vxlan-svcnodeip / vxlan-remoteip / vxlan-local-tunnelip at the same time";

Took me a minute to read the above condition as "they're mutually
exclusive". I think the code would be clearer if we counted them:
    my $ips = 0;
    ++$ips if defined $d->{'vxlan-svcnodeip'};
    ++$ips if defined $d->{'vxlan-remoteip'};
    ++$ips if defined $d->{'vxlan-local-tunnelip'};
    if ($ips > 1) {
    	die "ifac $iface : vxlan-svcnodeip, vxlan-remoteip and vxlan-localtunnelip aremutually exclusive\n";
    }

Also note the added \n to the error.

> +	}
> +
> +	if (($d->{'vxlan-svcnodeip'} && !$d->{'vxlan-physdev'}) || (!$d->{'vxlan-svcnodeip'} && $d->{'vxlan-physdev'})) {
> +	    die "iface $iface : vxlan-svcnodeip && vxlan-physdev must be define together"; 

If the defined-ness of two values must be the same you can just do:
    if (defined($d->{'vxlan-svcnodeip'}) != defined($d->{'vxlan-physdev'})) {
        die "iface $iface : vxlan-svcnodeip and vxlan-physdev must be define together\n"; 

(Note that similarly if it's not about defined()-ness, you can compare
their negations: if (!$a != !$b))

Alternatively we could have more specific error messages
    if (defined($d->{'vxlan-svcnodeip'}) && !defined($d->{'vxlan-physdev'})) {
        die "iface $iface : vxlan-svcnodeip requires vxlan-physdev\n"; 
    }
    if (!defined($d->{'vxlan-svcnodeip'}) && defined($d->{'vxlan-physdev'})) {
        die "iface $iface : vxlan-physdev requires vxlan-svcnodeip\n"; 
    }

Either way, the error should have a newline at the end ;-)


> +	}
> +    }
> +
>      my $raw = <<'NETWORKDOC';
>  # network interface settings; autogenerated
>  # Please do NOT modify this file directly, unless you know what
> @@ -1314,6 +1365,7 @@ NETWORKDOC
>  	eth => 200000,
>  	bond => 300000,
>  	bridge => 400000,
> +	vxlan => 500000,
>     };
>  
>      my $lookup_type_prio = sub {
> -- 
> 2.11.0