On Wed, Mar 27, 2019 at 10:39 AM Han Zhou <zhou...@gmail.com> wrote:
> On Thu, Mar 14, 2019 at 12:33 PM <nusid...@redhat.com> wrote:
> >
>
> > diff --git a/ovn/controller/bfd.h b/ovn/controller/bfd.h
> > index e36820afb..789f7b269 100644
> > --- a/ovn/controller/bfd.h
> > +++ b/ovn/controller/bfd.h
> > @@ -24,16 +24,17 @@ struct ovsrec_interface_table;
> > struct ovsrec_open_vswitch_table;
> > struct sbrec_chassis;
> > struct sbrec_sb_global_table;
> > +struct sbrec_ha_chassis_group_table;
> > struct sset;
> >
> > void bfd_register_ovs_idl(struct ovsdb_idl *);
> > -void bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name,
> > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
> > - const struct ovsrec_interface_table *interface_table,
> > +
> > +void bfd_run(const struct ovsrec_interface_table *interface_table,
> > const struct ovsrec_bridge *br_int,
> > const struct sbrec_chassis *chassis_rec,
> > - const struct sbrec_sb_global_table *sb_global_table,
> > - const struct hmap *local_datapaths);
> > + const struct sbrec_ha_chassis_group_table
> *ha_chassis_grp_table,
> > + const struct sbrec_sb_global_table *sb_global_table);
> > +
>
> Most parameter names can be omitted in this prototype.
>
> > +
> > + struct ha_chassis_ordered *ordered_ha_ch;
> > + if (n_ha_ch == 1) {
> > + if (active_tunnels) {
> > + /* If n_ha_ch is 1, it means only the local chassis is in
> the
> > + * ha_ch_order list. Check if this local chassis has active
> > + * bfd session with any of the referenced chassis. If so,
> > + * then the local chassis can be active. Otherwise it can't.
> > + * This can happen in the following scenario.
> > + * Lets say we have chassis HA1 (prioirty 20) and HA2
> (priority 10)
> > + * in the ha_chasis_group and compute chassis C1 and C2 are
> in the
> > + * reference chassis list. If HA1 chassis has lost the link
> and
> > + * when this function is called for HA2 we need to consider
> > + * HA2 as active since it has active BFD sessions with C1
> and C2.
> > + * On HA1 chassis, this function won't be called since
> > + * active_tunnels set will be empty.
> > + * */
> > + bool can_local_chassis_be_active = false;
> > + for (size_t i = 0; i < ha_ch_grp->n_ref_chassis; i++) {
> > + if (sset_contains(active_tunnels,
> > + ha_ch_grp->ref_chassis[i]->name)) {
> > + can_local_chassis_be_active = true;
> > + break;
> > + }
> > + }
> > + if (!can_local_chassis_be_active) {
> > + free(ha_ch_order);
> > + return NULL;
> > + }
> > + }
>
> What if, a HA-chassis-group has only one chassis, the n_ha_ch will be
> 1, and active_tunnels is not NULL but will be empty because bfd won't
> be setup for HA group with only 1 chassis. However, it will enter this
> branch and returns NULL, which will lead to
> ha_chassis_group_is_active() return false, and the GW will not
> function for the logical router port. Did I miss anything here?
>
This would not happen because the function " ha_chassis_group_is_active()"
will not even call this function.
It has the below checks before calling this function.
+ if (ha_ch_grp->n_ha_chassis == 1) {
+ return (ha_ch_grp->ha_chassis[0]->chassis == local_chassis);
+ }
+
+ if (sset_is_empty(active_tunnels)) {
+ /* If active tunnel sset is empty, it means it has lost
+ * connectivity with other chassis. */
+ return false;
+ }
Thanks
Numan
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
