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, 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. 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. 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); > + > 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
