[ovs-dev] [PATCH ovn] Don't suppress localport traffic directed to external port

Ihar Hrachyshka ihrachys at redhat.com
Wed Jul 14 00:43:01 UTC 2021


On Wed, Jul 7, 2021 at 4:16 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 7/7/21 5:20 AM, Ihar Hrachyshka wrote:
> > Recently, we stopped leaking localport traffic through localnet ports
> > into fabric to avoid unnecessary flipping between chassis hosting the
> > same localport.
> >
> > Despite the type name, in some scenarios localports are supposed to talk
> > outside the hosting chassis. Specifically, in OpenStack [1] metadata
> > service for SR-IOV ports is implemented as a localport hosted on another
> > chassis that is exposed to the chassis owning the SR-IOV port through an
> > "external" port. In this case, "leaking" localport traffic into fabric
> > is desirable.
> >
> > This patch inserts a higher priority flow per external port on the
> > same datapath that avoids dropping localport traffic.
> >
> > Fixes: 96959e56d634 ("physical: do not forward traffic from localport to
> > a localnet one")
> >
> > [1] https://docs.openstack.org/neutron/latest/admin/ovn/sriov.html
> >
> > Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
> > ---
>
> Hi Ihar,
>
> Thanks for working on this!
>
> I just had a glance at the change so this is not a full review.
>
> >  controller/physical.c | 48 ++++++++++++++++++++++++++++++++++++++
> >  tests/ovn.at          | 54 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 102 insertions(+)
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 17ca5afbb..c2de30941 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -920,6 +920,7 @@ get_binding_peer(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >
> >  static void
> >  consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > +                      const struct sbrec_port_binding_table *pb_table,
> >                        enum mf_field_id mff_ovn_geneve,
> >                        const struct simap *ct_zones,
> >                        const struct sset *active_tunnels,
> > @@ -1281,6 +1282,49 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >              ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160,
> >                              binding->header_.uuid.parts[0], &match,
> >                              ofpacts_p, &binding->header_.uuid);
> > +
> > +            /* Localport traffic directed to external is *not* local. */
> > +            const struct sbrec_port_binding *peer;
> > +            SBREC_PORT_BINDING_TABLE_FOR_EACH (peer, pb_table) {
> > +                if (strcmp(peer->type, "external")) {
> > +                    continue;
> > +                }
> > +                if (peer->datapath->tunnel_key != dp_key) {
> > +                    continue;
> > +                }
> > +                if (strcmp(peer->chassis->name, chassis->name)) {
> > +                    continue;
> > +                }
>
> Won't this create a scalability issue?  If I'm reading this correctly,
> every time consider_port_binding() is called for a localnet port we'll
> be walking all port_bindings in the SB DB table (there can be a lot of
> them in scaled scenarios) and skip most of them because they're not of
> type external or they're not owned by the local chassis or they're on a
> different datapath.
>
> One option would be to use an IDL index instead (although that's still
> log(n) complexity for every localnet port, I think).  Another option
> would be to precompute the set of external ports for each datapath so we
> don't have to walk all ports every time.
>

Thanks for the comment. I went with precalculation like we do for
local_datapath->localnet_port. I hope I haven't missed any free() /
clear () / destroy() calls. See v3 of the patch I just sent.

> > +
> > +                ofpbuf_clear(ofpacts_p);
> > +                for (int i = 0; i < MFF_N_LOG_REGS; i++) {
> > +                    put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p);
> > +                }
> > +                put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p);
> > +
> > +                for (int i = 0; i < peer->n_mac; i++) {
> > +                    char *err_str;
> > +                    struct eth_addr peer_mac;
> > +                    if ((err_str = str_to_mac(peer->mac[i], &peer_mac))) {
> > +                        VLOG_WARN("Parsing MAC failed for external port: %s, "
> > +                                "with error: %s", peer->logical_port, err_str);
>
> This probably needs rate limiting.
>

Done.

> Regards,
> Dumitru
>

Thanks for the review, Dumitru. I hope v3 is somewhat better. Let me know.

Cheers,
Ihar



More information about the dev mailing list