On Fri, Feb 16, 2024 at 2:50 PM Numan Siddique <[email protected]> wrote:
>
> 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
>
Thanks Numan. Applied to main and backported to branch-24.03.
Han
> > ---
> > 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
