| Message ID | ZzXVOra5KEuCN1NN@SIT-SDELAP1634.int.lidl.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4] controller: Add nat_addresses engine. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Recheck-request: github-robot On Thu, Nov 14, 2024 at 11:47:22AM +0100, Max Lamprecht via dev wrote: > This patch moves the nat_addresses calculation to an own engine node. > > Previously the nat_address calculation was executed at every pinctrl_run > (send_garp_rarp_prepare). This can cause high cpu usage and delayed > actions on gateway-chassis(500 LRPs+). > > perf trace: > - 98.83% main > - 79.69% pinctrl_run > - 77.71% send_garp_rarp_prepare > - 55.49% get_nat_addresses_and_keys > - 36.76% consider_nat_address > - 18.13% lport_lookup_by_name > - 13.58% sset_destroy > > Co-authored-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > Signed-off-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > Signed-off-by: Max Lamprecht <max.lamprecht@stackit.cloud> > --- > controller/ovn-controller.c | 341 +++++++++++++++++++++++++++++++- > controller/pinctrl.c | 381 ++++++------------------------------ > controller/pinctrl.h | 14 +- > 3 files changed, 410 insertions(+), 326 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 98b144699..49bc6d86c 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -3359,6 +3359,319 @@ en_dns_cache_cleanup(void *data OVS_UNUSED) > ovn_dns_cache_destroy(); > } > > +static void * > +en_nat_addresses_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct ed_type_nat_addresses *data = xzalloc(sizeof *data); > + shash_init(&data->nat_addresses); > + sset_init(&data->localnet_vifs); > + sset_init(&data->local_l3gw_ports); > + sset_init(&data->nat_ip_keys); > + return data; > +} > + > +/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > + * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > + * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid IPv4 > + * or IPv6 address, and stores them in the 'ipv4_addrs' and 'ipv6_addrs' > + * fields of 'laddrs'. The logical port name is stored in 'lport'. > + * > + * Returns true if at least 'MAC' is found in 'address', false otherwise. > + * > + * The caller must call destroy_lport_addresses() and free(*lport). */ > +static bool > +extract_addresses_with_port(const char *addresses, > + struct lport_addresses *laddrs, > + char **lport) > +{ > + int ofs; > + if (!extract_addresses(addresses, laddrs, &ofs)) { > + return false; > + } else if (!addresses[ofs]) { > + return true; > + } > + > + struct lexer lexer; > + lexer_init(&lexer, addresses + ofs); > + lexer_get(&lexer); > + > + if (lexer.error || lexer.token.type != LEX_T_ID > + || !lexer_match_id(&lexer, "is_chassis_resident")) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", addresses); > + lexer_destroy(&lexer); > + return true; > + } > + > + if (!lexer_match(&lexer, LEX_T_LPAREN)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > + "'is_chassis_resident' in address '%s'", addresses); > + lexer_destroy(&lexer); > + return false; > + } > + > + if (lexer.token.type != LEX_T_STRING) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_INFO_RL(&rl, > + "Syntax error: expecting quoted string after " > + "'is_chassis_resident' in address '%s'", addresses); > + lexer_destroy(&lexer); > + return false; > + } > + > + *lport = xstrdup(lexer.token.s); > + > + lexer_get(&lexer); > + if (!lexer_match(&lexer, LEX_T_RPAREN)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted string in " > + "'is_chassis_resident()' in address '%s'", > + addresses); > + lexer_destroy(&lexer); > + return false; > + } > + > + lexer_destroy(&lexer); > + return true; > +} > + > +static void > +consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const char *nat_address, > + const struct sbrec_port_binding *pb, > + struct sset *nat_address_keys, > + const struct sbrec_chassis *chassis, > + const struct sset *active_tunnels, > + struct shash *nat_addresses) > +{ > + struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > + char *lport = NULL; > + if (!extract_addresses_with_port(nat_address, laddrs, &lport) > + || (!lport && !strcmp(pb->type, "patch")) > + || (lport && !lport_is_chassis_resident( > + sbrec_port_binding_by_name, chassis, > + active_tunnels, lport))) { > + destroy_lport_addresses(laddrs); > + free(laddrs); > + free(lport); > + return; > + } > + free(lport); > + > + int i; > + for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > + char *name = xasprintf("%s-%s", pb->logical_port, > + laddrs->ipv4_addrs[i].addr_s); > + sset_add(nat_address_keys, name); > + free(name); > + } > + if (laddrs->n_ipv4_addrs == 0) { > + char *name = xasprintf("%s-noip", pb->logical_port); > + sset_add(nat_address_keys, name); > + free(name); > + } > + shash_add(nat_addresses, pb->logical_port, laddrs); > +} > + > +static void > +get_nat_addresses_and_keys(struct ovsdb_idl_index *sbrec_port_binding_by_name, > + struct sset *nat_address_keys, > + struct sset *local_l3gw_ports, > + const struct sbrec_chassis *chassis, > + const struct sset *active_tunnels, > + struct shash *nat_addresses) > +{ > + const char *gw_port; > + SSET_FOR_EACH (gw_port, local_l3gw_ports) { > + const struct sbrec_port_binding *pb; > + > + pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > + if (!pb) { > + continue; > + } > + > + if (pb->n_nat_addresses) { > + for (int i = 0; i < pb->n_nat_addresses; i++) { > + consider_nat_address(sbrec_port_binding_by_name, > + pb->nat_addresses[i], pb, > + nat_address_keys, chassis, > + active_tunnels, > + nat_addresses); > + } > + } else { > + /* Continue to support options:nat-addresses for version > + * upgrade. */ > + const char *nat_addresses_options = smap_get(&pb->options, > + "nat-addresses"); > + if (nat_addresses_options) { > + consider_nat_address(sbrec_port_binding_by_name, > + nat_addresses_options, pb, > + nat_address_keys, chassis, > + active_tunnels, > + nat_addresses); > + } > + } > + } > +} > + > +/* Get localnet vifs, local l3gw ports and ofport for localnet patch ports. */ > +static void > +get_localnet_vifs_l3gwports( > + struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const struct ovsrec_bridge *br_int, > + const struct sbrec_chassis *chassis, > + const struct hmap *local_datapaths, > + struct sset *localnet_vifs, > + struct sset *local_l3gw_ports) > +{ > + for (int i = 0; i < br_int->n_ports; i++) { > + const struct ovsrec_port *port_rec = br_int->ports[i]; > + if (!strcmp(port_rec->name, br_int->name)) { > + continue; > + } > + const char *tunnel_id = smap_get(&port_rec->external_ids, > + "ovn-chassis-id"); > + if (tunnel_id && > + encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, NULL)) { > + continue; > + } > + const char *localnet = smap_get(&port_rec->external_ids, > + "ovn-localnet-port"); > + if (localnet) { > + continue; > + } > + for (int j = 0; j < port_rec->n_interfaces; j++) { > + const struct ovsrec_interface *iface_rec = port_rec->interfaces[j]; > + if (!iface_rec->n_ofport) { > + continue; > + } > + /* Get localnet vif. */ > + const char *iface_id = smap_get(&iface_rec->external_ids, > + "iface-id"); > + if (!iface_id) { > + continue; > + } > + const struct sbrec_port_binding *pb > + = lport_lookup_by_name(sbrec_port_binding_by_name, iface_id); > + if (!pb || pb->chassis != chassis) { > + continue; > + } > + if (!iface_rec->link_state || > + strcmp(iface_rec->link_state, "up")) { > + continue; > + } > + struct local_datapath *ld > + = get_local_datapath(local_datapaths, > + pb->datapath->tunnel_key); > + if (ld && ld->localnet_port) { > + sset_add(localnet_vifs, iface_id); > + } > + } > + } > + > + struct sbrec_port_binding *target = sbrec_port_binding_index_init_row( > + sbrec_port_binding_by_datapath); > + > + const struct local_datapath *ld; > + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > + const struct sbrec_port_binding *pb; > + > + if (!ld->localnet_port) { > + continue; > + } > + > + /* Get l3gw ports. Consider port bindings with type "l3gateway" > + * that connect to gateway routers (if local), and consider port > + * bindings of type "patch" since they might connect to > + * distributed gateway ports with NAT addresses. */ > + > + sbrec_port_binding_index_set_datapath(target, ld->datapath); > + SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > + sbrec_port_binding_by_datapath) { > + if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) > + || !strcmp(pb->type, "patch")) { > + sset_add(local_l3gw_ports, pb->logical_port); > + } > + } > + } > + sbrec_port_binding_index_destroy_row(target); > +} > + > +static void > +en_nat_addresses_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + struct ed_type_nat_addresses *nat_addresses = data; > + const struct ovsrec_open_vswitch_table *ovs_table = > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > + struct ovsdb_idl_index *sbrec_chassis_by_name = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_chassis", node), > + "name"); > + const struct sbrec_chassis *chassis > + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > + > + struct ovsdb_idl_index *sbrec_pb_by_datapath = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "datapath"); > + struct ovsdb_idl_index *sbrec_pb_by_name = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "name"); > + const struct ovsrec_bridge_table *bridge_table = > + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > + const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table); > + > + struct ed_type_runtime_data *rt_data = > + engine_get_input_data("runtime_data", node); > + const struct hmap *local_datapaths = &rt_data->local_datapaths; > + const struct sset *active_tunnels = &rt_data->active_tunnels; > + > + struct shash_node *nat_node; > + SHASH_FOR_EACH (nat_node, &nat_addresses->nat_addresses) { > + struct lport_addresses *laddrs = nat_node->data; > + destroy_lport_addresses(laddrs); > + free(laddrs); > + } > + shash_clear(&nat_addresses->nat_addresses); > + sset_clear(&nat_addresses->localnet_vifs); > + sset_clear(&nat_addresses->local_l3gw_ports); > + sset_clear(&nat_addresses->nat_ip_keys); > + > + get_localnet_vifs_l3gwports(sbrec_pb_by_datapath, > + sbrec_pb_by_name, > + br_int, chassis, local_datapaths, > + &nat_addresses->localnet_vifs, > + &nat_addresses->local_l3gw_ports); > + > + get_nat_addresses_and_keys(sbrec_pb_by_name, > + &nat_addresses->nat_ip_keys, > + &nat_addresses->local_l3gw_ports, > + chassis, active_tunnels, > + &nat_addresses->nat_addresses); > + > + engine_set_node_state(node, EN_UPDATED); > +} > + > +static bool > +nat_addresses_change_handler(struct engine_node *node, void *data) > +{ > + en_nat_addresses_run(node, data); > + return true; > +} > + > +static void > +en_nat_addresses_cleanup(void *data OVS_UNUSED){ > + struct ed_type_nat_addresses *nat_addresses = data; > + shash_destroy(&nat_addresses->nat_addresses); > + sset_destroy(&nat_addresses->localnet_vifs); > + sset_destroy(&nat_addresses->local_l3gw_ports); > + sset_destroy(&nat_addresses->nat_ip_keys); > +} > > /* Engine node which is used to handle the Non VIF data like > * - OVS patch ports > @@ -4779,6 +5092,14 @@ controller_output_bfd_chassis_handler(struct engine_node *node, > return true; > } > > +static bool > +controller_output_nat_addresses_handler(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > + > /* Handles sbrec_chassis changes. > * If a new chassis is added or removed return false, so that > * flows are recomputed. For any updates, there is no need for > @@ -5093,6 +5414,7 @@ main(int argc, char *argv[]) > ENGINE_NODE(mac_cache, "mac_cache"); > ENGINE_NODE(bfd_chassis, "bfd_chassis"); > ENGINE_NODE(dns_cache, "dns_cache"); > + ENGINE_NODE(nat_addresses, "nat_addresses"); > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > SB_NODES > @@ -5137,6 +5459,14 @@ main(int argc, char *argv[]) > engine_add_input(&en_bfd_chassis, &en_sb_chassis, NULL); > engine_add_input(&en_bfd_chassis, &en_sb_ha_chassis_group, NULL); > > + engine_add_input(&en_nat_addresses, &en_ovs_open_vswitch, NULL); > + engine_add_input(&en_nat_addresses, &en_ovs_bridge, NULL); > + engine_add_input(&en_nat_addresses, &en_sb_chassis, NULL); > + engine_add_input(&en_nat_addresses, &en_sb_port_binding, > + nat_addresses_change_handler); > + engine_add_input(&en_nat_addresses, &en_runtime_data, > + nat_addresses_change_handler); > + > /* Note: The order of inputs is important, all OVS interface changes must > * be handled before any ct_zone changes. > */ > @@ -5300,6 +5630,8 @@ main(int argc, char *argv[]) > controller_output_mac_cache_handler); > engine_add_input(&en_controller_output, &en_bfd_chassis, > controller_output_bfd_chassis_handler); > + engine_add_input(&en_controller_output, &en_nat_addresses, > + controller_output_nat_addresses_handler); > > struct engine_arg engine_arg = { > .sb_idl = ovnsb_idl_loop.idl, > @@ -5346,6 +5678,8 @@ main(int argc, char *argv[]) > engine_get_internal_data(&en_ct_zones); > struct ed_type_bfd_chassis *bfd_chassis_data = > engine_get_internal_data(&en_bfd_chassis); > + struct ed_type_nat_addresses *nat_addresses_data = > + engine_get_internal_data(&en_nat_addresses); > struct ed_type_runtime_data *runtime_data = > engine_get_internal_data(&en_runtime_data); > struct ed_type_template_vars *template_vars_data = > @@ -5682,6 +6016,7 @@ main(int argc, char *argv[]) > } > > runtime_data = engine_get_data(&en_runtime_data); > + nat_addresses_data = engine_get_data(&en_nat_addresses); > if (runtime_data) { > stopwatch_start(PATCH_RUN_STOPWATCH_NAME, time_msec()); > patch_run(ovs_idl_txn, > @@ -5729,7 +6064,6 @@ main(int argc, char *argv[]) > pinctrl_update(ovnsb_idl_loop.idl); > pinctrl_run(ovnsb_idl_txn, > sbrec_datapath_binding_by_key, > - sbrec_port_binding_by_datapath, > sbrec_port_binding_by_key, > sbrec_port_binding_by_name, > sbrec_mac_binding_by_lport_ip, > @@ -5743,13 +6077,14 @@ main(int argc, char *argv[]) > sbrec_mac_binding_table_get( > ovnsb_idl_loop.idl), > sbrec_bfd_table_get(ovnsb_idl_loop.idl), > - br_int, chassis, > + chassis, > &runtime_data->local_datapaths, > &runtime_data->active_tunnels, > &runtime_data->local_active_ports_ipv6_pd, > &runtime_data->local_active_ports_ras, > ovsrec_open_vswitch_table_get( > - ovs_idl_loop.idl)); > + ovs_idl_loop.idl), > + nat_addresses_data); > stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME, > time_msec()); > mirror_run(ovs_idl_txn, > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 3fb7e2fd7..e8c4ff77b 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -227,14 +227,11 @@ static void destroy_send_garps_rarps(void); > static void send_garp_rarp_wait(long long int send_garp_rarp_time); > static void send_garp_rarp_prepare( > struct ovsdb_idl_txn *ovnsb_idl_txn, > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > - const struct ovsrec_bridge *, > - const struct sbrec_chassis *, > const struct hmap *local_datapaths, > - const struct sset *active_tunnels, > - const struct ovsrec_open_vswitch_table *ovs_table) > + const struct ovsrec_open_vswitch_table *ovs_table, > + struct ed_type_nat_addresses *nat_addresses) > OVS_REQUIRES(pinctrl_mutex); > static void send_garp_rarp_run(struct rconn *swconn, > long long int *send_garp_rarp_time) > @@ -4008,7 +4005,6 @@ pinctrl_update(const struct ovsdb_idl *idl) > void > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_key, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > @@ -4019,13 +4015,13 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sbrec_service_monitor_table *svc_mon_table, > const struct sbrec_mac_binding_table *mac_binding_table, > const struct sbrec_bfd_table *bfd_table, > - const struct ovsrec_bridge *br_int, > const struct sbrec_chassis *chassis, > const struct hmap *local_datapaths, > const struct sset *active_tunnels, > const struct shash *local_active_ports_ipv6_pd, > const struct shash *local_active_ports_ras, > - const struct ovsrec_open_vswitch_table *ovs_table) > + const struct ovsrec_open_vswitch_table *ovs_table, > + struct ed_type_nat_addresses *nat_addresses) > { > ovs_mutex_lock(&pinctrl_mutex); > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > @@ -4033,10 +4029,11 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > sbrec_mac_binding_by_lport_ip); > run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > sbrec_port_binding_by_key, chassis); > - send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath, > + send_garp_rarp_prepare(ovnsb_idl_txn, > sbrec_port_binding_by_name, > - sbrec_mac_binding_by_lport_ip, br_int, chassis, > - local_datapaths, active_tunnels, ovs_table); > + sbrec_mac_binding_by_lport_ip, > + local_datapaths, > + ovs_table, nat_addresses); > prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name); > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > local_active_ports_ipv6_pd, chassis, > @@ -6175,236 +6172,6 @@ ip_mcast_querier_wait(long long int query_time) > } > } > > -/* Get localnet vifs, local l3gw ports and ofport for localnet patch ports. */ > -static void > -get_localnet_vifs_l3gwports( > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > - struct ovsdb_idl_index *sbrec_port_binding_by_name, > - const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *chassis, > - const struct hmap *local_datapaths, > - struct sset *localnet_vifs, > - struct sset *local_l3gw_ports) > -{ > - for (int i = 0; i < br_int->n_ports; i++) { > - const struct ovsrec_port *port_rec = br_int->ports[i]; > - if (!strcmp(port_rec->name, br_int->name)) { > - continue; > - } > - const char *tunnel_id = smap_get(&port_rec->external_ids, > - "ovn-chassis-id"); > - if (tunnel_id && > - encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, NULL)) { > - continue; > - } > - const char *localnet = smap_get(&port_rec->external_ids, > - "ovn-localnet-port"); > - if (localnet) { > - continue; > - } > - for (int j = 0; j < port_rec->n_interfaces; j++) { > - const struct ovsrec_interface *iface_rec = port_rec->interfaces[j]; > - if (!iface_rec->n_ofport) { > - continue; > - } > - /* Get localnet vif. */ > - const char *iface_id = smap_get(&iface_rec->external_ids, > - "iface-id"); > - if (!iface_id) { > - continue; > - } > - const struct sbrec_port_binding *pb > - = lport_lookup_by_name(sbrec_port_binding_by_name, iface_id); > - if (!pb || pb->chassis != chassis) { > - continue; > - } > - if (!iface_rec->link_state || > - strcmp(iface_rec->link_state, "up")) { > - continue; > - } > - struct local_datapath *ld > - = get_local_datapath(local_datapaths, > - pb->datapath->tunnel_key); > - if (ld && ld->localnet_port) { > - sset_add(localnet_vifs, iface_id); > - } > - } > - } > - > - struct sbrec_port_binding *target = sbrec_port_binding_index_init_row( > - sbrec_port_binding_by_datapath); > - > - const struct local_datapath *ld; > - HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > - const struct sbrec_port_binding *pb; > - > - if (!ld->localnet_port) { > - continue; > - } > - > - /* Get l3gw ports. Consider port bindings with type "l3gateway" > - * that connect to gateway routers (if local), and consider port > - * bindings of type "patch" since they might connect to > - * distributed gateway ports with NAT addresses. */ > - > - sbrec_port_binding_index_set_datapath(target, ld->datapath); > - SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > - sbrec_port_binding_by_datapath) { > - if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) > - || !strcmp(pb->type, "patch")) { > - sset_add(local_l3gw_ports, pb->logical_port); > - } > - } > - } > - sbrec_port_binding_index_destroy_row(target); > -} > - > - > -/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > - * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > - * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid IPv4 > - * or IPv6 address, and stores them in the 'ipv4_addrs' and 'ipv6_addrs' > - * fields of 'laddrs'. The logical port name is stored in 'lport'. > - * > - * Returns true if at least 'MAC' is found in 'address', false otherwise. > - * > - * The caller must call destroy_lport_addresses() and free(*lport). */ > -static bool > -extract_addresses_with_port(const char *addresses, > - struct lport_addresses *laddrs, > - char **lport) > -{ > - int ofs; > - if (!extract_addresses(addresses, laddrs, &ofs)) { > - return false; > - } else if (!addresses[ofs]) { > - return true; > - } > - > - struct lexer lexer; > - lexer_init(&lexer, addresses + ofs); > - lexer_get(&lexer); > - > - if (lexer.error || lexer.token.type != LEX_T_ID > - || !lexer_match_id(&lexer, "is_chassis_resident")) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", addresses); > - lexer_destroy(&lexer); > - return true; > - } > - > - if (!lexer_match(&lexer, LEX_T_LPAREN)) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > - "'is_chassis_resident' in address '%s'", addresses); > - lexer_destroy(&lexer); > - return false; > - } > - > - if (lexer.token.type != LEX_T_STRING) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_INFO_RL(&rl, > - "Syntax error: expecting quoted string after " > - "'is_chassis_resident' in address '%s'", addresses); > - lexer_destroy(&lexer); > - return false; > - } > - > - *lport = xstrdup(lexer.token.s); > - > - lexer_get(&lexer); > - if (!lexer_match(&lexer, LEX_T_RPAREN)) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted string in " > - "'is_chassis_resident()' in address '%s'", > - addresses); > - lexer_destroy(&lexer); > - return false; > - } > - > - lexer_destroy(&lexer); > - return true; > -} > - > -static void > -consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name, > - const char *nat_address, > - const struct sbrec_port_binding *pb, > - struct sset *nat_address_keys, > - const struct sbrec_chassis *chassis, > - const struct sset *active_tunnels, > - struct shash *nat_addresses) > -{ > - struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > - char *lport = NULL; > - if (!extract_addresses_with_port(nat_address, laddrs, &lport) > - || (!lport && !strcmp(pb->type, "patch")) > - || (lport && !lport_is_chassis_resident( > - sbrec_port_binding_by_name, chassis, > - active_tunnels, lport))) { > - destroy_lport_addresses(laddrs); > - free(laddrs); > - free(lport); > - return; > - } > - free(lport); > - > - int i; > - for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > - char *name = xasprintf("%s-%s", pb->logical_port, > - laddrs->ipv4_addrs[i].addr_s); > - sset_add(nat_address_keys, name); > - free(name); > - } > - if (laddrs->n_ipv4_addrs == 0) { > - char *name = xasprintf("%s-noip", pb->logical_port); > - sset_add(nat_address_keys, name); > - free(name); > - } > - shash_add(nat_addresses, pb->logical_port, laddrs); > -} > - > -static void > -get_nat_addresses_and_keys(struct ovsdb_idl_index *sbrec_port_binding_by_name, > - struct sset *nat_address_keys, > - struct sset *local_l3gw_ports, > - const struct sbrec_chassis *chassis, > - const struct sset *active_tunnels, > - struct shash *nat_addresses) > -{ > - const char *gw_port; > - SSET_FOR_EACH(gw_port, local_l3gw_ports) { > - const struct sbrec_port_binding *pb; > - > - pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > - if (!pb) { > - continue; > - } > - > - if (pb->n_nat_addresses) { > - for (int i = 0; i < pb->n_nat_addresses; i++) { > - consider_nat_address(sbrec_port_binding_by_name, > - pb->nat_addresses[i], pb, > - nat_address_keys, chassis, > - active_tunnels, > - nat_addresses); > - } > - } else { > - /* Continue to support options:nat-addresses for version > - * upgrade. */ > - const char *nat_addresses_options = smap_get(&pb->options, > - "nat-addresses"); > - if (nat_addresses_options) { > - consider_nat_address(sbrec_port_binding_by_name, > - nat_addresses_options, pb, > - nat_address_keys, chassis, > - active_tunnels, > - nat_addresses); > - } > - } > - } > -} > - > static void > send_garp_rarp_wait(long long int send_garp_rarp_time) > { > @@ -6441,96 +6208,70 @@ send_garp_rarp_run(struct rconn *swconn, long long int *send_garp_rarp_time) > * thread context. */ > static void > send_garp_rarp_prepare(struct ovsdb_idl_txn *ovnsb_idl_txn, > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > - const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *chassis, > const struct hmap *local_datapaths, > - const struct sset *active_tunnels, > - const struct ovsrec_open_vswitch_table *ovs_table) > + const struct ovsrec_open_vswitch_table *ovs_table, > + struct ed_type_nat_addresses *nat_addresses) > OVS_REQUIRES(pinctrl_mutex) > { > - struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs); > - struct sset local_l3gw_ports = SSET_INITIALIZER(&local_l3gw_ports); > - struct sset nat_ip_keys = SSET_INITIALIZER(&nat_ip_keys); > - struct shash nat_addresses; > - unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > - bool garp_continuous = false; > - const struct ovsrec_open_vswitch *cfg = > - ovsrec_open_vswitch_table_first(ovs_table); > - if (cfg) { > - garp_max_timeout = smap_get_ullong( > - &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; > - garp_continuous = !!garp_max_timeout; > - if (!garp_max_timeout) { > - garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > - } > - } > - > - shash_init(&nat_addresses); > - > - get_localnet_vifs_l3gwports(sbrec_port_binding_by_datapath, > - sbrec_port_binding_by_name, > - br_int, chassis, local_datapaths, > - &localnet_vifs, &local_l3gw_ports); > - > - get_nat_addresses_and_keys(sbrec_port_binding_by_name, > - &nat_ip_keys, &local_l3gw_ports, > - chassis, active_tunnels, > - &nat_addresses); > - /* For deleted ports and deleted nat ips, remove from > - * send_garp_rarp_data. */ > - struct shash_node *iter; > - SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > - if (!sset_contains(&localnet_vifs, iter->name) && > - !sset_contains(&nat_ip_keys, iter->name)) { > - send_garp_rarp_delete(iter->name); > + if (nat_addresses) { > + unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > + bool garp_continuous = false; > + > + const struct ovsrec_open_vswitch *cfg = > + ovsrec_open_vswitch_table_first(ovs_table); > + if (cfg) { > + garp_max_timeout = smap_get_ullong( > + &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; > + garp_continuous = !!garp_max_timeout; > + if (!garp_max_timeout) { > + garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > + } > } > - } > - > - /* Update send_garp_rarp_data. */ > - const char *iface_id; > - SSET_FOR_EACH (iface_id, &localnet_vifs) { > - const struct sbrec_port_binding *pb = lport_lookup_by_name( > - sbrec_port_binding_by_name, iface_id); > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", false)) { > - send_garp_rarp_update(ovnsb_idl_txn, > - sbrec_mac_binding_by_lport_ip, > - local_datapaths, pb, &nat_addresses, > - garp_max_timeout, garp_continuous); > + /* For deleted ports and deleted nat ips, remove from > + * send_garp_rarp_data. */ > + struct shash_node *iter; > + SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > + if (!sset_contains(&nat_addresses->localnet_vifs, iter->name) && > + !sset_contains(&nat_addresses->nat_ip_keys, iter->name)) { > + send_garp_rarp_delete(iter->name); > + } > } > - } > - > - /* Update send_garp_rarp_data for nat-addresses. */ > - const char *gw_port; > - SSET_FOR_EACH (gw_port, &local_l3gw_ports) { > - const struct sbrec_port_binding *pb > - = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", false)) { > - send_garp_rarp_update(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip, > - local_datapaths, pb, &nat_addresses, > - garp_max_timeout, garp_continuous); > + /* Update send_garp_rarp_data. */ > + const char *iface_id; > + SSET_FOR_EACH (iface_id, &nat_addresses->localnet_vifs) { > + const struct sbrec_port_binding *pb = lport_lookup_by_name( > + sbrec_port_binding_by_name, iface_id); > + if (pb && !smap_get_bool( > + &pb->options, "disable_garp_rarp", false)) { > + send_garp_rarp_update(ovnsb_idl_txn, > + sbrec_mac_binding_by_lport_ip, > + local_datapaths, pb, > + &nat_addresses->nat_addresses, > + garp_max_timeout, garp_continuous); > + } > } > - } > > - /* pinctrl_handler thread will send the GARPs. */ > - > - sset_destroy(&localnet_vifs); > - sset_destroy(&local_l3gw_ports); > + /* Update send_garp_rarp_data for nat-addresses. */ > + const char *gw_port; > + SSET_FOR_EACH (gw_port, &nat_addresses->local_l3gw_ports) { > + const struct sbrec_port_binding *pb > + = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > + if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > + false)) { > + send_garp_rarp_update(ovnsb_idl_txn, > + sbrec_mac_binding_by_lport_ip, > + local_datapaths, pb, > + &nat_addresses->nat_addresses, > + garp_max_timeout, garp_continuous); > + } > + } > > - SHASH_FOR_EACH_SAFE (iter, &nat_addresses) { > - struct lport_addresses *laddrs = iter->data; > - destroy_lport_addresses(laddrs); > - shash_delete(&nat_addresses, iter); > - free(laddrs); > + /* pinctrl_handler thread will send the GARPs. */ > + garp_rarp_max_timeout = garp_max_timeout; > + garp_rarp_continuous = garp_continuous; > } > - shash_destroy(&nat_addresses); > - > - sset_destroy(&nat_ip_keys); > - > - garp_rarp_max_timeout = garp_max_timeout; > - garp_rarp_continuous = garp_continuous; > } > > static bool > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > index 846afe0a4..c6a7423b8 100644 > --- a/controller/pinctrl.h > +++ b/controller/pinctrl.h > @@ -20,6 +20,7 @@ > #include <stdint.h> > > #include "lib/sset.h" > +#include "openvswitch/shash.h" > #include "openvswitch/list.h" > #include "openvswitch/meta-flow.h" > > @@ -39,10 +40,16 @@ struct sbrec_bfd_table; > struct sbrec_port_binding; > struct sbrec_mac_binding_table; > > +struct ed_type_nat_addresses { > + struct shash nat_addresses; > + struct sset localnet_vifs; > + struct sset local_l3gw_ports; > + struct sset nat_ip_keys; > +}; > + > void pinctrl_init(void); > void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_key, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > @@ -53,12 +60,13 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sbrec_service_monitor_table *, > const struct sbrec_mac_binding_table *, > const struct sbrec_bfd_table *, > - const struct ovsrec_bridge *, const struct sbrec_chassis *, > + const struct sbrec_chassis *, > const struct hmap *local_datapaths, > const struct sset *active_tunnels, > const struct shash *local_active_ports_ipv6_pd, > const struct shash *local_active_ports_ras, > - const struct ovsrec_open_vswitch_table *ovs_table); > + const struct ovsrec_open_vswitch_table *ovs_table, > + struct ed_type_nat_addresses *nat_addresses); > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > void pinctrl_destroy(void); > > -- > 2.43.0 > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Nov 14, 2024 at 11:47 AM Max Lamprecht via dev < ovs-dev@openvswitch.org> wrote: > This patch moves the nat_addresses calculation to an own engine node. > > Previously the nat_address calculation was executed at every pinctrl_run > (send_garp_rarp_prepare). This can cause high cpu usage and delayed > actions on gateway-chassis(500 LRPs+). > > perf trace: > - 98.83% main > - 79.69% pinctrl_run > - 77.71% send_garp_rarp_prepare > - 55.49% get_nat_addresses_and_keys > - 36.76% consider_nat_address > - 18.13% lport_lookup_by_name > - 13.58% sset_destroy > > Co-authored-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > Signed-off-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > Signed-off-by: Max Lamprecht <max.lamprecht@stackit.cloud> > --- > Hello Max, thank you for your patience and sorry for the delay in reviews. I have a couple of comments down below. controller/ovn-controller.c | 341 +++++++++++++++++++++++++++++++- > controller/pinctrl.c | 381 ++++++------------------------------ > controller/pinctrl.h | 14 +- > 3 files changed, 410 insertions(+), 326 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 98b144699..49bc6d86c 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -3359,6 +3359,319 @@ en_dns_cache_cleanup(void *data OVS_UNUSED) > ovn_dns_cache_destroy(); > } > > +static void * > +en_nat_addresses_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct ed_type_nat_addresses *data = xzalloc(sizeof *data); > nit: No need to zero alloc if you init everything right away. > + shash_init(&data->nat_addresses); > + sset_init(&data->localnet_vifs); > + sset_init(&data->local_l3gw_ports); > + sset_init(&data->nat_ip_keys); > + return data; > +} > + > +/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > + * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > + * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid IPv4 > + * or IPv6 address, and stores them in the 'ipv4_addrs' and 'ipv6_addrs' > + * fields of 'laddrs'. The logical port name is stored in 'lport'. > + * > + * Returns true if at least 'MAC' is found in 'address', false otherwise. > + * > + * The caller must call destroy_lport_addresses() and free(*lport). */ > +static bool > +extract_addresses_with_port(const char *addresses, > + struct lport_addresses *laddrs, > + char **lport) > +{ > + int ofs; > + if (!extract_addresses(addresses, laddrs, &ofs)) { > + return false; > + } else if (!addresses[ofs]) { > + return true; > + } > + > + struct lexer lexer; > + lexer_init(&lexer, addresses + ofs); > + lexer_get(&lexer); > + > + if (lexer.error || lexer.token.type != LEX_T_ID > + || !lexer_match_id(&lexer, "is_chassis_resident")) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", addresses); > + lexer_destroy(&lexer); > + return true; > + } > + > + if (!lexer_match(&lexer, LEX_T_LPAREN)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > + "'is_chassis_resident' in address '%s'", > addresses); > + lexer_destroy(&lexer); > + return false; > + } > + > + if (lexer.token.type != LEX_T_STRING) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_INFO_RL(&rl, > + "Syntax error: expecting quoted string after " > + "'is_chassis_resident' in address '%s'", addresses); > + lexer_destroy(&lexer); > + return false; > + } > + > + *lport = xstrdup(lexer.token.s); > + > + lexer_get(&lexer); > + if (!lexer_match(&lexer, LEX_T_RPAREN)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted > string in " > + "'is_chassis_resident()' in address '%s'", > + addresses); > + lexer_destroy(&lexer); > + return false; > + } > + > + lexer_destroy(&lexer); > + return true; > +} > + > +static void > +consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const char *nat_address, > + const struct sbrec_port_binding *pb, > + struct sset *nat_address_keys, > + const struct sbrec_chassis *chassis, > + const struct sset *active_tunnels, > + struct shash *nat_addresses) > +{ > + struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > + char *lport = NULL; > + if (!extract_addresses_with_port(nat_address, laddrs, &lport) > + || (!lport && !strcmp(pb->type, "patch")) > + || (lport && !lport_is_chassis_resident( > + sbrec_port_binding_by_name, chassis, > + active_tunnels, lport))) { > + destroy_lport_addresses(laddrs); > + free(laddrs); > + free(lport); > + return; > + } > + free(lport); > + > + int i; > + for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > + char *name = xasprintf("%s-%s", pb->logical_port, > + laddrs->ipv4_addrs[i].addr_s); > + sset_add(nat_address_keys, name); > + free(name); > + } > + if (laddrs->n_ipv4_addrs == 0) { > + char *name = xasprintf("%s-noip", pb->logical_port); > + sset_add(nat_address_keys, name); > + free(name); > + } > + shash_add(nat_addresses, pb->logical_port, laddrs); > +} > + > +static void > +get_nat_addresses_and_keys(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > + struct sset *nat_address_keys, > + struct sset *local_l3gw_ports, > + const struct sbrec_chassis *chassis, > + const struct sset *active_tunnels, > + struct shash *nat_addresses) > +{ > + const char *gw_port; > + SSET_FOR_EACH (gw_port, local_l3gw_ports) { > + const struct sbrec_port_binding *pb; > + > + pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > + if (!pb) { > + continue; > + } > + > + if (pb->n_nat_addresses) { > + for (int i = 0; i < pb->n_nat_addresses; i++) { > + consider_nat_address(sbrec_port_binding_by_name, > + pb->nat_addresses[i], pb, > + nat_address_keys, chassis, > + active_tunnels, > + nat_addresses); > + } > + } else { > + /* Continue to support options:nat-addresses for version > + * upgrade. */ > + const char *nat_addresses_options = smap_get(&pb->options, > + "nat-addresses"); > + if (nat_addresses_options) { > + consider_nat_address(sbrec_port_binding_by_name, > + nat_addresses_options, pb, > + nat_address_keys, chassis, > + active_tunnels, > + nat_addresses); > + } > + } > + } > +} > + > +/* Get localnet vifs, local l3gw ports and ofport for localnet patch > ports. */ > +static void > +get_localnet_vifs_l3gwports( > + struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const struct ovsrec_bridge *br_int, > + const struct sbrec_chassis *chassis, > + const struct hmap *local_datapaths, > + struct sset *localnet_vifs, > + struct sset *local_l3gw_ports) > +{ > + for (int i = 0; i < br_int->n_ports; i++) { > + const struct ovsrec_port *port_rec = br_int->ports[i]; > + if (!strcmp(port_rec->name, br_int->name)) { > + continue; > + } > + const char *tunnel_id = smap_get(&port_rec->external_ids, > + "ovn-chassis-id"); > + if (tunnel_id && > + encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, > NULL)) { > + continue; > + } > + const char *localnet = smap_get(&port_rec->external_ids, > + "ovn-localnet-port"); > + if (localnet) { > + continue; > + } > + for (int j = 0; j < port_rec->n_interfaces; j++) { > + const struct ovsrec_interface *iface_rec = > port_rec->interfaces[j]; > + if (!iface_rec->n_ofport) { > + continue; > + } > + /* Get localnet vif. */ > + const char *iface_id = smap_get(&iface_rec->external_ids, > + "iface-id"); > + if (!iface_id) { > + continue; > + } > + const struct sbrec_port_binding *pb > + = lport_lookup_by_name(sbrec_port_binding_by_name, > iface_id); > + if (!pb || pb->chassis != chassis) { > + continue; > + } > + if (!iface_rec->link_state || > + strcmp(iface_rec->link_state, "up")) { > + continue; > + } > + struct local_datapath *ld > + = get_local_datapath(local_datapaths, > + pb->datapath->tunnel_key); > + if (ld && ld->localnet_port) { > + sset_add(localnet_vifs, iface_id); > + } > + } > + } > + > + struct sbrec_port_binding *target = sbrec_port_binding_index_init_row( > + sbrec_port_binding_by_datapath); > + > + const struct local_datapath *ld; > + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > + const struct sbrec_port_binding *pb; > + > + if (!ld->localnet_port) { > + continue; > + } > + > + /* Get l3gw ports. Consider port bindings with type "l3gateway" > + * that connect to gateway routers (if local), and consider port > + * bindings of type "patch" since they might connect to > + * distributed gateway ports with NAT addresses. */ > + > + sbrec_port_binding_index_set_datapath(target, ld->datapath); > + SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > + > sbrec_port_binding_by_datapath) { > + if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) > + || !strcmp(pb->type, "patch")) { > + sset_add(local_l3gw_ports, pb->logical_port); > + } > + } > + } > + sbrec_port_binding_index_destroy_row(target); > +} > + > +static void > +en_nat_addresses_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + struct ed_type_nat_addresses *nat_addresses = data; > + const struct ovsrec_open_vswitch_table *ovs_table = > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > + struct ovsdb_idl_index *sbrec_chassis_by_name = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_chassis", node), > + "name"); > + const struct sbrec_chassis *chassis > + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > + > + struct ovsdb_idl_index *sbrec_pb_by_datapath = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "datapath"); > + struct ovsdb_idl_index *sbrec_pb_by_name = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "name"); > + const struct ovsrec_bridge_table *bridge_table = > + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > + const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > ovs_table); > + > + struct ed_type_runtime_data *rt_data = > + engine_get_input_data("runtime_data", node); > + const struct hmap *local_datapaths = &rt_data->local_datapaths; > + const struct sset *active_tunnels = &rt_data->active_tunnels; > + > + struct shash_node *nat_node; > + SHASH_FOR_EACH (nat_node, &nat_addresses->nat_addresses) { > + struct lport_addresses *laddrs = nat_node->data; > + destroy_lport_addresses(laddrs); > + free(laddrs); > + } > + shash_clear(&nat_addresses->nat_addresses); > + sset_clear(&nat_addresses->localnet_vifs); > + sset_clear(&nat_addresses->local_l3gw_ports); > + sset_clear(&nat_addresses->nat_ip_keys); > + > + get_localnet_vifs_l3gwports(sbrec_pb_by_datapath, > + sbrec_pb_by_name, > + br_int, chassis, local_datapaths, > + &nat_addresses->localnet_vifs, > + &nat_addresses->local_l3gw_ports); > + > + get_nat_addresses_and_keys(sbrec_pb_by_name, > + &nat_addresses->nat_ip_keys, > + &nat_addresses->local_l3gw_ports, > + chassis, active_tunnels, > + &nat_addresses->nat_addresses); > + > + engine_set_node_state(node, EN_UPDATED); > +} > + > +static bool > +nat_addresses_change_handler(struct engine_node *node, void *data) > +{ > + en_nat_addresses_run(node, data); > + return true; > +} > There is no need for separate handler, engine will always run the "<NODE>_run()" function if handler is NULL. + > +static void > +en_nat_addresses_cleanup(void *data OVS_UNUSED){ > + struct ed_type_nat_addresses *nat_addresses = data; > + shash_destroy(&nat_addresses->nat_addresses); > We are leaking all the "struct lport_addresses" stored within this shash. > + sset_destroy(&nat_addresses->localnet_vifs); > + sset_destroy(&nat_addresses->local_l3gw_ports); > + sset_destroy(&nat_addresses->nat_ip_keys); > +} > > /* Engine node which is used to handle the Non VIF data like > * - OVS patch ports > @@ -4779,6 +5092,14 @@ controller_output_bfd_chassis_handler(struct > engine_node *node, > return true; > } > > +static bool > +controller_output_nat_addresses_handler(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > + > /* Handles sbrec_chassis changes. > * If a new chassis is added or removed return false, so that > * flows are recomputed. For any updates, there is no need for > @@ -5093,6 +5414,7 @@ main(int argc, char *argv[]) > ENGINE_NODE(mac_cache, "mac_cache"); > ENGINE_NODE(bfd_chassis, "bfd_chassis"); > ENGINE_NODE(dns_cache, "dns_cache"); > + ENGINE_NODE(nat_addresses, "nat_addresses"); > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > SB_NODES > @@ -5137,6 +5459,14 @@ main(int argc, char *argv[]) > engine_add_input(&en_bfd_chassis, &en_sb_chassis, NULL); > engine_add_input(&en_bfd_chassis, &en_sb_ha_chassis_group, NULL); > > + engine_add_input(&en_nat_addresses, &en_ovs_open_vswitch, NULL); > + engine_add_input(&en_nat_addresses, &en_ovs_bridge, NULL); > + engine_add_input(&en_nat_addresses, &en_sb_chassis, NULL); > + engine_add_input(&en_nat_addresses, &en_sb_port_binding, > + nat_addresses_change_handler); > + engine_add_input(&en_nat_addresses, &en_runtime_data, > + nat_addresses_change_handler); > + > /* Note: The order of inputs is important, all OVS interface changes > must > * be handled before any ct_zone changes. > */ > @@ -5300,6 +5630,8 @@ main(int argc, char *argv[]) > controller_output_mac_cache_handler); > engine_add_input(&en_controller_output, &en_bfd_chassis, > controller_output_bfd_chassis_handler); > + engine_add_input(&en_controller_output, &en_nat_addresses, > + controller_output_nat_addresses_handler); > > struct engine_arg engine_arg = { > .sb_idl = ovnsb_idl_loop.idl, > @@ -5346,6 +5678,8 @@ main(int argc, char *argv[]) > engine_get_internal_data(&en_ct_zones); > struct ed_type_bfd_chassis *bfd_chassis_data = > engine_get_internal_data(&en_bfd_chassis); > + struct ed_type_nat_addresses *nat_addresses_data = > + engine_get_internal_data(&en_nat_addresses); > struct ed_type_runtime_data *runtime_data = > engine_get_internal_data(&en_runtime_data); > struct ed_type_template_vars *template_vars_data = > @@ -5682,6 +6016,7 @@ main(int argc, char *argv[]) > } > > runtime_data = engine_get_data(&en_runtime_data); > + nat_addresses_data = > engine_get_data(&en_nat_addresses); > if (runtime_data) { > stopwatch_start(PATCH_RUN_STOPWATCH_NAME, > time_msec()); > patch_run(ovs_idl_txn, > @@ -5729,7 +6064,6 @@ main(int argc, char *argv[]) > pinctrl_update(ovnsb_idl_loop.idl); > pinctrl_run(ovnsb_idl_txn, > sbrec_datapath_binding_by_key, > - sbrec_port_binding_by_datapath, > sbrec_port_binding_by_key, > sbrec_port_binding_by_name, > sbrec_mac_binding_by_lport_ip, > @@ -5743,13 +6077,14 @@ main(int argc, char *argv[]) > sbrec_mac_binding_table_get( > ovnsb_idl_loop.idl), > > sbrec_bfd_table_get(ovnsb_idl_loop.idl), > - br_int, chassis, > + chassis, > &runtime_data->local_datapaths, > &runtime_data->active_tunnels, > > &runtime_data->local_active_ports_ipv6_pd, > &runtime_data->local_active_ports_ras, > ovsrec_open_vswitch_table_get( > - ovs_idl_loop.idl)); > + ovs_idl_loop.idl), > + nat_addresses_data); > stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME, > time_msec()); > mirror_run(ovs_idl_txn, > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 3fb7e2fd7..e8c4ff77b 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -227,14 +227,11 @@ static void destroy_send_garps_rarps(void); > static void send_garp_rarp_wait(long long int send_garp_rarp_time); > static void send_garp_rarp_prepare( > struct ovsdb_idl_txn *ovnsb_idl_txn, > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > - const struct ovsrec_bridge *, > - const struct sbrec_chassis *, > const struct hmap *local_datapaths, > - const struct sset *active_tunnels, > - const struct ovsrec_open_vswitch_table *ovs_table) > + const struct ovsrec_open_vswitch_table *ovs_table, > + struct ed_type_nat_addresses *nat_addresses) > OVS_REQUIRES(pinctrl_mutex); > static void send_garp_rarp_run(struct rconn *swconn, > long long int *send_garp_rarp_time) > @@ -4008,7 +4005,6 @@ pinctrl_update(const struct ovsdb_idl *idl) > void > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_key, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > @@ -4019,13 +4015,13 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sbrec_service_monitor_table *svc_mon_table, > const struct sbrec_mac_binding_table *mac_binding_table, > const struct sbrec_bfd_table *bfd_table, > - const struct ovsrec_bridge *br_int, > const struct sbrec_chassis *chassis, > const struct hmap *local_datapaths, > const struct sset *active_tunnels, > const struct shash *local_active_ports_ipv6_pd, > const struct shash *local_active_ports_ras, > - const struct ovsrec_open_vswitch_table *ovs_table) > + const struct ovsrec_open_vswitch_table *ovs_table, > + struct ed_type_nat_addresses *nat_addresses) > { > ovs_mutex_lock(&pinctrl_mutex); > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > @@ -4033,10 +4029,11 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > sbrec_mac_binding_by_lport_ip); > run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > sbrec_port_binding_by_key, chassis); > - send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath, > + send_garp_rarp_prepare(ovnsb_idl_txn, > sbrec_port_binding_by_name, > - sbrec_mac_binding_by_lport_ip, br_int, chassis, > - local_datapaths, active_tunnels, ovs_table); > + sbrec_mac_binding_by_lport_ip, > + local_datapaths, > + ovs_table, nat_addresses); > prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name); > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > local_active_ports_ipv6_pd, chassis, > @@ -6175,236 +6172,6 @@ ip_mcast_querier_wait(long long int query_time) > } > } > > -/* Get localnet vifs, local l3gw ports and ofport for localnet patch > ports. */ > -static void > -get_localnet_vifs_l3gwports( > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > - struct ovsdb_idl_index *sbrec_port_binding_by_name, > - const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *chassis, > - const struct hmap *local_datapaths, > - struct sset *localnet_vifs, > - struct sset *local_l3gw_ports) > -{ > - for (int i = 0; i < br_int->n_ports; i++) { > - const struct ovsrec_port *port_rec = br_int->ports[i]; > - if (!strcmp(port_rec->name, br_int->name)) { > - continue; > - } > - const char *tunnel_id = smap_get(&port_rec->external_ids, > - "ovn-chassis-id"); > - if (tunnel_id && > - encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, > NULL)) { > - continue; > - } > - const char *localnet = smap_get(&port_rec->external_ids, > - "ovn-localnet-port"); > - if (localnet) { > - continue; > - } > - for (int j = 0; j < port_rec->n_interfaces; j++) { > - const struct ovsrec_interface *iface_rec = > port_rec->interfaces[j]; > - if (!iface_rec->n_ofport) { > - continue; > - } > - /* Get localnet vif. */ > - const char *iface_id = smap_get(&iface_rec->external_ids, > - "iface-id"); > - if (!iface_id) { > - continue; > - } > - const struct sbrec_port_binding *pb > - = lport_lookup_by_name(sbrec_port_binding_by_name, > iface_id); > - if (!pb || pb->chassis != chassis) { > - continue; > - } > - if (!iface_rec->link_state || > - strcmp(iface_rec->link_state, "up")) { > - continue; > - } > - struct local_datapath *ld > - = get_local_datapath(local_datapaths, > - pb->datapath->tunnel_key); > - if (ld && ld->localnet_port) { > - sset_add(localnet_vifs, iface_id); > - } > - } > - } > - > - struct sbrec_port_binding *target = sbrec_port_binding_index_init_row( > - sbrec_port_binding_by_datapath); > - > - const struct local_datapath *ld; > - HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > - const struct sbrec_port_binding *pb; > - > - if (!ld->localnet_port) { > - continue; > - } > - > - /* Get l3gw ports. Consider port bindings with type "l3gateway" > - * that connect to gateway routers (if local), and consider port > - * bindings of type "patch" since they might connect to > - * distributed gateway ports with NAT addresses. */ > - > - sbrec_port_binding_index_set_datapath(target, ld->datapath); > - SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > - > sbrec_port_binding_by_datapath) { > - if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) > - || !strcmp(pb->type, "patch")) { > - sset_add(local_l3gw_ports, pb->logical_port); > - } > - } > - } > - sbrec_port_binding_index_destroy_row(target); > -} > - > - > -/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > - * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > - * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid IPv4 > - * or IPv6 address, and stores them in the 'ipv4_addrs' and 'ipv6_addrs' > - * fields of 'laddrs'. The logical port name is stored in 'lport'. > - * > - * Returns true if at least 'MAC' is found in 'address', false otherwise. > - * > - * The caller must call destroy_lport_addresses() and free(*lport). */ > -static bool > -extract_addresses_with_port(const char *addresses, > - struct lport_addresses *laddrs, > - char **lport) > -{ > - int ofs; > - if (!extract_addresses(addresses, laddrs, &ofs)) { > - return false; > - } else if (!addresses[ofs]) { > - return true; > - } > - > - struct lexer lexer; > - lexer_init(&lexer, addresses + ofs); > - lexer_get(&lexer); > - > - if (lexer.error || lexer.token.type != LEX_T_ID > - || !lexer_match_id(&lexer, "is_chassis_resident")) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", addresses); > - lexer_destroy(&lexer); > - return true; > - } > - > - if (!lexer_match(&lexer, LEX_T_LPAREN)) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > - "'is_chassis_resident' in address '%s'", > addresses); > - lexer_destroy(&lexer); > - return false; > - } > - > - if (lexer.token.type != LEX_T_STRING) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_INFO_RL(&rl, > - "Syntax error: expecting quoted string after " > - "'is_chassis_resident' in address '%s'", addresses); > - lexer_destroy(&lexer); > - return false; > - } > - > - *lport = xstrdup(lexer.token.s); > - > - lexer_get(&lexer); > - if (!lexer_match(&lexer, LEX_T_RPAREN)) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted > string in " > - "'is_chassis_resident()' in address '%s'", > - addresses); > - lexer_destroy(&lexer); > - return false; > - } > - > - lexer_destroy(&lexer); > - return true; > -} > - > -static void > -consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name, > - const char *nat_address, > - const struct sbrec_port_binding *pb, > - struct sset *nat_address_keys, > - const struct sbrec_chassis *chassis, > - const struct sset *active_tunnels, > - struct shash *nat_addresses) > -{ > - struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > - char *lport = NULL; > - if (!extract_addresses_with_port(nat_address, laddrs, &lport) > - || (!lport && !strcmp(pb->type, "patch")) > - || (lport && !lport_is_chassis_resident( > - sbrec_port_binding_by_name, chassis, > - active_tunnels, lport))) { > - destroy_lport_addresses(laddrs); > - free(laddrs); > - free(lport); > - return; > - } > - free(lport); > - > - int i; > - for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > - char *name = xasprintf("%s-%s", pb->logical_port, > - laddrs->ipv4_addrs[i].addr_s); > - sset_add(nat_address_keys, name); > - free(name); > - } > - if (laddrs->n_ipv4_addrs == 0) { > - char *name = xasprintf("%s-noip", pb->logical_port); > - sset_add(nat_address_keys, name); > - free(name); > - } > - shash_add(nat_addresses, pb->logical_port, laddrs); > -} > - > -static void > -get_nat_addresses_and_keys(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > - struct sset *nat_address_keys, > - struct sset *local_l3gw_ports, > - const struct sbrec_chassis *chassis, > - const struct sset *active_tunnels, > - struct shash *nat_addresses) > -{ > - const char *gw_port; > - SSET_FOR_EACH(gw_port, local_l3gw_ports) { > - const struct sbrec_port_binding *pb; > - > - pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > - if (!pb) { > - continue; > - } > - > - if (pb->n_nat_addresses) { > - for (int i = 0; i < pb->n_nat_addresses; i++) { > - consider_nat_address(sbrec_port_binding_by_name, > - pb->nat_addresses[i], pb, > - nat_address_keys, chassis, > - active_tunnels, > - nat_addresses); > - } > - } else { > - /* Continue to support options:nat-addresses for version > - * upgrade. */ > - const char *nat_addresses_options = smap_get(&pb->options, > - "nat-addresses"); > - if (nat_addresses_options) { > - consider_nat_address(sbrec_port_binding_by_name, > - nat_addresses_options, pb, > - nat_address_keys, chassis, > - active_tunnels, > - nat_addresses); > - } > - } > - } > -} > - > static void > send_garp_rarp_wait(long long int send_garp_rarp_time) > { > @@ -6441,96 +6208,70 @@ send_garp_rarp_run(struct rconn *swconn, long long > int *send_garp_rarp_time) > * thread context. */ > static void > send_garp_rarp_prepare(struct ovsdb_idl_txn *ovnsb_idl_txn, > - struct ovsdb_idl_index > *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index > *sbrec_mac_binding_by_lport_ip, > - const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *chassis, > const struct hmap *local_datapaths, > - const struct sset *active_tunnels, > - const struct ovsrec_open_vswitch_table *ovs_table) > + const struct ovsrec_open_vswitch_table *ovs_table, > + struct ed_type_nat_addresses *nat_addresses) > OVS_REQUIRES(pinctrl_mutex) > { > - struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs); > - struct sset local_l3gw_ports = SSET_INITIALIZER(&local_l3gw_ports); > - struct sset nat_ip_keys = SSET_INITIALIZER(&nat_ip_keys); > - struct shash nat_addresses; > - unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > - bool garp_continuous = false; > - const struct ovsrec_open_vswitch *cfg = > - ovsrec_open_vswitch_table_first(ovs_table); > - if (cfg) { > - garp_max_timeout = smap_get_ullong( > - &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; > - garp_continuous = !!garp_max_timeout; > - if (!garp_max_timeout) { > - garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > - } > - } > - > - shash_init(&nat_addresses); > - > - get_localnet_vifs_l3gwports(sbrec_port_binding_by_datapath, > - sbrec_port_binding_by_name, > - br_int, chassis, local_datapaths, > - &localnet_vifs, &local_l3gw_ports); > - > - get_nat_addresses_and_keys(sbrec_port_binding_by_name, > - &nat_ip_keys, &local_l3gw_ports, > - chassis, active_tunnels, > - &nat_addresses); > - /* For deleted ports and deleted nat ips, remove from > - * send_garp_rarp_data. */ > - struct shash_node *iter; > - SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > - if (!sset_contains(&localnet_vifs, iter->name) && > - !sset_contains(&nat_ip_keys, iter->name)) { > - send_garp_rarp_delete(iter->name); > + if (nat_addresses) { > + unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > + bool garp_continuous = false; > + > + const struct ovsrec_open_vswitch *cfg = > + ovsrec_open_vswitch_table_first(ovs_table); > + if (cfg) { > + garp_max_timeout = smap_get_ullong( > + &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; > + garp_continuous = !!garp_max_timeout; > + if (!garp_max_timeout) { > + garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > + } > } > - } > - > - /* Update send_garp_rarp_data. */ > - const char *iface_id; > - SSET_FOR_EACH (iface_id, &localnet_vifs) { > - const struct sbrec_port_binding *pb = lport_lookup_by_name( > - sbrec_port_binding_by_name, iface_id); > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > false)) { > - send_garp_rarp_update(ovnsb_idl_txn, > - sbrec_mac_binding_by_lport_ip, > - local_datapaths, pb, &nat_addresses, > - garp_max_timeout, garp_continuous); > + /* For deleted ports and deleted nat ips, remove from > + * send_garp_rarp_data. */ > + struct shash_node *iter; > + SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > + if (!sset_contains(&nat_addresses->localnet_vifs, iter->name) > && > + !sset_contains(&nat_addresses->nat_ip_keys, iter->name)) { > + send_garp_rarp_delete(iter->name); > + } > } > - } > - > - /* Update send_garp_rarp_data for nat-addresses. */ > - const char *gw_port; > - SSET_FOR_EACH (gw_port, &local_l3gw_ports) { > - const struct sbrec_port_binding *pb > - = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > false)) { > - send_garp_rarp_update(ovnsb_idl_txn, > sbrec_mac_binding_by_lport_ip, > - local_datapaths, pb, &nat_addresses, > - garp_max_timeout, garp_continuous); > + /* Update send_garp_rarp_data. */ > + const char *iface_id; > + SSET_FOR_EACH (iface_id, &nat_addresses->localnet_vifs) { > + const struct sbrec_port_binding *pb = lport_lookup_by_name( > + sbrec_port_binding_by_name, iface_id); > + if (pb && !smap_get_bool( > + &pb->options, "disable_garp_rarp", false)) { > + send_garp_rarp_update(ovnsb_idl_txn, > + sbrec_mac_binding_by_lport_ip, > + local_datapaths, pb, > + &nat_addresses->nat_addresses, > + garp_max_timeout, garp_continuous); > + } > } > - } > > - /* pinctrl_handler thread will send the GARPs. */ > - > - sset_destroy(&localnet_vifs); > - sset_destroy(&local_l3gw_ports); > + /* Update send_garp_rarp_data for nat-addresses. */ > + const char *gw_port; > + SSET_FOR_EACH (gw_port, &nat_addresses->local_l3gw_ports) { > + const struct sbrec_port_binding *pb > + = lport_lookup_by_name(sbrec_port_binding_by_name, > gw_port); > + if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > + false)) { > + send_garp_rarp_update(ovnsb_idl_txn, > + sbrec_mac_binding_by_lport_ip, > + local_datapaths, pb, > + &nat_addresses->nat_addresses, > + garp_max_timeout, garp_continuous); > + } > + } > > - SHASH_FOR_EACH_SAFE (iter, &nat_addresses) { > - struct lport_addresses *laddrs = iter->data; > - destroy_lport_addresses(laddrs); > - shash_delete(&nat_addresses, iter); > - free(laddrs); > + /* pinctrl_handler thread will send the GARPs. */ > + garp_rarp_max_timeout = garp_max_timeout; > + garp_rarp_continuous = garp_continuous; > } > - shash_destroy(&nat_addresses); > - > - sset_destroy(&nat_ip_keys); > - > - garp_rarp_max_timeout = garp_max_timeout; > - garp_rarp_continuous = garp_continuous; > } > > static bool > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > index 846afe0a4..c6a7423b8 100644 > --- a/controller/pinctrl.h > +++ b/controller/pinctrl.h > @@ -20,6 +20,7 @@ > #include <stdint.h> > > #include "lib/sset.h" > +#include "openvswitch/shash.h" > #include "openvswitch/list.h" > #include "openvswitch/meta-flow.h" > > @@ -39,10 +40,16 @@ struct sbrec_bfd_table; > struct sbrec_port_binding; > struct sbrec_mac_binding_table; > > +struct ed_type_nat_addresses { > + struct shash nat_addresses; > + struct sset localnet_vifs; > + struct sset local_l3gw_ports; > + struct sset nat_ip_keys; > +}; > This shouldn't be part of pinctrl.h as it doesn't have anything to do with pinctrl. In fact all functions that are handling the NAT addresses should be in a separate module. + > void pinctrl_init(void); > void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > struct ovsdb_idl_index *sbrec_port_binding_by_key, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > @@ -53,12 +60,13 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sbrec_service_monitor_table *, > const struct sbrec_mac_binding_table *, > const struct sbrec_bfd_table *, > - const struct ovsrec_bridge *, const struct sbrec_chassis > *, > + const struct sbrec_chassis *, > const struct hmap *local_datapaths, > const struct sset *active_tunnels, > const struct shash *local_active_ports_ipv6_pd, > const struct shash *local_active_ports_ras, > - const struct ovsrec_open_vswitch_table *ovs_table); > + const struct ovsrec_open_vswitch_table *ovs_table, > + struct ed_type_nat_addresses *nat_addresses); > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > void pinctrl_destroy(void); > > -- > 2.43.0 > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
On Tue, Dec 10, 2024 at 01:04:32PM +0100, Ales Musil wrote: > On Thu, Nov 14, 2024 at 11:47 AM Max Lamprecht via dev < > ovs-dev@openvswitch.org> wrote: > > > This patch moves the nat_addresses calculation to an own engine node. > > > > Previously the nat_address calculation was executed at every pinctrl_run > > (send_garp_rarp_prepare). This can cause high cpu usage and delayed > > actions on gateway-chassis(500 LRPs+). > > > > perf trace: > > - 98.83% main > > - 79.69% pinctrl_run > > - 77.71% send_garp_rarp_prepare > > - 55.49% get_nat_addresses_and_keys > > - 36.76% consider_nat_address > > - 18.13% lport_lookup_by_name > > - 13.58% sset_destroy > > > > Co-authored-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > > Signed-off-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > > Signed-off-by: Max Lamprecht <max.lamprecht@stackit.cloud> > > --- > > > > Hello Max, > > thank you for your patience and sorry for the delay in reviews. I have a > couple of comments down below. > > controller/ovn-controller.c | 341 +++++++++++++++++++++++++++++++- > > controller/pinctrl.c | 381 ++++++------------------------------ > > controller/pinctrl.h | 14 +- > > 3 files changed, 410 insertions(+), 326 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 98b144699..49bc6d86c 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -3359,6 +3359,319 @@ en_dns_cache_cleanup(void *data OVS_UNUSED) > > ovn_dns_cache_destroy(); > > } > > > > +static void * > > +en_nat_addresses_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + struct ed_type_nat_addresses *data = xzalloc(sizeof *data); > > > > nit: No need to zero alloc if you init everything right away. > > > > + shash_init(&data->nat_addresses); > > + sset_init(&data->localnet_vifs); > > + sset_init(&data->local_l3gw_ports); > > + sset_init(&data->nat_ip_keys); > > + return data; > > +} > > + > > +/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > > + * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > > + * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid IPv4 > > + * or IPv6 address, and stores them in the 'ipv4_addrs' and 'ipv6_addrs' > > + * fields of 'laddrs'. The logical port name is stored in 'lport'. > > + * > > + * Returns true if at least 'MAC' is found in 'address', false otherwise. > > + * > > + * The caller must call destroy_lport_addresses() and free(*lport). */ > > +static bool > > +extract_addresses_with_port(const char *addresses, > > + struct lport_addresses *laddrs, > > + char **lport) > > +{ > > + int ofs; > > + if (!extract_addresses(addresses, laddrs, &ofs)) { > > + return false; > > + } else if (!addresses[ofs]) { > > + return true; > > + } > > + > > + struct lexer lexer; > > + lexer_init(&lexer, addresses + ofs); > > + lexer_get(&lexer); > > + > > + if (lexer.error || lexer.token.type != LEX_T_ID > > + || !lexer_match_id(&lexer, "is_chassis_resident")) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > + VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", addresses); > > + lexer_destroy(&lexer); > > + return true; > > + } > > + > > + if (!lexer_match(&lexer, LEX_T_LPAREN)) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > + VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > > + "'is_chassis_resident' in address '%s'", > > addresses); > > + lexer_destroy(&lexer); > > + return false; > > + } > > + > > + if (lexer.token.type != LEX_T_STRING) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > + VLOG_INFO_RL(&rl, > > + "Syntax error: expecting quoted string after " > > + "'is_chassis_resident' in address '%s'", addresses); > > + lexer_destroy(&lexer); > > + return false; > > + } > > + > > + *lport = xstrdup(lexer.token.s); > > + > > + lexer_get(&lexer); > > + if (!lexer_match(&lexer, LEX_T_RPAREN)) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > + VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted > > string in " > > + "'is_chassis_resident()' in address '%s'", > > + addresses); > > + lexer_destroy(&lexer); > > + return false; > > + } > > + > > + lexer_destroy(&lexer); > > + return true; > > +} > > + > > +static void > > +consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > + const char *nat_address, > > + const struct sbrec_port_binding *pb, > > + struct sset *nat_address_keys, > > + const struct sbrec_chassis *chassis, > > + const struct sset *active_tunnels, > > + struct shash *nat_addresses) > > +{ > > + struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > > + char *lport = NULL; > > + if (!extract_addresses_with_port(nat_address, laddrs, &lport) > > + || (!lport && !strcmp(pb->type, "patch")) > > + || (lport && !lport_is_chassis_resident( > > + sbrec_port_binding_by_name, chassis, > > + active_tunnels, lport))) { > > + destroy_lport_addresses(laddrs); > > + free(laddrs); > > + free(lport); > > + return; > > + } > > + free(lport); > > + > > + int i; > > + for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > > + char *name = xasprintf("%s-%s", pb->logical_port, > > + laddrs->ipv4_addrs[i].addr_s); > > + sset_add(nat_address_keys, name); > > + free(name); > > + } > > + if (laddrs->n_ipv4_addrs == 0) { > > + char *name = xasprintf("%s-noip", pb->logical_port); > > + sset_add(nat_address_keys, name); > > + free(name); > > + } > > + shash_add(nat_addresses, pb->logical_port, laddrs); > > +} > > + > > +static void > > +get_nat_addresses_and_keys(struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > + struct sset *nat_address_keys, > > + struct sset *local_l3gw_ports, > > + const struct sbrec_chassis *chassis, > > + const struct sset *active_tunnels, > > + struct shash *nat_addresses) > > +{ > > + const char *gw_port; > > + SSET_FOR_EACH (gw_port, local_l3gw_ports) { > > + const struct sbrec_port_binding *pb; > > + > > + pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > > + if (!pb) { > > + continue; > > + } > > + > > + if (pb->n_nat_addresses) { > > + for (int i = 0; i < pb->n_nat_addresses; i++) { > > + consider_nat_address(sbrec_port_binding_by_name, > > + pb->nat_addresses[i], pb, > > + nat_address_keys, chassis, > > + active_tunnels, > > + nat_addresses); > > + } > > + } else { > > + /* Continue to support options:nat-addresses for version > > + * upgrade. */ > > + const char *nat_addresses_options = smap_get(&pb->options, > > + "nat-addresses"); > > + if (nat_addresses_options) { > > + consider_nat_address(sbrec_port_binding_by_name, > > + nat_addresses_options, pb, > > + nat_address_keys, chassis, > > + active_tunnels, > > + nat_addresses); > > + } > > + } > > + } > > +} > > + > > +/* Get localnet vifs, local l3gw ports and ofport for localnet patch > > ports. */ > > +static void > > +get_localnet_vifs_l3gwports( > > + struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > > + const struct ovsrec_bridge *br_int, > > + const struct sbrec_chassis *chassis, > > + const struct hmap *local_datapaths, > > + struct sset *localnet_vifs, > > + struct sset *local_l3gw_ports) > > +{ > > + for (int i = 0; i < br_int->n_ports; i++) { > > + const struct ovsrec_port *port_rec = br_int->ports[i]; > > + if (!strcmp(port_rec->name, br_int->name)) { > > + continue; > > + } > > + const char *tunnel_id = smap_get(&port_rec->external_ids, > > + "ovn-chassis-id"); > > + if (tunnel_id && > > + encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, > > NULL)) { > > + continue; > > + } > > + const char *localnet = smap_get(&port_rec->external_ids, > > + "ovn-localnet-port"); > > + if (localnet) { > > + continue; > > + } > > + for (int j = 0; j < port_rec->n_interfaces; j++) { > > + const struct ovsrec_interface *iface_rec = > > port_rec->interfaces[j]; > > + if (!iface_rec->n_ofport) { > > + continue; > > + } > > + /* Get localnet vif. */ > > + const char *iface_id = smap_get(&iface_rec->external_ids, > > + "iface-id"); > > + if (!iface_id) { > > + continue; > > + } > > + const struct sbrec_port_binding *pb > > + = lport_lookup_by_name(sbrec_port_binding_by_name, > > iface_id); > > + if (!pb || pb->chassis != chassis) { > > + continue; > > + } > > + if (!iface_rec->link_state || > > + strcmp(iface_rec->link_state, "up")) { > > + continue; > > + } > > + struct local_datapath *ld > > + = get_local_datapath(local_datapaths, > > + pb->datapath->tunnel_key); > > + if (ld && ld->localnet_port) { > > + sset_add(localnet_vifs, iface_id); > > + } > > + } > > + } > > + > > + struct sbrec_port_binding *target = sbrec_port_binding_index_init_row( > > + sbrec_port_binding_by_datapath); > > + > > + const struct local_datapath *ld; > > + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > > + const struct sbrec_port_binding *pb; > > + > > + if (!ld->localnet_port) { > > + continue; > > + } > > + > > + /* Get l3gw ports. Consider port bindings with type "l3gateway" > > + * that connect to gateway routers (if local), and consider port > > + * bindings of type "patch" since they might connect to > > + * distributed gateway ports with NAT addresses. */ > > + > > + sbrec_port_binding_index_set_datapath(target, ld->datapath); > > + SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > > + > > sbrec_port_binding_by_datapath) { > > + if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) > > + || !strcmp(pb->type, "patch")) { > > + sset_add(local_l3gw_ports, pb->logical_port); > > + } > > + } > > + } > > + sbrec_port_binding_index_destroy_row(target); > > +} > > + > > +static void > > +en_nat_addresses_run(struct engine_node *node, void *data OVS_UNUSED) > > +{ > > + struct ed_type_nat_addresses *nat_addresses = data; > > + const struct ovsrec_open_vswitch_table *ovs_table = > > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > > + struct ovsdb_idl_index *sbrec_chassis_by_name = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_chassis", node), > > + "name"); > > + const struct sbrec_chassis *chassis > > + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > > + > > + struct ovsdb_idl_index *sbrec_pb_by_datapath = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_port_binding", node), > > + "datapath"); > > + struct ovsdb_idl_index *sbrec_pb_by_name = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_port_binding", node), > > + "name"); > > + const struct ovsrec_bridge_table *bridge_table = > > + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > > + const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > > ovs_table); > > + > > + struct ed_type_runtime_data *rt_data = > > + engine_get_input_data("runtime_data", node); > > + const struct hmap *local_datapaths = &rt_data->local_datapaths; > > + const struct sset *active_tunnels = &rt_data->active_tunnels; > > + > > + struct shash_node *nat_node; > > + SHASH_FOR_EACH (nat_node, &nat_addresses->nat_addresses) { > > + struct lport_addresses *laddrs = nat_node->data; > > + destroy_lport_addresses(laddrs); > > + free(laddrs); > > + } > > + shash_clear(&nat_addresses->nat_addresses); > > + sset_clear(&nat_addresses->localnet_vifs); > > + sset_clear(&nat_addresses->local_l3gw_ports); > > + sset_clear(&nat_addresses->nat_ip_keys); > > + > > + get_localnet_vifs_l3gwports(sbrec_pb_by_datapath, > > + sbrec_pb_by_name, > > + br_int, chassis, local_datapaths, > > + &nat_addresses->localnet_vifs, > > + &nat_addresses->local_l3gw_ports); > > + > > + get_nat_addresses_and_keys(sbrec_pb_by_name, > > + &nat_addresses->nat_ip_keys, > > + &nat_addresses->local_l3gw_ports, > > + chassis, active_tunnels, > > + &nat_addresses->nat_addresses); > > + > > + engine_set_node_state(node, EN_UPDATED); > > +} > > + > > +static bool > > +nat_addresses_change_handler(struct engine_node *node, void *data) > > +{ > > + en_nat_addresses_run(node, data); > > + return true; > > +} > > > > There is no need for separate handler, engine will always run the > "<NODE>_run()" function if handler is NULL. Hi Ales, i took a look with Max on this. The reason this handler exists is to support cases where the southbound db is not accessible. In these cases we do not allow recomputes on the engine to happen. Not having this handler would mean that the engine run gets canceled if the southbound is unaccessible and e.g. a port is locally added or active_tunnels changes or whatever else changes "runtime_data". Adding the handler here allows the engine to process changes by "runtime_data" without canceling the run and thereby being able to continue processing changes incrementally. An option i would see to make this nicer would be to make the decission if we can recompute without sb on a per engine node basis. However i am not sure how good of an idea this is. Would you see other options to solve this? Thanks a lot Felix > > + > > +static void > > +en_nat_addresses_cleanup(void *data OVS_UNUSED){ > > + struct ed_type_nat_addresses *nat_addresses = data; > > + shash_destroy(&nat_addresses->nat_addresses); > > > > We are leaking all the "struct lport_addresses" stored within this shash. > > > > + sset_destroy(&nat_addresses->localnet_vifs); > > + sset_destroy(&nat_addresses->local_l3gw_ports); > > + sset_destroy(&nat_addresses->nat_ip_keys); > > +} > > > > /* Engine node which is used to handle the Non VIF data like > > * - OVS patch ports > > @@ -4779,6 +5092,14 @@ controller_output_bfd_chassis_handler(struct > > engine_node *node, > > return true; > > } > > > > +static bool > > +controller_output_nat_addresses_handler(struct engine_node *node, > > + void *data OVS_UNUSED) > > +{ > > + engine_set_node_state(node, EN_UPDATED); > > + return true; > > +} > > + > > /* Handles sbrec_chassis changes. > > * If a new chassis is added or removed return false, so that > > * flows are recomputed. For any updates, there is no need for > > @@ -5093,6 +5414,7 @@ main(int argc, char *argv[]) > > ENGINE_NODE(mac_cache, "mac_cache"); > > ENGINE_NODE(bfd_chassis, "bfd_chassis"); > > ENGINE_NODE(dns_cache, "dns_cache"); > > + ENGINE_NODE(nat_addresses, "nat_addresses"); > > > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > > SB_NODES > > @@ -5137,6 +5459,14 @@ main(int argc, char *argv[]) > > engine_add_input(&en_bfd_chassis, &en_sb_chassis, NULL); > > engine_add_input(&en_bfd_chassis, &en_sb_ha_chassis_group, NULL); > > > > + engine_add_input(&en_nat_addresses, &en_ovs_open_vswitch, NULL); > > + engine_add_input(&en_nat_addresses, &en_ovs_bridge, NULL); > > + engine_add_input(&en_nat_addresses, &en_sb_chassis, NULL); > > + engine_add_input(&en_nat_addresses, &en_sb_port_binding, > > + nat_addresses_change_handler); > > + engine_add_input(&en_nat_addresses, &en_runtime_data, > > + nat_addresses_change_handler); > > + > > /* Note: The order of inputs is important, all OVS interface changes > > must > > * be handled before any ct_zone changes. > > */ > > @@ -5300,6 +5630,8 @@ main(int argc, char *argv[]) > > controller_output_mac_cache_handler); > > engine_add_input(&en_controller_output, &en_bfd_chassis, > > controller_output_bfd_chassis_handler); > > + engine_add_input(&en_controller_output, &en_nat_addresses, > > + controller_output_nat_addresses_handler); > > > > struct engine_arg engine_arg = { > > .sb_idl = ovnsb_idl_loop.idl, > > @@ -5346,6 +5678,8 @@ main(int argc, char *argv[]) > > engine_get_internal_data(&en_ct_zones); > > struct ed_type_bfd_chassis *bfd_chassis_data = > > engine_get_internal_data(&en_bfd_chassis); > > + struct ed_type_nat_addresses *nat_addresses_data = > > + engine_get_internal_data(&en_nat_addresses); > > struct ed_type_runtime_data *runtime_data = > > engine_get_internal_data(&en_runtime_data); > > struct ed_type_template_vars *template_vars_data = > > @@ -5682,6 +6016,7 @@ main(int argc, char *argv[]) > > } > > > > runtime_data = engine_get_data(&en_runtime_data); > > + nat_addresses_data = > > engine_get_data(&en_nat_addresses); > > if (runtime_data) { > > stopwatch_start(PATCH_RUN_STOPWATCH_NAME, > > time_msec()); > > patch_run(ovs_idl_txn, > > @@ -5729,7 +6064,6 @@ main(int argc, char *argv[]) > > pinctrl_update(ovnsb_idl_loop.idl); > > pinctrl_run(ovnsb_idl_txn, > > sbrec_datapath_binding_by_key, > > - sbrec_port_binding_by_datapath, > > sbrec_port_binding_by_key, > > sbrec_port_binding_by_name, > > sbrec_mac_binding_by_lport_ip, > > @@ -5743,13 +6077,14 @@ main(int argc, char *argv[]) > > sbrec_mac_binding_table_get( > > ovnsb_idl_loop.idl), > > > > sbrec_bfd_table_get(ovnsb_idl_loop.idl), > > - br_int, chassis, > > + chassis, > > &runtime_data->local_datapaths, > > &runtime_data->active_tunnels, > > > > &runtime_data->local_active_ports_ipv6_pd, > > &runtime_data->local_active_ports_ras, > > ovsrec_open_vswitch_table_get( > > - ovs_idl_loop.idl)); > > + ovs_idl_loop.idl), > > + nat_addresses_data); > > stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME, > > time_msec()); > > mirror_run(ovs_idl_txn, > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 3fb7e2fd7..e8c4ff77b 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -227,14 +227,11 @@ static void destroy_send_garps_rarps(void); > > static void send_garp_rarp_wait(long long int send_garp_rarp_time); > > static void send_garp_rarp_prepare( > > struct ovsdb_idl_txn *ovnsb_idl_txn, > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > > - const struct ovsrec_bridge *, > > - const struct sbrec_chassis *, > > const struct hmap *local_datapaths, > > - const struct sset *active_tunnels, > > - const struct ovsrec_open_vswitch_table *ovs_table) > > + const struct ovsrec_open_vswitch_table *ovs_table, > > + struct ed_type_nat_addresses *nat_addresses) > > OVS_REQUIRES(pinctrl_mutex); > > static void send_garp_rarp_run(struct rconn *swconn, > > long long int *send_garp_rarp_time) > > @@ -4008,7 +4005,6 @@ pinctrl_update(const struct ovsdb_idl *idl) > > void > > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index *sbrec_port_binding_by_key, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > > @@ -4019,13 +4015,13 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > const struct sbrec_service_monitor_table *svc_mon_table, > > const struct sbrec_mac_binding_table *mac_binding_table, > > const struct sbrec_bfd_table *bfd_table, > > - const struct ovsrec_bridge *br_int, > > const struct sbrec_chassis *chassis, > > const struct hmap *local_datapaths, > > const struct sset *active_tunnels, > > const struct shash *local_active_ports_ipv6_pd, > > const struct shash *local_active_ports_ras, > > - const struct ovsrec_open_vswitch_table *ovs_table) > > + const struct ovsrec_open_vswitch_table *ovs_table, > > + struct ed_type_nat_addresses *nat_addresses) > > { > > ovs_mutex_lock(&pinctrl_mutex); > > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > > @@ -4033,10 +4029,11 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > sbrec_mac_binding_by_lport_ip); > > run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > > sbrec_port_binding_by_key, chassis); > > - send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath, > > + send_garp_rarp_prepare(ovnsb_idl_txn, > > sbrec_port_binding_by_name, > > - sbrec_mac_binding_by_lport_ip, br_int, chassis, > > - local_datapaths, active_tunnels, ovs_table); > > + sbrec_mac_binding_by_lport_ip, > > + local_datapaths, > > + ovs_table, nat_addresses); > > prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name); > > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > > local_active_ports_ipv6_pd, chassis, > > @@ -6175,236 +6172,6 @@ ip_mcast_querier_wait(long long int query_time) > > } > > } > > > > -/* Get localnet vifs, local l3gw ports and ofport for localnet patch > > ports. */ > > -static void > > -get_localnet_vifs_l3gwports( > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > - struct ovsdb_idl_index *sbrec_port_binding_by_name, > > - const struct ovsrec_bridge *br_int, > > - const struct sbrec_chassis *chassis, > > - const struct hmap *local_datapaths, > > - struct sset *localnet_vifs, > > - struct sset *local_l3gw_ports) > > -{ > > - for (int i = 0; i < br_int->n_ports; i++) { > > - const struct ovsrec_port *port_rec = br_int->ports[i]; > > - if (!strcmp(port_rec->name, br_int->name)) { > > - continue; > > - } > > - const char *tunnel_id = smap_get(&port_rec->external_ids, > > - "ovn-chassis-id"); > > - if (tunnel_id && > > - encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, > > NULL)) { > > - continue; > > - } > > - const char *localnet = smap_get(&port_rec->external_ids, > > - "ovn-localnet-port"); > > - if (localnet) { > > - continue; > > - } > > - for (int j = 0; j < port_rec->n_interfaces; j++) { > > - const struct ovsrec_interface *iface_rec = > > port_rec->interfaces[j]; > > - if (!iface_rec->n_ofport) { > > - continue; > > - } > > - /* Get localnet vif. */ > > - const char *iface_id = smap_get(&iface_rec->external_ids, > > - "iface-id"); > > - if (!iface_id) { > > - continue; > > - } > > - const struct sbrec_port_binding *pb > > - = lport_lookup_by_name(sbrec_port_binding_by_name, > > iface_id); > > - if (!pb || pb->chassis != chassis) { > > - continue; > > - } > > - if (!iface_rec->link_state || > > - strcmp(iface_rec->link_state, "up")) { > > - continue; > > - } > > - struct local_datapath *ld > > - = get_local_datapath(local_datapaths, > > - pb->datapath->tunnel_key); > > - if (ld && ld->localnet_port) { > > - sset_add(localnet_vifs, iface_id); > > - } > > - } > > - } > > - > > - struct sbrec_port_binding *target = sbrec_port_binding_index_init_row( > > - sbrec_port_binding_by_datapath); > > - > > - const struct local_datapath *ld; > > - HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > > - const struct sbrec_port_binding *pb; > > - > > - if (!ld->localnet_port) { > > - continue; > > - } > > - > > - /* Get l3gw ports. Consider port bindings with type "l3gateway" > > - * that connect to gateway routers (if local), and consider port > > - * bindings of type "patch" since they might connect to > > - * distributed gateway ports with NAT addresses. */ > > - > > - sbrec_port_binding_index_set_datapath(target, ld->datapath); > > - SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > > - > > sbrec_port_binding_by_datapath) { > > - if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) > > - || !strcmp(pb->type, "patch")) { > > - sset_add(local_l3gw_ports, pb->logical_port); > > - } > > - } > > - } > > - sbrec_port_binding_index_destroy_row(target); > > -} > > - > > - > > -/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > > - * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > > - * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid IPv4 > > - * or IPv6 address, and stores them in the 'ipv4_addrs' and 'ipv6_addrs' > > - * fields of 'laddrs'. The logical port name is stored in 'lport'. > > - * > > - * Returns true if at least 'MAC' is found in 'address', false otherwise. > > - * > > - * The caller must call destroy_lport_addresses() and free(*lport). */ > > -static bool > > -extract_addresses_with_port(const char *addresses, > > - struct lport_addresses *laddrs, > > - char **lport) > > -{ > > - int ofs; > > - if (!extract_addresses(addresses, laddrs, &ofs)) { > > - return false; > > - } else if (!addresses[ofs]) { > > - return true; > > - } > > - > > - struct lexer lexer; > > - lexer_init(&lexer, addresses + ofs); > > - lexer_get(&lexer); > > - > > - if (lexer.error || lexer.token.type != LEX_T_ID > > - || !lexer_match_id(&lexer, "is_chassis_resident")) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > - VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", addresses); > > - lexer_destroy(&lexer); > > - return true; > > - } > > - > > - if (!lexer_match(&lexer, LEX_T_LPAREN)) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > - VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > > - "'is_chassis_resident' in address '%s'", > > addresses); > > - lexer_destroy(&lexer); > > - return false; > > - } > > - > > - if (lexer.token.type != LEX_T_STRING) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > - VLOG_INFO_RL(&rl, > > - "Syntax error: expecting quoted string after " > > - "'is_chassis_resident' in address '%s'", addresses); > > - lexer_destroy(&lexer); > > - return false; > > - } > > - > > - *lport = xstrdup(lexer.token.s); > > - > > - lexer_get(&lexer); > > - if (!lexer_match(&lexer, LEX_T_RPAREN)) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > - VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted > > string in " > > - "'is_chassis_resident()' in address '%s'", > > - addresses); > > - lexer_destroy(&lexer); > > - return false; > > - } > > - > > - lexer_destroy(&lexer); > > - return true; > > -} > > - > > -static void > > -consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > - const char *nat_address, > > - const struct sbrec_port_binding *pb, > > - struct sset *nat_address_keys, > > - const struct sbrec_chassis *chassis, > > - const struct sset *active_tunnels, > > - struct shash *nat_addresses) > > -{ > > - struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > > - char *lport = NULL; > > - if (!extract_addresses_with_port(nat_address, laddrs, &lport) > > - || (!lport && !strcmp(pb->type, "patch")) > > - || (lport && !lport_is_chassis_resident( > > - sbrec_port_binding_by_name, chassis, > > - active_tunnels, lport))) { > > - destroy_lport_addresses(laddrs); > > - free(laddrs); > > - free(lport); > > - return; > > - } > > - free(lport); > > - > > - int i; > > - for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > > - char *name = xasprintf("%s-%s", pb->logical_port, > > - laddrs->ipv4_addrs[i].addr_s); > > - sset_add(nat_address_keys, name); > > - free(name); > > - } > > - if (laddrs->n_ipv4_addrs == 0) { > > - char *name = xasprintf("%s-noip", pb->logical_port); > > - sset_add(nat_address_keys, name); > > - free(name); > > - } > > - shash_add(nat_addresses, pb->logical_port, laddrs); > > -} > > - > > -static void > > -get_nat_addresses_and_keys(struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > - struct sset *nat_address_keys, > > - struct sset *local_l3gw_ports, > > - const struct sbrec_chassis *chassis, > > - const struct sset *active_tunnels, > > - struct shash *nat_addresses) > > -{ > > - const char *gw_port; > > - SSET_FOR_EACH(gw_port, local_l3gw_ports) { > > - const struct sbrec_port_binding *pb; > > - > > - pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > > - if (!pb) { > > - continue; > > - } > > - > > - if (pb->n_nat_addresses) { > > - for (int i = 0; i < pb->n_nat_addresses; i++) { > > - consider_nat_address(sbrec_port_binding_by_name, > > - pb->nat_addresses[i], pb, > > - nat_address_keys, chassis, > > - active_tunnels, > > - nat_addresses); > > - } > > - } else { > > - /* Continue to support options:nat-addresses for version > > - * upgrade. */ > > - const char *nat_addresses_options = smap_get(&pb->options, > > - "nat-addresses"); > > - if (nat_addresses_options) { > > - consider_nat_address(sbrec_port_binding_by_name, > > - nat_addresses_options, pb, > > - nat_address_keys, chassis, > > - active_tunnels, > > - nat_addresses); > > - } > > - } > > - } > > -} > > - > > static void > > send_garp_rarp_wait(long long int send_garp_rarp_time) > > { > > @@ -6441,96 +6208,70 @@ send_garp_rarp_run(struct rconn *swconn, long long > > int *send_garp_rarp_time) > > * thread context. */ > > static void > > send_garp_rarp_prepare(struct ovsdb_idl_txn *ovnsb_idl_txn, > > - struct ovsdb_idl_index > > *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > struct ovsdb_idl_index > > *sbrec_mac_binding_by_lport_ip, > > - const struct ovsrec_bridge *br_int, > > - const struct sbrec_chassis *chassis, > > const struct hmap *local_datapaths, > > - const struct sset *active_tunnels, > > - const struct ovsrec_open_vswitch_table *ovs_table) > > + const struct ovsrec_open_vswitch_table *ovs_table, > > + struct ed_type_nat_addresses *nat_addresses) > > OVS_REQUIRES(pinctrl_mutex) > > { > > - struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs); > > - struct sset local_l3gw_ports = SSET_INITIALIZER(&local_l3gw_ports); > > - struct sset nat_ip_keys = SSET_INITIALIZER(&nat_ip_keys); > > - struct shash nat_addresses; > > - unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > - bool garp_continuous = false; > > - const struct ovsrec_open_vswitch *cfg = > > - ovsrec_open_vswitch_table_first(ovs_table); > > - if (cfg) { > > - garp_max_timeout = smap_get_ullong( > > - &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; > > - garp_continuous = !!garp_max_timeout; > > - if (!garp_max_timeout) { > > - garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > - } > > - } > > - > > - shash_init(&nat_addresses); > > - > > - get_localnet_vifs_l3gwports(sbrec_port_binding_by_datapath, > > - sbrec_port_binding_by_name, > > - br_int, chassis, local_datapaths, > > - &localnet_vifs, &local_l3gw_ports); > > - > > - get_nat_addresses_and_keys(sbrec_port_binding_by_name, > > - &nat_ip_keys, &local_l3gw_ports, > > - chassis, active_tunnels, > > - &nat_addresses); > > - /* For deleted ports and deleted nat ips, remove from > > - * send_garp_rarp_data. */ > > - struct shash_node *iter; > > - SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > > - if (!sset_contains(&localnet_vifs, iter->name) && > > - !sset_contains(&nat_ip_keys, iter->name)) { > > - send_garp_rarp_delete(iter->name); > > + if (nat_addresses) { > > + unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > + bool garp_continuous = false; > > + > > + const struct ovsrec_open_vswitch *cfg = > > + ovsrec_open_vswitch_table_first(ovs_table); > > + if (cfg) { > > + garp_max_timeout = smap_get_ullong( > > + &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; > > + garp_continuous = !!garp_max_timeout; > > + if (!garp_max_timeout) { > > + garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > + } > > } > > - } > > - > > - /* Update send_garp_rarp_data. */ > > - const char *iface_id; > > - SSET_FOR_EACH (iface_id, &localnet_vifs) { > > - const struct sbrec_port_binding *pb = lport_lookup_by_name( > > - sbrec_port_binding_by_name, iface_id); > > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > > false)) { > > - send_garp_rarp_update(ovnsb_idl_txn, > > - sbrec_mac_binding_by_lport_ip, > > - local_datapaths, pb, &nat_addresses, > > - garp_max_timeout, garp_continuous); > > + /* For deleted ports and deleted nat ips, remove from > > + * send_garp_rarp_data. */ > > + struct shash_node *iter; > > + SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > > + if (!sset_contains(&nat_addresses->localnet_vifs, iter->name) > > && > > + !sset_contains(&nat_addresses->nat_ip_keys, iter->name)) { > > + send_garp_rarp_delete(iter->name); > > + } > > } > > - } > > - > > - /* Update send_garp_rarp_data for nat-addresses. */ > > - const char *gw_port; > > - SSET_FOR_EACH (gw_port, &local_l3gw_ports) { > > - const struct sbrec_port_binding *pb > > - = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); > > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > > false)) { > > - send_garp_rarp_update(ovnsb_idl_txn, > > sbrec_mac_binding_by_lport_ip, > > - local_datapaths, pb, &nat_addresses, > > - garp_max_timeout, garp_continuous); > > + /* Update send_garp_rarp_data. */ > > + const char *iface_id; > > + SSET_FOR_EACH (iface_id, &nat_addresses->localnet_vifs) { > > + const struct sbrec_port_binding *pb = lport_lookup_by_name( > > + sbrec_port_binding_by_name, iface_id); > > + if (pb && !smap_get_bool( > > + &pb->options, "disable_garp_rarp", false)) { > > + send_garp_rarp_update(ovnsb_idl_txn, > > + sbrec_mac_binding_by_lport_ip, > > + local_datapaths, pb, > > + &nat_addresses->nat_addresses, > > + garp_max_timeout, garp_continuous); > > + } > > } > > - } > > > > - /* pinctrl_handler thread will send the GARPs. */ > > - > > - sset_destroy(&localnet_vifs); > > - sset_destroy(&local_l3gw_ports); > > + /* Update send_garp_rarp_data for nat-addresses. */ > > + const char *gw_port; > > + SSET_FOR_EACH (gw_port, &nat_addresses->local_l3gw_ports) { > > + const struct sbrec_port_binding *pb > > + = lport_lookup_by_name(sbrec_port_binding_by_name, > > gw_port); > > + if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > > + false)) { > > + send_garp_rarp_update(ovnsb_idl_txn, > > + sbrec_mac_binding_by_lport_ip, > > + local_datapaths, pb, > > + &nat_addresses->nat_addresses, > > + garp_max_timeout, garp_continuous); > > + } > > + } > > > > - SHASH_FOR_EACH_SAFE (iter, &nat_addresses) { > > - struct lport_addresses *laddrs = iter->data; > > - destroy_lport_addresses(laddrs); > > - shash_delete(&nat_addresses, iter); > > - free(laddrs); > > + /* pinctrl_handler thread will send the GARPs. */ > > + garp_rarp_max_timeout = garp_max_timeout; > > + garp_rarp_continuous = garp_continuous; > > } > > - shash_destroy(&nat_addresses); > > - > > - sset_destroy(&nat_ip_keys); > > - > > - garp_rarp_max_timeout = garp_max_timeout; > > - garp_rarp_continuous = garp_continuous; > > } > > > > static bool > > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > > index 846afe0a4..c6a7423b8 100644 > > --- a/controller/pinctrl.h > > +++ b/controller/pinctrl.h > > @@ -20,6 +20,7 @@ > > #include <stdint.h> > > > > #include "lib/sset.h" > > +#include "openvswitch/shash.h" > > #include "openvswitch/list.h" > > #include "openvswitch/meta-flow.h" > > > > @@ -39,10 +40,16 @@ struct sbrec_bfd_table; > > struct sbrec_port_binding; > > struct sbrec_mac_binding_table; > > > > +struct ed_type_nat_addresses { > > + struct shash nat_addresses; > > + struct sset localnet_vifs; > > + struct sset local_l3gw_ports; > > + struct sset nat_ip_keys; > > +}; > > > > > This shouldn't be part of pinctrl.h as it doesn't have anything to do with > pinctrl. In fact all functions that are handling the NAT addresses should > be in a separate module. > > + > > void pinctrl_init(void); > > void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > struct ovsdb_idl_index *sbrec_port_binding_by_key, > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > > @@ -53,12 +60,13 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > const struct sbrec_service_monitor_table *, > > const struct sbrec_mac_binding_table *, > > const struct sbrec_bfd_table *, > > - const struct ovsrec_bridge *, const struct sbrec_chassis > > *, > > + const struct sbrec_chassis *, > > const struct hmap *local_datapaths, > > const struct sset *active_tunnels, > > const struct shash *local_active_ports_ipv6_pd, > > const struct shash *local_active_ports_ras, > > - const struct ovsrec_open_vswitch_table *ovs_table); > > + const struct ovsrec_open_vswitch_table *ovs_table, > > + struct ed_type_nat_addresses *nat_addresses); > > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > > void pinctrl_destroy(void); > > > > -- > > 2.43.0 > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > Thanks, > Ales > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Dec 19, 2024 at 11:54 AM Felix Huettner <felix.huettner@stackit.cloud> wrote: > On Tue, Dec 10, 2024 at 01:04:32PM +0100, Ales Musil wrote: > > On Thu, Nov 14, 2024 at 11:47 AM Max Lamprecht via dev < > > ovs-dev@openvswitch.org> wrote: > > > > > This patch moves the nat_addresses calculation to an own engine node. > > > > > > Previously the nat_address calculation was executed at every > pinctrl_run > > > (send_garp_rarp_prepare). This can cause high cpu usage and delayed > > > actions on gateway-chassis(500 LRPs+). > > > > > > perf trace: > > > - 98.83% main > > > - 79.69% pinctrl_run > > > - 77.71% send_garp_rarp_prepare > > > - 55.49% get_nat_addresses_and_keys > > > - 36.76% consider_nat_address > > > - 18.13% lport_lookup_by_name > > > - 13.58% sset_destroy > > > > > > Co-authored-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > > > Signed-off-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > > > Signed-off-by: Max Lamprecht <max.lamprecht@stackit.cloud> > > > --- > > > > > > > Hello Max, > > > > thank you for your patience and sorry for the delay in reviews. I have a > > couple of comments down below. > > > > controller/ovn-controller.c | 341 +++++++++++++++++++++++++++++++- > > > controller/pinctrl.c | 381 ++++++------------------------------ > > > controller/pinctrl.h | 14 +- > > > 3 files changed, 410 insertions(+), 326 deletions(-) > > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 98b144699..49bc6d86c 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -3359,6 +3359,319 @@ en_dns_cache_cleanup(void *data OVS_UNUSED) > > > ovn_dns_cache_destroy(); > > > } > > > > > > +static void * > > > +en_nat_addresses_init(struct engine_node *node OVS_UNUSED, > > > + struct engine_arg *arg OVS_UNUSED) > > > +{ > > > + struct ed_type_nat_addresses *data = xzalloc(sizeof *data); > > > > > > > nit: No need to zero alloc if you init everything right away. > > > > > > > + shash_init(&data->nat_addresses); > > > + sset_init(&data->localnet_vifs); > > > + sset_init(&data->local_l3gw_ports); > > > + sset_init(&data->nat_ip_keys); > > > + return data; > > > +} > > > + > > > +/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > > > + * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > > > + * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid > IPv4 > > > + * or IPv6 address, and stores them in the 'ipv4_addrs' and > 'ipv6_addrs' > > > + * fields of 'laddrs'. The logical port name is stored in 'lport'. > > > + * > > > + * Returns true if at least 'MAC' is found in 'address', false > otherwise. > > > + * > > > + * The caller must call destroy_lport_addresses() and free(*lport). */ > > > +static bool > > > +extract_addresses_with_port(const char *addresses, > > > + struct lport_addresses *laddrs, > > > + char **lport) > > > +{ > > > + int ofs; > > > + if (!extract_addresses(addresses, laddrs, &ofs)) { > > > + return false; > > > + } else if (!addresses[ofs]) { > > > + return true; > > > + } > > > + > > > + struct lexer lexer; > > > + lexer_init(&lexer, addresses + ofs); > > > + lexer_get(&lexer); > > > + > > > + if (lexer.error || lexer.token.type != LEX_T_ID > > > + || !lexer_match_id(&lexer, "is_chassis_resident")) { > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > + VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", > addresses); > > > + lexer_destroy(&lexer); > > > + return true; > > > + } > > > + > > > + if (!lexer_match(&lexer, LEX_T_LPAREN)) { > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > + VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > > > + "'is_chassis_resident' in address '%s'", > > > addresses); > > > + lexer_destroy(&lexer); > > > + return false; > > > + } > > > + > > > + if (lexer.token.type != LEX_T_STRING) { > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > + VLOG_INFO_RL(&rl, > > > + "Syntax error: expecting quoted string after " > > > + "'is_chassis_resident' in address '%s'", > addresses); > > > + lexer_destroy(&lexer); > > > + return false; > > > + } > > > + > > > + *lport = xstrdup(lexer.token.s); > > > + > > > + lexer_get(&lexer); > > > + if (!lexer_match(&lexer, LEX_T_RPAREN)) { > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > + VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted > > > string in " > > > + "'is_chassis_resident()' in address '%s'", > > > + addresses); > > > + lexer_destroy(&lexer); > > > + return false; > > > + } > > > + > > > + lexer_destroy(&lexer); > > > + return true; > > > +} > > > + > > > +static void > > > +consider_nat_address(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > > + const char *nat_address, > > > + const struct sbrec_port_binding *pb, > > > + struct sset *nat_address_keys, > > > + const struct sbrec_chassis *chassis, > > > + const struct sset *active_tunnels, > > > + struct shash *nat_addresses) > > > +{ > > > + struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > > > + char *lport = NULL; > > > + if (!extract_addresses_with_port(nat_address, laddrs, &lport) > > > + || (!lport && !strcmp(pb->type, "patch")) > > > + || (lport && !lport_is_chassis_resident( > > > + sbrec_port_binding_by_name, chassis, > > > + active_tunnels, lport))) { > > > + destroy_lport_addresses(laddrs); > > > + free(laddrs); > > > + free(lport); > > > + return; > > > + } > > > + free(lport); > > > + > > > + int i; > > > + for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > > > + char *name = xasprintf("%s-%s", pb->logical_port, > > > + laddrs->ipv4_addrs[i].addr_s); > > > + sset_add(nat_address_keys, name); > > > + free(name); > > > + } > > > + if (laddrs->n_ipv4_addrs == 0) { > > > + char *name = xasprintf("%s-noip", pb->logical_port); > > > + sset_add(nat_address_keys, name); > > > + free(name); > > > + } > > > + shash_add(nat_addresses, pb->logical_port, laddrs); > > > +} > > > + > > > +static void > > > +get_nat_addresses_and_keys(struct ovsdb_idl_index > > > *sbrec_port_binding_by_name, > > > + struct sset *nat_address_keys, > > > + struct sset *local_l3gw_ports, > > > + const struct sbrec_chassis *chassis, > > > + const struct sset *active_tunnels, > > > + struct shash *nat_addresses) > > > +{ > > > + const char *gw_port; > > > + SSET_FOR_EACH (gw_port, local_l3gw_ports) { > > > + const struct sbrec_port_binding *pb; > > > + > > > + pb = lport_lookup_by_name(sbrec_port_binding_by_name, > gw_port); > > > + if (!pb) { > > > + continue; > > > + } > > > + > > > + if (pb->n_nat_addresses) { > > > + for (int i = 0; i < pb->n_nat_addresses; i++) { > > > + consider_nat_address(sbrec_port_binding_by_name, > > > + pb->nat_addresses[i], pb, > > > + nat_address_keys, chassis, > > > + active_tunnels, > > > + nat_addresses); > > > + } > > > + } else { > > > + /* Continue to support options:nat-addresses for version > > > + * upgrade. */ > > > + const char *nat_addresses_options = smap_get(&pb->options, > > > + > "nat-addresses"); > > > + if (nat_addresses_options) { > > > + consider_nat_address(sbrec_port_binding_by_name, > > > + nat_addresses_options, pb, > > > + nat_address_keys, chassis, > > > + active_tunnels, > > > + nat_addresses); > > > + } > > > + } > > > + } > > > +} > > > + > > > +/* Get localnet vifs, local l3gw ports and ofport for localnet patch > > > ports. */ > > > +static void > > > +get_localnet_vifs_l3gwports( > > > + struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > + const struct ovsrec_bridge *br_int, > > > + const struct sbrec_chassis *chassis, > > > + const struct hmap *local_datapaths, > > > + struct sset *localnet_vifs, > > > + struct sset *local_l3gw_ports) > > > +{ > > > + for (int i = 0; i < br_int->n_ports; i++) { > > > + const struct ovsrec_port *port_rec = br_int->ports[i]; > > > + if (!strcmp(port_rec->name, br_int->name)) { > > > + continue; > > > + } > > > + const char *tunnel_id = smap_get(&port_rec->external_ids, > > > + "ovn-chassis-id"); > > > + if (tunnel_id && > > > + encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, > > > NULL)) { > > > + continue; > > > + } > > > + const char *localnet = smap_get(&port_rec->external_ids, > > > + "ovn-localnet-port"); > > > + if (localnet) { > > > + continue; > > > + } > > > + for (int j = 0; j < port_rec->n_interfaces; j++) { > > > + const struct ovsrec_interface *iface_rec = > > > port_rec->interfaces[j]; > > > + if (!iface_rec->n_ofport) { > > > + continue; > > > + } > > > + /* Get localnet vif. */ > > > + const char *iface_id = smap_get(&iface_rec->external_ids, > > > + "iface-id"); > > > + if (!iface_id) { > > > + continue; > > > + } > > > + const struct sbrec_port_binding *pb > > > + = lport_lookup_by_name(sbrec_port_binding_by_name, > > > iface_id); > > > + if (!pb || pb->chassis != chassis) { > > > + continue; > > > + } > > > + if (!iface_rec->link_state || > > > + strcmp(iface_rec->link_state, "up")) { > > > + continue; > > > + } > > > + struct local_datapath *ld > > > + = get_local_datapath(local_datapaths, > > > + pb->datapath->tunnel_key); > > > + if (ld && ld->localnet_port) { > > > + sset_add(localnet_vifs, iface_id); > > > + } > > > + } > > > + } > > > + > > > + struct sbrec_port_binding *target = > sbrec_port_binding_index_init_row( > > > + sbrec_port_binding_by_datapath); > > > + > > > + const struct local_datapath *ld; > > > + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > > > + const struct sbrec_port_binding *pb; > > > + > > > + if (!ld->localnet_port) { > > > + continue; > > > + } > > > + > > > + /* Get l3gw ports. Consider port bindings with type > "l3gateway" > > > + * that connect to gateway routers (if local), and consider > port > > > + * bindings of type "patch" since they might connect to > > > + * distributed gateway ports with NAT addresses. */ > > > + > > > + sbrec_port_binding_index_set_datapath(target, ld->datapath); > > > + SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > > > + > > > sbrec_port_binding_by_datapath) { > > > + if ((!strcmp(pb->type, "l3gateway") && pb->chassis == > chassis) > > > + || !strcmp(pb->type, "patch")) { > > > + sset_add(local_l3gw_ports, pb->logical_port); > > > + } > > > + } > > > + } > > > + sbrec_port_binding_index_destroy_row(target); > > > +} > > > + > > > +static void > > > +en_nat_addresses_run(struct engine_node *node, void *data OVS_UNUSED) > > > +{ > > > + struct ed_type_nat_addresses *nat_addresses = data; > > > + const struct ovsrec_open_vswitch_table *ovs_table = > > > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > > > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > > > + struct ovsdb_idl_index *sbrec_chassis_by_name = > > > + engine_ovsdb_node_get_index( > > > + engine_get_input("SB_chassis", node), > > > + "name"); > > > + const struct sbrec_chassis *chassis > > > + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > > > + > > > + struct ovsdb_idl_index *sbrec_pb_by_datapath = > > > + engine_ovsdb_node_get_index( > > > + engine_get_input("SB_port_binding", node), > > > + "datapath"); > > > + struct ovsdb_idl_index *sbrec_pb_by_name = > > > + engine_ovsdb_node_get_index( > > > + engine_get_input("SB_port_binding", node), > > > + "name"); > > > + const struct ovsrec_bridge_table *bridge_table = > > > + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > > > + const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > > > ovs_table); > > > + > > > + struct ed_type_runtime_data *rt_data = > > > + engine_get_input_data("runtime_data", node); > > > + const struct hmap *local_datapaths = &rt_data->local_datapaths; > > > + const struct sset *active_tunnels = &rt_data->active_tunnels; > > > + > > > + struct shash_node *nat_node; > > > + SHASH_FOR_EACH (nat_node, &nat_addresses->nat_addresses) { > > > + struct lport_addresses *laddrs = nat_node->data; > > > + destroy_lport_addresses(laddrs); > > > + free(laddrs); > > > + } > > > + shash_clear(&nat_addresses->nat_addresses); > > > + sset_clear(&nat_addresses->localnet_vifs); > > > + sset_clear(&nat_addresses->local_l3gw_ports); > > > + sset_clear(&nat_addresses->nat_ip_keys); > > > + > > > + get_localnet_vifs_l3gwports(sbrec_pb_by_datapath, > > > + sbrec_pb_by_name, > > > + br_int, chassis, local_datapaths, > > > + &nat_addresses->localnet_vifs, > > > + &nat_addresses->local_l3gw_ports); > > > + > > > + get_nat_addresses_and_keys(sbrec_pb_by_name, > > > + &nat_addresses->nat_ip_keys, > > > + &nat_addresses->local_l3gw_ports, > > > + chassis, active_tunnels, > > > + &nat_addresses->nat_addresses); > > > + > > > + engine_set_node_state(node, EN_UPDATED); > > > +} > > > + > > > +static bool > > > +nat_addresses_change_handler(struct engine_node *node, void *data) > > > +{ > > > + en_nat_addresses_run(node, data); > > > + return true; > > > +} > > > > > > > There is no need for separate handler, engine will always run the > > "<NODE>_run()" function if handler is NULL. > > Hi Ales, > Hi Felix, > i took a look with Max on this. > > The reason this handler exists is to support cases where the southbound > db is not accessible. > In these cases we do not allow recomputes on the engine to happen. > > Not having this handler would mean that the engine run gets canceled if > the southbound is unaccessible and e.g. a port is locally added or > active_tunnels changes or whatever else changes "runtime_data". > > Adding the handler here allows the engine to process changes by > "runtime_data" without canceling the run and thereby being able to > continue processing changes incrementally. > > An option i would see to make this nicer would be to make the decission > if we can recompute without sb on a per engine node basis. However i am > not sure how good of an idea this is. > > Would you see other options to solve this? > Ok I see why you would want to do that, however the question is how often do you expect to need the "incremental" processing for this specific node to work? When SB disconnects you can still do the full run for this node, I suppose that's fine. But once controller reconnects we will do full recompute anyway with new data. So there essentially two things to consider: 1) How often do you expect the SB disconnect to happen? IMO in normal working conditions that should be as low as possible I think we can agree on that. 2) How long does it take for controller to reconnect and do full recompute? This change is minimal however it has this strange side effect that wasn't intended by the engine API. If we can agree that the need for I-P during disconnect is minimal. I would suggest following the normal procedure of the I-P engine. If there is really a need for this to work during SB disconnect we should find a way to do it in a more explicit way rather than bending the existing API. Or even better, proper handler for runtime data and port binding. WDYT? > > Thanks a lot > Felix > > > > > + > > > +static void > > > +en_nat_addresses_cleanup(void *data OVS_UNUSED){ > > > + struct ed_type_nat_addresses *nat_addresses = data; > > > + shash_destroy(&nat_addresses->nat_addresses); > > > > > > > We are leaking all the "struct lport_addresses" stored within this shash. > > > > > > > + sset_destroy(&nat_addresses->localnet_vifs); > > > + sset_destroy(&nat_addresses->local_l3gw_ports); > > > + sset_destroy(&nat_addresses->nat_ip_keys); > > > +} > > > > > > /* Engine node which is used to handle the Non VIF data like > > > * - OVS patch ports > > > @@ -4779,6 +5092,14 @@ controller_output_bfd_chassis_handler(struct > > > engine_node *node, > > > return true; > > > } > > > > > > +static bool > > > +controller_output_nat_addresses_handler(struct engine_node *node, > > > + void *data OVS_UNUSED) > > > +{ > > > + engine_set_node_state(node, EN_UPDATED); > > > + return true; > > > +} > > > + > > > /* Handles sbrec_chassis changes. > > > * If a new chassis is added or removed return false, so that > > > * flows are recomputed. For any updates, there is no need for > > > @@ -5093,6 +5414,7 @@ main(int argc, char *argv[]) > > > ENGINE_NODE(mac_cache, "mac_cache"); > > > ENGINE_NODE(bfd_chassis, "bfd_chassis"); > > > ENGINE_NODE(dns_cache, "dns_cache"); > > > + ENGINE_NODE(nat_addresses, "nat_addresses"); > > > > > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > > > SB_NODES > > > @@ -5137,6 +5459,14 @@ main(int argc, char *argv[]) > > > engine_add_input(&en_bfd_chassis, &en_sb_chassis, NULL); > > > engine_add_input(&en_bfd_chassis, &en_sb_ha_chassis_group, NULL); > > > > > > + engine_add_input(&en_nat_addresses, &en_ovs_open_vswitch, NULL); > > > + engine_add_input(&en_nat_addresses, &en_ovs_bridge, NULL); > > > + engine_add_input(&en_nat_addresses, &en_sb_chassis, NULL); > > > + engine_add_input(&en_nat_addresses, &en_sb_port_binding, > > > + nat_addresses_change_handler); > > > + engine_add_input(&en_nat_addresses, &en_runtime_data, > > > + nat_addresses_change_handler); > > > + > > > /* Note: The order of inputs is important, all OVS interface > changes > > > must > > > * be handled before any ct_zone changes. > > > */ > > > @@ -5300,6 +5630,8 @@ main(int argc, char *argv[]) > > > controller_output_mac_cache_handler); > > > engine_add_input(&en_controller_output, &en_bfd_chassis, > > > controller_output_bfd_chassis_handler); > > > + engine_add_input(&en_controller_output, &en_nat_addresses, > > > + controller_output_nat_addresses_handler); > > > > > > struct engine_arg engine_arg = { > > > .sb_idl = ovnsb_idl_loop.idl, > > > @@ -5346,6 +5678,8 @@ main(int argc, char *argv[]) > > > engine_get_internal_data(&en_ct_zones); > > > struct ed_type_bfd_chassis *bfd_chassis_data = > > > engine_get_internal_data(&en_bfd_chassis); > > > + struct ed_type_nat_addresses *nat_addresses_data = > > > + engine_get_internal_data(&en_nat_addresses); > > > struct ed_type_runtime_data *runtime_data = > > > engine_get_internal_data(&en_runtime_data); > > > struct ed_type_template_vars *template_vars_data = > > > @@ -5682,6 +6016,7 @@ main(int argc, char *argv[]) > > > } > > > > > > runtime_data = engine_get_data(&en_runtime_data); > > > + nat_addresses_data = > > > engine_get_data(&en_nat_addresses); > > > if (runtime_data) { > > > stopwatch_start(PATCH_RUN_STOPWATCH_NAME, > > > time_msec()); > > > patch_run(ovs_idl_txn, > > > @@ -5729,7 +6064,6 @@ main(int argc, char *argv[]) > > > pinctrl_update(ovnsb_idl_loop.idl); > > > pinctrl_run(ovnsb_idl_txn, > > > sbrec_datapath_binding_by_key, > > > - sbrec_port_binding_by_datapath, > > > sbrec_port_binding_by_key, > > > sbrec_port_binding_by_name, > > > sbrec_mac_binding_by_lport_ip, > > > @@ -5743,13 +6077,14 @@ main(int argc, char *argv[]) > > > sbrec_mac_binding_table_get( > > > ovnsb_idl_loop.idl), > > > > > > sbrec_bfd_table_get(ovnsb_idl_loop.idl), > > > - br_int, chassis, > > > + chassis, > > > &runtime_data->local_datapaths, > > > &runtime_data->active_tunnels, > > > > > > &runtime_data->local_active_ports_ipv6_pd, > > > > &runtime_data->local_active_ports_ras, > > > ovsrec_open_vswitch_table_get( > > > - ovs_idl_loop.idl)); > > > + ovs_idl_loop.idl), > > > + nat_addresses_data); > > > stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME, > > > time_msec()); > > > mirror_run(ovs_idl_txn, > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > > index 3fb7e2fd7..e8c4ff77b 100644 > > > --- a/controller/pinctrl.c > > > +++ b/controller/pinctrl.c > > > @@ -227,14 +227,11 @@ static void destroy_send_garps_rarps(void); > > > static void send_garp_rarp_wait(long long int send_garp_rarp_time); > > > static void send_garp_rarp_prepare( > > > struct ovsdb_idl_txn *ovnsb_idl_txn, > > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > > > - const struct ovsrec_bridge *, > > > - const struct sbrec_chassis *, > > > const struct hmap *local_datapaths, > > > - const struct sset *active_tunnels, > > > - const struct ovsrec_open_vswitch_table *ovs_table) > > > + const struct ovsrec_open_vswitch_table *ovs_table, > > > + struct ed_type_nat_addresses *nat_addresses) > > > OVS_REQUIRES(pinctrl_mutex); > > > static void send_garp_rarp_run(struct rconn *swconn, > > > long long int *send_garp_rarp_time) > > > @@ -4008,7 +4005,6 @@ pinctrl_update(const struct ovsdb_idl *idl) > > > void > > > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > > struct ovsdb_idl_index *sbrec_port_binding_by_key, > > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > > > @@ -4019,13 +4015,13 @@ pinctrl_run(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > > const struct sbrec_service_monitor_table *svc_mon_table, > > > const struct sbrec_mac_binding_table *mac_binding_table, > > > const struct sbrec_bfd_table *bfd_table, > > > - const struct ovsrec_bridge *br_int, > > > const struct sbrec_chassis *chassis, > > > const struct hmap *local_datapaths, > > > const struct sset *active_tunnels, > > > const struct shash *local_active_ports_ipv6_pd, > > > const struct shash *local_active_ports_ras, > > > - const struct ovsrec_open_vswitch_table *ovs_table) > > > + const struct ovsrec_open_vswitch_table *ovs_table, > > > + struct ed_type_nat_addresses *nat_addresses) > > > { > > > ovs_mutex_lock(&pinctrl_mutex); > > > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > > > @@ -4033,10 +4029,11 @@ pinctrl_run(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > > sbrec_mac_binding_by_lport_ip); > > > run_put_vport_bindings(ovnsb_idl_txn, > sbrec_datapath_binding_by_key, > > > sbrec_port_binding_by_key, chassis); > > > - send_garp_rarp_prepare(ovnsb_idl_txn, > sbrec_port_binding_by_datapath, > > > + send_garp_rarp_prepare(ovnsb_idl_txn, > > > sbrec_port_binding_by_name, > > > - sbrec_mac_binding_by_lport_ip, br_int, > chassis, > > > - local_datapaths, active_tunnels, > ovs_table); > > > + sbrec_mac_binding_by_lport_ip, > > > + local_datapaths, > > > + ovs_table, nat_addresses); > > > prepare_ipv6_ras(local_active_ports_ras, > sbrec_port_binding_by_name); > > > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > > > local_active_ports_ipv6_pd, chassis, > > > @@ -6175,236 +6172,6 @@ ip_mcast_querier_wait(long long int query_time) > > > } > > > } > > > > > > -/* Get localnet vifs, local l3gw ports and ofport for localnet patch > > > ports. */ > > > -static void > > > -get_localnet_vifs_l3gwports( > > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > > - struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > - const struct ovsrec_bridge *br_int, > > > - const struct sbrec_chassis *chassis, > > > - const struct hmap *local_datapaths, > > > - struct sset *localnet_vifs, > > > - struct sset *local_l3gw_ports) > > > -{ > > > - for (int i = 0; i < br_int->n_ports; i++) { > > > - const struct ovsrec_port *port_rec = br_int->ports[i]; > > > - if (!strcmp(port_rec->name, br_int->name)) { > > > - continue; > > > - } > > > - const char *tunnel_id = smap_get(&port_rec->external_ids, > > > - "ovn-chassis-id"); > > > - if (tunnel_id && > > > - encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, > > > NULL)) { > > > - continue; > > > - } > > > - const char *localnet = smap_get(&port_rec->external_ids, > > > - "ovn-localnet-port"); > > > - if (localnet) { > > > - continue; > > > - } > > > - for (int j = 0; j < port_rec->n_interfaces; j++) { > > > - const struct ovsrec_interface *iface_rec = > > > port_rec->interfaces[j]; > > > - if (!iface_rec->n_ofport) { > > > - continue; > > > - } > > > - /* Get localnet vif. */ > > > - const char *iface_id = smap_get(&iface_rec->external_ids, > > > - "iface-id"); > > > - if (!iface_id) { > > > - continue; > > > - } > > > - const struct sbrec_port_binding *pb > > > - = lport_lookup_by_name(sbrec_port_binding_by_name, > > > iface_id); > > > - if (!pb || pb->chassis != chassis) { > > > - continue; > > > - } > > > - if (!iface_rec->link_state || > > > - strcmp(iface_rec->link_state, "up")) { > > > - continue; > > > - } > > > - struct local_datapath *ld > > > - = get_local_datapath(local_datapaths, > > > - pb->datapath->tunnel_key); > > > - if (ld && ld->localnet_port) { > > > - sset_add(localnet_vifs, iface_id); > > > - } > > > - } > > > - } > > > - > > > - struct sbrec_port_binding *target = > sbrec_port_binding_index_init_row( > > > - sbrec_port_binding_by_datapath); > > > - > > > - const struct local_datapath *ld; > > > - HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > > > - const struct sbrec_port_binding *pb; > > > - > > > - if (!ld->localnet_port) { > > > - continue; > > > - } > > > - > > > - /* Get l3gw ports. Consider port bindings with type > "l3gateway" > > > - * that connect to gateway routers (if local), and consider > port > > > - * bindings of type "patch" since they might connect to > > > - * distributed gateway ports with NAT addresses. */ > > > - > > > - sbrec_port_binding_index_set_datapath(target, ld->datapath); > > > - SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > > > - > > > sbrec_port_binding_by_datapath) { > > > - if ((!strcmp(pb->type, "l3gateway") && pb->chassis == > chassis) > > > - || !strcmp(pb->type, "patch")) { > > > - sset_add(local_l3gw_ports, pb->logical_port); > > > - } > > > - } > > > - } > > > - sbrec_port_binding_index_destroy_row(target); > > > -} > > > - > > > - > > > -/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > > > - * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > > > - * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid > IPv4 > > > - * or IPv6 address, and stores them in the 'ipv4_addrs' and > 'ipv6_addrs' > > > - * fields of 'laddrs'. The logical port name is stored in 'lport'. > > > - * > > > - * Returns true if at least 'MAC' is found in 'address', false > otherwise. > > > - * > > > - * The caller must call destroy_lport_addresses() and free(*lport). */ > > > -static bool > > > -extract_addresses_with_port(const char *addresses, > > > - struct lport_addresses *laddrs, > > > - char **lport) > > > -{ > > > - int ofs; > > > - if (!extract_addresses(addresses, laddrs, &ofs)) { > > > - return false; > > > - } else if (!addresses[ofs]) { > > > - return true; > > > - } > > > - > > > - struct lexer lexer; > > > - lexer_init(&lexer, addresses + ofs); > > > - lexer_get(&lexer); > > > - > > > - if (lexer.error || lexer.token.type != LEX_T_ID > > > - || !lexer_match_id(&lexer, "is_chassis_resident")) { > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > - VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", > addresses); > > > - lexer_destroy(&lexer); > > > - return true; > > > - } > > > - > > > - if (!lexer_match(&lexer, LEX_T_LPAREN)) { > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > - VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > > > - "'is_chassis_resident' in address '%s'", > > > addresses); > > > - lexer_destroy(&lexer); > > > - return false; > > > - } > > > - > > > - if (lexer.token.type != LEX_T_STRING) { > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > - VLOG_INFO_RL(&rl, > > > - "Syntax error: expecting quoted string after " > > > - "'is_chassis_resident' in address '%s'", > addresses); > > > - lexer_destroy(&lexer); > > > - return false; > > > - } > > > - > > > - *lport = xstrdup(lexer.token.s); > > > - > > > - lexer_get(&lexer); > > > - if (!lexer_match(&lexer, LEX_T_RPAREN)) { > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > - VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted > > > string in " > > > - "'is_chassis_resident()' in address '%s'", > > > - addresses); > > > - lexer_destroy(&lexer); > > > - return false; > > > - } > > > - > > > - lexer_destroy(&lexer); > > > - return true; > > > -} > > > - > > > -static void > > > -consider_nat_address(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > > - const char *nat_address, > > > - const struct sbrec_port_binding *pb, > > > - struct sset *nat_address_keys, > > > - const struct sbrec_chassis *chassis, > > > - const struct sset *active_tunnels, > > > - struct shash *nat_addresses) > > > -{ > > > - struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > > > - char *lport = NULL; > > > - if (!extract_addresses_with_port(nat_address, laddrs, &lport) > > > - || (!lport && !strcmp(pb->type, "patch")) > > > - || (lport && !lport_is_chassis_resident( > > > - sbrec_port_binding_by_name, chassis, > > > - active_tunnels, lport))) { > > > - destroy_lport_addresses(laddrs); > > > - free(laddrs); > > > - free(lport); > > > - return; > > > - } > > > - free(lport); > > > - > > > - int i; > > > - for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > > > - char *name = xasprintf("%s-%s", pb->logical_port, > > > - laddrs->ipv4_addrs[i].addr_s); > > > - sset_add(nat_address_keys, name); > > > - free(name); > > > - } > > > - if (laddrs->n_ipv4_addrs == 0) { > > > - char *name = xasprintf("%s-noip", pb->logical_port); > > > - sset_add(nat_address_keys, name); > > > - free(name); > > > - } > > > - shash_add(nat_addresses, pb->logical_port, laddrs); > > > -} > > > - > > > -static void > > > -get_nat_addresses_and_keys(struct ovsdb_idl_index > > > *sbrec_port_binding_by_name, > > > - struct sset *nat_address_keys, > > > - struct sset *local_l3gw_ports, > > > - const struct sbrec_chassis *chassis, > > > - const struct sset *active_tunnels, > > > - struct shash *nat_addresses) > > > -{ > > > - const char *gw_port; > > > - SSET_FOR_EACH(gw_port, local_l3gw_ports) { > > > - const struct sbrec_port_binding *pb; > > > - > > > - pb = lport_lookup_by_name(sbrec_port_binding_by_name, > gw_port); > > > - if (!pb) { > > > - continue; > > > - } > > > - > > > - if (pb->n_nat_addresses) { > > > - for (int i = 0; i < pb->n_nat_addresses; i++) { > > > - consider_nat_address(sbrec_port_binding_by_name, > > > - pb->nat_addresses[i], pb, > > > - nat_address_keys, chassis, > > > - active_tunnels, > > > - nat_addresses); > > > - } > > > - } else { > > > - /* Continue to support options:nat-addresses for version > > > - * upgrade. */ > > > - const char *nat_addresses_options = smap_get(&pb->options, > > > - > "nat-addresses"); > > > - if (nat_addresses_options) { > > > - consider_nat_address(sbrec_port_binding_by_name, > > > - nat_addresses_options, pb, > > > - nat_address_keys, chassis, > > > - active_tunnels, > > > - nat_addresses); > > > - } > > > - } > > > - } > > > -} > > > - > > > static void > > > send_garp_rarp_wait(long long int send_garp_rarp_time) > > > { > > > @@ -6441,96 +6208,70 @@ send_garp_rarp_run(struct rconn *swconn, long > long > > > int *send_garp_rarp_time) > > > * thread context. */ > > > static void > > > send_garp_rarp_prepare(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > - struct ovsdb_idl_index > > > *sbrec_port_binding_by_datapath, > > > struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > > struct ovsdb_idl_index > > > *sbrec_mac_binding_by_lport_ip, > > > - const struct ovsrec_bridge *br_int, > > > - const struct sbrec_chassis *chassis, > > > const struct hmap *local_datapaths, > > > - const struct sset *active_tunnels, > > > - const struct ovsrec_open_vswitch_table > *ovs_table) > > > + const struct ovsrec_open_vswitch_table > *ovs_table, > > > + struct ed_type_nat_addresses *nat_addresses) > > > OVS_REQUIRES(pinctrl_mutex) > > > { > > > - struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs); > > > - struct sset local_l3gw_ports = > SSET_INITIALIZER(&local_l3gw_ports); > > > - struct sset nat_ip_keys = SSET_INITIALIZER(&nat_ip_keys); > > > - struct shash nat_addresses; > > > - unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > > - bool garp_continuous = false; > > > - const struct ovsrec_open_vswitch *cfg = > > > - ovsrec_open_vswitch_table_first(ovs_table); > > > - if (cfg) { > > > - garp_max_timeout = smap_get_ullong( > > > - &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; > > > - garp_continuous = !!garp_max_timeout; > > > - if (!garp_max_timeout) { > > > - garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > > - } > > > - } > > > - > > > - shash_init(&nat_addresses); > > > - > > > - get_localnet_vifs_l3gwports(sbrec_port_binding_by_datapath, > > > - sbrec_port_binding_by_name, > > > - br_int, chassis, local_datapaths, > > > - &localnet_vifs, &local_l3gw_ports); > > > - > > > - get_nat_addresses_and_keys(sbrec_port_binding_by_name, > > > - &nat_ip_keys, &local_l3gw_ports, > > > - chassis, active_tunnels, > > > - &nat_addresses); > > > - /* For deleted ports and deleted nat ips, remove from > > > - * send_garp_rarp_data. */ > > > - struct shash_node *iter; > > > - SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > > > - if (!sset_contains(&localnet_vifs, iter->name) && > > > - !sset_contains(&nat_ip_keys, iter->name)) { > > > - send_garp_rarp_delete(iter->name); > > > + if (nat_addresses) { > > > + unsigned long long garp_max_timeout = > GARP_RARP_DEF_MAX_TIMEOUT; > > > + bool garp_continuous = false; > > > + > > > + const struct ovsrec_open_vswitch *cfg = > > > + ovsrec_open_vswitch_table_first(ovs_table); > > > + if (cfg) { > > > + garp_max_timeout = smap_get_ullong( > > > + &cfg->external_ids, "garp-max-timeout-sec", 0) * > 1000; > > > + garp_continuous = !!garp_max_timeout; > > > + if (!garp_max_timeout) { > > > + garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > > + } > > > } > > > - } > > > - > > > - /* Update send_garp_rarp_data. */ > > > - const char *iface_id; > > > - SSET_FOR_EACH (iface_id, &localnet_vifs) { > > > - const struct sbrec_port_binding *pb = lport_lookup_by_name( > > > - sbrec_port_binding_by_name, iface_id); > > > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > > > false)) { > > > - send_garp_rarp_update(ovnsb_idl_txn, > > > - sbrec_mac_binding_by_lport_ip, > > > - local_datapaths, pb, &nat_addresses, > > > - garp_max_timeout, garp_continuous); > > > + /* For deleted ports and deleted nat ips, remove from > > > + * send_garp_rarp_data. */ > > > + struct shash_node *iter; > > > + SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > > > + if (!sset_contains(&nat_addresses->localnet_vifs, > iter->name) > > > && > > > + !sset_contains(&nat_addresses->nat_ip_keys, > iter->name)) { > > > + send_garp_rarp_delete(iter->name); > > > + } > > > } > > > - } > > > - > > > - /* Update send_garp_rarp_data for nat-addresses. */ > > > - const char *gw_port; > > > - SSET_FOR_EACH (gw_port, &local_l3gw_ports) { > > > - const struct sbrec_port_binding *pb > > > - = lport_lookup_by_name(sbrec_port_binding_by_name, > gw_port); > > > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > > > false)) { > > > - send_garp_rarp_update(ovnsb_idl_txn, > > > sbrec_mac_binding_by_lport_ip, > > > - local_datapaths, pb, &nat_addresses, > > > - garp_max_timeout, garp_continuous); > > > + /* Update send_garp_rarp_data. */ > > > + const char *iface_id; > > > + SSET_FOR_EACH (iface_id, &nat_addresses->localnet_vifs) { > > > + const struct sbrec_port_binding *pb = > lport_lookup_by_name( > > > + sbrec_port_binding_by_name, iface_id); > > > + if (pb && !smap_get_bool( > > > + &pb->options, "disable_garp_rarp", false)) { > > > + send_garp_rarp_update(ovnsb_idl_txn, > > > + sbrec_mac_binding_by_lport_ip, > > > + local_datapaths, pb, > > > + &nat_addresses->nat_addresses, > > > + garp_max_timeout, > garp_continuous); > > > + } > > > } > > > - } > > > > > > - /* pinctrl_handler thread will send the GARPs. */ > > > - > > > - sset_destroy(&localnet_vifs); > > > - sset_destroy(&local_l3gw_ports); > > > + /* Update send_garp_rarp_data for nat-addresses. */ > > > + const char *gw_port; > > > + SSET_FOR_EACH (gw_port, &nat_addresses->local_l3gw_ports) { > > > + const struct sbrec_port_binding *pb > > > + = lport_lookup_by_name(sbrec_port_binding_by_name, > > > gw_port); > > > + if (pb && !smap_get_bool(&pb->options, > "disable_garp_rarp", > > > + false)) { > > > + send_garp_rarp_update(ovnsb_idl_txn, > > > + sbrec_mac_binding_by_lport_ip, > > > + local_datapaths, pb, > > > + &nat_addresses->nat_addresses, > > > + garp_max_timeout, > garp_continuous); > > > + } > > > + } > > > > > > - SHASH_FOR_EACH_SAFE (iter, &nat_addresses) { > > > - struct lport_addresses *laddrs = iter->data; > > > - destroy_lport_addresses(laddrs); > > > - shash_delete(&nat_addresses, iter); > > > - free(laddrs); > > > + /* pinctrl_handler thread will send the GARPs. */ > > > + garp_rarp_max_timeout = garp_max_timeout; > > > + garp_rarp_continuous = garp_continuous; > > > } > > > - shash_destroy(&nat_addresses); > > > - > > > - sset_destroy(&nat_ip_keys); > > > - > > > - garp_rarp_max_timeout = garp_max_timeout; > > > - garp_rarp_continuous = garp_continuous; > > > } > > > > > > static bool > > > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > > > index 846afe0a4..c6a7423b8 100644 > > > --- a/controller/pinctrl.h > > > +++ b/controller/pinctrl.h > > > @@ -20,6 +20,7 @@ > > > #include <stdint.h> > > > > > > #include "lib/sset.h" > > > +#include "openvswitch/shash.h" > > > #include "openvswitch/list.h" > > > #include "openvswitch/meta-flow.h" > > > > > > @@ -39,10 +40,16 @@ struct sbrec_bfd_table; > > > struct sbrec_port_binding; > > > struct sbrec_mac_binding_table; > > > > > > +struct ed_type_nat_addresses { > > > + struct shash nat_addresses; > > > + struct sset localnet_vifs; > > > + struct sset local_l3gw_ports; > > > + struct sset nat_ip_keys; > > > +}; > > > > > > > > > This shouldn't be part of pinctrl.h as it doesn't have anything to do > with > > pinctrl. In fact all functions that are handling the NAT addresses should > > be in a separate module. > > > > + > > > void pinctrl_init(void); > > > void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > > - struct ovsdb_idl_index > *sbrec_port_binding_by_datapath, > > > struct ovsdb_idl_index *sbrec_port_binding_by_key, > > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > struct ovsdb_idl_index > *sbrec_mac_binding_by_lport_ip, > > > @@ -53,12 +60,13 @@ void pinctrl_run(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > > const struct sbrec_service_monitor_table *, > > > const struct sbrec_mac_binding_table *, > > > const struct sbrec_bfd_table *, > > > - const struct ovsrec_bridge *, const struct > sbrec_chassis > > > *, > > > + const struct sbrec_chassis *, > > > const struct hmap *local_datapaths, > > > const struct sset *active_tunnels, > > > const struct shash *local_active_ports_ipv6_pd, > > > const struct shash *local_active_ports_ras, > > > - const struct ovsrec_open_vswitch_table *ovs_table); > > > + const struct ovsrec_open_vswitch_table *ovs_table, > > > + struct ed_type_nat_addresses *nat_addresses); > > > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > > > void pinctrl_destroy(void); > > > > > > -- > > > 2.43.0 > > > > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > Thanks, > > Ales > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
On Mon, Jan 06, 2025 at 08:46:41AM +0100, Ales Musil wrote: > On Thu, Dec 19, 2024 at 11:54 AM Felix Huettner > <felix.huettner@stackit.cloud> wrote: > > > On Tue, Dec 10, 2024 at 01:04:32PM +0100, Ales Musil wrote: > > > On Thu, Nov 14, 2024 at 11:47 AM Max Lamprecht via dev < > > > ovs-dev@openvswitch.org> wrote: > > > > > > > This patch moves the nat_addresses calculation to an own engine node. > > > > > > > > Previously the nat_address calculation was executed at every > > pinctrl_run > > > > (send_garp_rarp_prepare). This can cause high cpu usage and delayed > > > > actions on gateway-chassis(500 LRPs+). > > > > > > > > perf trace: > > > > - 98.83% main > > > > - 79.69% pinctrl_run > > > > - 77.71% send_garp_rarp_prepare > > > > - 55.49% get_nat_addresses_and_keys > > > > - 36.76% consider_nat_address > > > > - 18.13% lport_lookup_by_name > > > > - 13.58% sset_destroy > > > > > > > > Co-authored-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > > > > Signed-off-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> > > > > Signed-off-by: Max Lamprecht <max.lamprecht@stackit.cloud> > > > > --- > > > > > > > > > > Hello Max, > > > > > > thank you for your patience and sorry for the delay in reviews. I have a > > > couple of comments down below. > > > > > > controller/ovn-controller.c | 341 +++++++++++++++++++++++++++++++- > > > > controller/pinctrl.c | 381 ++++++------------------------------ > > > > controller/pinctrl.h | 14 +- > > > > 3 files changed, 410 insertions(+), 326 deletions(-) > > > > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > > index 98b144699..49bc6d86c 100644 > > > > --- a/controller/ovn-controller.c > > > > +++ b/controller/ovn-controller.c > > > > @@ -3359,6 +3359,319 @@ en_dns_cache_cleanup(void *data OVS_UNUSED) > > > > ovn_dns_cache_destroy(); > > > > } > > > > > > > > +static void * > > > > +en_nat_addresses_init(struct engine_node *node OVS_UNUSED, > > > > + struct engine_arg *arg OVS_UNUSED) > > > > +{ > > > > + struct ed_type_nat_addresses *data = xzalloc(sizeof *data); > > > > > > > > > > nit: No need to zero alloc if you init everything right away. > > > > > > > > > > + shash_init(&data->nat_addresses); > > > > + sset_init(&data->localnet_vifs); > > > > + sset_init(&data->local_l3gw_ports); > > > > + sset_init(&data->nat_ip_keys); > > > > + return data; > > > > +} > > > > + > > > > +/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > > > > + * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > > > > + * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid > > IPv4 > > > > + * or IPv6 address, and stores them in the 'ipv4_addrs' and > > 'ipv6_addrs' > > > > + * fields of 'laddrs'. The logical port name is stored in 'lport'. > > > > + * > > > > + * Returns true if at least 'MAC' is found in 'address', false > > otherwise. > > > > + * > > > > + * The caller must call destroy_lport_addresses() and free(*lport). */ > > > > +static bool > > > > +extract_addresses_with_port(const char *addresses, > > > > + struct lport_addresses *laddrs, > > > > + char **lport) > > > > +{ > > > > + int ofs; > > > > + if (!extract_addresses(addresses, laddrs, &ofs)) { > > > > + return false; > > > > + } else if (!addresses[ofs]) { > > > > + return true; > > > > + } > > > > + > > > > + struct lexer lexer; > > > > + lexer_init(&lexer, addresses + ofs); > > > > + lexer_get(&lexer); > > > > + > > > > + if (lexer.error || lexer.token.type != LEX_T_ID > > > > + || !lexer_match_id(&lexer, "is_chassis_resident")) { > > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > > + VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", > > addresses); > > > > + lexer_destroy(&lexer); > > > > + return true; > > > > + } > > > > + > > > > + if (!lexer_match(&lexer, LEX_T_LPAREN)) { > > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > > + VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > > > > + "'is_chassis_resident' in address '%s'", > > > > addresses); > > > > + lexer_destroy(&lexer); > > > > + return false; > > > > + } > > > > + > > > > + if (lexer.token.type != LEX_T_STRING) { > > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > > + VLOG_INFO_RL(&rl, > > > > + "Syntax error: expecting quoted string after " > > > > + "'is_chassis_resident' in address '%s'", > > addresses); > > > > + lexer_destroy(&lexer); > > > > + return false; > > > > + } > > > > + > > > > + *lport = xstrdup(lexer.token.s); > > > > + > > > > + lexer_get(&lexer); > > > > + if (!lexer_match(&lexer, LEX_T_RPAREN)) { > > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > > + VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted > > > > string in " > > > > + "'is_chassis_resident()' in address '%s'", > > > > + addresses); > > > > + lexer_destroy(&lexer); > > > > + return false; > > > > + } > > > > + > > > > + lexer_destroy(&lexer); > > > > + return true; > > > > +} > > > > + > > > > +static void > > > > +consider_nat_address(struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > > > + const char *nat_address, > > > > + const struct sbrec_port_binding *pb, > > > > + struct sset *nat_address_keys, > > > > + const struct sbrec_chassis *chassis, > > > > + const struct sset *active_tunnels, > > > > + struct shash *nat_addresses) > > > > +{ > > > > + struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > > > > + char *lport = NULL; > > > > + if (!extract_addresses_with_port(nat_address, laddrs, &lport) > > > > + || (!lport && !strcmp(pb->type, "patch")) > > > > + || (lport && !lport_is_chassis_resident( > > > > + sbrec_port_binding_by_name, chassis, > > > > + active_tunnels, lport))) { > > > > + destroy_lport_addresses(laddrs); > > > > + free(laddrs); > > > > + free(lport); > > > > + return; > > > > + } > > > > + free(lport); > > > > + > > > > + int i; > > > > + for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > > > > + char *name = xasprintf("%s-%s", pb->logical_port, > > > > + laddrs->ipv4_addrs[i].addr_s); > > > > + sset_add(nat_address_keys, name); > > > > + free(name); > > > > + } > > > > + if (laddrs->n_ipv4_addrs == 0) { > > > > + char *name = xasprintf("%s-noip", pb->logical_port); > > > > + sset_add(nat_address_keys, name); > > > > + free(name); > > > > + } > > > > + shash_add(nat_addresses, pb->logical_port, laddrs); > > > > +} > > > > + > > > > +static void > > > > +get_nat_addresses_and_keys(struct ovsdb_idl_index > > > > *sbrec_port_binding_by_name, > > > > + struct sset *nat_address_keys, > > > > + struct sset *local_l3gw_ports, > > > > + const struct sbrec_chassis *chassis, > > > > + const struct sset *active_tunnels, > > > > + struct shash *nat_addresses) > > > > +{ > > > > + const char *gw_port; > > > > + SSET_FOR_EACH (gw_port, local_l3gw_ports) { > > > > + const struct sbrec_port_binding *pb; > > > > + > > > > + pb = lport_lookup_by_name(sbrec_port_binding_by_name, > > gw_port); > > > > + if (!pb) { > > > > + continue; > > > > + } > > > > + > > > > + if (pb->n_nat_addresses) { > > > > + for (int i = 0; i < pb->n_nat_addresses; i++) { > > > > + consider_nat_address(sbrec_port_binding_by_name, > > > > + pb->nat_addresses[i], pb, > > > > + nat_address_keys, chassis, > > > > + active_tunnels, > > > > + nat_addresses); > > > > + } > > > > + } else { > > > > + /* Continue to support options:nat-addresses for version > > > > + * upgrade. */ > > > > + const char *nat_addresses_options = smap_get(&pb->options, > > > > + > > "nat-addresses"); > > > > + if (nat_addresses_options) { > > > > + consider_nat_address(sbrec_port_binding_by_name, > > > > + nat_addresses_options, pb, > > > > + nat_address_keys, chassis, > > > > + active_tunnels, > > > > + nat_addresses); > > > > + } > > > > + } > > > > + } > > > > +} > > > > + > > > > +/* Get localnet vifs, local l3gw ports and ofport for localnet patch > > > > ports. */ > > > > +static void > > > > +get_localnet_vifs_l3gwports( > > > > + struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > > > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > > + const struct ovsrec_bridge *br_int, > > > > + const struct sbrec_chassis *chassis, > > > > + const struct hmap *local_datapaths, > > > > + struct sset *localnet_vifs, > > > > + struct sset *local_l3gw_ports) > > > > +{ > > > > + for (int i = 0; i < br_int->n_ports; i++) { > > > > + const struct ovsrec_port *port_rec = br_int->ports[i]; > > > > + if (!strcmp(port_rec->name, br_int->name)) { > > > > + continue; > > > > + } > > > > + const char *tunnel_id = smap_get(&port_rec->external_ids, > > > > + "ovn-chassis-id"); > > > > + if (tunnel_id && > > > > + encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, > > > > NULL)) { > > > > + continue; > > > > + } > > > > + const char *localnet = smap_get(&port_rec->external_ids, > > > > + "ovn-localnet-port"); > > > > + if (localnet) { > > > > + continue; > > > > + } > > > > + for (int j = 0; j < port_rec->n_interfaces; j++) { > > > > + const struct ovsrec_interface *iface_rec = > > > > port_rec->interfaces[j]; > > > > + if (!iface_rec->n_ofport) { > > > > + continue; > > > > + } > > > > + /* Get localnet vif. */ > > > > + const char *iface_id = smap_get(&iface_rec->external_ids, > > > > + "iface-id"); > > > > + if (!iface_id) { > > > > + continue; > > > > + } > > > > + const struct sbrec_port_binding *pb > > > > + = lport_lookup_by_name(sbrec_port_binding_by_name, > > > > iface_id); > > > > + if (!pb || pb->chassis != chassis) { > > > > + continue; > > > > + } > > > > + if (!iface_rec->link_state || > > > > + strcmp(iface_rec->link_state, "up")) { > > > > + continue; > > > > + } > > > > + struct local_datapath *ld > > > > + = get_local_datapath(local_datapaths, > > > > + pb->datapath->tunnel_key); > > > > + if (ld && ld->localnet_port) { > > > > + sset_add(localnet_vifs, iface_id); > > > > + } > > > > + } > > > > + } > > > > + > > > > + struct sbrec_port_binding *target = > > sbrec_port_binding_index_init_row( > > > > + sbrec_port_binding_by_datapath); > > > > + > > > > + const struct local_datapath *ld; > > > > + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > > > > + const struct sbrec_port_binding *pb; > > > > + > > > > + if (!ld->localnet_port) { > > > > + continue; > > > > + } > > > > + > > > > + /* Get l3gw ports. Consider port bindings with type > > "l3gateway" > > > > + * that connect to gateway routers (if local), and consider > > port > > > > + * bindings of type "patch" since they might connect to > > > > + * distributed gateway ports with NAT addresses. */ > > > > + > > > > + sbrec_port_binding_index_set_datapath(target, ld->datapath); > > > > + SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > > > > + > > > > sbrec_port_binding_by_datapath) { > > > > + if ((!strcmp(pb->type, "l3gateway") && pb->chassis == > > chassis) > > > > + || !strcmp(pb->type, "patch")) { > > > > + sset_add(local_l3gw_ports, pb->logical_port); > > > > + } > > > > + } > > > > + } > > > > + sbrec_port_binding_index_destroy_row(target); > > > > +} > > > > + > > > > +static void > > > > +en_nat_addresses_run(struct engine_node *node, void *data OVS_UNUSED) > > > > +{ > > > > + struct ed_type_nat_addresses *nat_addresses = data; > > > > + const struct ovsrec_open_vswitch_table *ovs_table = > > > > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > > > > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > > > > + struct ovsdb_idl_index *sbrec_chassis_by_name = > > > > + engine_ovsdb_node_get_index( > > > > + engine_get_input("SB_chassis", node), > > > > + "name"); > > > > + const struct sbrec_chassis *chassis > > > > + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > > > > + > > > > + struct ovsdb_idl_index *sbrec_pb_by_datapath = > > > > + engine_ovsdb_node_get_index( > > > > + engine_get_input("SB_port_binding", node), > > > > + "datapath"); > > > > + struct ovsdb_idl_index *sbrec_pb_by_name = > > > > + engine_ovsdb_node_get_index( > > > > + engine_get_input("SB_port_binding", node), > > > > + "name"); > > > > + const struct ovsrec_bridge_table *bridge_table = > > > > + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > > > > + const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > > > > ovs_table); > > > > + > > > > + struct ed_type_runtime_data *rt_data = > > > > + engine_get_input_data("runtime_data", node); > > > > + const struct hmap *local_datapaths = &rt_data->local_datapaths; > > > > + const struct sset *active_tunnels = &rt_data->active_tunnels; > > > > + > > > > + struct shash_node *nat_node; > > > > + SHASH_FOR_EACH (nat_node, &nat_addresses->nat_addresses) { > > > > + struct lport_addresses *laddrs = nat_node->data; > > > > + destroy_lport_addresses(laddrs); > > > > + free(laddrs); > > > > + } > > > > + shash_clear(&nat_addresses->nat_addresses); > > > > + sset_clear(&nat_addresses->localnet_vifs); > > > > + sset_clear(&nat_addresses->local_l3gw_ports); > > > > + sset_clear(&nat_addresses->nat_ip_keys); > > > > + > > > > + get_localnet_vifs_l3gwports(sbrec_pb_by_datapath, > > > > + sbrec_pb_by_name, > > > > + br_int, chassis, local_datapaths, > > > > + &nat_addresses->localnet_vifs, > > > > + &nat_addresses->local_l3gw_ports); > > > > + > > > > + get_nat_addresses_and_keys(sbrec_pb_by_name, > > > > + &nat_addresses->nat_ip_keys, > > > > + &nat_addresses->local_l3gw_ports, > > > > + chassis, active_tunnels, > > > > + &nat_addresses->nat_addresses); > > > > + > > > > + engine_set_node_state(node, EN_UPDATED); > > > > +} > > > > + > > > > +static bool > > > > +nat_addresses_change_handler(struct engine_node *node, void *data) > > > > +{ > > > > + en_nat_addresses_run(node, data); > > > > + return true; > > > > +} > > > > > > > > > > There is no need for separate handler, engine will always run the > > > "<NODE>_run()" function if handler is NULL. > > > > Hi Ales, > > > > Hi Felix, > > > > i took a look with Max on this. > > > > The reason this handler exists is to support cases where the southbound > > db is not accessible. > > In these cases we do not allow recomputes on the engine to happen. > > > > Not having this handler would mean that the engine run gets canceled if > > the southbound is unaccessible and e.g. a port is locally added or > > active_tunnels changes or whatever else changes "runtime_data". > > > > > Adding the handler here allows the engine to process changes by > > "runtime_data" without canceling the run and thereby being able to > > continue processing changes incrementally. > > > > > An option i would see to make this nicer would be to make the decission > > if we can recompute without sb on a per engine node basis. However i am > > not sure how good of an idea this is. > > > > Would you see other options to solve this? > > Hi Ales, > > Ok I see why you would want to do that, however the question > is how often do you expect to need the "incremental" processing > for this specific node to work? When SB disconnects you can still > do the full run for this node, I suppose that's fine. But once controller > reconnects we will do full recompute anyway with new data. I guess this results in the question if we need to send garps for newly claimed ports/tunnel changes while the connection to SB is gone? As long as we want to keep compatibility with the existing logic the answer would be Yes. Since tunnel change would also include DGP failovers if a gateway chassis fails i also would not like to depend on the sb connection. In these cases we need to send a garp if we are connected to a localnet port. So i guess waiting for a reconnect is not desirable. > > So there essentially two things to consider: > > 1) How often do you expect the SB disconnect to happen? IMO in > normal working conditions that should be as low as possible I think we > can agree on that. > > 2) How long does it take for controller to reconnect and do full recompute? > > This change is minimal however it has this strange side effect that wasn't > intended by the engine API. If we can agree that the need for I-P during > disconnect is minimal. I would suggest following the normal procedure > of the I-P engine. If there is really a need for this to work during SB > disconnect we should find a way to do it in a more explicit way rather than > bending the existing API. Or even better, proper handler for runtime data > and port binding. I took a look at the needed change handlers. I guess the most tricky part would be the active_tunnels from rt_data as they have currently neither incremental processing for changes to br-int nor actual change tracking for these things. The alternative i would see is to add a bool to "struct engine_node" to define if we can always recompute this engine node. That should be the case if the following are true: * the engine node does not depend on a txn to SB, readonly access is fine * AND the engine node does not depend on ofctrl messages A connection to vswitchd itself is acceptable as otherwise engine_run is not even called. If this is true then "engine_recompute" could skip the "allowed" check for such nodes. Actually if i understand en-runtime_data correctly this option could also be used there. That would allow it to also work if the local OpenVSwitch table changes while the SB is not connected (even though that is probably unlikely). So maybe this is generally a helpful option to allow always recomputes in certain cases. I would be realy interested in opinions on that. Thanks a lot Felix > > WDYT? > > > > > > > Thanks a lot > > Felix > > > > > > > > + > > > > +static void > > > > +en_nat_addresses_cleanup(void *data OVS_UNUSED){ > > > > + struct ed_type_nat_addresses *nat_addresses = data; > > > > + shash_destroy(&nat_addresses->nat_addresses); > > > > > > > > > > We are leaking all the "struct lport_addresses" stored within this shash. > > > > > > > > > > + sset_destroy(&nat_addresses->localnet_vifs); > > > > + sset_destroy(&nat_addresses->local_l3gw_ports); > > > > + sset_destroy(&nat_addresses->nat_ip_keys); > > > > +} > > > > > > > > /* Engine node which is used to handle the Non VIF data like > > > > * - OVS patch ports > > > > @@ -4779,6 +5092,14 @@ controller_output_bfd_chassis_handler(struct > > > > engine_node *node, > > > > return true; > > > > } > > > > > > > > +static bool > > > > +controller_output_nat_addresses_handler(struct engine_node *node, > > > > + void *data OVS_UNUSED) > > > > +{ > > > > + engine_set_node_state(node, EN_UPDATED); > > > > + return true; > > > > +} > > > > + > > > > /* Handles sbrec_chassis changes. > > > > * If a new chassis is added or removed return false, so that > > > > * flows are recomputed. For any updates, there is no need for > > > > @@ -5093,6 +5414,7 @@ main(int argc, char *argv[]) > > > > ENGINE_NODE(mac_cache, "mac_cache"); > > > > ENGINE_NODE(bfd_chassis, "bfd_chassis"); > > > > ENGINE_NODE(dns_cache, "dns_cache"); > > > > + ENGINE_NODE(nat_addresses, "nat_addresses"); > > > > > > > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > > > > SB_NODES > > > > @@ -5137,6 +5459,14 @@ main(int argc, char *argv[]) > > > > engine_add_input(&en_bfd_chassis, &en_sb_chassis, NULL); > > > > engine_add_input(&en_bfd_chassis, &en_sb_ha_chassis_group, NULL); > > > > > > > > + engine_add_input(&en_nat_addresses, &en_ovs_open_vswitch, NULL); > > > > + engine_add_input(&en_nat_addresses, &en_ovs_bridge, NULL); > > > > + engine_add_input(&en_nat_addresses, &en_sb_chassis, NULL); > > > > + engine_add_input(&en_nat_addresses, &en_sb_port_binding, > > > > + nat_addresses_change_handler); > > > > + engine_add_input(&en_nat_addresses, &en_runtime_data, > > > > + nat_addresses_change_handler); > > > > + > > > > /* Note: The order of inputs is important, all OVS interface > > changes > > > > must > > > > * be handled before any ct_zone changes. > > > > */ > > > > @@ -5300,6 +5630,8 @@ main(int argc, char *argv[]) > > > > controller_output_mac_cache_handler); > > > > engine_add_input(&en_controller_output, &en_bfd_chassis, > > > > controller_output_bfd_chassis_handler); > > > > + engine_add_input(&en_controller_output, &en_nat_addresses, > > > > + controller_output_nat_addresses_handler); > > > > > > > > struct engine_arg engine_arg = { > > > > .sb_idl = ovnsb_idl_loop.idl, > > > > @@ -5346,6 +5678,8 @@ main(int argc, char *argv[]) > > > > engine_get_internal_data(&en_ct_zones); > > > > struct ed_type_bfd_chassis *bfd_chassis_data = > > > > engine_get_internal_data(&en_bfd_chassis); > > > > + struct ed_type_nat_addresses *nat_addresses_data = > > > > + engine_get_internal_data(&en_nat_addresses); > > > > struct ed_type_runtime_data *runtime_data = > > > > engine_get_internal_data(&en_runtime_data); > > > > struct ed_type_template_vars *template_vars_data = > > > > @@ -5682,6 +6016,7 @@ main(int argc, char *argv[]) > > > > } > > > > > > > > runtime_data = engine_get_data(&en_runtime_data); > > > > + nat_addresses_data = > > > > engine_get_data(&en_nat_addresses); > > > > if (runtime_data) { > > > > stopwatch_start(PATCH_RUN_STOPWATCH_NAME, > > > > time_msec()); > > > > patch_run(ovs_idl_txn, > > > > @@ -5729,7 +6064,6 @@ main(int argc, char *argv[]) > > > > pinctrl_update(ovnsb_idl_loop.idl); > > > > pinctrl_run(ovnsb_idl_txn, > > > > sbrec_datapath_binding_by_key, > > > > - sbrec_port_binding_by_datapath, > > > > sbrec_port_binding_by_key, > > > > sbrec_port_binding_by_name, > > > > sbrec_mac_binding_by_lport_ip, > > > > @@ -5743,13 +6077,14 @@ main(int argc, char *argv[]) > > > > sbrec_mac_binding_table_get( > > > > ovnsb_idl_loop.idl), > > > > > > > > sbrec_bfd_table_get(ovnsb_idl_loop.idl), > > > > - br_int, chassis, > > > > + chassis, > > > > &runtime_data->local_datapaths, > > > > &runtime_data->active_tunnels, > > > > > > > > &runtime_data->local_active_ports_ipv6_pd, > > > > > > &runtime_data->local_active_ports_ras, > > > > ovsrec_open_vswitch_table_get( > > > > - ovs_idl_loop.idl)); > > > > + ovs_idl_loop.idl), > > > > + nat_addresses_data); > > > > stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME, > > > > time_msec()); > > > > mirror_run(ovs_idl_txn, > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > > > index 3fb7e2fd7..e8c4ff77b 100644 > > > > --- a/controller/pinctrl.c > > > > +++ b/controller/pinctrl.c > > > > @@ -227,14 +227,11 @@ static void destroy_send_garps_rarps(void); > > > > static void send_garp_rarp_wait(long long int send_garp_rarp_time); > > > > static void send_garp_rarp_prepare( > > > > struct ovsdb_idl_txn *ovnsb_idl_txn, > > > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > > > > - const struct ovsrec_bridge *, > > > > - const struct sbrec_chassis *, > > > > const struct hmap *local_datapaths, > > > > - const struct sset *active_tunnels, > > > > - const struct ovsrec_open_vswitch_table *ovs_table) > > > > + const struct ovsrec_open_vswitch_table *ovs_table, > > > > + struct ed_type_nat_addresses *nat_addresses) > > > > OVS_REQUIRES(pinctrl_mutex); > > > > static void send_garp_rarp_run(struct rconn *swconn, > > > > long long int *send_garp_rarp_time) > > > > @@ -4008,7 +4005,6 @@ pinctrl_update(const struct ovsdb_idl *idl) > > > > void > > > > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > > struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > > > struct ovsdb_idl_index *sbrec_port_binding_by_key, > > > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > > struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, > > > > @@ -4019,13 +4015,13 @@ pinctrl_run(struct ovsdb_idl_txn > > *ovnsb_idl_txn, > > > > const struct sbrec_service_monitor_table *svc_mon_table, > > > > const struct sbrec_mac_binding_table *mac_binding_table, > > > > const struct sbrec_bfd_table *bfd_table, > > > > - const struct ovsrec_bridge *br_int, > > > > const struct sbrec_chassis *chassis, > > > > const struct hmap *local_datapaths, > > > > const struct sset *active_tunnels, > > > > const struct shash *local_active_ports_ipv6_pd, > > > > const struct shash *local_active_ports_ras, > > > > - const struct ovsrec_open_vswitch_table *ovs_table) > > > > + const struct ovsrec_open_vswitch_table *ovs_table, > > > > + struct ed_type_nat_addresses *nat_addresses) > > > > { > > > > ovs_mutex_lock(&pinctrl_mutex); > > > > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > > > > @@ -4033,10 +4029,11 @@ pinctrl_run(struct ovsdb_idl_txn > > *ovnsb_idl_txn, > > > > sbrec_mac_binding_by_lport_ip); > > > > run_put_vport_bindings(ovnsb_idl_txn, > > sbrec_datapath_binding_by_key, > > > > sbrec_port_binding_by_key, chassis); > > > > - send_garp_rarp_prepare(ovnsb_idl_txn, > > sbrec_port_binding_by_datapath, > > > > + send_garp_rarp_prepare(ovnsb_idl_txn, > > > > sbrec_port_binding_by_name, > > > > - sbrec_mac_binding_by_lport_ip, br_int, > > chassis, > > > > - local_datapaths, active_tunnels, > > ovs_table); > > > > + sbrec_mac_binding_by_lport_ip, > > > > + local_datapaths, > > > > + ovs_table, nat_addresses); > > > > prepare_ipv6_ras(local_active_ports_ras, > > sbrec_port_binding_by_name); > > > > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > > > > local_active_ports_ipv6_pd, chassis, > > > > @@ -6175,236 +6172,6 @@ ip_mcast_querier_wait(long long int query_time) > > > > } > > > > } > > > > > > > > -/* Get localnet vifs, local l3gw ports and ofport for localnet patch > > > > ports. */ > > > > -static void > > > > -get_localnet_vifs_l3gwports( > > > > - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > > > - struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > > - const struct ovsrec_bridge *br_int, > > > > - const struct sbrec_chassis *chassis, > > > > - const struct hmap *local_datapaths, > > > > - struct sset *localnet_vifs, > > > > - struct sset *local_l3gw_ports) > > > > -{ > > > > - for (int i = 0; i < br_int->n_ports; i++) { > > > > - const struct ovsrec_port *port_rec = br_int->ports[i]; > > > > - if (!strcmp(port_rec->name, br_int->name)) { > > > > - continue; > > > > - } > > > > - const char *tunnel_id = smap_get(&port_rec->external_ids, > > > > - "ovn-chassis-id"); > > > > - if (tunnel_id && > > > > - encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, > > > > NULL)) { > > > > - continue; > > > > - } > > > > - const char *localnet = smap_get(&port_rec->external_ids, > > > > - "ovn-localnet-port"); > > > > - if (localnet) { > > > > - continue; > > > > - } > > > > - for (int j = 0; j < port_rec->n_interfaces; j++) { > > > > - const struct ovsrec_interface *iface_rec = > > > > port_rec->interfaces[j]; > > > > - if (!iface_rec->n_ofport) { > > > > - continue; > > > > - } > > > > - /* Get localnet vif. */ > > > > - const char *iface_id = smap_get(&iface_rec->external_ids, > > > > - "iface-id"); > > > > - if (!iface_id) { > > > > - continue; > > > > - } > > > > - const struct sbrec_port_binding *pb > > > > - = lport_lookup_by_name(sbrec_port_binding_by_name, > > > > iface_id); > > > > - if (!pb || pb->chassis != chassis) { > > > > - continue; > > > > - } > > > > - if (!iface_rec->link_state || > > > > - strcmp(iface_rec->link_state, "up")) { > > > > - continue; > > > > - } > > > > - struct local_datapath *ld > > > > - = get_local_datapath(local_datapaths, > > > > - pb->datapath->tunnel_key); > > > > - if (ld && ld->localnet_port) { > > > > - sset_add(localnet_vifs, iface_id); > > > > - } > > > > - } > > > > - } > > > > - > > > > - struct sbrec_port_binding *target = > > sbrec_port_binding_index_init_row( > > > > - sbrec_port_binding_by_datapath); > > > > - > > > > - const struct local_datapath *ld; > > > > - HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > > > > - const struct sbrec_port_binding *pb; > > > > - > > > > - if (!ld->localnet_port) { > > > > - continue; > > > > - } > > > > - > > > > - /* Get l3gw ports. Consider port bindings with type > > "l3gateway" > > > > - * that connect to gateway routers (if local), and consider > > port > > > > - * bindings of type "patch" since they might connect to > > > > - * distributed gateway ports with NAT addresses. */ > > > > - > > > > - sbrec_port_binding_index_set_datapath(target, ld->datapath); > > > > - SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > > > > - > > > > sbrec_port_binding_by_datapath) { > > > > - if ((!strcmp(pb->type, "l3gateway") && pb->chassis == > > chassis) > > > > - || !strcmp(pb->type, "patch")) { > > > > - sset_add(local_l3gw_ports, pb->logical_port); > > > > - } > > > > - } > > > > - } > > > > - sbrec_port_binding_index_destroy_row(target); > > > > -} > > > > - > > > > - > > > > -/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from > > > > - * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] > > > > - * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid > > IPv4 > > > > - * or IPv6 address, and stores them in the 'ipv4_addrs' and > > 'ipv6_addrs' > > > > - * fields of 'laddrs'. The logical port name is stored in 'lport'. > > > > - * > > > > - * Returns true if at least 'MAC' is found in 'address', false > > otherwise. > > > > - * > > > > - * The caller must call destroy_lport_addresses() and free(*lport). */ > > > > -static bool > > > > -extract_addresses_with_port(const char *addresses, > > > > - struct lport_addresses *laddrs, > > > > - char **lport) > > > > -{ > > > > - int ofs; > > > > - if (!extract_addresses(addresses, laddrs, &ofs)) { > > > > - return false; > > > > - } else if (!addresses[ofs]) { > > > > - return true; > > > > - } > > > > - > > > > - struct lexer lexer; > > > > - lexer_init(&lexer, addresses + ofs); > > > > - lexer_get(&lexer); > > > > - > > > > - if (lexer.error || lexer.token.type != LEX_T_ID > > > > - || !lexer_match_id(&lexer, "is_chassis_resident")) { > > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > > - VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", > > addresses); > > > > - lexer_destroy(&lexer); > > > > - return true; > > > > - } > > > > - > > > > - if (!lexer_match(&lexer, LEX_T_LPAREN)) { > > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > > - VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " > > > > - "'is_chassis_resident' in address '%s'", > > > > addresses); > > > > - lexer_destroy(&lexer); > > > > - return false; > > > > - } > > > > - > > > > - if (lexer.token.type != LEX_T_STRING) { > > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > > - VLOG_INFO_RL(&rl, > > > > - "Syntax error: expecting quoted string after " > > > > - "'is_chassis_resident' in address '%s'", > > addresses); > > > > - lexer_destroy(&lexer); > > > > - return false; > > > > - } > > > > - > > > > - *lport = xstrdup(lexer.token.s); > > > > - > > > > - lexer_get(&lexer); > > > > - if (!lexer_match(&lexer, LEX_T_RPAREN)) { > > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > > > - VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted > > > > string in " > > > > - "'is_chassis_resident()' in address '%s'", > > > > - addresses); > > > > - lexer_destroy(&lexer); > > > > - return false; > > > > - } > > > > - > > > > - lexer_destroy(&lexer); > > > > - return true; > > > > -} > > > > - > > > > -static void > > > > -consider_nat_address(struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > > > - const char *nat_address, > > > > - const struct sbrec_port_binding *pb, > > > > - struct sset *nat_address_keys, > > > > - const struct sbrec_chassis *chassis, > > > > - const struct sset *active_tunnels, > > > > - struct shash *nat_addresses) > > > > -{ > > > > - struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); > > > > - char *lport = NULL; > > > > - if (!extract_addresses_with_port(nat_address, laddrs, &lport) > > > > - || (!lport && !strcmp(pb->type, "patch")) > > > > - || (lport && !lport_is_chassis_resident( > > > > - sbrec_port_binding_by_name, chassis, > > > > - active_tunnels, lport))) { > > > > - destroy_lport_addresses(laddrs); > > > > - free(laddrs); > > > > - free(lport); > > > > - return; > > > > - } > > > > - free(lport); > > > > - > > > > - int i; > > > > - for (i = 0; i < laddrs->n_ipv4_addrs; i++) { > > > > - char *name = xasprintf("%s-%s", pb->logical_port, > > > > - laddrs->ipv4_addrs[i].addr_s); > > > > - sset_add(nat_address_keys, name); > > > > - free(name); > > > > - } > > > > - if (laddrs->n_ipv4_addrs == 0) { > > > > - char *name = xasprintf("%s-noip", pb->logical_port); > > > > - sset_add(nat_address_keys, name); > > > > - free(name); > > > > - } > > > > - shash_add(nat_addresses, pb->logical_port, laddrs); > > > > -} > > > > - > > > > -static void > > > > -get_nat_addresses_and_keys(struct ovsdb_idl_index > > > > *sbrec_port_binding_by_name, > > > > - struct sset *nat_address_keys, > > > > - struct sset *local_l3gw_ports, > > > > - const struct sbrec_chassis *chassis, > > > > - const struct sset *active_tunnels, > > > > - struct shash *nat_addresses) > > > > -{ > > > > - const char *gw_port; > > > > - SSET_FOR_EACH(gw_port, local_l3gw_ports) { > > > > - const struct sbrec_port_binding *pb; > > > > - > > > > - pb = lport_lookup_by_name(sbrec_port_binding_by_name, > > gw_port); > > > > - if (!pb) { > > > > - continue; > > > > - } > > > > - > > > > - if (pb->n_nat_addresses) { > > > > - for (int i = 0; i < pb->n_nat_addresses; i++) { > > > > - consider_nat_address(sbrec_port_binding_by_name, > > > > - pb->nat_addresses[i], pb, > > > > - nat_address_keys, chassis, > > > > - active_tunnels, > > > > - nat_addresses); > > > > - } > > > > - } else { > > > > - /* Continue to support options:nat-addresses for version > > > > - * upgrade. */ > > > > - const char *nat_addresses_options = smap_get(&pb->options, > > > > - > > "nat-addresses"); > > > > - if (nat_addresses_options) { > > > > - consider_nat_address(sbrec_port_binding_by_name, > > > > - nat_addresses_options, pb, > > > > - nat_address_keys, chassis, > > > > - active_tunnels, > > > > - nat_addresses); > > > > - } > > > > - } > > > > - } > > > > -} > > > > - > > > > static void > > > > send_garp_rarp_wait(long long int send_garp_rarp_time) > > > > { > > > > @@ -6441,96 +6208,70 @@ send_garp_rarp_run(struct rconn *swconn, long > > long > > > > int *send_garp_rarp_time) > > > > * thread context. */ > > > > static void > > > > send_garp_rarp_prepare(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > > - struct ovsdb_idl_index > > > > *sbrec_port_binding_by_datapath, > > > > struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > > > struct ovsdb_idl_index > > > > *sbrec_mac_binding_by_lport_ip, > > > > - const struct ovsrec_bridge *br_int, > > > > - const struct sbrec_chassis *chassis, > > > > const struct hmap *local_datapaths, > > > > - const struct sset *active_tunnels, > > > > - const struct ovsrec_open_vswitch_table > > *ovs_table) > > > > + const struct ovsrec_open_vswitch_table > > *ovs_table, > > > > + struct ed_type_nat_addresses *nat_addresses) > > > > OVS_REQUIRES(pinctrl_mutex) > > > > { > > > > - struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs); > > > > - struct sset local_l3gw_ports = > > SSET_INITIALIZER(&local_l3gw_ports); > > > > - struct sset nat_ip_keys = SSET_INITIALIZER(&nat_ip_keys); > > > > - struct shash nat_addresses; > > > > - unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > > > - bool garp_continuous = false; > > > > - const struct ovsrec_open_vswitch *cfg = > > > > - ovsrec_open_vswitch_table_first(ovs_table); > > > > - if (cfg) { > > > > - garp_max_timeout = smap_get_ullong( > > > > - &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; > > > > - garp_continuous = !!garp_max_timeout; > > > > - if (!garp_max_timeout) { > > > > - garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > > > - } > > > > - } > > > > - > > > > - shash_init(&nat_addresses); > > > > - > > > > - get_localnet_vifs_l3gwports(sbrec_port_binding_by_datapath, > > > > - sbrec_port_binding_by_name, > > > > - br_int, chassis, local_datapaths, > > > > - &localnet_vifs, &local_l3gw_ports); > > > > - > > > > - get_nat_addresses_and_keys(sbrec_port_binding_by_name, > > > > - &nat_ip_keys, &local_l3gw_ports, > > > > - chassis, active_tunnels, > > > > - &nat_addresses); > > > > - /* For deleted ports and deleted nat ips, remove from > > > > - * send_garp_rarp_data. */ > > > > - struct shash_node *iter; > > > > - SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > > > > - if (!sset_contains(&localnet_vifs, iter->name) && > > > > - !sset_contains(&nat_ip_keys, iter->name)) { > > > > - send_garp_rarp_delete(iter->name); > > > > + if (nat_addresses) { > > > > + unsigned long long garp_max_timeout = > > GARP_RARP_DEF_MAX_TIMEOUT; > > > > + bool garp_continuous = false; > > > > + > > > > + const struct ovsrec_open_vswitch *cfg = > > > > + ovsrec_open_vswitch_table_first(ovs_table); > > > > + if (cfg) { > > > > + garp_max_timeout = smap_get_ullong( > > > > + &cfg->external_ids, "garp-max-timeout-sec", 0) * > > 1000; > > > > + garp_continuous = !!garp_max_timeout; > > > > + if (!garp_max_timeout) { > > > > + garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; > > > > + } > > > > } > > > > - } > > > > - > > > > - /* Update send_garp_rarp_data. */ > > > > - const char *iface_id; > > > > - SSET_FOR_EACH (iface_id, &localnet_vifs) { > > > > - const struct sbrec_port_binding *pb = lport_lookup_by_name( > > > > - sbrec_port_binding_by_name, iface_id); > > > > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > > > > false)) { > > > > - send_garp_rarp_update(ovnsb_idl_txn, > > > > - sbrec_mac_binding_by_lport_ip, > > > > - local_datapaths, pb, &nat_addresses, > > > > - garp_max_timeout, garp_continuous); > > > > + /* For deleted ports and deleted nat ips, remove from > > > > + * send_garp_rarp_data. */ > > > > + struct shash_node *iter; > > > > + SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { > > > > + if (!sset_contains(&nat_addresses->localnet_vifs, > > iter->name) > > > > && > > > > + !sset_contains(&nat_addresses->nat_ip_keys, > > iter->name)) { > > > > + send_garp_rarp_delete(iter->name); > > > > + } > > > > } > > > > - } > > > > - > > > > - /* Update send_garp_rarp_data for nat-addresses. */ > > > > - const char *gw_port; > > > > - SSET_FOR_EACH (gw_port, &local_l3gw_ports) { > > > > - const struct sbrec_port_binding *pb > > > > - = lport_lookup_by_name(sbrec_port_binding_by_name, > > gw_port); > > > > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", > > > > false)) { > > > > - send_garp_rarp_update(ovnsb_idl_txn, > > > > sbrec_mac_binding_by_lport_ip, > > > > - local_datapaths, pb, &nat_addresses, > > > > - garp_max_timeout, garp_continuous); > > > > + /* Update send_garp_rarp_data. */ > > > > + const char *iface_id; > > > > + SSET_FOR_EACH (iface_id, &nat_addresses->localnet_vifs) { > > > > + const struct sbrec_port_binding *pb = > > lport_lookup_by_name( > > > > + sbrec_port_binding_by_name, iface_id); > > > > + if (pb && !smap_get_bool( > > > > + &pb->options, "disable_garp_rarp", false)) { > > > > + send_garp_rarp_update(ovnsb_idl_txn, > > > > + sbrec_mac_binding_by_lport_ip, > > > > + local_datapaths, pb, > > > > + &nat_addresses->nat_addresses, > > > > + garp_max_timeout, > > garp_continuous); > > > > + } > > > > } > > > > - } > > > > > > > > - /* pinctrl_handler thread will send the GARPs. */ > > > > - > > > > - sset_destroy(&localnet_vifs); > > > > - sset_destroy(&local_l3gw_ports); > > > > + /* Update send_garp_rarp_data for nat-addresses. */ > > > > + const char *gw_port; > > > > + SSET_FOR_EACH (gw_port, &nat_addresses->local_l3gw_ports) { > > > > + const struct sbrec_port_binding *pb > > > > + = lport_lookup_by_name(sbrec_port_binding_by_name, > > > > gw_port); > > > > + if (pb && !smap_get_bool(&pb->options, > > "disable_garp_rarp", > > > > + false)) { > > > > + send_garp_rarp_update(ovnsb_idl_txn, > > > > + sbrec_mac_binding_by_lport_ip, > > > > + local_datapaths, pb, > > > > + &nat_addresses->nat_addresses, > > > > + garp_max_timeout, > > garp_continuous); > > > > + } > > > > + } > > > > > > > > - SHASH_FOR_EACH_SAFE (iter, &nat_addresses) { > > > > - struct lport_addresses *laddrs = iter->data; > > > > - destroy_lport_addresses(laddrs); > > > > - shash_delete(&nat_addresses, iter); > > > > - free(laddrs); > > > > + /* pinctrl_handler thread will send the GARPs. */ > > > > + garp_rarp_max_timeout = garp_max_timeout; > > > > + garp_rarp_continuous = garp_continuous; > > > > } > > > > - shash_destroy(&nat_addresses); > > > > - > > > > - sset_destroy(&nat_ip_keys); > > > > - > > > > - garp_rarp_max_timeout = garp_max_timeout; > > > > - garp_rarp_continuous = garp_continuous; > > > > } > > > > > > > > static bool > > > > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > > > > index 846afe0a4..c6a7423b8 100644 > > > > --- a/controller/pinctrl.h > > > > +++ b/controller/pinctrl.h > > > > @@ -20,6 +20,7 @@ > > > > #include <stdint.h> > > > > > > > > #include "lib/sset.h" > > > > +#include "openvswitch/shash.h" > > > > #include "openvswitch/list.h" > > > > #include "openvswitch/meta-flow.h" > > > > > > > > @@ -39,10 +40,16 @@ struct sbrec_bfd_table; > > > > struct sbrec_port_binding; > > > > struct sbrec_mac_binding_table; > > > > > > > > +struct ed_type_nat_addresses { > > > > + struct shash nat_addresses; > > > > + struct sset localnet_vifs; > > > > + struct sset local_l3gw_ports; > > > > + struct sset nat_ip_keys; > > > > +}; > > > > > > > > > > > > > This shouldn't be part of pinctrl.h as it doesn't have anything to do > > with > > > pinctrl. In fact all functions that are handling the NAT addresses should > > > be in a separate module. > > > > > > + > > > > void pinctrl_init(void); > > > > void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > > struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > > - struct ovsdb_idl_index > > *sbrec_port_binding_by_datapath, > > > > struct ovsdb_idl_index *sbrec_port_binding_by_key, > > > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > > struct ovsdb_idl_index > > *sbrec_mac_binding_by_lport_ip, > > > > @@ -53,12 +60,13 @@ void pinctrl_run(struct ovsdb_idl_txn > > *ovnsb_idl_txn, > > > > const struct sbrec_service_monitor_table *, > > > > const struct sbrec_mac_binding_table *, > > > > const struct sbrec_bfd_table *, > > > > - const struct ovsrec_bridge *, const struct > > sbrec_chassis > > > > *, > > > > + const struct sbrec_chassis *, > > > > const struct hmap *local_datapaths, > > > > const struct sset *active_tunnels, > > > > const struct shash *local_active_ports_ipv6_pd, > > > > const struct shash *local_active_ports_ras, > > > > - const struct ovsrec_open_vswitch_table *ovs_table); > > > > + const struct ovsrec_open_vswitch_table *ovs_table, > > > > + struct ed_type_nat_addresses *nat_addresses); > > > > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > > > > void pinctrl_destroy(void); > > > > > > > > -- > > > > 2.43.0 > > > > > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > > Thanks, > > > Ales > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > Thanks, > Ales > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 98b144699..49bc6d86c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3359,6 +3359,319 @@ en_dns_cache_cleanup(void *data OVS_UNUSED) ovn_dns_cache_destroy(); } +static void * +en_nat_addresses_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + struct ed_type_nat_addresses *data = xzalloc(sizeof *data); + shash_init(&data->nat_addresses); + sset_init(&data->localnet_vifs); + sset_init(&data->local_l3gw_ports); + sset_init(&data->nat_ip_keys); + return data; +} + +/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from + * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] + * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid IPv4 + * or IPv6 address, and stores them in the 'ipv4_addrs' and 'ipv6_addrs' + * fields of 'laddrs'. The logical port name is stored in 'lport'. + * + * Returns true if at least 'MAC' is found in 'address', false otherwise. + * + * The caller must call destroy_lport_addresses() and free(*lport). */ +static bool +extract_addresses_with_port(const char *addresses, + struct lport_addresses *laddrs, + char **lport) +{ + int ofs; + if (!extract_addresses(addresses, laddrs, &ofs)) { + return false; + } else if (!addresses[ofs]) { + return true; + } + + struct lexer lexer; + lexer_init(&lexer, addresses + ofs); + lexer_get(&lexer); + + if (lexer.error || lexer.token.type != LEX_T_ID + || !lexer_match_id(&lexer, "is_chassis_resident")) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", addresses); + lexer_destroy(&lexer); + return true; + } + + if (!lexer_match(&lexer, LEX_T_LPAREN)) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " + "'is_chassis_resident' in address '%s'", addresses); + lexer_destroy(&lexer); + return false; + } + + if (lexer.token.type != LEX_T_STRING) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_INFO_RL(&rl, + "Syntax error: expecting quoted string after " + "'is_chassis_resident' in address '%s'", addresses); + lexer_destroy(&lexer); + return false; + } + + *lport = xstrdup(lexer.token.s); + + lexer_get(&lexer); + if (!lexer_match(&lexer, LEX_T_RPAREN)) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted string in " + "'is_chassis_resident()' in address '%s'", + addresses); + lexer_destroy(&lexer); + return false; + } + + lexer_destroy(&lexer); + return true; +} + +static void +consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name, + const char *nat_address, + const struct sbrec_port_binding *pb, + struct sset *nat_address_keys, + const struct sbrec_chassis *chassis, + const struct sset *active_tunnels, + struct shash *nat_addresses) +{ + struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); + char *lport = NULL; + if (!extract_addresses_with_port(nat_address, laddrs, &lport) + || (!lport && !strcmp(pb->type, "patch")) + || (lport && !lport_is_chassis_resident( + sbrec_port_binding_by_name, chassis, + active_tunnels, lport))) { + destroy_lport_addresses(laddrs); + free(laddrs); + free(lport); + return; + } + free(lport); + + int i; + for (i = 0; i < laddrs->n_ipv4_addrs; i++) { + char *name = xasprintf("%s-%s", pb->logical_port, + laddrs->ipv4_addrs[i].addr_s); + sset_add(nat_address_keys, name); + free(name); + } + if (laddrs->n_ipv4_addrs == 0) { + char *name = xasprintf("%s-noip", pb->logical_port); + sset_add(nat_address_keys, name); + free(name); + } + shash_add(nat_addresses, pb->logical_port, laddrs); +} + +static void +get_nat_addresses_and_keys(struct ovsdb_idl_index *sbrec_port_binding_by_name, + struct sset *nat_address_keys, + struct sset *local_l3gw_ports, + const struct sbrec_chassis *chassis, + const struct sset *active_tunnels, + struct shash *nat_addresses) +{ + const char *gw_port; + SSET_FOR_EACH (gw_port, local_l3gw_ports) { + const struct sbrec_port_binding *pb; + + pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); + if (!pb) { + continue; + } + + if (pb->n_nat_addresses) { + for (int i = 0; i < pb->n_nat_addresses; i++) { + consider_nat_address(sbrec_port_binding_by_name, + pb->nat_addresses[i], pb, + nat_address_keys, chassis, + active_tunnels, + nat_addresses); + } + } else { + /* Continue to support options:nat-addresses for version + * upgrade. */ + const char *nat_addresses_options = smap_get(&pb->options, + "nat-addresses"); + if (nat_addresses_options) { + consider_nat_address(sbrec_port_binding_by_name, + nat_addresses_options, pb, + nat_address_keys, chassis, + active_tunnels, + nat_addresses); + } + } + } +} + +/* Get localnet vifs, local l3gw ports and ofport for localnet patch ports. */ +static void +get_localnet_vifs_l3gwports( + struct ovsdb_idl_index *sbrec_port_binding_by_datapath, + struct ovsdb_idl_index *sbrec_port_binding_by_name, + const struct ovsrec_bridge *br_int, + const struct sbrec_chassis *chassis, + const struct hmap *local_datapaths, + struct sset *localnet_vifs, + struct sset *local_l3gw_ports) +{ + for (int i = 0; i < br_int->n_ports; i++) { + const struct ovsrec_port *port_rec = br_int->ports[i]; + if (!strcmp(port_rec->name, br_int->name)) { + continue; + } + const char *tunnel_id = smap_get(&port_rec->external_ids, + "ovn-chassis-id"); + if (tunnel_id && + encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, NULL)) { + continue; + } + const char *localnet = smap_get(&port_rec->external_ids, + "ovn-localnet-port"); + if (localnet) { + continue; + } + for (int j = 0; j < port_rec->n_interfaces; j++) { + const struct ovsrec_interface *iface_rec = port_rec->interfaces[j]; + if (!iface_rec->n_ofport) { + continue; + } + /* Get localnet vif. */ + const char *iface_id = smap_get(&iface_rec->external_ids, + "iface-id"); + if (!iface_id) { + continue; + } + const struct sbrec_port_binding *pb + = lport_lookup_by_name(sbrec_port_binding_by_name, iface_id); + if (!pb || pb->chassis != chassis) { + continue; + } + if (!iface_rec->link_state || + strcmp(iface_rec->link_state, "up")) { + continue; + } + struct local_datapath *ld + = get_local_datapath(local_datapaths, + pb->datapath->tunnel_key); + if (ld && ld->localnet_port) { + sset_add(localnet_vifs, iface_id); + } + } + } + + struct sbrec_port_binding *target = sbrec_port_binding_index_init_row( + sbrec_port_binding_by_datapath); + + const struct local_datapath *ld; + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { + const struct sbrec_port_binding *pb; + + if (!ld->localnet_port) { + continue; + } + + /* Get l3gw ports. Consider port bindings with type "l3gateway" + * that connect to gateway routers (if local), and consider port + * bindings of type "patch" since they might connect to + * distributed gateway ports with NAT addresses. */ + + sbrec_port_binding_index_set_datapath(target, ld->datapath); + SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, + sbrec_port_binding_by_datapath) { + if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) + || !strcmp(pb->type, "patch")) { + sset_add(local_l3gw_ports, pb->logical_port); + } + } + } + sbrec_port_binding_index_destroy_row(target); +} + +static void +en_nat_addresses_run(struct engine_node *node, void *data OVS_UNUSED) +{ + struct ed_type_nat_addresses *nat_addresses = data; + const struct ovsrec_open_vswitch_table *ovs_table = + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); + const char *chassis_id = get_ovs_chassis_id(ovs_table); + struct ovsdb_idl_index *sbrec_chassis_by_name = + engine_ovsdb_node_get_index( + engine_get_input("SB_chassis", node), + "name"); + const struct sbrec_chassis *chassis + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); + + struct ovsdb_idl_index *sbrec_pb_by_datapath = + engine_ovsdb_node_get_index( + engine_get_input("SB_port_binding", node), + "datapath"); + struct ovsdb_idl_index *sbrec_pb_by_name = + engine_ovsdb_node_get_index( + engine_get_input("SB_port_binding", node), + "name"); + const struct ovsrec_bridge_table *bridge_table = + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); + const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table); + + struct ed_type_runtime_data *rt_data = + engine_get_input_data("runtime_data", node); + const struct hmap *local_datapaths = &rt_data->local_datapaths; + const struct sset *active_tunnels = &rt_data->active_tunnels; + + struct shash_node *nat_node; + SHASH_FOR_EACH (nat_node, &nat_addresses->nat_addresses) { + struct lport_addresses *laddrs = nat_node->data; + destroy_lport_addresses(laddrs); + free(laddrs); + } + shash_clear(&nat_addresses->nat_addresses); + sset_clear(&nat_addresses->localnet_vifs); + sset_clear(&nat_addresses->local_l3gw_ports); + sset_clear(&nat_addresses->nat_ip_keys); + + get_localnet_vifs_l3gwports(sbrec_pb_by_datapath, + sbrec_pb_by_name, + br_int, chassis, local_datapaths, + &nat_addresses->localnet_vifs, + &nat_addresses->local_l3gw_ports); + + get_nat_addresses_and_keys(sbrec_pb_by_name, + &nat_addresses->nat_ip_keys, + &nat_addresses->local_l3gw_ports, + chassis, active_tunnels, + &nat_addresses->nat_addresses); + + engine_set_node_state(node, EN_UPDATED); +} + +static bool +nat_addresses_change_handler(struct engine_node *node, void *data) +{ + en_nat_addresses_run(node, data); + return true; +} + +static void +en_nat_addresses_cleanup(void *data OVS_UNUSED){ + struct ed_type_nat_addresses *nat_addresses = data; + shash_destroy(&nat_addresses->nat_addresses); + sset_destroy(&nat_addresses->localnet_vifs); + sset_destroy(&nat_addresses->local_l3gw_ports); + sset_destroy(&nat_addresses->nat_ip_keys); +} /* Engine node which is used to handle the Non VIF data like * - OVS patch ports @@ -4779,6 +5092,14 @@ controller_output_bfd_chassis_handler(struct engine_node *node, return true; } +static bool +controller_output_nat_addresses_handler(struct engine_node *node, + void *data OVS_UNUSED) +{ + engine_set_node_state(node, EN_UPDATED); + return true; +} + /* Handles sbrec_chassis changes. * If a new chassis is added or removed return false, so that * flows are recomputed. For any updates, there is no need for @@ -5093,6 +5414,7 @@ main(int argc, char *argv[]) ENGINE_NODE(mac_cache, "mac_cache"); ENGINE_NODE(bfd_chassis, "bfd_chassis"); ENGINE_NODE(dns_cache, "dns_cache"); + ENGINE_NODE(nat_addresses, "nat_addresses"); #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); SB_NODES @@ -5137,6 +5459,14 @@ main(int argc, char *argv[]) engine_add_input(&en_bfd_chassis, &en_sb_chassis, NULL); engine_add_input(&en_bfd_chassis, &en_sb_ha_chassis_group, NULL); + engine_add_input(&en_nat_addresses, &en_ovs_open_vswitch, NULL); + engine_add_input(&en_nat_addresses, &en_ovs_bridge, NULL); + engine_add_input(&en_nat_addresses, &en_sb_chassis, NULL); + engine_add_input(&en_nat_addresses, &en_sb_port_binding, + nat_addresses_change_handler); + engine_add_input(&en_nat_addresses, &en_runtime_data, + nat_addresses_change_handler); + /* Note: The order of inputs is important, all OVS interface changes must * be handled before any ct_zone changes. */ @@ -5300,6 +5630,8 @@ main(int argc, char *argv[]) controller_output_mac_cache_handler); engine_add_input(&en_controller_output, &en_bfd_chassis, controller_output_bfd_chassis_handler); + engine_add_input(&en_controller_output, &en_nat_addresses, + controller_output_nat_addresses_handler); struct engine_arg engine_arg = { .sb_idl = ovnsb_idl_loop.idl, @@ -5346,6 +5678,8 @@ main(int argc, char *argv[]) engine_get_internal_data(&en_ct_zones); struct ed_type_bfd_chassis *bfd_chassis_data = engine_get_internal_data(&en_bfd_chassis); + struct ed_type_nat_addresses *nat_addresses_data = + engine_get_internal_data(&en_nat_addresses); struct ed_type_runtime_data *runtime_data = engine_get_internal_data(&en_runtime_data); struct ed_type_template_vars *template_vars_data = @@ -5682,6 +6016,7 @@ main(int argc, char *argv[]) } runtime_data = engine_get_data(&en_runtime_data); + nat_addresses_data = engine_get_data(&en_nat_addresses); if (runtime_data) { stopwatch_start(PATCH_RUN_STOPWATCH_NAME, time_msec()); patch_run(ovs_idl_txn, @@ -5729,7 +6064,6 @@ main(int argc, char *argv[]) pinctrl_update(ovnsb_idl_loop.idl); pinctrl_run(ovnsb_idl_txn, sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, sbrec_port_binding_by_key, sbrec_port_binding_by_name, sbrec_mac_binding_by_lport_ip, @@ -5743,13 +6077,14 @@ main(int argc, char *argv[]) sbrec_mac_binding_table_get( ovnsb_idl_loop.idl), sbrec_bfd_table_get(ovnsb_idl_loop.idl), - br_int, chassis, + chassis, &runtime_data->local_datapaths, &runtime_data->active_tunnels, &runtime_data->local_active_ports_ipv6_pd, &runtime_data->local_active_ports_ras, ovsrec_open_vswitch_table_get( - ovs_idl_loop.idl)); + ovs_idl_loop.idl), + nat_addresses_data); stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME, time_msec()); mirror_run(ovs_idl_txn, diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 3fb7e2fd7..e8c4ff77b 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -227,14 +227,11 @@ static void destroy_send_garps_rarps(void); static void send_garp_rarp_wait(long long int send_garp_rarp_time); static void send_garp_rarp_prepare( struct ovsdb_idl_txn *ovnsb_idl_txn, - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_name, struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, - const struct ovsrec_bridge *, - const struct sbrec_chassis *, const struct hmap *local_datapaths, - const struct sset *active_tunnels, - const struct ovsrec_open_vswitch_table *ovs_table) + const struct ovsrec_open_vswitch_table *ovs_table, + struct ed_type_nat_addresses *nat_addresses) OVS_REQUIRES(pinctrl_mutex); static void send_garp_rarp_run(struct rconn *swconn, long long int *send_garp_rarp_time) @@ -4008,7 +4005,6 @@ pinctrl_update(const struct ovsdb_idl *idl) void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, struct ovsdb_idl_index *sbrec_datapath_binding_by_key, - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_name, struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, @@ -4019,13 +4015,13 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_service_monitor_table *svc_mon_table, const struct sbrec_mac_binding_table *mac_binding_table, const struct sbrec_bfd_table *bfd_table, - const struct ovsrec_bridge *br_int, const struct sbrec_chassis *chassis, const struct hmap *local_datapaths, const struct sset *active_tunnels, const struct shash *local_active_ports_ipv6_pd, const struct shash *local_active_ports_ras, - const struct ovsrec_open_vswitch_table *ovs_table) + const struct ovsrec_open_vswitch_table *ovs_table, + struct ed_type_nat_addresses *nat_addresses) { ovs_mutex_lock(&pinctrl_mutex); run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, @@ -4033,10 +4029,11 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip); run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, sbrec_port_binding_by_key, chassis); - send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath, + send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_name, - sbrec_mac_binding_by_lport_ip, br_int, chassis, - local_datapaths, active_tunnels, ovs_table); + sbrec_mac_binding_by_lport_ip, + local_datapaths, + ovs_table, nat_addresses); prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name); prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, local_active_ports_ipv6_pd, chassis, @@ -6175,236 +6172,6 @@ ip_mcast_querier_wait(long long int query_time) } } -/* Get localnet vifs, local l3gw ports and ofport for localnet patch ports. */ -static void -get_localnet_vifs_l3gwports( - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, - struct ovsdb_idl_index *sbrec_port_binding_by_name, - const struct ovsrec_bridge *br_int, - const struct sbrec_chassis *chassis, - const struct hmap *local_datapaths, - struct sset *localnet_vifs, - struct sset *local_l3gw_ports) -{ - for (int i = 0; i < br_int->n_ports; i++) { - const struct ovsrec_port *port_rec = br_int->ports[i]; - if (!strcmp(port_rec->name, br_int->name)) { - continue; - } - const char *tunnel_id = smap_get(&port_rec->external_ids, - "ovn-chassis-id"); - if (tunnel_id && - encaps_tunnel_id_match(tunnel_id, chassis->name, NULL, NULL)) { - continue; - } - const char *localnet = smap_get(&port_rec->external_ids, - "ovn-localnet-port"); - if (localnet) { - continue; - } - for (int j = 0; j < port_rec->n_interfaces; j++) { - const struct ovsrec_interface *iface_rec = port_rec->interfaces[j]; - if (!iface_rec->n_ofport) { - continue; - } - /* Get localnet vif. */ - const char *iface_id = smap_get(&iface_rec->external_ids, - "iface-id"); - if (!iface_id) { - continue; - } - const struct sbrec_port_binding *pb - = lport_lookup_by_name(sbrec_port_binding_by_name, iface_id); - if (!pb || pb->chassis != chassis) { - continue; - } - if (!iface_rec->link_state || - strcmp(iface_rec->link_state, "up")) { - continue; - } - struct local_datapath *ld - = get_local_datapath(local_datapaths, - pb->datapath->tunnel_key); - if (ld && ld->localnet_port) { - sset_add(localnet_vifs, iface_id); - } - } - } - - struct sbrec_port_binding *target = sbrec_port_binding_index_init_row( - sbrec_port_binding_by_datapath); - - const struct local_datapath *ld; - HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { - const struct sbrec_port_binding *pb; - - if (!ld->localnet_port) { - continue; - } - - /* Get l3gw ports. Consider port bindings with type "l3gateway" - * that connect to gateway routers (if local), and consider port - * bindings of type "patch" since they might connect to - * distributed gateway ports with NAT addresses. */ - - sbrec_port_binding_index_set_datapath(target, ld->datapath); - SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, - sbrec_port_binding_by_datapath) { - if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) - || !strcmp(pb->type, "patch")) { - sset_add(local_l3gw_ports, pb->logical_port); - } - } - } - sbrec_port_binding_index_destroy_row(target); -} - - -/* Extracts the mac, IPv4 and IPv6 addresses, and logical port from - * 'addresses' which should be of the format 'MAC [IP1 IP2 ..] - * [is_chassis_resident("LPORT_NAME")]', where IPn should be a valid IPv4 - * or IPv6 address, and stores them in the 'ipv4_addrs' and 'ipv6_addrs' - * fields of 'laddrs'. The logical port name is stored in 'lport'. - * - * Returns true if at least 'MAC' is found in 'address', false otherwise. - * - * The caller must call destroy_lport_addresses() and free(*lport). */ -static bool -extract_addresses_with_port(const char *addresses, - struct lport_addresses *laddrs, - char **lport) -{ - int ofs; - if (!extract_addresses(addresses, laddrs, &ofs)) { - return false; - } else if (!addresses[ofs]) { - return true; - } - - struct lexer lexer; - lexer_init(&lexer, addresses + ofs); - lexer_get(&lexer); - - if (lexer.error || lexer.token.type != LEX_T_ID - || !lexer_match_id(&lexer, "is_chassis_resident")) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_INFO_RL(&rl, "invalid syntax '%s' in address", addresses); - lexer_destroy(&lexer); - return true; - } - - if (!lexer_match(&lexer, LEX_T_LPAREN)) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_INFO_RL(&rl, "Syntax error: expecting '(' after " - "'is_chassis_resident' in address '%s'", addresses); - lexer_destroy(&lexer); - return false; - } - - if (lexer.token.type != LEX_T_STRING) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_INFO_RL(&rl, - "Syntax error: expecting quoted string after " - "'is_chassis_resident' in address '%s'", addresses); - lexer_destroy(&lexer); - return false; - } - - *lport = xstrdup(lexer.token.s); - - lexer_get(&lexer); - if (!lexer_match(&lexer, LEX_T_RPAREN)) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_INFO_RL(&rl, "Syntax error: expecting ')' after quoted string in " - "'is_chassis_resident()' in address '%s'", - addresses); - lexer_destroy(&lexer); - return false; - } - - lexer_destroy(&lexer); - return true; -} - -static void -consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name, - const char *nat_address, - const struct sbrec_port_binding *pb, - struct sset *nat_address_keys, - const struct sbrec_chassis *chassis, - const struct sset *active_tunnels, - struct shash *nat_addresses) -{ - struct lport_addresses *laddrs = xmalloc(sizeof *laddrs); - char *lport = NULL; - if (!extract_addresses_with_port(nat_address, laddrs, &lport) - || (!lport && !strcmp(pb->type, "patch")) - || (lport && !lport_is_chassis_resident( - sbrec_port_binding_by_name, chassis, - active_tunnels, lport))) { - destroy_lport_addresses(laddrs); - free(laddrs); - free(lport); - return; - } - free(lport); - - int i; - for (i = 0; i < laddrs->n_ipv4_addrs; i++) { - char *name = xasprintf("%s-%s", pb->logical_port, - laddrs->ipv4_addrs[i].addr_s); - sset_add(nat_address_keys, name); - free(name); - } - if (laddrs->n_ipv4_addrs == 0) { - char *name = xasprintf("%s-noip", pb->logical_port); - sset_add(nat_address_keys, name); - free(name); - } - shash_add(nat_addresses, pb->logical_port, laddrs); -} - -static void -get_nat_addresses_and_keys(struct ovsdb_idl_index *sbrec_port_binding_by_name, - struct sset *nat_address_keys, - struct sset *local_l3gw_ports, - const struct sbrec_chassis *chassis, - const struct sset *active_tunnels, - struct shash *nat_addresses) -{ - const char *gw_port; - SSET_FOR_EACH(gw_port, local_l3gw_ports) { - const struct sbrec_port_binding *pb; - - pb = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); - if (!pb) { - continue; - } - - if (pb->n_nat_addresses) { - for (int i = 0; i < pb->n_nat_addresses; i++) { - consider_nat_address(sbrec_port_binding_by_name, - pb->nat_addresses[i], pb, - nat_address_keys, chassis, - active_tunnels, - nat_addresses); - } - } else { - /* Continue to support options:nat-addresses for version - * upgrade. */ - const char *nat_addresses_options = smap_get(&pb->options, - "nat-addresses"); - if (nat_addresses_options) { - consider_nat_address(sbrec_port_binding_by_name, - nat_addresses_options, pb, - nat_address_keys, chassis, - active_tunnels, - nat_addresses); - } - } - } -} - static void send_garp_rarp_wait(long long int send_garp_rarp_time) { @@ -6441,96 +6208,70 @@ send_garp_rarp_run(struct rconn *swconn, long long int *send_garp_rarp_time) * thread context. */ static void send_garp_rarp_prepare(struct ovsdb_idl_txn *ovnsb_idl_txn, - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_name, struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, - const struct ovsrec_bridge *br_int, - const struct sbrec_chassis *chassis, const struct hmap *local_datapaths, - const struct sset *active_tunnels, - const struct ovsrec_open_vswitch_table *ovs_table) + const struct ovsrec_open_vswitch_table *ovs_table, + struct ed_type_nat_addresses *nat_addresses) OVS_REQUIRES(pinctrl_mutex) { - struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs); - struct sset local_l3gw_ports = SSET_INITIALIZER(&local_l3gw_ports); - struct sset nat_ip_keys = SSET_INITIALIZER(&nat_ip_keys); - struct shash nat_addresses; - unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; - bool garp_continuous = false; - const struct ovsrec_open_vswitch *cfg = - ovsrec_open_vswitch_table_first(ovs_table); - if (cfg) { - garp_max_timeout = smap_get_ullong( - &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; - garp_continuous = !!garp_max_timeout; - if (!garp_max_timeout) { - garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; - } - } - - shash_init(&nat_addresses); - - get_localnet_vifs_l3gwports(sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - br_int, chassis, local_datapaths, - &localnet_vifs, &local_l3gw_ports); - - get_nat_addresses_and_keys(sbrec_port_binding_by_name, - &nat_ip_keys, &local_l3gw_ports, - chassis, active_tunnels, - &nat_addresses); - /* For deleted ports and deleted nat ips, remove from - * send_garp_rarp_data. */ - struct shash_node *iter; - SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { - if (!sset_contains(&localnet_vifs, iter->name) && - !sset_contains(&nat_ip_keys, iter->name)) { - send_garp_rarp_delete(iter->name); + if (nat_addresses) { + unsigned long long garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; + bool garp_continuous = false; + + const struct ovsrec_open_vswitch *cfg = + ovsrec_open_vswitch_table_first(ovs_table); + if (cfg) { + garp_max_timeout = smap_get_ullong( + &cfg->external_ids, "garp-max-timeout-sec", 0) * 1000; + garp_continuous = !!garp_max_timeout; + if (!garp_max_timeout) { + garp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT; + } } - } - - /* Update send_garp_rarp_data. */ - const char *iface_id; - SSET_FOR_EACH (iface_id, &localnet_vifs) { - const struct sbrec_port_binding *pb = lport_lookup_by_name( - sbrec_port_binding_by_name, iface_id); - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", false)) { - send_garp_rarp_update(ovnsb_idl_txn, - sbrec_mac_binding_by_lport_ip, - local_datapaths, pb, &nat_addresses, - garp_max_timeout, garp_continuous); + /* For deleted ports and deleted nat ips, remove from + * send_garp_rarp_data. */ + struct shash_node *iter; + SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { + if (!sset_contains(&nat_addresses->localnet_vifs, iter->name) && + !sset_contains(&nat_addresses->nat_ip_keys, iter->name)) { + send_garp_rarp_delete(iter->name); + } } - } - - /* Update send_garp_rarp_data for nat-addresses. */ - const char *gw_port; - SSET_FOR_EACH (gw_port, &local_l3gw_ports) { - const struct sbrec_port_binding *pb - = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", false)) { - send_garp_rarp_update(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip, - local_datapaths, pb, &nat_addresses, - garp_max_timeout, garp_continuous); + /* Update send_garp_rarp_data. */ + const char *iface_id; + SSET_FOR_EACH (iface_id, &nat_addresses->localnet_vifs) { + const struct sbrec_port_binding *pb = lport_lookup_by_name( + sbrec_port_binding_by_name, iface_id); + if (pb && !smap_get_bool( + &pb->options, "disable_garp_rarp", false)) { + send_garp_rarp_update(ovnsb_idl_txn, + sbrec_mac_binding_by_lport_ip, + local_datapaths, pb, + &nat_addresses->nat_addresses, + garp_max_timeout, garp_continuous); + } } - } - /* pinctrl_handler thread will send the GARPs. */ - - sset_destroy(&localnet_vifs); - sset_destroy(&local_l3gw_ports); + /* Update send_garp_rarp_data for nat-addresses. */ + const char *gw_port; + SSET_FOR_EACH (gw_port, &nat_addresses->local_l3gw_ports) { + const struct sbrec_port_binding *pb + = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); + if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp", + false)) { + send_garp_rarp_update(ovnsb_idl_txn, + sbrec_mac_binding_by_lport_ip, + local_datapaths, pb, + &nat_addresses->nat_addresses, + garp_max_timeout, garp_continuous); + } + } - SHASH_FOR_EACH_SAFE (iter, &nat_addresses) { - struct lport_addresses *laddrs = iter->data; - destroy_lport_addresses(laddrs); - shash_delete(&nat_addresses, iter); - free(laddrs); + /* pinctrl_handler thread will send the GARPs. */ + garp_rarp_max_timeout = garp_max_timeout; + garp_rarp_continuous = garp_continuous; } - shash_destroy(&nat_addresses); - - sset_destroy(&nat_ip_keys); - - garp_rarp_max_timeout = garp_max_timeout; - garp_rarp_continuous = garp_continuous; } static bool diff --git a/controller/pinctrl.h b/controller/pinctrl.h index 846afe0a4..c6a7423b8 100644 --- a/controller/pinctrl.h +++ b/controller/pinctrl.h @@ -20,6 +20,7 @@ #include <stdint.h> #include "lib/sset.h" +#include "openvswitch/shash.h" #include "openvswitch/list.h" #include "openvswitch/meta-flow.h" @@ -39,10 +40,16 @@ struct sbrec_bfd_table; struct sbrec_port_binding; struct sbrec_mac_binding_table; +struct ed_type_nat_addresses { + struct shash nat_addresses; + struct sset localnet_vifs; + struct sset local_l3gw_ports; + struct sset nat_ip_keys; +}; + void pinctrl_init(void); void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, struct ovsdb_idl_index *sbrec_datapath_binding_by_key, - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_name, struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, @@ -53,12 +60,13 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_service_monitor_table *, const struct sbrec_mac_binding_table *, const struct sbrec_bfd_table *, - const struct ovsrec_bridge *, const struct sbrec_chassis *, + const struct sbrec_chassis *, const struct hmap *local_datapaths, const struct sset *active_tunnels, const struct shash *local_active_ports_ipv6_pd, const struct shash *local_active_ports_ras, - const struct ovsrec_open_vswitch_table *ovs_table); + const struct ovsrec_open_vswitch_table *ovs_table, + struct ed_type_nat_addresses *nat_addresses); void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); void pinctrl_destroy(void);
This patch moves the nat_addresses calculation to an own engine node. Previously the nat_address calculation was executed at every pinctrl_run (send_garp_rarp_prepare). This can cause high cpu usage and delayed actions on gateway-chassis(500 LRPs+). perf trace: - 98.83% main - 79.69% pinctrl_run - 77.71% send_garp_rarp_prepare - 55.49% get_nat_addresses_and_keys - 36.76% consider_nat_address - 18.13% lport_lookup_by_name - 13.58% sset_destroy Co-authored-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> Signed-off-by: Ihtisham ul Haq <Ihtisham.ul_Haq@stackit.cloud> Signed-off-by: Max Lamprecht <max.lamprecht@stackit.cloud> --- controller/ovn-controller.c | 341 +++++++++++++++++++++++++++++++- controller/pinctrl.c | 381 ++++++------------------------------ controller/pinctrl.h | 14 +- 3 files changed, 410 insertions(+), 326 deletions(-)