On Mon, Jul 27, 2020 at 11:35 PM Mark Michelson <[email protected]> wrote:
> Acked-by: Mark Michelson <[email protected] Thanks Mark. I applied this patch to master and branch-20.06. Numan > > > On 7/24/20 5:02 PM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > 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 <[email protected]> > > --- > > > > v1 -> v2 > > ---- > > * Addressed the review comment from Mark. > > > > controller/binding.c | 54 ++++++++++++++++++++++++----- > > tests/ovn.at | 82 ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 128 insertions(+), 8 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index b73abb982c..520bff080f 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,34 @@ 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; > > + struct local_datapath *peer_ld = NULL; > > + peer = get_peer_lport(pb, b_ctx_in); > > + if (peer) { > > + 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 > > > > _______________________________________________ > 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
