[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