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

[Han Zhou]

On Tue, Mar 26, 2019 at 10:01 PM Han Zhou <zhouhan at gmail.com> wrote:
>
> On Tue, Mar 26, 2019 at 6:34 PM Han Zhou <zhouhan at gmail.com> wrote:
> >
> > On Tue, Mar 5, 2019 at 8:06 AM <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?
> >
> >
> >
> > > @@ -753,34 +753,36 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_chassis_by_name,
> > >              /* Output to tunnel. */
> > >              ofpact_put_OUTPUT(ofpacts_p)->port = rem_tun->ofport;
> > >          } else {
> > > -            struct gateway_chassis *gwc;
> > >              /* Make sure all tunnel endpoints use the same encapsulation,
> > >               * and set it up */
> > > -            LIST_FOR_EACH (gwc, node, gateway_chassis) {
> > > -                if (gwc->db->chassis) {
> > > -                    if (!tun) {
> > > -                        tun = chassis_tunnel_find(gwc->db->chassis->name, NULL);
> > > -                    } else {
> > > -                        struct chassis_tunnel *chassis_tunnel =
> > > -                            chassis_tunnel_find(gwc->db->chassis->name, NULL);
> > > -                        if (chassis_tunnel &&
> > > -                            tun->type != chassis_tunnel->type) {
> > > -                            static struct vlog_rate_limit rl =
> > > -                                VLOG_RATE_LIMIT_INIT(1, 1);
> > > -                            VLOG_ERR_RL(&rl, "Port %s has Gateway_Chassis "
> > > -                                             "with mixed encapsulations, only "
> > > -                                             "uniform encapsulations are "
> > > -                                             "supported.",
> > > -                                        binding->logical_port);
> > > -                            goto out;
> > > -                        }
> > > +            for (size_t i = 0; i < ha_ch_ordered->n_ha_ch; i++) {
> > > +                const struct sbrec_chassis *ch =
> > > +                    ha_ch_ordered->ha_ch[i].chassis;
> > > +                if (!ch) {
> >
> > If ha_ch[i].chassis is NULL, it means a chassis is deleted before
> > deleting the HA_Chassis referencing it. This seems to be a situation
> > we never want. So I wonder maybe it is better to use strong reference
> > for HA_Chassis -> Chassis to avoid this situation completely. From CMS
> > point of view, it is also reasonable to require deleting
> > HA_Chassis(es) before deleting the Chassis being referenced, isn't it?
> >
> Please ignore this comment. I realized that weak reference may be used
> purposely so that when ovn-controller stops gracefully on GW node,
> removing the row from SB will not fail.

Sorry that I replied the old version. (although I reviewed the v4). I
will reply the same comments to the v4.