On Thu, Nov 28, 2019 at 2:36 PM Numan Siddique <[email protected]> wrote:
>
> On Sat, Nov 23, 2019 at 3:51 AM Mark Michelson <[email protected]> wrote:
> >
> > Acked-by: Mark Michelson <[email protected]>
> >
> > 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 <[email protected]>
> > > Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
> > > Signed-off-by: Dumitru Ceara <[email protected]>
>
> Thanks. I applied this patch to master.
>
> Numan
Thanks Mark and 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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
