On 12/6/23 04:38, Numan Siddique wrote: > On Thu, Nov 23, 2023 at 4:15 PM Dumitru Ceara <[email protected]> wrote: >> >> On 10/26/23 20:15, [email protected] wrote: >>> From: Numan Siddique <[email protected]> >>> >>> Previous commits added new engine nodes to store logical router's lb >>> and NAT data. Make use of the data stored by these engine nodes >>> to generate logical flows related to router's LBs and NATs. >>> >>> Signed-off-by: Numan Siddique <[email protected]> >>> --- >>> northd/en-lflow.c | 3 - >>> northd/en-lr-lb-nat-data.h | 4 + >>> northd/inc-proc-northd.c | 1 - >>> northd/northd.c | 752 ++++++++++++++++++++++++------------- >>> northd/northd.h | 1 - >>> 5 files changed, 496 insertions(+), 265 deletions(-) >>> >>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >>> index 9cb0ead3f0..229f4be1d0 100644 >>> --- a/northd/en-lflow.c >>> +++ b/northd/en-lflow.c >>> @@ -42,8 +42,6 @@ lflow_get_input_data(struct engine_node *node, >>> engine_get_input_data("port_group", node); >>> struct sync_meters_data *sync_meters_data = >>> engine_get_input_data("sync_meters", node); >>> - struct ed_type_lr_nat_data *lr_nat_data = >>> - engine_get_input_data("lr_nat", node); >>> struct ed_type_lr_lb_nat_data *lr_lb_nat_data = >>> engine_get_input_data("lr_lb_nat_data", node); >>> >>> @@ -68,7 +66,6 @@ lflow_get_input_data(struct engine_node *node, >>> lflow_input->ls_ports = &northd_data->ls_ports; >>> lflow_input->lr_ports = &northd_data->lr_ports; >>> lflow_input->ls_port_groups = &pg_data->ls_port_groups; >>> - lflow_input->lr_nats = &lr_nat_data->lr_nats; >>> lflow_input->lr_lbnats = &lr_lb_nat_data->lr_lbnats; >>> lflow_input->meter_groups = &sync_meters_data->meter_groups; >>> lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map; >>> diff --git a/northd/en-lr-lb-nat-data.h b/northd/en-lr-lb-nat-data.h >>> index 9029aee339..ffe41cad73 100644 >>> --- a/northd/en-lr-lb-nat-data.h >>> +++ b/northd/en-lr-lb-nat-data.h >>> @@ -56,6 +56,10 @@ struct lr_lb_nat_data_table { >>> #define LR_LB_NAT_DATA_TABLE_FOR_EACH(LR_LB_NAT_REC, TABLE) \ >>> HMAP_FOR_EACH (LR_LB_NAT_REC, key_node, &(TABLE)->entries) >>> >>> +#define LR_LB_NAT_DATA_TABLE_FOR_EACH_IN_P(LR_LB_NAT_REC, JOBID, TABLE) \ >>> + HMAP_FOR_EACH_IN_PARALLEL (LR_LB_NAT_REC, key_node, JOBID, \ >>> + &(TABLE)->entries) >>> + >>> struct lr_lb_nat_data_tracked_data { >>> /* Created or updated logical router with LB data. */ >>> struct hmapx crupdated; /* Stores 'struct lr_lb_nat_data_record'. */ >>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >>> index 369a151fa3..84627070a8 100644 >>> --- a/northd/inc-proc-northd.c >>> +++ b/northd/inc-proc-northd.c >>> @@ -228,7 +228,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >>> engine_add_input(&en_lflow, &en_sb_igmp_group, NULL); >>> engine_add_input(&en_lflow, &en_northd, lflow_northd_handler); >>> engine_add_input(&en_lflow, &en_port_group, lflow_port_group_handler); >>> - engine_add_input(&en_lflow, &en_lr_nat, NULL); >> >> Was this supposed to go in the previous patch, the one that adds >> en_lr_lb_nat_data? > > Yes. It should have been. Addressed in v3. > > Thanks > > > >> >>> engine_add_input(&en_lflow, &en_lr_lb_nat_data, NULL); >>> >>> engine_add_input(&en_sync_to_sb_addr_set, &en_nb_address_set, >>> diff --git a/northd/northd.c b/northd/northd.c >>> index 24df14c0de..1877cbc7df 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -8854,18 +8854,14 @@ build_lrouter_groups(struct hmap *lr_ports, struct >>> ovs_list *lr_list) >>> */ >>> static void >>> build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op, >>> - uint32_t priority, >>> - struct ovn_datapath *od, >>> - const struct lr_nat_table >>> *lr_nats, >>> - struct hmap *lflows) >>> + uint32_t priority, >>> + const struct ovn_datapath *od, >>> + const struct lr_nat_record >>> *lrnat_rec, >>> + struct hmap *lflows) >> >> >> Nit: indentation. > > I had to do this way to keep under 80. > > >> >>> { >>> struct ds eth_src = DS_EMPTY_INITIALIZER; >>> struct ds match = DS_EMPTY_INITIALIZER; >>> >>> - const struct lr_nat_record *lrnat_rec = lr_nat_table_find_by_index( >>> - lr_nats, op->od->index); >>> - ovs_assert(lrnat_rec); >>> - >>> /* Self originated ARP requests/RARP/ND need to be flooded to the L2 >>> domain >>> * (except on router ports). Determine that packets are self >>> originated >>> * by also matching on source MAC. Matching on ingress port is not >>> @@ -8952,7 +8948,8 @@ lrouter_port_ipv6_reachable(const struct ovn_port *op, >>> */ >>> static void >>> build_lswitch_rport_arp_req_flow(const char *ips, >>> - int addr_family, struct ovn_port *patch_op, struct ovn_datapath *od, >>> + int addr_family, struct ovn_port *patch_op, >>> + const struct ovn_datapath *od, >>> uint32_t priority, struct hmap *lflows, >>> const struct ovsdb_idl_row *stage_hint) >>> { >>> @@ -8993,8 +8990,6 @@ static void >>> build_lswitch_rport_arp_req_flows(struct ovn_port *op, >>> struct ovn_datapath *sw_od, >>> struct ovn_port *sw_op, >>> - const struct lr_nat_table *lr_nats, >>> - const struct lr_lb_nat_data_table >>> *lr_lbnats, >>> struct hmap *lflows, >>> const struct ovsdb_idl_row *stage_hint) >>> { >>> @@ -9010,12 +9005,48 @@ build_lswitch_rport_arp_req_flows(struct ovn_port >>> *op, >>> * router port. >>> * Priority: 80. >>> */ >>> - const struct lr_lb_nat_data_record *lr_lbnat_rec = NULL; >>> - if (op->od->nbr->n_load_balancer || >>> op->od->nbr->n_load_balancer_group) { >>> - lr_lbnat_rec = lr_lb_nat_data_table_find_by_index(lr_lbnats, >>> - op->od->index); >>> - ovs_assert(lr_lbnat_rec); >>> + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { >>> + build_lswitch_rport_arp_req_flow( >>> + op->lrp_networks.ipv4_addrs[i].addr_s, AF_INET, sw_op, sw_od, >>> 80, >>> + lflows, stage_hint); >>> + } >>> + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { >>> + build_lswitch_rport_arp_req_flow( >>> + op->lrp_networks.ipv6_addrs[i].addr_s, AF_INET6, sw_op, sw_od, >>> 80, >>> + lflows, stage_hint); >>> + } >>> +} >>> >>> +/* >>> + * Ingress table 25: Flows that forward ARP/ND requests only to the routers >>> + * that own the addresses. >>> + * Priorities: >>> + * - 80: self originated GARPs that need to follow regular processing. >>> + * - 75: ARP requests to router owned IPs (interface IP/LB/NAT). >>> + */ >>> +static void >>> +build_lswitch_rport_arp_req_flows_for_lbnats(struct ovn_port *op, >>> + const struct lr_lb_nat_data_record >>> *lr_lbnat_rec, >>> + const struct ovn_datapath *sw_od, >>> + struct ovn_port *sw_op, >>> + struct hmap *lflows, >>> + const struct ovsdb_idl_row *stage_hint) >> >> Nit: indentation. > > Same here too. > > >> >>> +{ >>> + if (!op || !op->nbrp) { >>> + return; >>> + } >>> + >>> + if (!lrport_is_enabled(op->nbrp)) { >>> + return; >>> + } >>> + >>> + ovs_assert(op->od == lr_lbnat_rec->od); >>> + >>> + /* Forward ARP requests for owned IP addresses (L3, VIP, NAT) only to >>> this >>> + * router port. >>> + * Priority: 80. >>> + */ >>> + if (op->od->nbr->n_load_balancer || >>> op->od->nbr->n_load_balancer_group) { >>> const char *ip_addr; >>> SSET_FOR_EACH (ip_addr, &lr_lbnat_rec->lb_ips->ips_v4_reachable) { >>> ovs_be32 ipv4_addr; >>> @@ -9045,17 +9076,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port >>> *op, >>> } >>> } >>> >>> - for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { >>> - build_lswitch_rport_arp_req_flow( >>> - op->lrp_networks.ipv4_addrs[i].addr_s, AF_INET, sw_op, sw_od, >>> 80, >>> - lflows, stage_hint); >>> - } >>> - for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { >>> - build_lswitch_rport_arp_req_flow( >>> - op->lrp_networks.ipv6_addrs[i].addr_s, AF_INET6, sw_op, sw_od, >>> 80, >>> - lflows, stage_hint); >>> - } >>> - >>> /* Self originated ARP requests/RARP/ND need to be flooded as usual. >>> * >>> * However, if the switch doesn't have any non-router ports we >>> shouldn't >>> @@ -9064,19 +9084,13 @@ build_lswitch_rport_arp_req_flows(struct ovn_port >>> *op, >>> * Priority: 75. >>> */ >>> if (sw_od->n_router_ports != sw_od->nbs->n_ports) { >>> - build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lr_nats, >>> + build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, >>> + lr_lbnat_rec->lrnat_rec, >>> lflows); >>> } >>> >>> - const struct lr_nat_record *lrnat_rec = >>> - lr_nat_table_find_by_index(lr_nats, op->od->index); >>> - >>> - if (!lrnat_rec) { >>> - return; >>> - } >>> - >>> - for (size_t i = 0; i < lrnat_rec->n_nat_entries; i++) { >>> - struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i]; >>> + for (size_t i = 0; i < lr_lbnat_rec->lrnat_rec->n_nat_entries; i++) { >>> + struct ovn_nat *nat_entry = >>> &lr_lbnat_rec->lrnat_rec->nat_entries[i]; >>> const struct nbrec_nat *nat = nat_entry->nb; >>> >>> if (!nat_entry_is_valid(nat_entry)) { >>> @@ -9091,15 +9105,15 @@ build_lswitch_rport_arp_req_flows(struct ovn_port >>> *op, >>> * expect ARP requests/NS for the DNAT external_ip. >>> */ >>> if (nat_entry_is_v6(nat_entry)) { >>> - if (!lr_lbnat_rec || >>> !sset_contains(&lr_lbnat_rec->lb_ips->ips_v6, >>> - nat->external_ip)) { >>> + if (!sset_contains(&lr_lbnat_rec->lb_ips->ips_v6, >>> + nat->external_ip)) { >>> build_lswitch_rport_arp_req_flow( >>> nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows, >>> stage_hint); >>> } >>> } else { >>> - if (!lr_lbnat_rec || >>> !sset_contains(&lr_lbnat_rec->lb_ips->ips_v4, >>> - nat->external_ip)) { >>> + if (!sset_contains(&lr_lbnat_rec->lb_ips->ips_v4, >>> + nat->external_ip)) { >>> build_lswitch_rport_arp_req_flow( >>> nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows, >>> stage_hint); >>> @@ -10158,12 +10172,8 @@ build_lswitch_ip_mcast_igmp_mld(struct >>> ovn_igmp_group *igmp_group, >>> >>> /* Ingress table 25: Destination lookup, unicast handling (priority 50), */ >>> static void >>> -build_lswitch_ip_unicast_lookup(struct ovn_port *op, >>> - const struct lr_nat_table *lr_nats, >>> - const struct lr_lb_nat_data_table >>> *lr_lbnats, >>> - struct hmap *lflows, >>> - struct ds *actions, >>> - struct ds *match) >>> +build_lswitch_ip_unicast_lookup(struct ovn_port *op, struct hmap *lflows, >>> + struct ds *actions, struct ds *match) >>> { >>> ovs_assert(op->nbsp); >>> if (lsp_is_external(op->nbsp)) { >>> @@ -10175,8 +10185,7 @@ build_lswitch_ip_unicast_lookup(struct ovn_port *op, >>> * requests only to the router port that owns the IP address. >>> */ >>> if (lsp_is_router(op->nbsp)) { >>> - build_lswitch_rport_arp_req_flows(op->peer, op->od, op, lr_nats, >>> - lr_lbnats, lflows, >>> + build_lswitch_rport_arp_req_flows(op->peer, op->od, op, lflows, >>> &op->nbsp->header_); >>> } >>> >>> @@ -10273,33 +10282,6 @@ build_lswitch_ip_unicast_lookup(struct ovn_port >>> *op, >>> S_SWITCH_IN_L2_LKUP, 50, >>> ds_cstr(match), ds_cstr(actions), >>> &op->nbsp->header_); >>> - >>> - /* Add ethernet addresses specified in NAT rules on >>> - * distributed logical routers. */ >>> - if (is_l3dgw_port(op->peer)) { >>> - for (int j = 0; j < op->peer->od->nbr->n_nat; j++) { >>> - const struct nbrec_nat *nat >>> - = op->peer->od->nbr->nat[j]; >>> - if (!strcmp(nat->type, "dnat_and_snat") >>> - && nat->logical_port && nat->external_mac >>> - && eth_addr_from_string(nat->external_mac, &mac)) { >>> - >>> - ds_clear(match); >>> - ds_put_format(match, "eth.dst == "ETH_ADDR_FMT >>> - " && is_chassis_resident(\"%s\")", >>> - ETH_ADDR_ARGS(mac), >>> - nat->logical_port); >>> - >>> - ds_clear(actions); >>> - ds_put_format(actions, action, op->json_key); >>> - ovn_lflow_add_with_hint(lflows, op->od, >>> - S_SWITCH_IN_L2_LKUP, 50, >>> - ds_cstr(match), >>> - ds_cstr(actions), >>> - &op->nbsp->header_); >>> - } >>> - } >>> - } >>> } else { >>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); >>> >>> @@ -10310,6 +10292,52 @@ build_lswitch_ip_unicast_lookup(struct ovn_port >>> *op, >>> } >>> } >>> >>> +/* Ingress table 25: Destination lookup, unicast handling (priority 50), */ >>> +static void >>> +build_lswitch_ip_unicast_lookup_for_nats(struct ovn_port *op, >>> + const struct lr_lb_nat_data_record >>> *lr_lbnat_rec, >>> + struct hmap *lflows, struct ds *match, >>> + struct ds *actions) >> >> Nit: indentation. > > Same here too. > > Normally I'd have done something like below > > --- > static void > build_lswitch_ip_unicast_lookup_for_nats( > struct ovn_port *op, const struct lr_stateful_record *lr_sful_rec, > struct hmap *lflows, struct ds *match, struct ds *actions)
I dislike this the least. :) > ---- > > But I saw a few patches in OVN indented in this way. > See commit 2a57b204459612a9a4edb49b9c5ebc44e101ee93 > I missed it there, I think, I probably should've commented about it there too. > > I've no strong preference. Let me know if you've a strong preference. No strong preference on my side either, I'll leave it up to you at this point. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
