Thanks Han for the review. I will address them Please see comments inline Thanks Numan
On Tue, Mar 26, 2019 at 8:07 AM Han Zhou <[email protected]> wrote: > Mostly looks good to me, just minor comments. > > On Thu, Mar 14, 2019 at 12:32 PM <[email protected]> wrote: > > > > +static void > > +build_lrouter_groups__(struct hmap *ports, struct ovn_datapath *od) > > +{ > > + ovs_assert((od && od->nbr && od->lr_group)); > > + > > + if (od->l3dgw_port && od->l3redirect_port) { > > + /* It's a logical router with gateway port. If it > > + * has HA_Chassis_Group associated to it in SB DB, then store > the > > + * ha chassis group name. */ > > + if (od->l3redirect_port->sb->ha_chassis_group) { > > + sset_add(&od->lr_group->ha_chassis_groups, > > + od->l3redirect_port->sb->ha_chassis_group->name); > > + } > > + } > > + > > + 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; > > + } > > + > > + /* Get the peer logical switch/logical router datapath. */ > > + struct ovn_datapath *peer_dp = router_port->peer->od; > > + if (peer_dp->nbr) { > > + if (!peer_dp->lr_group) { > > + peer_dp->lr_group = od->lr_group; > > + od->lr_group->router_dps[od->lr_group->n_router_dps++] > > + = peer_dp; > > + build_lrouter_groups__(ports, peer_dp); > > + } > > + } else { > > + for (size_t j = 0; j < peer_dp->n_router_ports; j++) { > > + if (!peer_dp->router_ports[j]->peer) { > > + /* If there is no peer port connecting to the > > + * router datapath, ignore it. */ > > s/router datapath/router port > > > + continue; > > + } > > + > > > +static void > > +build_lrouter_groups(struct hmap *ports, struct ovs_list *lr_list) > > +{ > > + struct ovn_datapath *od; > > + size_t n_router_dps = ovs_list_size(lr_list); > > + > > + LIST_FOR_EACH (od, lr_list, lr_list) { > > + if (!od->lr_group) { > > + od->lr_group = xzalloc(sizeof *od->lr_group); > > + /* Each logical router group can have max > > + * 'n_router_dps'. So allocate enough memory. */ > > + od->lr_group->router_dps = xcalloc(sizeof *od, > n_router_dps); > > + od->lr_group->router_dps[od->lr_group->n_router_dps] = od; > > Here it is more clear to be: od->lr_group->router_dps[0] = od; > > > + od->lr_group->n_router_dps = 1; > > + sset_init(&od->lr_group->ha_chassis_groups); > > + build_lrouter_groups__(ports, od); > > + } > > + } > > +} > > + > > > static void > > -destroy_datapaths_and_ports(struct hmap *datapaths, struct hmap *ports) > > +destroy_datapaths_and_ports(struct hmap *datapaths, struct hmap *ports, > > + struct ovs_list *lr_list) > > { > > + struct ovn_datapath *router_dp; > > + LIST_FOR_EACH_POP (router_dp, lr_list, lr_list) { > > + if (router_dp->lr_group) { > > + struct lrouter_group *lr_group = router_dp->lr_group; > > + > > + for (size_t i = 0; i < router_dp->lr_group->n_router_dps; > i++) { > > + if (router_dp->lr_group->router_dps[i] != router_dp) { > > This if-condition seems not needed. > > > + router_dp->lr_group->router_dps[i]->lr_group = NULL; > > + } > > + } > > s/router_dp->lr_group/lr_group in above for-loop. > > > + > > + free(lr_group->router_dps); > > + sset_destroy(&lr_group->ha_chassis_groups); > > + free(lr_group); > > + router_dp->lr_group = NULL; > > + } > > + } > > + > > > diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema > > index 10a59649a..48d27b960 100644 > > --- a/ovn/ovn-nb.ovsschema > > +++ b/ovn/ovn-nb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Northbound", > > - "version": "5.14.1", > > - "cksum": "3758097843 20509", > > + "version": "5.15.0", > > + "cksum": "1078795414 21917", > > "tables": { > > "NB_Global": { > > "columns": { > > @@ -271,6 +271,12 @@ > > "refType": "strong"}, > > "min": 0, > > "max": "unlimited"}}, > > + "ha_chassis_group": { > > + "type": {"key": {"type": "uuid", > > + "refTable": "HA_Chassis_Group", > > + "refType": "weak"}, > > Shall this be strong ref instead? In normal case logical ports hosted > by a HA-chassis-group should be deleted before the HA-chassis-group is > removed. (same for SB schema) > I would prefer weak ref because it would be easier for CMS to manage the references. It doesn't need to keep track of the ref count before deleting the HA_Chassis_Group row. > > > + "min": 0, > > + "max": 1}}, > > "options": { > > "type": {"key": "string", > > "value": "string", > > @@ -392,5 +398,29 @@ > > "type": {"key": "string", "value": "string", > > "min": 0, "max": "unlimited"}}}, > > "indexes": [["name"]], > > - "isRoot": false}} > > + "isRoot": false}, > > + "HA_Chassis": { > > + "columns": { > > + "chassis_name": {"type": "string"}, > > + "priority": {"type": {"key": {"type": "integer", > > + "minInteger": 0, > > + "maxInteger": 32767}}}, > > + "external_ids": { > > + "type": {"key": "string", "value": "string", > > + "min": 0, "max": "unlimited"}}}, > > + "isRoot": false}, > > + "HA_Chassis_Group": { > > + "columns": { > > + "name": {"type": "string"}, > > + "ha_chassis": { > > + "type": {"key": {"type": "uuid", > > + "refTable": "HA_Chassis", > > + "refType": "strong"}, > > Is it better to be weak ref here, considering that multiple a > HA-chassis can belong to multiple groups? (same for SB schema) > If you see HA_Chassis is non root. and hence I think it should be strong ref. And I think it would be easier for CMS to manage. Deleting HA_Chassis_Group will delete its ha chassis list. I think it would complicate the code in ovn-northd when syncing with the SB DB if HA_Chassis is root and it is referenced by multiple HA_Chassis_Group rows. So I would prefer HA_Chassis to be non root and HA_Chassis_Group has a strong ref to HA_Chassis table. Thanks Numan > > > + "min": 0, > > + "max": "unlimited"}}, > > + "external_ids": { > > + "type": {"key": "string", "value": "string", > > + "min": 0, "max": "unlimited"}}}, > > + "indexes": [["name"]], > > + "isRoot": true}} > > } > > I will get back to 3/5 - 5/5 reviewing asap. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
