[ovs-dev] [PATCH v4 3/5] ovn-controller: Make use of ha_chassis_group table to bind the chassisredirect ports

[Han Zhou]

On Thu, Mar 14, 2019 at 12:33 PM <nusiddiq at 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?