Message ID | 20200724193845.1956595-1-numans@ovn.org |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,ovn] ovn-controller: Fix the missing flows when logical router port is added after its peer. | expand |
Hi Numan, just one comment down below: On 7/24/20 3:38 PM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > When the logical router port is created by CMS after its peer port is created like > below, ovn-controller doesn't add the logical router to the local_datapaths and > hence misses programming the flows for the router datapath if the logical switch > has logical ports which are already bound. This breaks routing for these logical ports > connected to this router. > > ovn-nbctl lsp-add sw0 sw0-lr0 > ovn-nbctl lsp-set-type sw0-lr0 router > ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:00:01 > > ovn-nbctl lr-add lr0 > ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:00:01 192.168.1.1/24 > > This patch fixes this issue. > > Fixes: 354bdba51abf("ovn-controller: I-P for SB port binding and OVS interface in runtime_data.") > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1860053 > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/binding.c | 51 ++++++++++++++++++++++----- > tests/ovn.at | 82 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 125 insertions(+), 8 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index b73abb982c..1936b93e7a 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1522,6 +1522,22 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > return !any_changes; > } > > +static const struct sbrec_port_binding * > +get_peer_lport(const struct sbrec_port_binding *pb, > + struct binding_ctx_in *b_ctx_in) > +{ > + const char *peer_name = smap_get(&pb->options, "peer"); > + if (strcmp(pb->type, "patch") || !peer_name) { > + return NULL; > + } > + > + const struct sbrec_port_binding *peer; > + peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, > + peer_name); > + > + return (peer && peer->datapath) ? peer : NULL; > +} > + > /* This function adds the local datapath of the 'peer' of > * lport 'pb' to the local datapaths if it is not yet added. > */ > @@ -1531,16 +1547,10 @@ add_local_datapath_peer_port(const struct sbrec_port_binding *pb, > struct binding_ctx_out *b_ctx_out, > struct local_datapath *ld) > { > - const char *peer_name = smap_get(&pb->options, "peer"); > - if (strcmp(pb->type, "patch") || !peer_name) { > - return; > - } > - > const struct sbrec_port_binding *peer; > - peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, > - peer_name); > + peer = get_peer_lport(pb, b_ctx_in); > > - if (!peer || !peer->datapath) { > + if (!peer) { > return; > } > > @@ -2118,6 +2128,31 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, > case LP_VTEP: > update_local_lport_ids(pb, b_ctx_out); > if (lport_type == LP_PATCH) { > + if (!ld) { > + /* If 'ld' for this lport is not present, then check if > + * there is a peer for this lport. If peer is present > + * and peer's datapath is already in the local datapaths, > + * then add this lport's datapath to the local_datapaths. > + * */ > + const struct sbrec_port_binding *peer; > + peer = get_peer_lport(pb, b_ctx_in); Should there be a NULL check here? > + struct local_datapath *peer_ld = > + get_local_datapath(b_ctx_out->local_datapaths, > + peer->datapath->tunnel_key); > + if (peer_ld) { > + add_local_datapath( > + b_ctx_in->sbrec_datapath_binding_by_key, > + b_ctx_in->sbrec_port_binding_by_datapath, > + b_ctx_in->sbrec_port_binding_by_name, > + pb->datapath, false, > + b_ctx_out->local_datapaths, > + b_ctx_out->tracked_dp_bindings); > + } > + > + ld = get_local_datapath(b_ctx_out->local_datapaths, > + pb->datapath->tunnel_key); > + } > + > /* Add the peer datapath to the local datapaths if it's > * not present yet. > */ > diff --git a/tests/ovn.at b/tests/ovn.at > index 7c9e14e2ea..3be62584d2 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -20806,3 +20806,85 @@ AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"]) > > OVN_CLEANUP([hv1], [hv2]) > AT_CLEANUP > + > + > +AT_SETUP([ovn -- controller I-P handling when lrp added last]) > +AT_KEYWORDS([lb]) > + > +ovn_start > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +# Step 1. Add OVS interface with external_ids:iface-id set. > +# Step 2. Create the logical switch and logical port. > +# Step 3. Create logical switch port of type router and set the peer. > +# Step 4. Create logical router and the logical router port (peer to logical switch) > +# Step 5. Check that all the flows are added and logical port gets arp reply for > +# router IP. > + > +as hv1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +ovn-nbctl ls-add sw0 > +ovn-nbctl lsp-add sw0 sw0-p1 > +ovn-nbctl lsp-set-addresses sw0-p1 "00:00:00:01:01:02 192.168.1.2" > + > +ovn-nbctl lsp-add sw0 sw0-lr0 > +ovn-nbctl lsp-set-type sw0-lr0 router > +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > +ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:00:01 > + > +ovn-nbctl lr-add lr0 > +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:00:01 192.168.1.1/24 aef0:0:0:0:0:0:0:1/64 > + > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > + > +sw0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding sw0) > +lr0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding lr0) > + > +AT_CHECK([test $(as hv1 ovs-ofctl dump-flows br-int metadata=0x${sw0_dpkey} | wc -l) -gt 80]) > +AT_CHECK([test $(as hv1 ovs-ofctl dump-flows br-int metadata=0x${lr0_dpkey} | wc -l) -gt 80]) > + > +# test_arp INPORT SHA SPA TPA [REPLY_HA] > +# > +# Causes a packet to be received on INPORT. The packet is an ARP > +# request with SHA, SPA, and TPA as specified. If REPLY_HA is provided, then > +# it should be the hardware address of the target to expect to receive in an > +# ARP reply; otherwise no reply is expected. > +# > +# INPORT is an logical switch port number, e.g. 11 for vif11. > +# SHA and REPLY_HA are each 12 hex digits. > +# SPA and TPA are each 8 hex digits. > +test_arp() { > + local hv=$1 inport=$2 sha=$3 spa=$4 tpa=$5 reply_ha=$6 > + local request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa} > + as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request > + > + if test X$reply_ha != X; then > + # Expect to receive the reply, if any. > + local reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa} > + echo $reply >> hv${hv}-vif$inport.expected > + fi > +} > + > +ip_to_hex() { > + printf "%02x%02x%02x%02x" "$@" > +} > + > +sw0p1_ip=$(ip_to_hex 192 168 1 2) > +rtr_ip=$(ip_to_hex 192 168 1 1) > +test_arp 1 1 000000010102 $sw0p1_ip $rtr_ip 000000000001 > + > +# Now check the packets actually received against the ones expected. > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [hv1-vif1.expected]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP >
diff --git a/controller/binding.c b/controller/binding.c index b73abb982c..1936b93e7a 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1522,6 +1522,22 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, return !any_changes; } +static const struct sbrec_port_binding * +get_peer_lport(const struct sbrec_port_binding *pb, + struct binding_ctx_in *b_ctx_in) +{ + const char *peer_name = smap_get(&pb->options, "peer"); + if (strcmp(pb->type, "patch") || !peer_name) { + return NULL; + } + + const struct sbrec_port_binding *peer; + peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, + peer_name); + + return (peer && peer->datapath) ? peer : NULL; +} + /* This function adds the local datapath of the 'peer' of * lport 'pb' to the local datapaths if it is not yet added. */ @@ -1531,16 +1547,10 @@ add_local_datapath_peer_port(const struct sbrec_port_binding *pb, struct binding_ctx_out *b_ctx_out, struct local_datapath *ld) { - const char *peer_name = smap_get(&pb->options, "peer"); - if (strcmp(pb->type, "patch") || !peer_name) { - return; - } - const struct sbrec_port_binding *peer; - peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, - peer_name); + peer = get_peer_lport(pb, b_ctx_in); - if (!peer || !peer->datapath) { + if (!peer) { return; } @@ -2118,6 +2128,31 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, case LP_VTEP: update_local_lport_ids(pb, b_ctx_out); if (lport_type == LP_PATCH) { + if (!ld) { + /* If 'ld' for this lport is not present, then check if + * there is a peer for this lport. If peer is present + * and peer's datapath is already in the local datapaths, + * then add this lport's datapath to the local_datapaths. + * */ + const struct sbrec_port_binding *peer; + peer = get_peer_lport(pb, b_ctx_in); + struct local_datapath *peer_ld = + get_local_datapath(b_ctx_out->local_datapaths, + peer->datapath->tunnel_key); + if (peer_ld) { + add_local_datapath( + b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + pb->datapath, false, + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); + } + + ld = get_local_datapath(b_ctx_out->local_datapaths, + pb->datapath->tunnel_key); + } + /* Add the peer datapath to the local datapaths if it's * not present yet. */ diff --git a/tests/ovn.at b/tests/ovn.at index 7c9e14e2ea..3be62584d2 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -20806,3 +20806,85 @@ AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"]) OVN_CLEANUP([hv1], [hv2]) AT_CLEANUP + + +AT_SETUP([ovn -- controller I-P handling when lrp added last]) +AT_KEYWORDS([lb]) + +ovn_start +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +# Step 1. Add OVS interface with external_ids:iface-id set. +# Step 2. Create the logical switch and logical port. +# Step 3. Create logical switch port of type router and set the peer. +# Step 4. Create logical router and the logical router port (peer to logical switch) +# Step 5. Check that all the flows are added and logical port gets arp reply for +# router IP. + +as hv1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +ovn-nbctl ls-add sw0 +ovn-nbctl lsp-add sw0 sw0-p1 +ovn-nbctl lsp-set-addresses sw0-p1 "00:00:00:01:01:02 192.168.1.2" + +ovn-nbctl lsp-add sw0 sw0-lr0 +ovn-nbctl lsp-set-type sw0-lr0 router +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 +ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:00:01 + +ovn-nbctl lr-add lr0 +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:00:01 192.168.1.1/24 aef0:0:0:0:0:0:0:1/64 + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) + +sw0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding sw0) +lr0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding lr0) + +AT_CHECK([test $(as hv1 ovs-ofctl dump-flows br-int metadata=0x${sw0_dpkey} | wc -l) -gt 80]) +AT_CHECK([test $(as hv1 ovs-ofctl dump-flows br-int metadata=0x${lr0_dpkey} | wc -l) -gt 80]) + +# test_arp INPORT SHA SPA TPA [REPLY_HA] +# +# Causes a packet to be received on INPORT. The packet is an ARP +# request with SHA, SPA, and TPA as specified. If REPLY_HA is provided, then +# it should be the hardware address of the target to expect to receive in an +# ARP reply; otherwise no reply is expected. +# +# INPORT is an logical switch port number, e.g. 11 for vif11. +# SHA and REPLY_HA are each 12 hex digits. +# SPA and TPA are each 8 hex digits. +test_arp() { + local hv=$1 inport=$2 sha=$3 spa=$4 tpa=$5 reply_ha=$6 + local request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa} + as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request + + if test X$reply_ha != X; then + # Expect to receive the reply, if any. + local reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa} + echo $reply >> hv${hv}-vif$inport.expected + fi +} + +ip_to_hex() { + printf "%02x%02x%02x%02x" "$@" +} + +sw0p1_ip=$(ip_to_hex 192 168 1 2) +rtr_ip=$(ip_to_hex 192 168 1 1) +test_arp 1 1 000000010102 $sw0p1_ip $rtr_ip 000000000001 + +# Now check the packets actually received against the ones expected. +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [hv1-vif1.expected]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP