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 <
> [email protected]> 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 <[email protected]>
> > Signed-off-by: Ihtisham ul Haq <[email protected]>
> > Signed-off-by: Max Lamprecht <[email protected]>
> > ---
> >
>
> 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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> Thanks,
> Ales
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
