On Mon, Sep 11, 2023 at 9:03 AM <[email protected]> wrote: > > From: Numan Siddique <[email protected]> > > 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. > > Reviewed-by: Ales Musil <[email protected]> > Acked-by: Mark Michelson <[email protected]> > Signed-off-by: Numan Siddique <[email protected]>
Thanks Numan. I have tested and confirmed that the new handler fixed the performance regression that was introduced in v6. Acked-by: Han Zhou <[email protected]> > --- > northd/en-sync-sb.c | 52 ++++++++ > northd/en-sync-sb.h | 5 + > northd/inc-proc-northd.c | 8 +- > northd/northd.c | 274 +++++++++++++++++++++++---------------- > northd/northd.h | 3 + > tests/ovn-northd.at | 17 ++- > 6 files changed, 239 insertions(+), 120 deletions(-) > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index f6dbd7b2b6..47ace03417 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -244,6 +244,58 @@ 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) > +{ > + > +} > + > +bool > +sync_to_sb_pb_northd_handler(struct engine_node *node, void *data OVS_UNUSED) > +{ > + const struct engine_context *eng_ctx = engine_get_context(); > + if (!eng_ctx->ovnsb_idl_txn) { > + return false; > + } > + > + struct northd_data *nd = engine_get_input_data("northd", node); > + if (!nd->change_tracked) { > + return false; > + } > + > + if (!sync_pbs_for_northd_ls_changes(&nd->tracked_ls_changes)) { > + return false; > + } > + > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > + > /* 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..f08565eee1 100644 > --- a/northd/en-sync-sb.h > +++ b/northd/en-sync-sb.h > @@ -22,4 +22,9 @@ 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); > +bool sync_to_sb_pb_northd_handler(struct engine_node *, void *data OVS_UNUSED); > + > #endif /* end of EN_SYNC_SB_H */ > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 303b58d43f..e9e28c4bea 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -144,6 +144,7 @@ static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_group, "port_group"); > 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, > @@ -228,15 +229,20 @@ 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, > + sync_to_sb_pb_northd_handler); > + > /* 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, Port_Group table > - * SB Meter/Meter_Band tables and SB Load_Balancer table. > + * SB Meter/Meter_Band tables and 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_port_group, NULL); > engine_add_input(&en_sync_to_sb, &en_sync_meters, 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 c5e1d1c5a4..5651ee38c5 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3527,8 +3527,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( > @@ -3672,116 +3670,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); > @@ -4735,6 +4623,168 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn, > } > } > > +/* Syncs the SB port binding for the ovn_port 'op'. Caller should make sure > + * that the OVN SB IDL txn is not NULL. Presently it only syncs the nat > + * column of port binding corresponding to the 'op->nbsp' */ > +static void > +sync_pb_for_op(struct ovn_port *op) > +{ > + 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); > + } > +} > + > +/* 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) { > + sync_pb_for_op(op); > + } > +} > + > +/* Sync the SB Port bindings for the added and updated logical switch ports > + * of the tracked logical switches (from the northd engine node). */ > +bool > +sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *ls_changes) > +{ > + struct ls_change *ls_change; > + LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) { > + struct ovn_port *op; > + > + LIST_FOR_EACH (op, list, &ls_change->added_ports) { > + sync_pb_for_op(op); > + } > + > + LIST_FOR_EACH (op, list, &ls_change->updated_ports) { > + sync_pb_for_op(op); > + } > + } > + > + return true; > +} > + > 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 0d206a4e52..5d8ac6fea0 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -365,4 +365,7 @@ const struct ovn_datapath *northd_get_datapath_for_port( > 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); > +bool sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *); > + > #endif /* NORTHD_H */ > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 34e10663d0..1f8b264bde 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9997,6 +9997,9 @@ check_recompute_counter() { > > lflow_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute) > AT_CHECK([test x$lflow_recomp = x$2]) > + > + sync_sb_pb_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_to_sb_pb recompute) > + AT_CHECK([test x$sync_sb_pb_recomp = x$3]) > } > > check ovn-nbctl --wait=hv ls-add ls0 > @@ -10013,29 +10016,29 @@ check ovn-nbctl --wait=hv lsp-add ls0 lsp0-0 -- lsp-set-addresses lsp0-0 "unknow > ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0 external_ids:iface-id=lsp0-0 > wait_for_ports_up > check ovn-nbctl --wait=hv sync > -check_recompute_counter 5 5 > +check_recompute_counter 5 5 5 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 "aa:aa:aa:00:00:01 192.168.0.11" > ovs-vsctl add-port br-int lsp0-1 -- set interface lsp0-1 external_ids:iface-id=lsp0-1 > wait_for_ports_up > check ovn-nbctl --wait=hv sync > -check_recompute_counter 0 0 > +check_recompute_counter 0 0 0 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=hv lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 "aa:aa:aa:00:00:02 192.168.0.12" > ovs-vsctl add-port br-int lsp0-2 -- set interface lsp0-2 external_ids:iface-id=lsp0-2 > wait_for_ports_up > check ovn-nbctl --wait=hv sync > -check_recompute_counter 0 0 > +check_recompute_counter 0 0 0 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=hv lsp-del lsp0-1 > -check_recompute_counter 0 0 > +check_recompute_counter 0 0 0 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=hv lsp-set-addresses lsp0-2 "aa:aa:aa:00:00:88 192.168.0.88" > -check_recompute_counter 0 0 > +check_recompute_counter 0 0 0 > > # Delete and re-add a LSP for several times continuously, to ensure > # frequent operations do not trigger recompute when there are in-flight > @@ -10049,12 +10052,12 @@ for i in $(seq 10); do > check ovn-nbctl lsp-del lsp0-2 > check ovn-nbctl lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 "aa:aa:aa:00:00:02 192.168.0.12" > done > -check_recompute_counter 0 0 > +check_recompute_counter 0 0 0 > > # No change, no recompute > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb sync > -check_recompute_counter 0 0 > +check_recompute_counter 0 0 0 > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > -- > 2.41.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
