On Tue, Mar 26, 2019 at 10:01 PM Han Zhou <[email protected]> wrote: > > On Tue, Mar 26, 2019 at 6:34 PM Han Zhou <[email protected]> wrote: > > > > On Tue, Mar 5, 2019 at 8:06 AM <[email protected]> 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
