On Thu, Mar 7, 2019, 10:46 PM Han Zhou <zhou...@gmail.com> wrote: > On Thu, Mar 7, 2019 at 8:37 AM Numan Siddique <nusid...@redhat.com> wrote: > > > > > > > > On Tue, Mar 5, 2019 at 11:34 PM Han Zhou <zhou...@gmail.com> wrote: > >> > >> > > + > >> > > +static void > >> > > +add_to_ha_ref_chassis_info(struct ha_ref_chassis_info *ref_ch_info, > >> > > + const struct sbrec_chassis *chassis) > >> > > +{ > >> > > + for (size_t j = 0; j < ref_ch_info->n_ref_chassis; j++) { > >> > > + if (ref_ch_info->ref_chassis[j] == chassis) { > >> > > + return; > >> > > + } > >> > > + } > >> > > + > >> > > + ref_ch_info->ref_chassis = xrealloc(ref_ch_info->ref_chassis, > >> > > + sizeof > *ref_ch_info->ref_chassis * > >> > > + > (++ref_ch_info->n_ref_chassis)); > >> > > >> > This may be inefficient, considering the amount of ref chassises to be > >> > added for each HA group. It is better to xrealloc for original_size * > >> > 2 every time and expand only when more space is needed. > >> > > >> > > + ref_ch_info->ref_chassis[ref_ch_info->n_ref_chassis - 1] = > >> > > + CONST_CAST(struct sbrec_chassis *, chassis); > >> > > +} > >> > > + > >> > > +static void > >> > > +update_sb_ha_group_ref_chassis(struct shash *ha_ref_chassis_map) > >> > > +{ > >> > > + struct shash_node *node, *next; > >> > > + SHASH_FOR_EACH_SAFE (node, next, ha_ref_chassis_map) { > >> > > + struct ha_ref_chassis_info *ha_ref_info = node->data; > >> > > + > sbrec_ha_chassis_group_set_ref_chassis(ha_ref_info->ha_chassis_group, > >> > > + > ha_ref_info->ref_chassis, > >> > > + > ha_ref_info->n_ref_chassis); > >> > > + free(ha_ref_info->ref_chassis); > >> > > + free(ha_ref_info); > >> > > + shash_delete(ha_ref_chassis_map, node); > >> > > + } > >> > > +} > >> > > + > >> > > +/* This function returns logical router datapath (with a > distributed > >> > > + * gateway port) to which 'od' is connected to - either directly > >> > > + * or indirectly (via transit logical switches). > >> > > + * Returns NULL if no logical router with gw port found. > >> > > + */ > >> > > +static struct ovn_datapath * > >> > > +get_router_dp_with_gw_port(struct hmap *ports, > >> > > + struct ovn_datapath *od, > >> > > + struct ovn_datapath *peer_od) > >> > > +{ > >> > > + if (!od) { > >> > > + return NULL; > >> > > + } > >> > > + > >> > > + if (od->nbs) { > >> > > + /* It's a logical switch datapath. */ > >> > > + if (peer_od) { > >> > > + /* If peer datapath is not logical router, then > >> > > + * something is wrong. */ > >> > > + ovs_assert(peer_od->nbr); > >> > > + } > >> > > + > >> > > + for (size_t i = 0; i < od->n_router_ports; i++) { > >> > > + if (!od->router_ports[i]->peer) { > >> > > + /* If there is no peer port connecting to the > >> > > + * router datapath, ignore it. */ > >> > > + continue; > >> > > + } > >> > > + > >> > > + struct ovn_datapath *router_dp = > od->router_ports[i]->peer->od; > >> > > + if (router_dp->l3dgw_port && > router_dp->l3dgw_port->nbrp) { > >> > > + /* Router datapath has a distributed gateway > router port. */ > >> > > + return router_dp; > >> > > >> > I think we can't return when just one router_dp is found. There can be > >> > more than one connected router that has gateway router ports. So the > >> > return value of this function should be a set. > >> > > >> > > + } > >> > > + } > >> > > + > >> > > + /* The logical switch datapath is not connected to any > >> > > + * logical router with a distributed gateway port. Check > if it > >> > > + * is indirectly connected to a logical router with a gw > port. */ > >> > > + for (size_t i = 0; i < od->n_router_ports; i++) { > >> > > + if (!od->router_ports[i]->peer) { > >> > > + continue; > >> > > + } > >> > > + > >> > > + struct ovn_datapath *router_dp = > >> > > + od->router_ports[i]->peer->od; > >> > > + > >> > > + /* If we don't check this, we will be in an infinite > loop. */ > >> > > + if (router_dp != peer_od) { > >> > > >> > peer_od should also be a set, it should skip checking any datapath > >> > that has already been checked. Consider the case LR1 is connected with > >> > LS1, LS2 and LS3. > >> > > >> > > + router_dp = get_router_dp_with_gw_port(ports, > router_dp, > >> > > + od); > >> > > + if (router_dp) { > >> > > + /* Found a logical router with gw port > indirectly connected > >> > > + * to 'od'. */ > >> > > + return router_dp; > >> > > + } > >> > > + } > >> > > + } > >> > > + } else if (od->nbr) { > >> > > + /* It's a logical router datapath. */ > >> > > + if (peer_od) { > >> > > + /* If peer datapath is not logical switch, then > >> > > + * something is wrong. */ > >> > > + ovs_assert(peer_od->nbs); > >> > > >> > A router port can be peered with another router port directly, so this > >> > assert is not true. > >> > > >> > > + } > >> > > + > >> > > + /* Check if this logical router datapath is indirectly > connected > >> > > + * to another logical router via a transit logical > switch(es). */ > >> > > + for (size_t i = 0; i < od->nbr->n_ports; i++) { > >> > > + struct ovn_port *router_port = > >> > > + ovn_port_find(ports, od->nbr->ports[i]->name); > >> > > + > >> > > + if (!router_port || !router_port->peer) { > >> > > + continue; > >> > > + } > >> > > + /* If we don't check this, we will be in an infinite > loop. */ > >> > > + if (router_port->peer->od != peer_od) { > >> > > + struct ovn_datapath *router_dp; > >> > > + /* router_port->peer->od points a logical switch > datapath. */ > >> > > + router_dp = get_router_dp_with_gw_port(ports, > >> > > + > router_port->peer->od, > >> > > + od); > >> > > + if (router_dp) { > >> > > + /* Found a logical router with gw port > indirectly connected > >> > > + * to 'od'. */ > >> > > + return router_dp; > >> > > + } > >> > > + } > >> > > + } > >> > > + } > >> > > + > >> > > + return NULL; > >> > > +} > >> > > >> > In general, it may refer to the flood-fill approach implemented in > >> > ovn-controller for populating local datapaths in binding.c, which is > >> > similar to the purpose here. However, we should consider an > >> > optimization here since the flood-fill cost would apply for every > >> > port-binding. It can be optimized with a cache, which maps between > >> > each datapath and its related router_dps with gw ports, to avoid > >> > repeated traversing. (In ovn-controller it can be optimized, too, but > >> > the gain is not as big as here since only local port-bindings are > >> > checked in ovn-controller.) > >> > > > > > > > Thanks for the comments Han. I will work on them. > > I haven't thought of an optimization yet. I will try to explore based > > on your suggestions. At the same time, I personally think the code > > shouldn't get complicated because of the optimization. (I feel the > present > > code in ovn-controller [1] is a bit complicated - > > [1] - > https://github.com/openvswitch/ovs/blob/master/ovn/controller/bfd.c#L98 > > ) > > Sorry, I should point out the location more clearly. I was talking > about this code: > > https://github.com/openvswitch/ovs/blob/master/ovn/controller/binding.c#L113 > It is about the algorithm that calculates related datapaths. I think > we need similar logic in northd, but since northd handles all port > bindings, it is worth to optimize with a cache to avoid repeat this > calculation for every port-binding. >
Thanks. Makes sense. I will work on it. > > > > I think it is uncommon to have indirect logical router connections which > eventually > > connect to a router with a gateway router port. > > May be I am wrong. Do you think it's common to have such setups ? > > > > It may be uncommon, but I think it is important to make it correct in > these situations. > Agree. Thanks Numan > > > >> I forgot to mention, this implementation is for finding out the > >> required BFD sessions, however, it can be reused for a more generic > >> purpose - to figure out whether a tunnel is needed between each pair > >> of chassises. There happens to be a related discussion ongoing here: > >> > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-March/048281.html > >> > > > > I have seen thread. I will try to make it generic. > > If not I think we can revisit the code later and make it generic and > more optimized. > > As a first step I think it should be reasonable to aim to have the > solution for the HA_Chassis_Group's ref_chassis calculation. > > Does this sound good ? > > > > I think it is fair to have separate patches to address the > optimizations. For current patch, if it is only about making HA group > generic for both GW and external port, I think we can simply put all > chassises as ref_chassis for all GW HA-groups, and enable bfd for > them, to make it work just like how it behaves today. Further > optimizations can be added separately. > > Thanks, > Han > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev