On 23.01.2026 08:15, Numan Siddique wrote:
On Fri, Nov 28, 2025 at 7:56 AM Dumitru Ceara via dev <[email protected]><mailto:[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, 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). 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; } 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; } } } 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? [...] The changes I'm suggesting are minor, here's the full diff (some more style issues are addressed in it): https://github.com/dceara/ovn/commit/a2307c9 If it looks good to you too I can squash it in and push the patch to main. Please let me know what you think. Regards, Dumitru _______________________________________________ dev mailing list [email protected]<mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- regards, Alexandra. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
