Skip to content

Commit

Permalink
physical: Don't reset encap ID across pipelines.
Browse files Browse the repository at this point in the history 
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: 17b6a12 ("ovn-controller: Support VIF-based local encap IPs selection.")
Signed-off-by: Han Zhou <[email protected]>
Acked-by: Numan Siddique <[email protected]>
  • Loading branch information
@hzhou8
hzhou8 committed Feb 20, 2024
1 parent 65f9f01 commit 68acb36
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 13 deletions.
28 changes: 16 additions & 12 deletions controller/physical.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);

Expand All @@ -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 *
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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++) {
Expand Down Expand Up @@ -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 */
Expand Down
83 changes: 82 additions & 1 deletion tests/ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30569,7 +30569,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
Expand Down Expand Up @@ -30664,6 +30664,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
Expand Down

0 comments on commit 68acb36

Please sign in to comment.