[ovs-dev] [PATCH ovn] ovn-controller: Consider non-virtual ports first when updating bindings.

Numan Siddique numans at ovn.org
Thu Nov 28 13:36:28 UTC 2019


On Sat, Nov 23, 2019 at 3:51 AM Mark Michelson <mmichels at redhat.com> wrote:
>
> Acked-by: Mark Michelson <mmichels at redhat.com>
>
> On 11/22/19 9:22 AM, Dumitru Ceara wrote:
> > There's no guarantee SBREC_PORT_BINDING_TABLE_FOR_EACH will first
> > return the non-virtual ports and then virtual ports. In the case when a
> > virtual port is processed before its virtual_parent,
> > consider_local_datapath might not release it in the current
> > ovn-controller iteration even though the virtual_parent gets released.
> >
> > Right now this doesn't trigger any functionality issue because releasing
> > the virtual_parent triggers a change in the SB DB which will wake up
> > ovn-controller and trigger a new computation which will also update the
> > virtual port.
> >
> > However, this is suboptimal, and we can notice that often ovn-controller
> > gets the SB update notification before the "transaction successful"
> > notification. In such cases the incremental engine doesn't run
> > (ovnsb_idl_txn == NULL) and a full recompute is scheduled for the next
> > run. By batching the two SB updates in a single transaction we
> > lower the risk of this situation happening.
> >
> > CC: Numan Siddique <numans at ovn.org>
> > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
> > Signed-off-by: Dumitru Ceara <dceara at redhat.com>

Thanks. I applied this patch to master.

Numan

> > ---
> >   controller/binding.c | 97 +++++++++++++++++++++++++++++++++++++---------------
> >   1 file changed, 69 insertions(+), 28 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index aad9d39..4c107c1 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -472,7 +472,12 @@ is_our_chassis(const struct sbrec_chassis *chassis_rec,
> >       return our_chassis;
> >   }
> >
> > -static void
> > +/* Updates 'binding_rec' and if the port binding is local also updates the
> > + * local datapaths and ports.
> > + * Updates and returns the array of local virtual ports that will require
> > + * additional processing.
> > + */
> > +static const struct sbrec_port_binding **
> >   consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >                           struct ovsdb_idl_txn *ovs_idl_txn,
> >                           struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> > @@ -485,7 +490,9 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >                           struct hmap *local_datapaths,
> >                           struct shash *lport_to_iface,
> >                           struct sset *local_lports,
> > -                        struct sset *local_lport_ids)
> > +                        struct sset *local_lport_ids,
> > +                        const struct sbrec_port_binding **vpbs,
> > +                        size_t *n_vpbs, size_t *n_allocated_vpbs)
> >   {
> >       const struct ovsrec_interface *iface_rec
> >           = shash_find_data(lport_to_iface, binding_rec->logical_port);
> > @@ -587,22 +594,11 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >               }
> >           } else if (binding_rec->chassis == chassis_rec) {
> >               if (!strcmp(binding_rec->type, "virtual")) {
> > -                /* pinctrl module takes care of binding the ports
> > -                 * of type 'virtual'.
> > -                 * Release such ports if their virtual parents are no
> > -                 * longer claimed by this chassis. */
> > -                const struct sbrec_port_binding *parent
> > -                    = lport_lookup_by_name(sbrec_port_binding_by_name,
> > -                                        binding_rec->virtual_parent);
> > -                if (!parent || parent->chassis != chassis_rec) {
> > -                    VLOG_INFO("Releasing lport %s from this chassis.",
> > -                            binding_rec->logical_port);
> > -                    if (binding_rec->encap) {
> > -                        sbrec_port_binding_set_encap(binding_rec, NULL);
> > -                    }
> > -                    sbrec_port_binding_set_chassis(binding_rec, NULL);
> > -                    sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
> > +                if (*n_vpbs == *n_allocated_vpbs) {
> > +                    vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs);
> >                   }
> > +                vpbs[(*n_vpbs)] = binding_rec;
> > +                (*n_vpbs)++;
> >               } else {
> >                   VLOG_INFO("Releasing lport %s from this chassis.",
> >                             binding_rec->logical_port);
> > @@ -621,6 +617,30 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >                            vif_chassis);
> >           }
> >       }
> > +    return vpbs;
> > +}
> > +
> > +static void
> > +consider_local_virtual_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > +                            const struct sbrec_chassis *chassis_rec,
> > +                            const struct sbrec_port_binding *binding_rec)
> > +{
> > +    /* pinctrl module takes care of binding the ports of type 'virtual'.
> > +     * Release such ports if their virtual parents are no longer claimed by
> > +     * this chassis.
> > +     */
> > +    const struct sbrec_port_binding *parent =
> > +        lport_lookup_by_name(sbrec_port_binding_by_name,
> > +                             binding_rec->virtual_parent);
> > +    if (!parent || parent->chassis != chassis_rec) {
> > +        VLOG_INFO("Releasing lport %s from this chassis.",
> > +                  binding_rec->logical_port);
> > +        if (binding_rec->encap) {
> > +            sbrec_port_binding_set_encap(binding_rec, NULL);
> > +        }
> > +        sbrec_port_binding_set_chassis(binding_rec, NULL);
> > +        sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
> > +    }
> >   }
> >
> >   static void
> > @@ -718,20 +738,41 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >                               &egress_ifaces);
> >       }
> >
> > +    /* Array to store pointers to local virtual ports. It is populated by
> > +     * consider_local_datapath.
> > +     */
> > +    const struct sbrec_port_binding **vpbs = NULL;
> > +    size_t n_vpbs = 0;
> > +    size_t n_allocated_vpbs = 0;
> > +
> >       /* Run through each binding record to see if it is resident on this
> >        * chassis and update the binding accordingly.  This includes both
> > -     * directly connected logical ports and children of those ports. */
> > +     * directly connected logical ports and children of those ports.
> > +     * Virtual ports are just added to vpbs array and will be processed
> > +     * later. This is special case for virtual ports is needed in order to
> > +     * make sure we update the virtual_parent port bindings first.
> > +     */
> >       SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table) {
> > -        consider_local_datapath(ovnsb_idl_txn, ovs_idl_txn,
> > -                                sbrec_datapath_binding_by_key,
> > -                                sbrec_port_binding_by_datapath,
> > -                                sbrec_port_binding_by_name,
> > -                                active_tunnels, chassis_rec, binding_rec,
> > -                                sset_is_empty(&egress_ifaces) ? NULL :
> > -                                &qos_map, local_datapaths, &lport_to_iface,
> > -                                local_lports, local_lport_ids);
> > -
> > -    }
> > +        vpbs =
> > +            consider_local_datapath(ovnsb_idl_txn, ovs_idl_txn,
> > +                                    sbrec_datapath_binding_by_key,
> > +                                    sbrec_port_binding_by_datapath,
> > +                                    sbrec_port_binding_by_name,
> > +                                    active_tunnels, chassis_rec, binding_rec,
> > +                                    sset_is_empty(&egress_ifaces) ? NULL :
> > +                                    &qos_map, local_datapaths, &lport_to_iface,
> > +                                    local_lports, local_lport_ids,
> > +                                    vpbs, &n_vpbs, &n_allocated_vpbs);
> > +    }
> > +
> > +    /* Now also update the virtual ports in case their parent ports were
> > +     * updated above.
> > +     */
> > +    for (size_t i = 0; i < n_vpbs; i++) {
> > +        consider_local_virtual_port(sbrec_port_binding_by_name, chassis_rec,
> > +                                    vpbs[i]);
> > +    }
> > +    free(vpbs);
> >
> >       add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings);
> >
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list