On 2/27/26 1:49 PM, Numan Siddique wrote: > On Thu, Feb 26, 2026 at 6:02 PM Lana Honcharuk > <[email protected]> wrote: >> >> Register a noop change handler for the en_sb_dns input to the northd >> engine node. When SB DNS records are modified directly (outside of >> northd), the noop handler avoids triggering an unnecessary full >> recompute. Northd will correct any stale SB DNS state on the next >> recompute. >> >> Signed-off-by: Lana Honcharuk <[email protected]> >> Assisted-by: Claude Opus 4.6 > > Hi Lana, >
Hi Lana, Numan, > Thanks for the patch. Your patch seems to have checkpatch errors. > Run "utilities/checkpatch.py -1" and fix the warnings reported by it. > > Also your patch seems to be failing in CI. Please check and fix the > failures - https://github.com/ovsrobot/ovn/actions/runs/22465500118 > > It's also possible the tests not related to your change are flaky. > Please take a look at it. > > Overall the patch LGTM. But I think the commit message needs some change. > Sorry, I only briefly looked at the patch so this is not a full review but I have a comment below. > From what I understand, when northd handles the Northbound DNS > changes, it recomputes (which is fine) and it writes to the SB DNS. > When SB ovsdb-server commits the changes and sends the updates back, > ovn-northd recomputes again since en_sb_dns change chandler > for en_northd engine node is NULL. And this results in a recompute > again which this patch tries to avoid. > I think it makes sense to me. I would suggest changing the commit > message to highlight this aspect. > Wouldn't it be better (and more inline with what we do for other write-only tables) to add something like: for (size_t i = 0; i < SBREC_DNS_N_COLUMNS i++) { ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_dns_columns[i]); } in ovn-northd's main function, after the part with: /* Disable alerting for pure write-only columns. */ ? > Please see below for one more comment. > > > >> --- >> northd/inc-proc-northd.c | 7 ++++++- >> tests/ovn-northd.at | 45 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >> index 10bad4bab..640b60c89 100644 >> --- a/northd/inc-proc-northd.c >> +++ b/northd/inc-proc-northd.c >> @@ -263,7 +263,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_northd, &en_sb_chassis, NULL); >> engine_add_input(&en_northd, &en_sb_mirror, NULL); >> engine_add_input(&en_northd, &en_sb_meter, NULL); >> - engine_add_input(&en_northd, &en_sb_dns, NULL); >> + >> + /* en_northd is the only writer to the SB DNS table, so any >> + * changes detected here are from northd's own previous >> + * transaction and can be safely ignored. */ >> + engine_add_input(&en_northd, &en_sb_dns, engine_noop_handler); With my suggestion we don't need the noop_handler anymore, I think. >> + >> engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL); >> engine_add_input(&en_northd, &en_sb_service_monitor, NULL); >> engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL); >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index a8b5436ba..827f92914 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -18327,6 +18327,51 @@ OVN_CLEANUP_NORTHD >> AT_CLEANUP >> ]) >> >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> +AT_SETUP([SB DNS incremental processing]) >> +ovn_start >> + >> +check ovn-nbctl --wait=sb ls-add sw0 >> + >> +# Create NB DNS record and associate it with sw0. >> +dns_uuid=$(ovn-nbctl create DNS records:vm1.ovn.org="10.0.0.4") >> +check ovn-nbctl set Logical_Switch sw0 dns_records="$dns_uuid" >> +check ovn-nbctl --wait=sb sync >> + >> +# Verify SB DNS entry was created with correct records. >> +sb_dns_uuid=$(fetch_column sb:DNS _uuid) >> +check test -n "$sb_dns_uuid" >> +wait_column '{"vm1.ovn.org"="10.0.0.4"}' sb:DNS records >> + > > I would also suggest to enhance the test > - to ensure that when you update the NB DNS record, en_northd > engine recomputes only once > - call the macro CHECK_NO_CHANGE_AFTER_RECOMPUTE to ensure that there were > no > changes to the SB logical flows after the recompute. This ensures > that the noop handler had no side effects. > > > Thanks > Numan > > >> +# Verify DNS lookup and response flows exist. >> +AT_CHECK([ovn-sbctl dump-flows sw0 | grep -c "dns_lookup\|dns_response" | >> grep -q '[1-9]']) >> + >> +# Directly modify the SB DNS records field. The noop handler should >> +# not trigger a recompute, so the stale value persists until a >> +# recompute corrects it. >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-sbctl set DNS $sb_dns_uuid records:vm2.ovn.org="10.0.0.5" >> +check ovn-nbctl --wait=sb sync >> +check_engine_stats northd norecompute compute >> +check_engine_stats lflow norecompute nocompute >> + >> +# The stale record should still be present since northd did not recompute. >> +AT_CHECK([ovn-sbctl get DNS $sb_dns_uuid records:vm2.ovn.org], [0], [dnl >> +"10.0.0.5" >> +]) >> + >> +# Force a recompute and verify northd corrects the SB DNS records >> +# back to what NB defines (only vm1.ovn.org). >> +check as northd ovn-appctl -t ovn-northd inc-engine/recompute >> +check ovn-nbctl --wait=sb sync >> +wait_column '{"vm1.ovn.org"="10.0.0.4"}' sb:DNS records >> + >> +# Verify DNS flows are still correct after recompute. >> +AT_CHECK([ovn-sbctl dump-flows sw0 | grep -c "dns_lookup\|dns_response" | >> grep -q '[1-9]']) >> + >> +AT_CLEANUP >> +]) >> + >> OVN_FOR_EACH_NORTHD_NO_HV([ >> AT_SETUP([ip_port_mappings validation: IPv4]) >> ovn_start >> -- >> 2.39.5 (Apple Git-154) >> >> _______________________________________________ >> 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 Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
