On Feb 24, Numan Siddique wrote:
> On Thu, Feb 24, 2022 at 5:23 PM Lorenzo Bianconi
> <[email protected]> wrote:
> >
> > The CMS can configure the same lbs on multiple gw routers so the
> > same IPs/mac bindings will be adverised by multiple routers
> > through GARPs.
> > In order to fix the issue, introduce nat-only-router flag for
> > nat-addresses option in order to advertise in GARPs packets all
> > SNAT/DNAT configured in the connected logical router but skip
> > all configured load_balancers.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2054394
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2053013
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
>
> Hi Lorenzo,
Hi Numan,
>
> Thanks for updating the commit message and for the rebase.
>
> I see one problem with this approach. We will break the functionality of
> garps
> if CMS uses the new value "nat-only-router", and if ovn-northd is not updated
> to the version which has this support.
>
> Although we could argue that it is the problem of CMS, I think it's
> better we avoid
> this in OVN.
if ovn-northd is not updated and the CMS set nat-only-router in nat-addresses,
we should continue sending garp for all ips (including lbs)? If so, are still
having the issue of sending garps for lbs for multiple ports?
>
> Instead I would suggest adding another option -
> "exclude-lb-vips-from-garp=true".
> If CMS sets this option in the options column of logical_switch_port,
> ovn-northd
> can exclude the load balancer vips from the garps.
I guess it is more clear to keep the info in nat-address option but I can live
with it :)
>
> Let me know what you think. Can you please also add a test case in
> ovn-northd.at
ack, will do.
> which checks that the lb vips are not set in the port_bindings options
> column if this option is set.
>
> Can you please also add an entry in the NEWS item for this.,
ack, will do
>
> Also one small typo below.
ack, I will fix it.
Regards,
Lorenzo
>
> Thanks
> Numan
>
> > ---
> > Changes since v1:
> > - improve commit message
> > - rebase on top of ovn master
> > ---
> > northd/northd.c | 51 +++++++++++++++++++++++++++----------------------
> > ovn-nb.xml | 21 ++++++++++++++++++++
> > tests/ovn.at | 38 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 87 insertions(+), 23 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 219398f96..0d0fb7b95 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -1641,13 +1641,13 @@ destroy_routable_addresses(struct
> > ovn_port_routable_addresses *ra)
> > }
> >
> > static char **get_nat_addresses(const struct ovn_port *op, size_t *n,
> > - bool routable_only);
> > + bool routable_only, bool include_lb_ips);
> >
> > static void
> > assign_routable_addresses(struct ovn_port *op)
> > {
> > size_t n;
> > - char **nats = get_nat_addresses(op, &n, true);
> > + char **nats = get_nat_addresses(op, &n, true, true);
> >
> > if (!nats) {
> > return;
> > @@ -2711,7 +2711,8 @@ join_logical_ports(struct northd_input *input_data,
> > * The caller must free each of the n returned strings with free(),
> > * and must free the returned array when it is no longer needed. */
> > static char **
> > -get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only)
> > +get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only,
> > + bool include_lb_ips)
> > {
> > size_t n_nats = 0;
> > struct eth_addr mac;
> > @@ -2791,24 +2792,26 @@ get_nat_addresses(const struct ovn_port *op, size_t
> > *n, bool routable_only)
> > }
> > }
> >
> > - const char *ip_address;
> > - if (routable_only) {
> > - SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4_routable) {
> > - ds_put_format(&c_addresses, " %s", ip_address);
> > - central_ip_address = true;
> > - }
> > - SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6_routable) {
> > - ds_put_format(&c_addresses, " %s", ip_address);
> > - central_ip_address = true;
> > - }
> > - } else {
> > - SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) {
> > - ds_put_format(&c_addresses, " %s", ip_address);
> > - central_ip_address = true;
> > - }
> > - SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) {
> > - ds_put_format(&c_addresses, " %s", ip_address);
> > - central_ip_address = true;
> > + if (include_lb_ips) {
> > + const char *ip_address;
> > + if (routable_only) {
> > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4_routable) {
> > + ds_put_format(&c_addresses, " %s", ip_address);
> > + central_ip_address = true;
> > + }
> > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6_routable) {
> > + ds_put_format(&c_addresses, " %s", ip_address);
> > + central_ip_address = true;
> > + }
> > + } else {
> > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) {
> > + ds_put_format(&c_addresses, " %s", ip_address);
> > + central_ip_address = true;
> > + }
> > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) {
> > + ds_put_format(&c_addresses, " %s", ip_address);
> > + central_ip_address = true;
> > + }
> > }
> > }
> >
> > @@ -3374,10 +3377,12 @@ ovn_port_update_sbrec(struct northd_input
> > *input_data,
> > "nat-addresses");
> > size_t n_nats = 0;
> > char **nats = NULL;
> > - if (nat_addresses && !strcmp(nat_addresses, "router")) {
> > + if (nat_addresses && (!strcmp(nat_addresses,
> > "nat-only-router") ||
> > + !strcmp(nat_addresses, "router"))) {
> > if (op->peer && op->peer->od
> > && (chassis || op->peer->od->n_l3dgw_ports)) {
> > - nats = get_nat_addresses(op->peer, &n_nats, false);
> > + nats = get_nat_addresses(op->peer, &n_nats, false,
> > + !strcmp(nat_addresses,
> > "router"));
> > }
> > /* Only accept manual specification of ethernet address
> > * followed by IPv4 addresses on type "l3gateway" ports. */
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 642e91845..2d8d6ed4b 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -913,6 +913,27 @@
> > </p>
> > </dd>
> >
> > + <dt><code>nat-only-router</code></dt>
> > + <dd>
> > + <p>
> > + Gratuitous ARPs will be sent for all SNAT and DNAT
> > external IP
> > + addresses defined on the <ref column="options"
> > + key="router-port"/>'s logical router, using the
> > + <ref column="options" key="router-port"/>'s MAC address.
> > + </p>
> > +
> > + <p>
> > + This form of <ref column="options" key="nat-addresses"/> is
> > + valid for logical switch ports where <ref column="options"
> > + key="router-port"/> is the name of a port on a gateway
> > router,
> > + or the name of a distributed gateway port.
> > + </p>
> > +
> > + <p>
> > + Supported only in OVN 2.21.12.1 and later.
> s/2.21.12.1/21.12.1
>
> Although I'd say we can probably skip this in the documentation since
> the NEWS entry would cover it.
>
> Thanks
> Numan
>
> > + </p>
> > + </dd>
> > +
> > <dt><code>Ethernet address followed by one or more IPv4
> > addresses</code></dt>
> > <dd>
> > <p>
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index fa0e0a4a2..7f85aa6b3 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -8651,6 +8651,44 @@
> > expected="fffffffffffff0000000000108060001080006040001f00000000001c0a80003000000
> > echo $expected >> expout
> > AT_CHECK([sort packets], [0], [expout])
> >
> > +# Temporarily remove nat-addresses option to avoid race conditions
> > +# due to GARP backoff
> > +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses=""
> > +
> > +reset_pcap_file() {
> > + local iface=$1
> > + local pcap_file=$2
> > + ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \
> > +options:rxq_pcap=dummy-rx.pcap
> > + rm -f ${pcap_file}*.pcap
> > + ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap
> > \
> > +options:rxq_pcap=${pcap_file}-rx.pcap
> > +}
> > +
> > +as hv1 reset_pcap_file snoopvif hv1/snoopvif
> > +
> > +# Re-add nat-addresses option
> > +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0
> > nat-addresses="nat-only-router"
> > +
> > +# Wait for packets to be received.
> > +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 250])
> > +trim_zeros() {
> > + sed 's/\(00\)\{1,\}$//'
> > +}
> > +
> > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap |
> > trim_zeros > packets
> > +g0="fffffffffffff0000000000108060001080006040001f00000000001c0a80001000000000000c0a80001"
> > +echo $g0 > expout
> > +g1="fffffffffffff0000000000108060001080006040001f00000000001c0a80002000000000000c0a80002"
> > +echo $g1 >> expout
> > +
> > +grep $g0 packets | head -1 > exp
> > +grep $g1 packets | head -1 >> exp
> > +AT_CHECK([cat exp], [0], [expout])
> > +
> > +g3="fffffffffffff0000000000108060001080006040001f00000000001c0a80003000000000000c0a80003"
> > +AT_CHECK([grep -q $g3 packets], [1])
> > +
> > OVN_CLEANUP([hv1])
> >
> > AT_CLEANUP
> > --
> > 2.35.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
