On Fri, Feb 27, 2026 at 8:20 AM Dumitru Ceara <[email protected]> wrote:
>
> 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. */
Totally agree. Thanks for pointing this out.
Numan
>
> ?
>
> > 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
