[pve-devel] [PATCH] add support for network trunks

[Alexandre DERUMIER]

>>Given the bridge command syntax we used here: 
>>system("/sbin/bridge vlan add dev $iface vid 2-4094") == 0 || 
>>
>>And the new command you introduced here: 
>>+ system("/sbin/bridge vlan add dev $iface vid $trunk") == 0 || 
>>
>>Should this accept ranges via '-'? If so, does OVS also do ranges in 
>>`ovs-ctl add-port` with the syntax you added there? 
>>+ $cmd .= " trunks=". join(',', $trunks) if $trunks; 

for linux bridge, yes , we can pass ranges.
 /sbin/bridge vlan add dev $iface vid 10-20

but we can't pass lists
 /sbin/bridge vlan add dev $iface vid 10,11,12

(that's why I do it in a loop, 1 command for each vlan)



For ovs, this is the inverse


we can't pass range

#ovs-vsctl set port tap171i0 trunk=10-20
 ovs-vsctl: "10-20" is not a valid integer


but we can pass list
ovs-vsctl set port tap171i0 trunks=10,11,12



we could allow ranges in configuration, and do the format convertion if needed.




> if (&$safe_string_ne($oldnet->{bridge}, $newnet->{bridge}) || 
> &$safe_num_ne($oldnet->{tag}, $newnet->{tag}) || 
> + &$safe_num_ne($oldnet->{trunks}, $newnet->{trunks}) || 

>>Should this be $safe_string_ne()? (Since it's a ';' separated list and 
>>possibly contains 'x-y' ranges. 

I'll look at it, maybe I can improve this.



----- Mail original -----
De: "Wolfgang Bumiller" <w.bumiller at proxmox.com>
À: "aderumier" <aderumier at odiso.com>
Cc: "pve-devel" <pve-devel at pve.proxmox.com>
Envoyé: Vendredi 15 Janvier 2016 09:05:19
Objet: Re: [pve-devel] [PATCH] add support for network trunks

On Fri, Jan 15, 2016 at 03:15:35AM +0100, Alexandre Derumier wrote: 
> @@ -454,7 +454,7 @@ my $nic_model_list_txt = join(' ', sort @$nic_model_list); 
> my $netdesc = { 
> optional => 1, 
> type => 'string', format => 'pve-qm-net', 
> - typetext => "MODEL=XX:XX:XX:XX:XX:XX [,bridge=<dev>][,queues=<nbqueues>][,rate=<mbps>] [,tag=<vlanid>][,firewall=0|1],link_down=0|1]", 
> + typetext => "MODEL=XX:XX:XX:XX:XX:XX [,bridge=<dev>][,queues=<nbqueues>][,rate=<mbps>] [,tag=<vlanid>][,trunks=<vlanid[;vlanid]>][,firewall=0|1],link_down=0|1]", 
> description => <<EODESCR, 
> Specify network devices. 
> 
> @@ -1500,6 +1500,8 @@ sub parse_net { 
> $res->{rate} = $1; 
> } elsif ($kvp =~ m/^tag=(\d+)$/) { 
> $res->{tag} = $1; 
> + } elsif ($kvp =~ m/^trunks=([0-9;]+)$/) { 
> + $res->{trunks} = $1; 

Given the bridge command syntax we used here: 
system("/sbin/bridge vlan add dev $iface vid 2-4094") == 0 || 

And the new command you introduced here: 
+ system("/sbin/bridge vlan add dev $iface vid $trunk") == 0 || 

Should this accept ranges via '-'? If so, does OVS also do ranges in 
`ovs-ctl add-port` with the syntax you added there? 
+ $cmd .= " trunks=". join(',', $trunks) if $trunks; 


If so then the regex should include '-'. 

> } elsif ($kvp =~ m/^firewall=([01])$/) { 
> $res->{firewall} = $1; 
> } elsif ($kvp =~ m/^link_down=([01])$/) { 
> @@ -1523,6 +1525,7 @@ sub print_net { 
> $res .= ",bridge=$net->{bridge}" if $net->{bridge}; 
> $res .= ",rate=$net->{rate}" if $net->{rate}; 
> $res .= ",tag=$net->{tag}" if $net->{tag}; 
> + $res .= ",trunks=$net->{trunks}" if $net->{trunks}; 
> $res .= ",firewall=1" if $net->{firewall}; 
> $res .= ",link_down=1" if $net->{link_down}; 
> $res .= ",queues=$net->{queues}" if $net->{queues}; 
> @@ -4337,9 +4340,10 @@ sub vmconfig_update_net { 
> 
> if (&$safe_string_ne($oldnet->{bridge}, $newnet->{bridge}) || 
> &$safe_num_ne($oldnet->{tag}, $newnet->{tag}) || 
> + &$safe_num_ne($oldnet->{trunks}, $newnet->{trunks}) || 

Should this be $safe_string_ne()? (Since it's a ';' separated list and 
possibly contains 'x-y' ranges. 

> &$safe_num_ne($oldnet->{firewall}, $newnet->{firewall})) { 
> PVE::Network::tap_unplug($iface); 
> - PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}); 
> + PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}); 
> } 
> 
> if (&$safe_string_ne($oldnet->{link_down}, $newnet->{link_down})) { 
> diff --git a/pve-bridge b/pve-bridge 
> index c23c643..4426c65 100755 
> --- a/pve-bridge 
> +++ b/pve-bridge 
> @@ -40,7 +40,7 @@ PVE::Network::tap_create($iface, $net->{bridge}); 
> 
> # if ovs is under this bridge all traffic control settings will be flushed. 
> # so we need to call tap_rate_limit after tap_plug 
> -PVE::Network::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}); 
> +PVE::Network::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}); 
> 
> PVE::Network::tap_rate_limit($iface, $net->{rate}) if $net->{rate}; 
> 
> -- 
> 2.1.4