On 2/13/24 08:04, Han Zhou wrote:
> On Thu, Feb 8, 2024 at 1:49 PM <[email protected]> wrote:
>>
>> From: Numan Siddique <[email protected]>
>>
>> If an SNAT external_ip belongs to the router IP, then there
>> is no need to generate ARP request responder flows in
>> build_lswitch_rport_arp_req_flows_for_lbnats() as these flows
>> for router ips are generated by build_lswitch_rport_arp_req_flows().
>> Otherwise this results in the lflow_table_add_lflow() to be called
>> multiple times for the same match and actions and the ovn_lflow to
>> have multiple dp_refcnts.
>>
>> Fixes: fe1c5df98b6f ("northd: forward arp request to lrp snat on.")
>> Signed-off-by: Numan Siddique <[email protected]>
>> ---
>> northd/en-lr-nat.c | 6 ++++++
>> northd/en-lr-nat.h | 2 ++
>> northd/northd.c | 28 ++++++++++++++++++++++++----
>> northd/northd.h | 1 +
>> 4 files changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c
>> index 7664ec0ca9..ad11025c69 100644
>> --- a/northd/en-lr-nat.c
>> +++ b/northd/en-lr-nat.c
>> @@ -288,6 +288,8 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec,
>> struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i];
>>
>> nat_entry->nb = nat;
>> + nat_entry->is_router_ip = false;
>> +
>> if (!extract_ip_addresses(nat->external_ip,
>> &nat_entry->ext_addrs) ||
>> !nat_entry_is_valid(nat_entry)) {
>> @@ -302,6 +304,10 @@ lr_nat_record_init(struct lr_nat_record *lrnat_rec,
>>
>> /* If this is a SNAT rule add the IP to the set of unique SNAT
> IPs. */
>> if (!strcmp(nat->type, "snat")) {
>> + if (sset_contains(&od->router_ips, nat->external_ip)) {
>> + nat_entry->is_router_ip = true;
>> + }
>> +
>> if (!nat_entry_is_v6(nat_entry)) {
>> snat_ip_add(lrnat_rec,
>> nat_entry->ext_addrs.ipv4_addrs[0].addr_s,
>> diff --git a/northd/en-lr-nat.h b/northd/en-lr-nat.h
>> index 16b166ee05..6d3b2b6d65 100644
>> --- a/northd/en-lr-nat.h
>> +++ b/northd/en-lr-nat.h
>> @@ -37,6 +37,8 @@ struct ovn_nat {
>> * list of nat entries. Currently
>> * only used for SNAT.
>> */
>> + bool is_router_ip; /* Indicates if the NAT external_ip is also one of
>> + * router's lrp ip. Initialized only for SNAT. */
Nit: we always initialize it, we just don't set it to true unless this
is a SNAT rule which uses a router IP. I'd change this to "Can be
'true' only for SNAT."
>> };
>>
>> /* Stores the list of SNAT entries referencing a unique SNAT IP address.
>> diff --git a/northd/northd.c b/northd/northd.c
>> index a5d5e67117..c59aa8d304 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -431,6 +431,7 @@ ovn_datapath_create(struct hmap *datapaths, const
> struct uuid *key,
>> hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key));
>> od->lr_group = NULL;
>> hmap_init(&od->ports);
>> + sset_init(&od->router_ips);
>> return od;
>> }
>>
>> @@ -459,6 +460,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
> ovn_datapath *od)
>> free(od->l3dgw_ports);
>> destroy_mcast_info_for_datapath(od);
>> destroy_ports_for_datapath(od);
>> + sset_destroy(&od->router_ips);
>> free(od);
>> }
>> }
>> @@ -2190,6 +2192,19 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
>>
>> op->lrp_networks = lrp_networks;
>> op->od = od;
>> +
>> + for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) {
>> + sset_add(&op->od->router_ips,
>> + op->lrp_networks.ipv4_addrs[j].addr_s);
>> + }
>> + for (size_t j = 0; j < op->lrp_networks.n_ipv6_addrs; j++) {
>> + /* Exclude the LLA. */
>> + if (!in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr)) {
>> + sset_add(&op->od->router_ips,
>> + op->lrp_networks.ipv6_addrs[j].addr_s);
>> + }
>> + }
>> +
>> hmap_insert(&od->ports, &op->dp_node,
>> hmap_node_hash(&op->key_node));
>>
>> @@ -8302,22 +8317,27 @@ build_lswitch_rport_arp_req_flows_for_lbnats(
>> struct ovn_nat *nat_entry =
>> CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries),
>> struct ovn_nat, ext_addr_list_node);
>> + if (nat_entry->is_router_ip) {
>> + /* If its a router ip, then there is no need to add the ARP
>> + * request forwarder flows as it will be added by
>> + * build_lswitch_rport_arp_req_flows(). */
>> + continue;
>> + }
>> +
>> const struct nbrec_nat *nat = nat_entry->nb;
>>
>> /* Check if the ovn port has a network configured on which we
> could
>> * expect ARP requests/NS for the SNAT external_ip.
>> */
>> if (nat_entry_is_v6(nat_entry)) {
>> - if (!lr_stateful_rec ||
>> - !sset_contains(&lr_stateful_rec->lb_ips->ips_v6,
>> + if (!sset_contains(&lr_stateful_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, lflow_ref);
>> }
>> } else {
>> - if (!lr_stateful_rec ||
>> - !sset_contains(&lr_stateful_rec->lb_ips->ips_v4,
>> + if (!sset_contains(&lr_stateful_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,
>> diff --git a/northd/northd.h b/northd/northd.h
>> index b5c175929e..3f1cd83413 100644
>> --- a/northd/northd.h
>> +++ b/northd/northd.h
>> @@ -293,6 +293,7 @@ struct ovn_datapath {
>> struct ovn_datapath **ls_peers;
>> size_t n_ls_peers;
>> size_t n_allocated_ls_peers;
>> + struct sset router_ips; /* Router port IPs except the IPv6 LLAs. */
>>
>> /* Logical switch data. */
>> struct ovn_port **router_ports;
>> --
>> 2.43.0
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Thanks Numan.
> Acked-by: Han Zhou <[email protected]>
Looks good to me too:
Acked-by: Dumitru Ceara <[email protected]>
Thanks!
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
