On 1/23/26 10:18 AM, Rukomoinikova Aleksandra wrote: > On 23.01.2026 08:15, Numan Siddique wrote: >> On Fri, Nov 28, 2025 at 7:56 AM Dumitru Ceara via dev >> <[email protected]> wrote: >>> On 11/28/25 1:16 PM, Rukomoinikova Aleksandra wrote: >>>> Hi! Thank you so much for the review. I'd be glad if you merged it after >>>> the squash. Thank you so much for the edits you made. There really are a >>>> lot of them. I'll take a look at them and keep them in mind for the future. >>>> >>> Hi Alexandra, >>> >>> No worries! I squashed in the suggested minor changes and pushed the >>> patch to main. >>> >>> Regards, >>> Dumitru >>> >>> >> Hi Aleksandra, >>
Hi Numan, Aleksandra, >> Unfortunately this patch breaks the traffic from external to the >> dnat_and_snat distributed IP of a logical port if the >> logical router has only dnat_and_snat distributed entries (with NO >> snat entries). >> Ah, sorry this slipped through. >> You can easily reproduce it using ovn-fake-multinode by deleting the >> snat entries from the logical router lr1 >> and then trying to reach 172.16.1.110 (dnat_and_snat ip) of sw01-port1 >> from ovnfake-ext1 namespace in the host. >> >> I tried to fix the issue but I was finding it hard to fix it in the >> handler of en-ls-arp engine node which this patch adds. >> >> Basically this commit is excluding all the NAT entries of a logical >> router if there are no snat entries. I'm not sure why the below >> check is added here >> https://github.com/ovn-org/ovn/blob/main/northd/en-ls-arp.c#L101 >> >> ---------------- >> for (size_t i = 0; i < lrnat_rec->n_nat_entries; i++) { >> const struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i]; >> >> if (is_centralized_nat_record(nat_entry)) { >> hmapx_add(&ls_arp_record->nat_records, >> (struct lrnat_rec *) lrnat_rec); >> } >> } >> ------------ >> >> In the above for loop, we iterate through all the nat entries of lrnat_rec >> and only add if it has atleast one centralized nat record. >> >> Why is this required ? Why can't the whole lrnat_rec be stored in the >> ls_arp_record ? >> >> Can you please take a look at it ? >> >> Thanks >> Numan > > Hi Numan and Dumitru, it seems the problem is that I need to remove > the !nat_entry->is_distributed check from the is_centralized_nat_record > function. Then such records will be added and everything should work. > And also I think I missed the return after adding because one entry is > enough for us. So total diff woud be like this: > > --- a/northd/en-ls-arp.c > +++ b/northd/en-ls-arp.c > @@ -76,8 +76,7 @@ is_centralized_nat_record(const struct ovn_nat *nat_entry) > return nat_entry->is_valid > && nat_entry->l3dgw_port > && nat_entry->l3dgw_port->peer > - && nat_entry->l3dgw_port->peer->od > - && !nat_entry->is_distributed; > + && nat_entry->l3dgw_port->peer->od; > } I think this part makes sense. > > static void > @@ -101,6 +100,7 @@ nat_record_data_create(struct ls_arp_record > *ls_arp_record, > if (is_centralized_nat_record(nat_entry)) { > hmapx_add(&ls_arp_record->nat_records, > (struct lrnat_rec *) lrnat_rec); > + return; I'm not sure this would be correct. The function is called with a switch datapath as 'od' argument. So we're essentially checking all routers connected to the 'od' logical switch. If you'd return here you'd potentially skip other routers connected to 'od'. We probably need to break out of the inner loop instead and move on to the next router. > } > } > } > > > But then it seems like checking that a router with NAT has an l3gw_port > is pointless if we're only adding logical flow in northd for those > distributed in the build_lswitch_arp_chassis_resident function, and the > check can be done there. But this is a significant change, and I'd like > to thoroughly test it before submitting. > > Should I submit the quick fix suggested above with tests today, or can I > submit a full fix after the branch is created? > I'd prefer if we fix this sooner rather than later if we're confident enough the quick fix is correct. Waiting for the branch creation (4 weeks from now) is too long IMO. We can follow up after the branch is created with a more intrusive change if we think it's needed (on main). Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
