Hi Mark, thanks for comments. I figured the issues you pointed out and there's a new version up for review. Please take a look.
Ihar On Wed, Mar 31, 2021 at 9:37 PM Mark Michelson <[email protected]> wrote: > > Hi Ihar, > > See comments in-line > > On 3/29/21 9:29 PM, Ihar Hrachyshka wrote: > > In some environments, hardware serving the fabric network doesn't > > support double 802.1q (0x8100) VLAN tags, but does support 802.11ad > > (0x8a88) EthType for two layer VLAN traffic. Specifically, Cisco > > hardware UCS VIC was identified affected by this limitation. > > > > With vlan-passthru=true set for a logical switch, VLAN tagged traffic > > may be generated by VIFs. This patch allows to support the feature in > > affected hardware environments. > > > > Signed-off-by: Ihar Hrachyshka <[email protected]> > > --- > > NEWS | 1 + > > controller/physical.c | 33 +++++++++++++---- > > ovn-nb.xml | 6 +++ > > tests/ovn.at | 85 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 117 insertions(+), 8 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 530c5d42f..9e45837c7 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -7,6 +7,7 @@ Post-v21.03.0 > > (This may take testing and tuning to be effective.) This version of > > OVN > > requires DDLog 0.36. > > - Introduce ovn-controller incremetal processing engine statistics > > + - Support custom (802.11ad, 0x8a88) EthType for localnet ports. > > > > OVN v21.03.0 - 12 Mar 2021 > > ------------------------- > > diff --git a/controller/physical.c b/controller/physical.c > > index fa5d0d692..66395bc09 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -609,6 +609,28 @@ put_replace_chassis_mac_flows(const struct simap > > *ct_zones, > > } > > } > > > > +static void > > +ofpact_put_push_vlan(struct ofpbuf *ofpacts, const struct smap *options, > > int tag) > > +{ > > + const char *ethtype_opt = NULL; > > + if (options) { > > + ethtype_opt = smap_get(options, "ethtype"); > > + } > > + > > + int ethtype; > > + if (!ethtype_opt || !str_to_int(ethtype_opt, 16, ðtype)) { > > + ethtype = 0x8100; > > + } > > + struct ofpact_push_vlan *push_vlan; > > + push_vlan = ofpact_put_PUSH_VLAN(ofpacts); > > + push_vlan->ethertype = htons(ethtype); > > + > > + struct ofpact_vlan_vid *vlan_vid; > > + vlan_vid = ofpact_put_SET_VLAN_VID(ofpacts); > > + vlan_vid->vlan_vid = tag; > > + vlan_vid->push_vlan_if_needed = false; > > +} > > + > > static void > > put_replace_router_port_mac_flows(struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > @@ -696,10 +718,7 @@ put_replace_router_port_mac_flows(struct > > ovsdb_idl_index > > replace_mac->mac = chassis_mac; > > > > if (tag) { > > - struct ofpact_vlan_vid *vlan_vid; > > - vlan_vid = ofpact_put_SET_VLAN_VID(ofpacts_p); > > - vlan_vid->vlan_vid = tag; > > - vlan_vid->push_vlan_if_needed = true; > > + ofpact_put_push_vlan(ofpacts_p, &localnet_port->options, tag); > > } > > > > ofpact_put_OUTPUT(ofpacts_p)->port = ofport; > > @@ -1195,10 +1214,8 @@ consider_port_binding(struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > if (tag) { > > /* For containers sitting behind a local vif, tag the packets > > * before delivering them. */ > > - struct ofpact_vlan_vid *vlan_vid; > > - vlan_vid = ofpact_put_SET_VLAN_VID(ofpacts_p); > > - vlan_vid->vlan_vid = tag; > > - vlan_vid->push_vlan_if_needed = true; > > + ofpact_put_push_vlan( > > + ofpacts_p, localnet_port? &localnet_port->options: NULL, > > tag); > > Nit: The '?' and ':' should have a single space before them. > > > } > > ofpact_put_OUTPUT(ofpacts_p)->port = ofport; > > if (tag) { > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index b0a4adffe..fca22988b 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -830,6 +830,12 @@ > > uses its local configuration to determine exactly how to > > connect to > > this locally accessible network, if at all. > > </column> > > + > > + <column name="options" key="ethtype"> > > + Optional. VLAN EtherType field value for encapsulating VLAN > > + headers. Supported values: 8100 (default), 8a88 (QinQ). > > + </column> > > + > > </group> > > > > <group title="Options for l2gateway ports"> > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 391a8bcd9..03b10b793 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -3164,6 +3164,91 @@ OVN_CLEANUP([hv-1],[hv-2]) > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- VLAN transparency, passthru=true, multiple hosts, custom > > ethtype]) > > +ovn_start > > + > > +ethtype=88a8 > > + > > +check ovn-nbctl ls-add ls > > +check ovn-nbctl --wait=sb add Logical-Switch ls other_config > > vlan-passthru=true > > + > > +ln_port_name=ln-100 > > +ovn-nbctl lsp-add ls $ln_port_name "" 100 > > +ovn-nbctl lsp-set-addresses $ln_port_name unknown > > +ovn-nbctl lsp-set-type $ln_port_name localnet > > +ovn-nbctl lsp-set-options $ln_port_name network_name=phys-100 > > ethtype=0x$ethtype > > +net_add n-100 > > + > > +# two hypervisors, each connected to the same network > > +for i in 1 2; do > > + sim_add hv-$i > > + as hv-$i > > + ovs-vsctl add-br br-phys > > + ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys-100:br-phys > > + ovn_attach n-100 br-phys 192.168.0.$i > > +done > > + > > +for i in 1 2; do > > + check ovn-nbctl lsp-add ls lsp$i > > + check ovn-nbctl lsp-set-addresses lsp$i f0:00:00:00:00:0$i > > +done > > + > > +for i in 1 2; do > > + as hv-$i > > + ovs-vsctl add-port br-int vif$i -- set Interface vif$i > > external-ids:iface-id=lsp$i \ > > + options:tx_pcap=vif$i-tx.pcap \ > > + options:rxq_pcap=vif$i-rx.pcap \ > > + ofport-request=$i > > + OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lsp$i` = xup]) > > +done > > + > > +# create taps on fabric to check vlan encapsulation there > > +for i in 1 2; do > > + as hv-$i > > + ovs-vsctl add-port br-phys tap$i -- set Interface tap$i \ > > + options:tx_pcap=tap$i-tx.pcap \ > > + options:rxq_pcap=tap$i-rx.pcap > > +done > > + > > +for i in 1 2; do > > + : > $i.expected > > +done > > +: > tap.expected > > + > > +test_packet() { > > + local inport=$1 dst=$2 src=$3 eth=$4 eout=$5 lout=$6 > > + > > + # First try tracing the packet. > > + uflow="inport==\"lsp$inport\" && eth.dst==$dst && eth.src==$src && > > eth.type==0x$eth && vlan.present==1" > > + echo "output(\"$lout\");" > expout > > + AT_CAPTURE_FILE([trace]) > > + AT_CHECK([ovn-trace --all ls "$uflow" | tee trace | sed '1,/Minimal > > trace/d'], [0], [expout]) > > + > > + # Then actually send a packet, for an end-to-end test. > > + payload=fefefefe > > + local packet=$(echo $dst$src | sed 's/://g')${eth}${payload} > > + vif=vif$inport > > + as hv-$1 > > + ovs-appctl netdev-dummy/receive $vif $packet > > + echo $packet >> ${eout#lsp}.expected > > $eout is set to ln-100 in both calls to test_packet(), so you are > appending $packet to ln-100.expected. I suspect you meant to use "lsp1" > and "lsp2" here. This way, 1.expected and 2.expected would get populated > with the packet. > > > + > > + phys_packet=$(echo $dst$src | sed > > 's/://g')${ethtype}0064${eth}${payload} > > + echo $phys_packet >> tap.expected > > +} > > + > > +test_packet 1 f0:00:00:00:00:03 f0:00:00:00:00:01 8100 ln-100 ln-100 > > +test_packet 2 f0:00:00:00:00:03 f0:00:00:00:00:02 8100 ln-100 ln-100 > > +for i in 1 2; do > > + OVN_CHECK_PACKETS_REMOVE_BROADCAST([vif$i-tx.pcap], [$i.expected]) > > Because of the error in test_packet(), 1.expected and 2.expected are > both empty at this point. It happens that vif1-tx.pcap and vif2-tx.pcap > are also empty. So this check succeeds because it's comparing empty files. > > As for why vif1-tx.pcap and vif2-tx.pcap are empty, I don't know. When I > tested locally, I found that vif1-rx.pcap and vif2-rx.pcap had the > expected packets in them, interestingly. Doesn't that seem backwards? > > > +done > > +for i in 1 2; do > > + OVN_CHECK_PACKETS_REMOVE_BROADCAST([tap$i-tx.pcap], [tap.expected]) > > +done > > + > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([ovn -- VLAN transparency, passthru=true]) > > ovn_start > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
