On Mon, Feb 12, 2024 at 2:49 PM Han Zhou <[email protected]> wrote: > > The MFF_LOG_ENCAP_ID register was defined to save the encap ID and avoid > changing across pipelines, but in the function > load_logical_ingress_metadata it was reset unconditionally. Because of > this, the encap selection doesn't work when traffic traverses L3 > boundaries. This patch fixes it by ensuring the register is loaded only > in the flows of table 0, which is where packets from VIFs enter the > pipeline for the first time. > > Fixes: 17b6a12fa286 ("ovn-controller: Support VIF-based local encap IPs > selection.") > Signed-off-by: Han Zhou <[email protected]>
Acked-by: Numan Siddique <[email protected]> Numan > --- > controller/physical.c | 28 ++++++++------- > tests/ovn.at | 83 ++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 98 insertions(+), 13 deletions(-) > > diff --git a/controller/physical.c b/controller/physical.c > index c32642d2c69b..7ef259da44b1 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -73,7 +73,8 @@ load_logical_ingress_metadata(const struct > sbrec_port_binding *binding, > const struct zone_ids *zone_ids, > size_t n_encap_ips, > const char **encap_ips, > - struct ofpbuf *ofpacts_p); > + struct ofpbuf *ofpacts_p, > + bool load_encap_id); > static int64_t get_vxlan_port_key(int64_t port_key); > > /* UUID to identify OF flows not associated with ovsdb rows. */ > @@ -689,7 +690,7 @@ put_replace_chassis_mac_flows(const struct simap > *ct_zones, > ofpact_put_STRIP_VLAN(ofpacts_p); > } > load_logical_ingress_metadata(localnet_port, &zone_ids, 0, NULL, > - ofpacts_p); > + ofpacts_p, true); > replace_mac = ofpact_put_SET_ETH_SRC(ofpacts_p); > replace_mac->mac = router_port_mac; > > @@ -1047,7 +1048,8 @@ load_logical_ingress_metadata(const struct > sbrec_port_binding *binding, > const struct zone_ids *zone_ids, > size_t n_encap_ips, > const char **encap_ips, > - struct ofpbuf *ofpacts_p) > + struct ofpbuf *ofpacts_p, > + bool load_encap_id) > { > put_zones_ofpacts(zone_ids, ofpacts_p); > > @@ -1057,13 +1059,15 @@ load_logical_ingress_metadata(const struct > sbrec_port_binding *binding, > put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, ofpacts_p); > put_load(port_key, MFF_LOG_INPORT, 0, 32, ofpacts_p); > > - /* Default encap_id 0. */ > - size_t encap_id = 0; > - if (encap_ips && binding->encap) { > - encap_id = encap_ip_to_id(n_encap_ips, encap_ips, > - binding->encap->ip); > + if (load_encap_id) { > + /* Default encap_id 0. */ > + size_t encap_id = 0; > + if (encap_ips && binding->encap) { > + encap_id = encap_ip_to_id(n_encap_ips, encap_ips, > + binding->encap->ip); > + } > + put_load(encap_id, MFF_LOG_ENCAP_ID, 16, 16, ofpacts_p); > } > - put_load(encap_id, MFF_LOG_ENCAP_ID, 16, 16, ofpacts_p); > } > > static const struct sbrec_port_binding * > @@ -1108,7 +1112,7 @@ setup_rarp_activation_strategy(const struct > sbrec_port_binding *binding, > match_set_dl_type(&match, htons(ETH_TYPE_RARP)); > match_set_in_port(&match, ofport); > > - load_logical_ingress_metadata(binding, zone_ids, 0, NULL, &ofpacts); > + load_logical_ingress_metadata(binding, zone_ids, 0, NULL, &ofpacts, > true); > > encode_controller_op(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP, > NX_CTLR_NO_METER, &ofpacts); > @@ -1522,7 +1526,7 @@ consider_port_binding(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > put_load(0, MFF_LOG_CT_ZONE, 0, 16, ofpacts_p); > struct zone_ids peer_zones = get_zone_ids(peer, ct_zones); > load_logical_ingress_metadata(peer, &peer_zones, n_encap_ips, > - encap_ips, ofpacts_p); > + encap_ips, ofpacts_p, false); > put_load(0, MFF_LOG_FLAGS, 0, 32, ofpacts_p); > put_load(0, MFF_LOG_OUTPORT, 0, 32, ofpacts_p); > for (int i = 0; i < MFF_N_LOG_REGS; i++) { > @@ -1739,7 +1743,7 @@ consider_port_binding(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > uint32_t ofpacts_orig_size = ofpacts_p->size; > > load_logical_ingress_metadata(binding, &zone_ids, n_encap_ips, > - encap_ips, ofpacts_p); > + encap_ips, ofpacts_p, true); > > if (!strcmp(binding->type, "localport")) { > /* mark the packet as incoming from a localport */ > diff --git a/tests/ovn.at b/tests/ovn.at > index 902dd3793b92..30748c96e1c6 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -30426,7 +30426,7 @@ AT_CLEANUP > > > OVN_FOR_EACH_NORTHD([ > -AT_SETUP([multiple encap ips selection based on VIF's encap_ip]) > +AT_SETUP([multiple encap ips selection based on VIF's encap_ip - L2]) > AT_SKIP_IF([test $HAVE_SCAPY = no]) > ovn_start > net_add n1 > @@ -30521,6 +30521,87 @@ AT_CLEANUP > ]) > > > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([multiple encap ips selection based on VIF's encap_ip - L3]) > +AT_SKIP_IF([test $HAVE_SCAPY = no]) > +ovn_start > +net_add n1 > + > +ovn-nbctl lr-add lr > + > +# 2 HVs, each with 2 encap-ips, and each hosting a separate LS, connected by > a LR. > +# lspXY is the Yth LSP on hvX, with encap-ip 192.168.0.XY, with ip 10.0.X.Y > +for i in 1 2; do > + sim_add hv$i > + as hv$i > + ovs-vsctl add-br br-phys-$j > + ovn_attach n1 br-phys-$j 192.168.0.${i}1 > + ovs-vsctl set open . > external_ids:ovn-encap-ip=192.168.0.${i}1,192.168.0.${i}2 > + > + check ovn-nbctl ls-add ls$i -- \ > + lrp-add lr lrp-ls$i f0:00:00:00:88:0$i 10.0.$i.88/24 -- \ > + lsp-add ls$i lsp-lr-$i -- lsp-set-type lsp-lr-$i router -- \ > + lsp-set-addresses lsp-lr-$i router -- \ > + lsp-set-options lsp-lr-$i router-port=lrp-ls$i > + > + for j in 1 2; do > + ovs-vsctl add-port br-int vif$i$j -- set Interface vif$i$j \ > + external_ids:iface-id=lsp$i$j \ > + external_ids:encap-ip=192.168.0.$i$j \ > + options:tx_pcap=hv$i/vif$i$j-tx.pcap > options:rxq_pcap=hv$i/vif$i$j-rx.pcap > + ovn-nbctl lsp-add ls$i lsp$i$j -- lsp-set-addresses lsp$i$j > "f0:00:00:00:00:$i$j 10.0.$i.$j" > + > + done > +done > + > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +vif_to_hv() { > + case $1 in dnl ( > + vif[[1]]?) echo hv1 ;; dnl ( > + vif[[2]]?) echo hv2 ;; dnl ( > + *) AT_FAIL_IF([:]) ;; > + esac > +} > + > +vif_to_ip() { > + vif=$1 > + echo 10.0.${vif:3:1}.${vif:4:1} > +} > + > +# check_packet_tunnel SRC_VIF DST_VIF > +# Verify that a packet from SRC_VIF to DST_VIF goes through the corresponding > +# tunnel that matches the VIFs' encap_ip configurations. > +check_packet_tunnel() { > + local src=$1 dst=$2 > + local src_mac=f0:00:00:00:00:$src > + local dst_mac=f0:00:00:00:88:01 # lrp-ls1's MAC > + local src_ip=$(vif_to_ip vif$src) > + local dst_ip=$(vif_to_ip vif$dst) > + local local_encap_ip=192.168.0.$src > + local remote_encap_ip=192.168.0.$dst > + local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \ > + IP(dst='${dst_ip}', src='${src_ip}')/ \ > + ICMP(type=8)") > + hv=`vif_to_hv vif$src` > + as $hv > + echo "vif$src -> vif$dst should go through tunnel $local_encap_ip -> > $remote_encap_ip" > + tunnel_ofport=$(ovs-vsctl --bare --column=ofport find interface > options:local_ip=$local_encap_ip options:remote_ip=$remote_encap_ip) > + AT_CHECK([test $(ovs-appctl ofproto/trace br-int in_port=vif$src $packet > | grep "output:" | awk -F ':' '{ print $2 }') == $tunnel_ofport]) > +} > + > +for i in 1 2; do > + for j in 1 2; do > + check_packet_tunnel 1$i 2$j > + done > +done > + > +OVN_CLEANUP([hv1],[hv2]) > +AT_CLEANUP > +]) > + > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([Load Balancer LS hairpin OF flows]) > ovn_start > -- > 2.38.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
