| Message ID | 20230818085756.1031010-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | northd: I-P for load balancer and lb groups | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
| ovsrobot/apply-robot | success | apply and check: success |
On Fri, Aug 18, 2023 at 10:59 AM <numans@ovn.org> wrote: > From: Numan Siddique <numans@ovn.org> > > A new engine node 'sync_to_sb_pb' is added within 'sync_to_sb' > node to sync NAT column of Port bindings table. This separation > is required in order to add load balancer group I-P handling > in 'northd' engine node (which is handled in the next commit). > > 'sync_to_sb_pb' engine node can be later expanded to sync other > Port binding columns if required. > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/en-sync-sb.c | 31 +++++ > northd/en-sync-sb.h | 4 + > northd/inc-proc-northd.c | 8 +- > northd/northd.c | 243 +++++++++++++++++++++------------------ > northd/northd.h | 2 + > 5 files changed, 174 insertions(+), 114 deletions(-) > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 9832fce30a..552ed56452 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -254,6 +254,37 @@ sync_to_sb_lb_northd_handler(struct engine_node > *node, void *data OVS_UNUSED) > return false; > } > > +/* sync_to_sb_pb engine node functions. > + * This engine node syncs the SB Port Bindings (partly). > + * en_northd engine create the SB Port binding rows and > + * updates most of the columns. > + * This engine node updates the port binding columns which > + * needs to be updated after northd engine is run. > + */ > + > +void * > +en_sync_to_sb_pb_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return NULL; > +} > + > +void > +en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + const struct engine_context *eng_ctx = engine_get_context(); > + struct northd_data *northd_data = engine_get_input_data("northd", > node); > + > + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); > + engine_set_node_state(node, EN_UPDATED); > +} > + > +void > +en_sync_to_sb_pb_cleanup(void *data OVS_UNUSED) > +{ > + > +} > + > /* static functions. */ > static void > sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h > index 06d2a57710..700d3340e4 100644 > --- a/northd/en-sync-sb.h > +++ b/northd/en-sync-sb.h > @@ -22,4 +22,8 @@ void en_sync_to_sb_lb_run(struct engine_node *, void > *data); > void en_sync_to_sb_lb_cleanup(void *data); > bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data > OVS_UNUSED); > > +void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *); > +void en_sync_to_sb_pb_run(struct engine_node *, void *data); > +void en_sync_to_sb_pb_cleanup(void *data); > + > #endif /* end of EN_SYNC_SB_H */ > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 402c94e88c..dc8b880fd8 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -141,6 +141,7 @@ static ENGINE_NODE(sync_to_sb_addr_set, > "sync_to_sb_addr_set"); > static ENGINE_NODE(fdb_aging, "fdb_aging"); > static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker"); > static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb"); > +static ENGINE_NODE(sync_to_sb_pb, "sync_to_sb_pb"); > static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data"); > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > @@ -215,13 +216,16 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > sync_to_sb_lb_northd_handler); > engine_add_input(&en_sync_to_sb_lb, &en_sb_load_balancer, NULL); > > + engine_add_input(&en_sync_to_sb_pb, &en_northd, NULL); > + > /* en_sync_to_sb engine node syncs the SB database tables from > * the NB database tables. > - * Right now this engine syncs the SB Address_Set table and > - * SB Load_Balancer table. > + * Right now this engine syncs the SB Address_Set table, > + * SB Load_Balancer table and (partly) SB Port_Binding table. > */ > engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL); > engine_add_input(&en_sync_to_sb, &en_sync_to_sb_lb, NULL); > + engine_add_input(&en_sync_to_sb, &en_sync_to_sb_pb, NULL); > > engine_add_input(&en_sync_from_sb, &en_northd, > sync_from_sb_northd_handler); > diff --git a/northd/northd.c b/northd/northd.c > index 1477b79331..a3bd21e0b4 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3514,8 +3514,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > ds_destroy(&s); > > sbrec_port_binding_set_external_ids(op->sb, > &op->nbrp->external_ids); > - > - sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); > } else { > if (!lsp_is_router(op->nbsp)) { > uint32_t queue_id = smap_get_int( > @@ -3631,116 +3629,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > } else { > sbrec_port_binding_set_options(op->sb, NULL); > } > - const char *nat_addresses = smap_get(&op->nbsp->options, > - "nat-addresses"); > - size_t n_nats = 0; > - char **nats = NULL; > - bool l3dgw_ports = op->peer && op->peer->od && > - op->peer->od->n_l3dgw_ports; > - if (nat_addresses && !strcmp(nat_addresses, "router")) { > - if (op->peer && op->peer->od > - && (chassis || op->peer->od->n_l3dgw_ports)) { > - bool exclude_lb_vips = > smap_get_bool(&op->nbsp->options, > - "exclude-lb-vips-from-garp", false); > - nats = get_nat_addresses(op->peer, &n_nats, false, > - !exclude_lb_vips); > - } > - } else if (nat_addresses && (chassis || l3dgw_ports)) { > - struct lport_addresses laddrs; > - if (!extract_lsp_addresses(nat_addresses, &laddrs)) { > - static struct vlog_rate_limit rl = > - VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "Error extracting nat-addresses."); > - } else { > - destroy_lport_addresses(&laddrs); > - n_nats = 1; > - nats = xcalloc(1, sizeof *nats); > - struct ds nat_addr = DS_EMPTY_INITIALIZER; > - ds_put_format(&nat_addr, "%s", nat_addresses); > - if (l3dgw_ports) { > - const struct ovn_port *l3dgw_port = ( > - is_l3dgw_port(op->peer) > - ? op->peer > - : op->peer->od->l3dgw_ports[0]); > - ds_put_format(&nat_addr, " > is_chassis_resident(%s)", > - l3dgw_port->cr_port->json_key); > - } > - nats[0] = xstrdup(ds_cstr(&nat_addr)); > - ds_destroy(&nat_addr); > - } > - } > - > - /* Add the router mac and IPv4 addresses to > - * Port_Binding.nat_addresses so that GARP is sent for these > - * IPs by the ovn-controller on which the distributed gateway > - * router port resides if: > - * > - * - op->peer has 'reside-on-redirect-chassis' set and the > - * the logical router datapath has distributed router port. > - * > - * - op->peer is distributed gateway router port. > - * > - * - op->peer's router is a gateway router and op has a > localnet > - * port. > - * > - * Note: Port_Binding.nat_addresses column is also used for > - * sending the GARPs for the router port IPs. > - * */ > - bool add_router_port_garp = false; > - if (op->peer && op->peer->nbrp && > op->peer->od->n_l3dgw_ports) { > - if (is_l3dgw_port(op->peer)) { > - add_router_port_garp = true; > - } else if (smap_get_bool(&op->peer->nbrp->options, > - "reside-on-redirect-chassis", false)) { > - if (op->peer->od->n_l3dgw_ports == 1) { > - add_router_port_garp = true; > - } else { > - static struct vlog_rate_limit rl = > - VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "\"reside-on-redirect-chassis\" > is " > - "set on logical router port %s, > which " > - "is on logical router %s, which has > %" > - PRIuSIZE" distributed gateway ports. > This" > - "option can only be used when there > is " > - "a single distributed gateway port.", > - op->peer->key, > op->peer->od->nbr->name, > - op->peer->od->n_l3dgw_ports); > - } > - } > - } else if (chassis && op->od->n_localnet_ports) { > - add_router_port_garp = true; > - } > - > - if (add_router_port_garp) { > - struct ds garp_info = DS_EMPTY_INITIALIZER; > - ds_put_format(&garp_info, "%s", > op->peer->lrp_networks.ea_s); > - > - for (size_t i = 0; i < > op->peer->lrp_networks.n_ipv4_addrs; > - i++) { > - ds_put_format(&garp_info, " %s", > - > op->peer->lrp_networks.ipv4_addrs[i].addr_s); > - } > - > - if (op->peer->od->n_l3dgw_ports) { > - const struct ovn_port *l3dgw_port = ( > - is_l3dgw_port(op->peer) > - ? op->peer > - : op->peer->od->l3dgw_ports[0]); > - ds_put_format(&garp_info, " is_chassis_resident(%s)", > - l3dgw_port->cr_port->json_key); > - } > - > - n_nats++; > - nats = xrealloc(nats, (n_nats * sizeof *nats)); > - nats[n_nats - 1] = ds_steal_cstr(&garp_info); > - ds_destroy(&garp_info); > - } > - sbrec_port_binding_set_nat_addresses(op->sb, > - (const char **) nats, > n_nats); > - for (size_t i = 0; i < n_nats; i++) { > - free(nats[i]); > - } > - free(nats); > } > > sbrec_port_binding_set_parent_port(op->sb, op->nbsp->parent_name); > @@ -4695,6 +4583,137 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn, > } > } > > +/* Sync the SB Port bindings which needs to be updated. > + * Presently it syncs the nat column of port bindings corresponding to > + * the logical switch ports. */ > +void sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) > +{ > + ovs_assert(ovnsb_idl_txn); > + > + struct ovn_port *op; > + HMAP_FOR_EACH (op, key_node, ls_ports) { > + if (lsp_is_router(op->nbsp)) { > + const char *chassis = NULL; > + if (op->peer && op->peer->od && op->peer->od->nbr) { > + chassis = smap_get(&op->peer->od->nbr->options, > "chassis"); > + } > + > + const char *nat_addresses = smap_get(&op->nbsp->options, > + "nat-addresses"); > + size_t n_nats = 0; > + char **nats = NULL; > + bool l3dgw_ports = op->peer && op->peer->od && > + op->peer->od->n_l3dgw_ports; > + if (nat_addresses && !strcmp(nat_addresses, "router")) { > + if (op->peer && op->peer->od > + && (chassis || op->peer->od->n_l3dgw_ports)) { > + bool exclude_lb_vips = > smap_get_bool(&op->nbsp->options, > + "exclude-lb-vips-from-garp", false); > + nats = get_nat_addresses(op->peer, &n_nats, false, > + !exclude_lb_vips); > + } > + } else if (nat_addresses && (chassis || l3dgw_ports)) { > + struct lport_addresses laddrs; > + if (!extract_lsp_addresses(nat_addresses, &laddrs)) { > + static struct vlog_rate_limit rl = > + VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Error extracting nat-addresses."); > + } else { > + destroy_lport_addresses(&laddrs); > + n_nats = 1; > + nats = xcalloc(1, sizeof *nats); > + struct ds nat_addr = DS_EMPTY_INITIALIZER; > + ds_put_format(&nat_addr, "%s", nat_addresses); > + if (l3dgw_ports) { > + const struct ovn_port *l3dgw_port = ( > + is_l3dgw_port(op->peer) > + ? op->peer > + : op->peer->od->l3dgw_ports[0]); > + ds_put_format(&nat_addr, " > is_chassis_resident(%s)", > + l3dgw_port->cr_port->json_key); > + } > + nats[0] = xstrdup(ds_cstr(&nat_addr)); > + ds_destroy(&nat_addr); > + } > + } > + > + /* Add the router mac and IPv4 addresses to > + * Port_Binding.nat_addresses so that GARP is sent for these > + * IPs by the ovn-controller on which the distributed gateway > + * router port resides if: > + * > + * - op->peer has 'reside-on-redirect-chassis' set and the > + * the logical router datapath has distributed router port. > + * > + * - op->peer is distributed gateway router port. > + * > + * - op->peer's router is a gateway router and op has a > localnet > + * port. > + * > + * Note: Port_Binding.nat_addresses column is also used for > + * sending the GARPs for the router port IPs. > + * */ > + bool add_router_port_garp = false; > + if (op->peer && op->peer->nbrp && > op->peer->od->n_l3dgw_ports) { > + if (is_l3dgw_port(op->peer)) { > + add_router_port_garp = true; > + } else if (smap_get_bool(&op->peer->nbrp->options, > + "reside-on-redirect-chassis", false)) { > + if (op->peer->od->n_l3dgw_ports == 1) { > + add_router_port_garp = true; > + } else { > + static struct vlog_rate_limit rl = > + VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "\"reside-on-redirect-chassis\" > is " > + "set on logical router port %s, > which " > + "is on logical router %s, which has > %" > + PRIuSIZE" distributed gateway ports. > This" > + "option can only be used when there > is " > + "a single distributed gateway port.", > + op->peer->key, > op->peer->od->nbr->name, > + op->peer->od->n_l3dgw_ports); > + } > + } > + } else if (chassis && op->od->n_localnet_ports) { > + add_router_port_garp = true; > + } > + > + if (add_router_port_garp) { > + struct ds garp_info = DS_EMPTY_INITIALIZER; > + ds_put_format(&garp_info, "%s", > op->peer->lrp_networks.ea_s); > + > + for (size_t i = 0; i < > op->peer->lrp_networks.n_ipv4_addrs; > + i++) { > + ds_put_format(&garp_info, " %s", > + > op->peer->lrp_networks.ipv4_addrs[i].addr_s); > + } > + > + if (op->peer->od->n_l3dgw_ports) { > + const struct ovn_port *l3dgw_port = ( > + is_l3dgw_port(op->peer) > + ? op->peer > + : op->peer->od->l3dgw_ports[0]); > + ds_put_format(&garp_info, " is_chassis_resident(%s)", > + l3dgw_port->cr_port->json_key); > + } > + > + n_nats++; > + nats = xrealloc(nats, (n_nats * sizeof *nats)); > + nats[n_nats - 1] = ds_steal_cstr(&garp_info); > + ds_destroy(&garp_info); > + } > + sbrec_port_binding_set_nat_addresses(op->sb, > + (const char **) nats, > n_nats); > + for (size_t i = 0; i < n_nats; i++) { > + free(nats[i]); > + } > + free(nats); > + } else { > + sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); > + } > + } > +} > + > static bool > ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) > { > diff --git a/northd/northd.h b/northd/northd.h > index 044d4ee0c0..cd2e5394c2 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -374,4 +374,6 @@ const char *northd_get_svc_monitor_mac(void); > void sync_lbs(struct ovsdb_idl_txn *, const struct > sbrec_load_balancer_table *, > struct ovn_datapaths *ls_datapaths, struct hmap *lbs); > > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); > + > #endif /* NORTHD_H */ > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Reviewed-by: Ales Musil <amusil@redhat.com>
On Fri, Aug 18, 2023 at 1:58 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > A new engine node 'sync_to_sb_pb' is added within 'sync_to_sb' > node to sync NAT column of Port bindings table. This separation > is required in order to add load balancer group I-P handling > in 'northd' engine node (which is handled in the next commit). > > 'sync_to_sb_pb' engine node can be later expanded to sync other > Port binding columns if required. > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/en-sync-sb.c | 31 +++++ > northd/en-sync-sb.h | 4 + > northd/inc-proc-northd.c | 8 +- > northd/northd.c | 243 +++++++++++++++++++++------------------ > northd/northd.h | 2 + > 5 files changed, 174 insertions(+), 114 deletions(-) > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 9832fce30a..552ed56452 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -254,6 +254,37 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data OVS_UNUSED) > return false; > } > > +/* sync_to_sb_pb engine node functions. > + * This engine node syncs the SB Port Bindings (partly). > + * en_northd engine create the SB Port binding rows and > + * updates most of the columns. > + * This engine node updates the port binding columns which > + * needs to be updated after northd engine is run. > + */ > + > +void * > +en_sync_to_sb_pb_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return NULL; > +} > + > +void > +en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + const struct engine_context *eng_ctx = engine_get_context(); > + struct northd_data *northd_data = engine_get_input_data("northd", node); > + > + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); > + engine_set_node_state(node, EN_UPDATED); > +} > + > +void > +en_sync_to_sb_pb_cleanup(void *data OVS_UNUSED) > +{ > + > +} > + > /* static functions. */ > static void > sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h > index 06d2a57710..700d3340e4 100644 > --- a/northd/en-sync-sb.h > +++ b/northd/en-sync-sb.h > @@ -22,4 +22,8 @@ void en_sync_to_sb_lb_run(struct engine_node *, void *data); > void en_sync_to_sb_lb_cleanup(void *data); > bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data OVS_UNUSED); > > +void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *); > +void en_sync_to_sb_pb_run(struct engine_node *, void *data); > +void en_sync_to_sb_pb_cleanup(void *data); > + > #endif /* end of EN_SYNC_SB_H */ > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 402c94e88c..dc8b880fd8 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -141,6 +141,7 @@ static ENGINE_NODE(sync_to_sb_addr_set, "sync_to_sb_addr_set"); > static ENGINE_NODE(fdb_aging, "fdb_aging"); > static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker"); > static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb"); > +static ENGINE_NODE(sync_to_sb_pb, "sync_to_sb_pb"); > static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data"); > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > @@ -215,13 +216,16 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > sync_to_sb_lb_northd_handler); > engine_add_input(&en_sync_to_sb_lb, &en_sb_load_balancer, NULL); > > + engine_add_input(&en_sync_to_sb_pb, &en_northd, NULL); NULL handler here always triggers recompute for any en_northd change, which causes a performance regression for VIF I-P. Before this change, only tracked LSP changes will have related SB port-binding synced to SB, but now it iterates through all the ls_ports and resync each of them even if there is only one LSP in tracked_changes. I ran the scale test of the simulated ovn-k8s topology of 500 chassis x 50 lsp, for each single VIF creation/deletion the "ovn-northd completion" time increased from 25ms to 40ms - nearly doubled. I think a simple handler can be implemented so that only sync tracked LSPs, and fallback to recompute only if changes are not tracked. Thanks, Han > + > /* en_sync_to_sb engine node syncs the SB database tables from > * the NB database tables. > - * Right now this engine syncs the SB Address_Set table and > - * SB Load_Balancer table. > + * Right now this engine syncs the SB Address_Set table, > + * SB Load_Balancer table and (partly) SB Port_Binding table. > */ > engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL); > engine_add_input(&en_sync_to_sb, &en_sync_to_sb_lb, NULL); > + engine_add_input(&en_sync_to_sb, &en_sync_to_sb_pb, NULL); > > engine_add_input(&en_sync_from_sb, &en_northd, > sync_from_sb_northd_handler); > diff --git a/northd/northd.c b/northd/northd.c > index 1477b79331..a3bd21e0b4 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3514,8 +3514,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > ds_destroy(&s); > > sbrec_port_binding_set_external_ids(op->sb, &op->nbrp->external_ids); > - > - sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); > } else { > if (!lsp_is_router(op->nbsp)) { > uint32_t queue_id = smap_get_int( > @@ -3631,116 +3629,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > } else { > sbrec_port_binding_set_options(op->sb, NULL); > } > - const char *nat_addresses = smap_get(&op->nbsp->options, > - "nat-addresses"); > - size_t n_nats = 0; > - char **nats = NULL; > - bool l3dgw_ports = op->peer && op->peer->od && > - op->peer->od->n_l3dgw_ports; > - if (nat_addresses && !strcmp(nat_addresses, "router")) { > - if (op->peer && op->peer->od > - && (chassis || op->peer->od->n_l3dgw_ports)) { > - bool exclude_lb_vips = smap_get_bool(&op->nbsp->options, > - "exclude-lb-vips-from-garp", false); > - nats = get_nat_addresses(op->peer, &n_nats, false, > - !exclude_lb_vips); > - } > - } else if (nat_addresses && (chassis || l3dgw_ports)) { > - struct lport_addresses laddrs; > - if (!extract_lsp_addresses(nat_addresses, &laddrs)) { > - static struct vlog_rate_limit rl = > - VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "Error extracting nat-addresses."); > - } else { > - destroy_lport_addresses(&laddrs); > - n_nats = 1; > - nats = xcalloc(1, sizeof *nats); > - struct ds nat_addr = DS_EMPTY_INITIALIZER; > - ds_put_format(&nat_addr, "%s", nat_addresses); > - if (l3dgw_ports) { > - const struct ovn_port *l3dgw_port = ( > - is_l3dgw_port(op->peer) > - ? op->peer > - : op->peer->od->l3dgw_ports[0]); > - ds_put_format(&nat_addr, " is_chassis_resident(%s)", > - l3dgw_port->cr_port->json_key); > - } > - nats[0] = xstrdup(ds_cstr(&nat_addr)); > - ds_destroy(&nat_addr); > - } > - } > - > - /* Add the router mac and IPv4 addresses to > - * Port_Binding.nat_addresses so that GARP is sent for these > - * IPs by the ovn-controller on which the distributed gateway > - * router port resides if: > - * > - * - op->peer has 'reside-on-redirect-chassis' set and the > - * the logical router datapath has distributed router port. > - * > - * - op->peer is distributed gateway router port. > - * > - * - op->peer's router is a gateway router and op has a localnet > - * port. > - * > - * Note: Port_Binding.nat_addresses column is also used for > - * sending the GARPs for the router port IPs. > - * */ > - bool add_router_port_garp = false; > - if (op->peer && op->peer->nbrp && op->peer->od->n_l3dgw_ports) { > - if (is_l3dgw_port(op->peer)) { > - add_router_port_garp = true; > - } else if (smap_get_bool(&op->peer->nbrp->options, > - "reside-on-redirect-chassis", false)) { > - if (op->peer->od->n_l3dgw_ports == 1) { > - add_router_port_garp = true; > - } else { > - static struct vlog_rate_limit rl = > - VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "\"reside-on-redirect-chassis\" is " > - "set on logical router port %s, which " > - "is on logical router %s, which has %" > - PRIuSIZE" distributed gateway ports. This" > - "option can only be used when there is " > - "a single distributed gateway port.", > - op->peer->key, op->peer->od->nbr->name, > - op->peer->od->n_l3dgw_ports); > - } > - } > - } else if (chassis && op->od->n_localnet_ports) { > - add_router_port_garp = true; > - } > - > - if (add_router_port_garp) { > - struct ds garp_info = DS_EMPTY_INITIALIZER; > - ds_put_format(&garp_info, "%s", op->peer->lrp_networks.ea_s); > - > - for (size_t i = 0; i < op->peer->lrp_networks.n_ipv4_addrs; > - i++) { > - ds_put_format(&garp_info, " %s", > - op->peer->lrp_networks.ipv4_addrs[i].addr_s); > - } > - > - if (op->peer->od->n_l3dgw_ports) { > - const struct ovn_port *l3dgw_port = ( > - is_l3dgw_port(op->peer) > - ? op->peer > - : op->peer->od->l3dgw_ports[0]); > - ds_put_format(&garp_info, " is_chassis_resident(%s)", > - l3dgw_port->cr_port->json_key); > - } > - > - n_nats++; > - nats = xrealloc(nats, (n_nats * sizeof *nats)); > - nats[n_nats - 1] = ds_steal_cstr(&garp_info); > - ds_destroy(&garp_info); > - } > - sbrec_port_binding_set_nat_addresses(op->sb, > - (const char **) nats, n_nats); > - for (size_t i = 0; i < n_nats; i++) { > - free(nats[i]); > - } > - free(nats); > } > > sbrec_port_binding_set_parent_port(op->sb, op->nbsp->parent_name); > @@ -4695,6 +4583,137 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn, > } > } > > +/* Sync the SB Port bindings which needs to be updated. > + * Presently it syncs the nat column of port bindings corresponding to > + * the logical switch ports. */ > +void sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) > +{ > + ovs_assert(ovnsb_idl_txn); > + > + struct ovn_port *op; > + HMAP_FOR_EACH (op, key_node, ls_ports) { > + if (lsp_is_router(op->nbsp)) { > + const char *chassis = NULL; > + if (op->peer && op->peer->od && op->peer->od->nbr) { > + chassis = smap_get(&op->peer->od->nbr->options, "chassis"); > + } > + > + const char *nat_addresses = smap_get(&op->nbsp->options, > + "nat-addresses"); > + size_t n_nats = 0; > + char **nats = NULL; > + bool l3dgw_ports = op->peer && op->peer->od && > + op->peer->od->n_l3dgw_ports; > + if (nat_addresses && !strcmp(nat_addresses, "router")) { > + if (op->peer && op->peer->od > + && (chassis || op->peer->od->n_l3dgw_ports)) { > + bool exclude_lb_vips = smap_get_bool(&op->nbsp->options, > + "exclude-lb-vips-from-garp", false); > + nats = get_nat_addresses(op->peer, &n_nats, false, > + !exclude_lb_vips); > + } > + } else if (nat_addresses && (chassis || l3dgw_ports)) { > + struct lport_addresses laddrs; > + if (!extract_lsp_addresses(nat_addresses, &laddrs)) { > + static struct vlog_rate_limit rl = > + VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Error extracting nat-addresses."); > + } else { > + destroy_lport_addresses(&laddrs); > + n_nats = 1; > + nats = xcalloc(1, sizeof *nats); > + struct ds nat_addr = DS_EMPTY_INITIALIZER; > + ds_put_format(&nat_addr, "%s", nat_addresses); > + if (l3dgw_ports) { > + const struct ovn_port *l3dgw_port = ( > + is_l3dgw_port(op->peer) > + ? op->peer > + : op->peer->od->l3dgw_ports[0]); > + ds_put_format(&nat_addr, " is_chassis_resident(%s)", > + l3dgw_port->cr_port->json_key); > + } > + nats[0] = xstrdup(ds_cstr(&nat_addr)); > + ds_destroy(&nat_addr); > + } > + } > + > + /* Add the router mac and IPv4 addresses to > + * Port_Binding.nat_addresses so that GARP is sent for these > + * IPs by the ovn-controller on which the distributed gateway > + * router port resides if: > + * > + * - op->peer has 'reside-on-redirect-chassis' set and the > + * the logical router datapath has distributed router port. > + * > + * - op->peer is distributed gateway router port. > + * > + * - op->peer's router is a gateway router and op has a localnet > + * port. > + * > + * Note: Port_Binding.nat_addresses column is also used for > + * sending the GARPs for the router port IPs. > + * */ > + bool add_router_port_garp = false; > + if (op->peer && op->peer->nbrp && op->peer->od->n_l3dgw_ports) { > + if (is_l3dgw_port(op->peer)) { > + add_router_port_garp = true; > + } else if (smap_get_bool(&op->peer->nbrp->options, > + "reside-on-redirect-chassis", false)) { > + if (op->peer->od->n_l3dgw_ports == 1) { > + add_router_port_garp = true; > + } else { > + static struct vlog_rate_limit rl = > + VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "\"reside-on-redirect-chassis\" is " > + "set on logical router port %s, which " > + "is on logical router %s, which has %" > + PRIuSIZE" distributed gateway ports. This" > + "option can only be used when there is " > + "a single distributed gateway port.", > + op->peer->key, op->peer->od->nbr->name, > + op->peer->od->n_l3dgw_ports); > + } > + } > + } else if (chassis && op->od->n_localnet_ports) { > + add_router_port_garp = true; > + } > + > + if (add_router_port_garp) { > + struct ds garp_info = DS_EMPTY_INITIALIZER; > + ds_put_format(&garp_info, "%s", op->peer->lrp_networks.ea_s); > + > + for (size_t i = 0; i < op->peer->lrp_networks.n_ipv4_addrs; > + i++) { > + ds_put_format(&garp_info, " %s", > + op->peer->lrp_networks.ipv4_addrs[i].addr_s); > + } > + > + if (op->peer->od->n_l3dgw_ports) { > + const struct ovn_port *l3dgw_port = ( > + is_l3dgw_port(op->peer) > + ? op->peer > + : op->peer->od->l3dgw_ports[0]); > + ds_put_format(&garp_info, " is_chassis_resident(%s)", > + l3dgw_port->cr_port->json_key); > + } > + > + n_nats++; > + nats = xrealloc(nats, (n_nats * sizeof *nats)); > + nats[n_nats - 1] = ds_steal_cstr(&garp_info); > + ds_destroy(&garp_info); > + } > + sbrec_port_binding_set_nat_addresses(op->sb, > + (const char **) nats, n_nats); > + for (size_t i = 0; i < n_nats; i++) { > + free(nats[i]); > + } > + free(nats); > + } else { > + sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); > + } > + } > +} > + > static bool > ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) > { > diff --git a/northd/northd.h b/northd/northd.h > index 044d4ee0c0..cd2e5394c2 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -374,4 +374,6 @@ const char *northd_get_svc_monitor_mac(void); > void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *, > struct ovn_datapaths *ls_datapaths, struct hmap *lbs); > > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); > + > #endif /* NORTHD_H */ > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Sep 1, 2023 at 2:41 AM Han Zhou <hzhou@ovn.org> wrote: > > On Fri, Aug 18, 2023 at 1:58 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > A new engine node 'sync_to_sb_pb' is added within 'sync_to_sb' > > node to sync NAT column of Port bindings table. This separation > > is required in order to add load balancer group I-P handling > > in 'northd' engine node (which is handled in the next commit). > > > > 'sync_to_sb_pb' engine node can be later expanded to sync other > > Port binding columns if required. > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > northd/en-sync-sb.c | 31 +++++ > > northd/en-sync-sb.h | 4 + > > northd/inc-proc-northd.c | 8 +- > > northd/northd.c | 243 +++++++++++++++++++++------------------ > > northd/northd.h | 2 + > > 5 files changed, 174 insertions(+), 114 deletions(-) > > > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > > index 9832fce30a..552ed56452 100644 > > --- a/northd/en-sync-sb.c > > +++ b/northd/en-sync-sb.c > > @@ -254,6 +254,37 @@ sync_to_sb_lb_northd_handler(struct engine_node > *node, void *data OVS_UNUSED) > > return false; > > } > > > > +/* sync_to_sb_pb engine node functions. > > + * This engine node syncs the SB Port Bindings (partly). > > + * en_northd engine create the SB Port binding rows and > > + * updates most of the columns. > > + * This engine node updates the port binding columns which > > + * needs to be updated after northd engine is run. > > + */ > > + > > +void * > > +en_sync_to_sb_pb_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + return NULL; > > +} > > + > > +void > > +en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED) > > +{ > > + const struct engine_context *eng_ctx = engine_get_context(); > > + struct northd_data *northd_data = engine_get_input_data("northd", > node); > > + > > + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); > > + engine_set_node_state(node, EN_UPDATED); > > +} > > + > > +void > > +en_sync_to_sb_pb_cleanup(void *data OVS_UNUSED) > > +{ > > + > > +} > > + > > /* static functions. */ > > static void > > sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > > diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h > > index 06d2a57710..700d3340e4 100644 > > --- a/northd/en-sync-sb.h > > +++ b/northd/en-sync-sb.h > > @@ -22,4 +22,8 @@ void en_sync_to_sb_lb_run(struct engine_node *, void > *data); > > void en_sync_to_sb_lb_cleanup(void *data); > > bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data > OVS_UNUSED); > > > > +void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *); > > +void en_sync_to_sb_pb_run(struct engine_node *, void *data); > > +void en_sync_to_sb_pb_cleanup(void *data); > > + > > #endif /* end of EN_SYNC_SB_H */ > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index 402c94e88c..dc8b880fd8 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -141,6 +141,7 @@ static ENGINE_NODE(sync_to_sb_addr_set, > "sync_to_sb_addr_set"); > > static ENGINE_NODE(fdb_aging, "fdb_aging"); > > static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker"); > > static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb"); > > +static ENGINE_NODE(sync_to_sb_pb, "sync_to_sb_pb"); > > static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data"); > > > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > @@ -215,13 +216,16 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > sync_to_sb_lb_northd_handler); > > engine_add_input(&en_sync_to_sb_lb, &en_sb_load_balancer, NULL); > > > > + engine_add_input(&en_sync_to_sb_pb, &en_northd, NULL); > > NULL handler here always triggers recompute for any en_northd change, which > causes a performance regression for VIF I-P. Before this change, only > tracked LSP changes will have related SB port-binding synced to SB, but now > it iterates through all the ls_ports and resync each of them even if there > is only one LSP in tracked_changes. > > I ran the scale test of the simulated ovn-k8s topology of 500 chassis x 50 > lsp, for each single VIF creation/deletion the "ovn-northd completion" time > increased from 25ms to 40ms - nearly doubled. > > I think a simple handler can be implemented so that only sync tracked LSPs, > and fallback to recompute only if changes are not tracked. Thanks for the reviews. I've added a handler in v7. Please take a look. Thanks Numan > > Thanks, > Han > > > + > > /* en_sync_to_sb engine node syncs the SB database tables from > > * the NB database tables. > > - * Right now this engine syncs the SB Address_Set table and > > - * SB Load_Balancer table. > > + * Right now this engine syncs the SB Address_Set table, > > + * SB Load_Balancer table and (partly) SB Port_Binding table. > > */ > > engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL); > > engine_add_input(&en_sync_to_sb, &en_sync_to_sb_lb, NULL); > > + engine_add_input(&en_sync_to_sb, &en_sync_to_sb_pb, NULL); > > > > engine_add_input(&en_sync_from_sb, &en_northd, > > sync_from_sb_northd_handler); > > diff --git a/northd/northd.c b/northd/northd.c > > index 1477b79331..a3bd21e0b4 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -3514,8 +3514,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > > ds_destroy(&s); > > > > sbrec_port_binding_set_external_ids(op->sb, > &op->nbrp->external_ids); > > - > > - sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); > > } else { > > if (!lsp_is_router(op->nbsp)) { > > uint32_t queue_id = smap_get_int( > > @@ -3631,116 +3629,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > > } else { > > sbrec_port_binding_set_options(op->sb, NULL); > > } > > - const char *nat_addresses = smap_get(&op->nbsp->options, > > - "nat-addresses"); > > - size_t n_nats = 0; > > - char **nats = NULL; > > - bool l3dgw_ports = op->peer && op->peer->od && > > - op->peer->od->n_l3dgw_ports; > > - if (nat_addresses && !strcmp(nat_addresses, "router")) { > > - if (op->peer && op->peer->od > > - && (chassis || op->peer->od->n_l3dgw_ports)) { > > - bool exclude_lb_vips = > smap_get_bool(&op->nbsp->options, > > - "exclude-lb-vips-from-garp", false); > > - nats = get_nat_addresses(op->peer, &n_nats, false, > > - !exclude_lb_vips); > > - } > > - } else if (nat_addresses && (chassis || l3dgw_ports)) { > > - struct lport_addresses laddrs; > > - if (!extract_lsp_addresses(nat_addresses, &laddrs)) { > > - static struct vlog_rate_limit rl = > > - VLOG_RATE_LIMIT_INIT(1, 1); > > - VLOG_WARN_RL(&rl, "Error extracting nat-addresses."); > > - } else { > > - destroy_lport_addresses(&laddrs); > > - n_nats = 1; > > - nats = xcalloc(1, sizeof *nats); > > - struct ds nat_addr = DS_EMPTY_INITIALIZER; > > - ds_put_format(&nat_addr, "%s", nat_addresses); > > - if (l3dgw_ports) { > > - const struct ovn_port *l3dgw_port = ( > > - is_l3dgw_port(op->peer) > > - ? op->peer > > - : op->peer->od->l3dgw_ports[0]); > > - ds_put_format(&nat_addr, " > is_chassis_resident(%s)", > > - l3dgw_port->cr_port->json_key); > > - } > > - nats[0] = xstrdup(ds_cstr(&nat_addr)); > > - ds_destroy(&nat_addr); > > - } > > - } > > - > > - /* Add the router mac and IPv4 addresses to > > - * Port_Binding.nat_addresses so that GARP is sent for these > > - * IPs by the ovn-controller on which the distributed gateway > > - * router port resides if: > > - * > > - * - op->peer has 'reside-on-redirect-chassis' set and the > > - * the logical router datapath has distributed router > port. > > - * > > - * - op->peer is distributed gateway router port. > > - * > > - * - op->peer's router is a gateway router and op has a > localnet > > - * port. > > - * > > - * Note: Port_Binding.nat_addresses column is also used for > > - * sending the GARPs for the router port IPs. > > - * */ > > - bool add_router_port_garp = false; > > - if (op->peer && op->peer->nbrp && > op->peer->od->n_l3dgw_ports) { > > - if (is_l3dgw_port(op->peer)) { > > - add_router_port_garp = true; > > - } else if (smap_get_bool(&op->peer->nbrp->options, > > - "reside-on-redirect-chassis", false)) { > > - if (op->peer->od->n_l3dgw_ports == 1) { > > - add_router_port_garp = true; > > - } else { > > - static struct vlog_rate_limit rl = > > - VLOG_RATE_LIMIT_INIT(1, 1); > > - VLOG_WARN_RL(&rl, > "\"reside-on-redirect-chassis\" is " > > - "set on logical router port %s, > which " > > - "is on logical router %s, which has > %" > > - PRIuSIZE" distributed gateway > ports. This" > > - "option can only be used when there > is " > > - "a single distributed gateway > port.", > > - op->peer->key, > op->peer->od->nbr->name, > > - op->peer->od->n_l3dgw_ports); > > - } > > - } > > - } else if (chassis && op->od->n_localnet_ports) { > > - add_router_port_garp = true; > > - } > > - > > - if (add_router_port_garp) { > > - struct ds garp_info = DS_EMPTY_INITIALIZER; > > - ds_put_format(&garp_info, "%s", > op->peer->lrp_networks.ea_s); > > - > > - for (size_t i = 0; i < > op->peer->lrp_networks.n_ipv4_addrs; > > - i++) { > > - ds_put_format(&garp_info, " %s", > > - > op->peer->lrp_networks.ipv4_addrs[i].addr_s); > > - } > > - > > - if (op->peer->od->n_l3dgw_ports) { > > - const struct ovn_port *l3dgw_port = ( > > - is_l3dgw_port(op->peer) > > - ? op->peer > > - : op->peer->od->l3dgw_ports[0]); > > - ds_put_format(&garp_info, " is_chassis_resident(%s)", > > - l3dgw_port->cr_port->json_key); > > - } > > - > > - n_nats++; > > - nats = xrealloc(nats, (n_nats * sizeof *nats)); > > - nats[n_nats - 1] = ds_steal_cstr(&garp_info); > > - ds_destroy(&garp_info); > > - } > > - sbrec_port_binding_set_nat_addresses(op->sb, > > - (const char **) nats, > n_nats); > > - for (size_t i = 0; i < n_nats; i++) { > > - free(nats[i]); > > - } > > - free(nats); > > } > > > > sbrec_port_binding_set_parent_port(op->sb, > op->nbsp->parent_name); > > @@ -4695,6 +4583,137 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn, > > } > > } > > > > +/* Sync the SB Port bindings which needs to be updated. > > + * Presently it syncs the nat column of port bindings corresponding to > > + * the logical switch ports. */ > > +void sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) > > +{ > > + ovs_assert(ovnsb_idl_txn); > > + > > + struct ovn_port *op; > > + HMAP_FOR_EACH (op, key_node, ls_ports) { > > + if (lsp_is_router(op->nbsp)) { > > + const char *chassis = NULL; > > + if (op->peer && op->peer->od && op->peer->od->nbr) { > > + chassis = smap_get(&op->peer->od->nbr->options, > "chassis"); > > + } > > + > > + const char *nat_addresses = smap_get(&op->nbsp->options, > > + "nat-addresses"); > > + size_t n_nats = 0; > > + char **nats = NULL; > > + bool l3dgw_ports = op->peer && op->peer->od && > > + op->peer->od->n_l3dgw_ports; > > + if (nat_addresses && !strcmp(nat_addresses, "router")) { > > + if (op->peer && op->peer->od > > + && (chassis || op->peer->od->n_l3dgw_ports)) { > > + bool exclude_lb_vips = > smap_get_bool(&op->nbsp->options, > > + "exclude-lb-vips-from-garp", false); > > + nats = get_nat_addresses(op->peer, &n_nats, false, > > + !exclude_lb_vips); > > + } > > + } else if (nat_addresses && (chassis || l3dgw_ports)) { > > + struct lport_addresses laddrs; > > + if (!extract_lsp_addresses(nat_addresses, &laddrs)) { > > + static struct vlog_rate_limit rl = > > + VLOG_RATE_LIMIT_INIT(1, 1); > > + VLOG_WARN_RL(&rl, "Error extracting nat-addresses."); > > + } else { > > + destroy_lport_addresses(&laddrs); > > + n_nats = 1; > > + nats = xcalloc(1, sizeof *nats); > > + struct ds nat_addr = DS_EMPTY_INITIALIZER; > > + ds_put_format(&nat_addr, "%s", nat_addresses); > > + if (l3dgw_ports) { > > + const struct ovn_port *l3dgw_port = ( > > + is_l3dgw_port(op->peer) > > + ? op->peer > > + : op->peer->od->l3dgw_ports[0]); > > + ds_put_format(&nat_addr, " > is_chassis_resident(%s)", > > + l3dgw_port->cr_port->json_key); > > + } > > + nats[0] = xstrdup(ds_cstr(&nat_addr)); > > + ds_destroy(&nat_addr); > > + } > > + } > > + > > + /* Add the router mac and IPv4 addresses to > > + * Port_Binding.nat_addresses so that GARP is sent for these > > + * IPs by the ovn-controller on which the distributed gateway > > + * router port resides if: > > + * > > + * - op->peer has 'reside-on-redirect-chassis' set and the > > + * the logical router datapath has distributed router > port. > > + * > > + * - op->peer is distributed gateway router port. > > + * > > + * - op->peer's router is a gateway router and op has a > localnet > > + * port. > > + * > > + * Note: Port_Binding.nat_addresses column is also used for > > + * sending the GARPs for the router port IPs. > > + * */ > > + bool add_router_port_garp = false; > > + if (op->peer && op->peer->nbrp && > op->peer->od->n_l3dgw_ports) { > > + if (is_l3dgw_port(op->peer)) { > > + add_router_port_garp = true; > > + } else if (smap_get_bool(&op->peer->nbrp->options, > > + "reside-on-redirect-chassis", false)) { > > + if (op->peer->od->n_l3dgw_ports == 1) { > > + add_router_port_garp = true; > > + } else { > > + static struct vlog_rate_limit rl = > > + VLOG_RATE_LIMIT_INIT(1, 1); > > + VLOG_WARN_RL(&rl, > "\"reside-on-redirect-chassis\" is " > > + "set on logical router port %s, > which " > > + "is on logical router %s, which has > %" > > + PRIuSIZE" distributed gateway > ports. This" > > + "option can only be used when there > is " > > + "a single distributed gateway > port.", > > + op->peer->key, > op->peer->od->nbr->name, > > + op->peer->od->n_l3dgw_ports); > > + } > > + } > > + } else if (chassis && op->od->n_localnet_ports) { > > + add_router_port_garp = true; > > + } > > + > > + if (add_router_port_garp) { > > + struct ds garp_info = DS_EMPTY_INITIALIZER; > > + ds_put_format(&garp_info, "%s", > op->peer->lrp_networks.ea_s); > > + > > + for (size_t i = 0; i < > op->peer->lrp_networks.n_ipv4_addrs; > > + i++) { > > + ds_put_format(&garp_info, " %s", > > + > op->peer->lrp_networks.ipv4_addrs[i].addr_s); > > + } > > + > > + if (op->peer->od->n_l3dgw_ports) { > > + const struct ovn_port *l3dgw_port = ( > > + is_l3dgw_port(op->peer) > > + ? op->peer > > + : op->peer->od->l3dgw_ports[0]); > > + ds_put_format(&garp_info, " is_chassis_resident(%s)", > > + l3dgw_port->cr_port->json_key); > > + } > > + > > + n_nats++; > > + nats = xrealloc(nats, (n_nats * sizeof *nats)); > > + nats[n_nats - 1] = ds_steal_cstr(&garp_info); > > + ds_destroy(&garp_info); > > + } > > + sbrec_port_binding_set_nat_addresses(op->sb, > > + (const char **) nats, > n_nats); > > + for (size_t i = 0; i < n_nats; i++) { > > + free(nats[i]); > > + } > > + free(nats); > > + } else { > > + sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); > > + } > > + } > > +} > > + > > static bool > > ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) > > { > > diff --git a/northd/northd.h b/northd/northd.h > > index 044d4ee0c0..cd2e5394c2 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -374,4 +374,6 @@ const char *northd_get_svc_monitor_mac(void); > > void sync_lbs(struct ovsdb_idl_txn *, const struct > sbrec_load_balancer_table *, > > struct ovn_datapaths *ls_datapaths, struct hmap *lbs); > > > > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); > > + > > #endif /* NORTHD_H */ > > -- > > 2.40.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index 9832fce30a..552ed56452 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -254,6 +254,37 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data OVS_UNUSED) return false; } +/* sync_to_sb_pb engine node functions. + * This engine node syncs the SB Port Bindings (partly). + * en_northd engine create the SB Port binding rows and + * updates most of the columns. + * This engine node updates the port binding columns which + * needs to be updated after northd engine is run. + */ + +void * +en_sync_to_sb_pb_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + return NULL; +} + +void +en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED) +{ + const struct engine_context *eng_ctx = engine_get_context(); + struct northd_data *northd_data = engine_get_input_data("northd", node); + + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); + engine_set_node_state(node, EN_UPDATED); +} + +void +en_sync_to_sb_pb_cleanup(void *data OVS_UNUSED) +{ + +} + /* static functions. */ static void sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h index 06d2a57710..700d3340e4 100644 --- a/northd/en-sync-sb.h +++ b/northd/en-sync-sb.h @@ -22,4 +22,8 @@ void en_sync_to_sb_lb_run(struct engine_node *, void *data); void en_sync_to_sb_lb_cleanup(void *data); bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data OVS_UNUSED); +void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *); +void en_sync_to_sb_pb_run(struct engine_node *, void *data); +void en_sync_to_sb_pb_cleanup(void *data); + #endif /* end of EN_SYNC_SB_H */ diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 402c94e88c..dc8b880fd8 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -141,6 +141,7 @@ static ENGINE_NODE(sync_to_sb_addr_set, "sync_to_sb_addr_set"); static ENGINE_NODE(fdb_aging, "fdb_aging"); static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker"); static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb"); +static ENGINE_NODE(sync_to_sb_pb, "sync_to_sb_pb"); static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data"); void inc_proc_northd_init(struct ovsdb_idl_loop *nb, @@ -215,13 +216,16 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, sync_to_sb_lb_northd_handler); engine_add_input(&en_sync_to_sb_lb, &en_sb_load_balancer, NULL); + engine_add_input(&en_sync_to_sb_pb, &en_northd, NULL); + /* en_sync_to_sb engine node syncs the SB database tables from * the NB database tables. - * Right now this engine syncs the SB Address_Set table and - * SB Load_Balancer table. + * Right now this engine syncs the SB Address_Set table, + * SB Load_Balancer table and (partly) SB Port_Binding table. */ engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL); engine_add_input(&en_sync_to_sb, &en_sync_to_sb_lb, NULL); + engine_add_input(&en_sync_to_sb, &en_sync_to_sb_pb, NULL); engine_add_input(&en_sync_from_sb, &en_northd, sync_from_sb_northd_handler); diff --git a/northd/northd.c b/northd/northd.c index 1477b79331..a3bd21e0b4 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3514,8 +3514,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, ds_destroy(&s); sbrec_port_binding_set_external_ids(op->sb, &op->nbrp->external_ids); - - sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } else { if (!lsp_is_router(op->nbsp)) { uint32_t queue_id = smap_get_int( @@ -3631,116 +3629,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, } else { sbrec_port_binding_set_options(op->sb, NULL); } - const char *nat_addresses = smap_get(&op->nbsp->options, - "nat-addresses"); - size_t n_nats = 0; - char **nats = NULL; - bool l3dgw_ports = op->peer && op->peer->od && - op->peer->od->n_l3dgw_ports; - if (nat_addresses && !strcmp(nat_addresses, "router")) { - if (op->peer && op->peer->od - && (chassis || op->peer->od->n_l3dgw_ports)) { - bool exclude_lb_vips = smap_get_bool(&op->nbsp->options, - "exclude-lb-vips-from-garp", false); - nats = get_nat_addresses(op->peer, &n_nats, false, - !exclude_lb_vips); - } - } else if (nat_addresses && (chassis || l3dgw_ports)) { - struct lport_addresses laddrs; - if (!extract_lsp_addresses(nat_addresses, &laddrs)) { - static struct vlog_rate_limit rl = - VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "Error extracting nat-addresses."); - } else { - destroy_lport_addresses(&laddrs); - n_nats = 1; - nats = xcalloc(1, sizeof *nats); - struct ds nat_addr = DS_EMPTY_INITIALIZER; - ds_put_format(&nat_addr, "%s", nat_addresses); - if (l3dgw_ports) { - const struct ovn_port *l3dgw_port = ( - is_l3dgw_port(op->peer) - ? op->peer - : op->peer->od->l3dgw_ports[0]); - ds_put_format(&nat_addr, " is_chassis_resident(%s)", - l3dgw_port->cr_port->json_key); - } - nats[0] = xstrdup(ds_cstr(&nat_addr)); - ds_destroy(&nat_addr); - } - } - - /* Add the router mac and IPv4 addresses to - * Port_Binding.nat_addresses so that GARP is sent for these - * IPs by the ovn-controller on which the distributed gateway - * router port resides if: - * - * - op->peer has 'reside-on-redirect-chassis' set and the - * the logical router datapath has distributed router port. - * - * - op->peer is distributed gateway router port. - * - * - op->peer's router is a gateway router and op has a localnet - * port. - * - * Note: Port_Binding.nat_addresses column is also used for - * sending the GARPs for the router port IPs. - * */ - bool add_router_port_garp = false; - if (op->peer && op->peer->nbrp && op->peer->od->n_l3dgw_ports) { - if (is_l3dgw_port(op->peer)) { - add_router_port_garp = true; - } else if (smap_get_bool(&op->peer->nbrp->options, - "reside-on-redirect-chassis", false)) { - if (op->peer->od->n_l3dgw_ports == 1) { - add_router_port_garp = true; - } else { - static struct vlog_rate_limit rl = - VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "\"reside-on-redirect-chassis\" is " - "set on logical router port %s, which " - "is on logical router %s, which has %" - PRIuSIZE" distributed gateway ports. This" - "option can only be used when there is " - "a single distributed gateway port.", - op->peer->key, op->peer->od->nbr->name, - op->peer->od->n_l3dgw_ports); - } - } - } else if (chassis && op->od->n_localnet_ports) { - add_router_port_garp = true; - } - - if (add_router_port_garp) { - struct ds garp_info = DS_EMPTY_INITIALIZER; - ds_put_format(&garp_info, "%s", op->peer->lrp_networks.ea_s); - - for (size_t i = 0; i < op->peer->lrp_networks.n_ipv4_addrs; - i++) { - ds_put_format(&garp_info, " %s", - op->peer->lrp_networks.ipv4_addrs[i].addr_s); - } - - if (op->peer->od->n_l3dgw_ports) { - const struct ovn_port *l3dgw_port = ( - is_l3dgw_port(op->peer) - ? op->peer - : op->peer->od->l3dgw_ports[0]); - ds_put_format(&garp_info, " is_chassis_resident(%s)", - l3dgw_port->cr_port->json_key); - } - - n_nats++; - nats = xrealloc(nats, (n_nats * sizeof *nats)); - nats[n_nats - 1] = ds_steal_cstr(&garp_info); - ds_destroy(&garp_info); - } - sbrec_port_binding_set_nat_addresses(op->sb, - (const char **) nats, n_nats); - for (size_t i = 0; i < n_nats; i++) { - free(nats[i]); - } - free(nats); } sbrec_port_binding_set_parent_port(op->sb, op->nbsp->parent_name); @@ -4695,6 +4583,137 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn, } } +/* Sync the SB Port bindings which needs to be updated. + * Presently it syncs the nat column of port bindings corresponding to + * the logical switch ports. */ +void sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) +{ + ovs_assert(ovnsb_idl_txn); + + struct ovn_port *op; + HMAP_FOR_EACH (op, key_node, ls_ports) { + if (lsp_is_router(op->nbsp)) { + const char *chassis = NULL; + if (op->peer && op->peer->od && op->peer->od->nbr) { + chassis = smap_get(&op->peer->od->nbr->options, "chassis"); + } + + const char *nat_addresses = smap_get(&op->nbsp->options, + "nat-addresses"); + size_t n_nats = 0; + char **nats = NULL; + bool l3dgw_ports = op->peer && op->peer->od && + op->peer->od->n_l3dgw_ports; + if (nat_addresses && !strcmp(nat_addresses, "router")) { + if (op->peer && op->peer->od + && (chassis || op->peer->od->n_l3dgw_ports)) { + bool exclude_lb_vips = smap_get_bool(&op->nbsp->options, + "exclude-lb-vips-from-garp", false); + nats = get_nat_addresses(op->peer, &n_nats, false, + !exclude_lb_vips); + } + } else if (nat_addresses && (chassis || l3dgw_ports)) { + struct lport_addresses laddrs; + if (!extract_lsp_addresses(nat_addresses, &laddrs)) { + static struct vlog_rate_limit rl = + VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "Error extracting nat-addresses."); + } else { + destroy_lport_addresses(&laddrs); + n_nats = 1; + nats = xcalloc(1, sizeof *nats); + struct ds nat_addr = DS_EMPTY_INITIALIZER; + ds_put_format(&nat_addr, "%s", nat_addresses); + if (l3dgw_ports) { + const struct ovn_port *l3dgw_port = ( + is_l3dgw_port(op->peer) + ? op->peer + : op->peer->od->l3dgw_ports[0]); + ds_put_format(&nat_addr, " is_chassis_resident(%s)", + l3dgw_port->cr_port->json_key); + } + nats[0] = xstrdup(ds_cstr(&nat_addr)); + ds_destroy(&nat_addr); + } + } + + /* Add the router mac and IPv4 addresses to + * Port_Binding.nat_addresses so that GARP is sent for these + * IPs by the ovn-controller on which the distributed gateway + * router port resides if: + * + * - op->peer has 'reside-on-redirect-chassis' set and the + * the logical router datapath has distributed router port. + * + * - op->peer is distributed gateway router port. + * + * - op->peer's router is a gateway router and op has a localnet + * port. + * + * Note: Port_Binding.nat_addresses column is also used for + * sending the GARPs for the router port IPs. + * */ + bool add_router_port_garp = false; + if (op->peer && op->peer->nbrp && op->peer->od->n_l3dgw_ports) { + if (is_l3dgw_port(op->peer)) { + add_router_port_garp = true; + } else if (smap_get_bool(&op->peer->nbrp->options, + "reside-on-redirect-chassis", false)) { + if (op->peer->od->n_l3dgw_ports == 1) { + add_router_port_garp = true; + } else { + static struct vlog_rate_limit rl = + VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "\"reside-on-redirect-chassis\" is " + "set on logical router port %s, which " + "is on logical router %s, which has %" + PRIuSIZE" distributed gateway ports. This" + "option can only be used when there is " + "a single distributed gateway port.", + op->peer->key, op->peer->od->nbr->name, + op->peer->od->n_l3dgw_ports); + } + } + } else if (chassis && op->od->n_localnet_ports) { + add_router_port_garp = true; + } + + if (add_router_port_garp) { + struct ds garp_info = DS_EMPTY_INITIALIZER; + ds_put_format(&garp_info, "%s", op->peer->lrp_networks.ea_s); + + for (size_t i = 0; i < op->peer->lrp_networks.n_ipv4_addrs; + i++) { + ds_put_format(&garp_info, " %s", + op->peer->lrp_networks.ipv4_addrs[i].addr_s); + } + + if (op->peer->od->n_l3dgw_ports) { + const struct ovn_port *l3dgw_port = ( + is_l3dgw_port(op->peer) + ? op->peer + : op->peer->od->l3dgw_ports[0]); + ds_put_format(&garp_info, " is_chassis_resident(%s)", + l3dgw_port->cr_port->json_key); + } + + n_nats++; + nats = xrealloc(nats, (n_nats * sizeof *nats)); + nats[n_nats - 1] = ds_steal_cstr(&garp_info); + ds_destroy(&garp_info); + } + sbrec_port_binding_set_nat_addresses(op->sb, + (const char **) nats, n_nats); + for (size_t i = 0; i < n_nats; i++) { + free(nats[i]); + } + free(nats); + } else { + sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); + } + } +} + static bool ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) { diff --git a/northd/northd.h b/northd/northd.h index 044d4ee0c0..cd2e5394c2 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -374,4 +374,6 @@ const char *northd_get_svc_monitor_mac(void); void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *, struct ovn_datapaths *ls_datapaths, struct hmap *lbs); +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); + #endif /* NORTHD_H */