[ovs-dev] [PATCH v4 2/5] ovn: Add generic HA chassis group
Numan Siddique
nusiddiq at redhat.com
Tue Mar 26 16:59:07 UTC 2019
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 <zhouhan at gmail.com> wrote:
> Mostly looks good to me, just minor comments.
>
> On Thu, Mar 14, 2019 at 12:32 PM <nusiddiq at redhat.com> 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.
>
More information about the dev
mailing list