On Wed, Mar 22, 2023 at 2:19 PM Dumitru Ceara <dce...@redhat.com> wrote:
> On 3/22/23 12:51, Enrique Llorente wrote:
> > The localnet LSPs skip responding ARP/NDP [1], this change relax this
> > restriction by allowing responding from localnet LSPs when owner
> > switch has a router LSP with a proper arp_proxy configuration.
> >
> > [1] ovn-northd.8.xml
> >
> > Signed-off-by: Enrique Llorente <ellor...@redhat.com>
> > ---
>
> Thanks for the patch, Enrique!
>
> This is not a full review, just a couple of initial comments.
>
> > northd/northd.c | 21 +++++++++++++--------
> > northd/northd.h | 1 +
> > ovs | 2 +-
> > tests/system-ovn.at | 34 ++++++++++++++++++++++++++++++----
>
> We should probably document this behavior in ovn-nb.xml.
>
Added some docs at ovn-northd man page at v2
>
> > 4 files changed, 45 insertions(+), 13 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 5f0b436c2..941ef4204 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -2721,12 +2721,17 @@ join_logical_ports(struct northd_input
> *input_data,
> > */
> > const char *arp_proxy =
> smap_get(&op->nbsp->options,"arp_proxy");
> > int ofs = 0;
> > - if (arp_proxy &&
> > - !extract_addresses(arp_proxy, &op->proxy_arp_addrs,
> &ofs) &&
> > - !extract_ip_addresses(arp_proxy, &op->proxy_arp_addrs))
> {
> > - static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 5);
> > - VLOG_WARN_RL(&rl, "Invalid arp_proxy option: '%s' at
> lsp '%s'",
> > - arp_proxy, op->nbsp->name);
> > + if (arp_proxy) {
> > + if (extract_addresses(arp_proxy, &op->proxy_arp_addrs,
> &ofs) ||
> > + extract_ip_addresses(arp_proxy,
> &op->proxy_arp_addrs)) {
> > + op->od->has_arp_proxy_port = true;
> > + } else {
> > + static struct vlog_rate_limit rl =
> > + VLOG_RATE_LIMIT_INIT(1, 5);
> > + VLOG_WARN_RL(&rl,
> > + "Invalid arp_proxy option: '%s' at lsp '%s'",
> > + arp_proxy, op->nbsp->name);
> > + }
> > }
> > } else if (op->nbrp && op->nbrp->peer && !op->l3dgw_port) {
> > struct ovn_port *peer = ovn_port_find(ports,
> op->nbrp->peer);
> > @@ -8428,7 +8433,7 @@ build_lswitch_arp_nd_responder_skip_local(struct
> ovn_port *op,
> > struct hmap *lflows,
> > struct ds *match)
> > {
> > - if (op->nbsp && lsp_is_localnet(op->nbsp)) {
> > + if (op->nbsp && lsp_is_localnet(op->nbsp) &&
> !op->od->has_arp_proxy_port) {
> > ds_clear(match);
> > ds_put_format(match, "inport == %s", op->json_key);
> > ovn_lflow_add_with_lport_and_hint(lflows, op->od,
> > @@ -14602,7 +14607,7 @@ build_lswitch_and_lrouter_iterate_by_op(struct
> ovn_port *op,
> > build_lswitch_learn_fdb_op(op, lsi->lflows, &lsi->actions,
> > &lsi->match);
> > build_lswitch_arp_nd_responder_skip_local(op, lsi->lflows,
> > - &lsi->match);
> > + &lsi->match);
> > build_lswitch_arp_nd_responder_known_ips(op, lsi->lflows,
> > lsi->ports,
> > lsi->meter_groups,
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 4d9055296..5a666bbd3 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -214,6 +214,7 @@ struct ovn_datapath {
> > bool has_unknown;
> > bool has_acls;
> > bool has_vtep_lports;
> > + bool has_arp_proxy_port;
> >
> > /* IPAM data. */
> > struct ipam_info ipam_info;
> > diff --git a/ovs b/ovs
> > index b72a7f925..8986d4d55 160000
> > --- a/ovs
> > +++ b/ovs
> > @@ -1 +1 @@
> > -Subproject commit b72a7f92573aa4e6205e57cb978532b4c04702e1
> > +Subproject commit 8986d4d5564401eeef3dea828b51fe8bae2cc8aa
>
>
> This is probably an accidental change. And it's also causing the system
> userspace datapath LB ICMP related traffic test to fail.
>
Fixed at v2
>
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index ad1188078..f820a5206 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -10431,6 +10431,8 @@ AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> > ovn_start
> > OVS_TRAFFIC_VSWITCHD_START()
> > ADD_BR([br-int])
> > +ADD_BR([br-ext])
> > +ovs-ofctl add-flow br-ext action=normal
> >
> > # Set external-ids in br-int needed for ovn-controller
> > ovs-vsctl \
> > @@ -10447,7 +10449,9 @@ start_daemon ovn-controller
> > # One LR - R1 and two LSs - foo and bar, R1 has switches foo (
> 192.168.1.0/24) and
> > # bar (192.168.2.0/24) connected to it
> > #
> > -# foo -- R1 -- bar
> > +# br-ext -- localnet -- foo -- R1 -- bar
> > +
> > +check ovs-vsctl set open .
> external-ids:ovn-bridge-mappings=provider:br-ext
> >
> > check ovn-nbctl lr-add R1
> > check ovn-nbctl ls-add foo
> > @@ -10456,13 +10460,14 @@ check ovn-nbctl ls-add bar
> > # Connect foo to R1
> > check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24
> > check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
> > - type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254
> 169.254.239.2 169.254.238.0/24 " options:router-port=foo
> addresses='"router"'
> > + type=router options:arp_proxy="0a:58:a9:fe:01:01 169.254.239.254
> 169.254.239.2 169.254.238.0/24 192.168.1.100" options:router-port=foo
> addresses='"router"'
> >
> > # Connect bar to R1
> > check ovn-nbctl lrp-add R1 bar 00:00:01:01:02:04 192.168.2.1/24
> > check ovn-nbctl lsp-add bar rp-bar -- set Logical_Switch_Port rp-bar \
> > type=router options:arp_proxy="169.254.239.253"
> options:router-port=bar addresses='"router"'
> >
> > +
> > # Logical port 'foo1' in switch 'foo'.
> > ADD_NAMESPACES(foo1)
> > ADD_VETH(foo1, foo1, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> > @@ -10484,12 +10489,20 @@ ADD_VETH(foo3, foo3, br-int, "192.168.1.4/24",
> "f0:00:00:01:02:05", \
> > check ovn-nbctl lsp-add foo foo3 \
> > -- lsp-set-addresses foo3 "f0:00:00:01:02:05 192.168.1.4"
> >
> > +# Logical port 'ext1' in switch 'foo'
> > +ADD_NAMESPACES(ext1)
> > +ADD_VETH(ext1, ext1, br-ext, "192.168.1.5/24", "f0:00:00:01:02:06", \
> > + "192.168.1.100",)
> > +check ovn-nbctl lsp-add foo ln -- lsp-set-options ln
> network_name=provider
> > +check ovn-nbctl lsp-set-type ln localnet
> > +check ovn-nbctl lsp-set-addresses ln unknown
> > +
> > # Logical port 'bar1' in switch 'bar'.
> > ADD_NAMESPACES(bar1)
> > -ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:06", \
> > +ADD_VETH(bar1, bar1, br-int, "192.168.2.2/24", "f0:00:00:01:02:07", \
> > "169.254.239.253")
> > check ovn-nbctl lsp-add bar bar1 \
> > --- lsp-set-addresses bar1 "f0:00:00:01:02:06 192.168.2.2"
> > +-- lsp-set-addresses bar1 "f0:00:00:01:02:07 192.168.2.2"
> >
> > # wait for ovn-controller to catch up.
> > check ovn-nbctl --wait=hv sync
> > @@ -10533,6 +10546,19 @@ OVS_WAIT_UNTIL([
> > test "${total_pkts}" = "3"
> > ])
> >
> > +NETNS_DAEMONIZE([ext1], [tcpdump -l -nn -e -i ext1 'ether dst
> 0a:58:a9:fe:01:01 and icmp' > ext1-icmp.pcap 2>ext1-tcpdump.stderr],
> [ext1-icmp-tcpdump.pid])
> > +OVS_WAIT_UNTIL([grep "listening" ext1-tcpdump.stderr])
> > +
> > +# 'ext1' should be able to ping 'bar1'
> > +NS_CHECK_EXEC([ext1], [ping -q -c 3 -i 0.3 -w 2 192.168.2.2 |
> FORMAT_PING], \
> > +[0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +OVS_WAIT_UNTIL([
> > + total_pkts=$(cat ext1-icmp.pcap| wc -l)
> > + test "${total_pkts}" = "3"
> > +])
> > +
> >
> > OVS_APP_EXIT_AND_WAIT([ovn-controller])
> >
>
> Regards,
> Dumitru
>
>
--
*Quique Llorente*
CNV networking Senior Software Engineer
Red Hat EMEA <https://www.redhat.com/>
ellor...@redhat.com <arxc...@redhat.com>
@RedHat <https://twitter.com/redhat> Red Hat
<https://www.linkedin.com/company/red-hat> Red Hat
<https://www.facebook.com/RedHatInc>
<https://www.redhat.com/>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
