On Mon, Dec 18, 2023 at 8:36 PM Han Zhou <hz...@ovn.org> wrote:
>
> On Wed, Dec 13, 2023 at 6:47 AM Dumitru Ceara <dce...@redhat.com> wrote:
> >
> > On 11/28/23 03:37, num...@ovn.org wrote:
> > > From: Numan Siddique <num...@ovn.org>
> > >
> > > A new engine node "global_config" is added which handles the changes
> > > to NB_Global an SB_Global tables. It also creates these rows if
> > > not present.
> > >
> > > Without the I-P, any changes to the options column of these tables
> > > result in recompute of 'northd' and 'lflow' engine nodes.
>
> This is not true. Common updates to NB_Global and SB_Global, such as nb_cfg
> and timestamps changes, or external_ids changes populated by ovn-k8s, will
> not trigger recompute.
This commit talks about the changes to the options column of these
tables and not about
the common updates. Any key added or deleted to the options column of
NB_Global and SB_Global results in full recompute.
Every few seconds (maybe its configurable) ovn-k writes a timestamp
value in the NB_Global's options column
and this results in 2 recompute nodes. First for the NB_Global's
update and second when ovn-northd copies the
options column to the SB_Global.options and its update is received.
i,e
---
2024-01-03T14:47:42.976Z|05434|jsonrpc|DBG|tcp:10.89.0.4:6641:
received notification, method="update3",
params=[["monid","OVN_Northbound"],"00000000-0000-0000-0000-000000000000",{"NB_Global":{"2a7bbc45-90ab-4757-93c9-5f6ea4769c7f":{"modify":{"options":["map",[["e2e_timestamp","1704293262"]]]}}}}]
2024-01-03T14:47:42.976Z|00156|poll_loop(stopwatch0)|DBG|wakeup due to
[POLLIN] on fd 10 (FIFO pipe:[79138091]) at lib/stopwatch.c:456
2024-01-03T14:47:42.976Z|05435|inc_proc_eng|DBG|Initializing new run
2024-01-03T14:47:42.977Z|00157|poll_loop(stopwatch0)|DBG|wakeup due to
[POLLIN] on fd 10 (FIFO pipe:[79138091]) at lib/stopwatch.c:456
2024-01-03T14:47:42.977Z|00158|poll_loop(stopwatch0)|DBG|wakeup due to
[POLLIN] on fd 10 (FIFO pipe:[79138091]) at lib/stopwatch.c:456
2024-01-03T14:47:42.977Z|05510|inc_proc_eng|DBG|northd/en-northd.c:131:
node: northd, old_state Stale, new_state Updated
2024-01-03T14:47:42.977Z|05511|inc_proc_eng|DBG|node: northd,
recompute (failed handler for input NB_nb_global) took 0ms
# recomputes here
2024-01-03T14:47:42.979Z|05555|jsonrpc|DBG|tcp:10.89.0.4:6642: send
request, method="transact",
params=["OVN_Southbound",{"where":[["_uuid","==",["uuid","cb81dba5-4bff-49e0-ae9f-107a43616b77"]]],"row":{"options":["map",[["e2e_timestamp","1704293262"],["mac_prefix","9e:c3:b0"],["max_tunid","16711680"],["name","global"],["northd-backoff-interval-ms","300"],["northd_internal_version","23.09.2-20.29.0-71.6"],["northd_probe_interval","5000"],["svc_monitor_mac","f2:ba:19:99:f1:17"]]]},"op":"update","table":"SB_Global"},{"comment":"ovn-northd","op":"comment"},{"lock":"ovn_northd","op":"assert"}],
id=228
2024-01-03T14:47:42.979Z|00165|poll_loop(stopwatch0)|DBG|wakeup due to
[POLLIN] on fd 10 (FIFO pipe:[79138091]) at lib/stopwatch.c:456
2024-01-03T14:47:42.980Z|05556|poll_loop|DBG|wakeup due to [POLLIN] on
fd 17 (10.89.0.4:37450<->10.89.0.4:6642) at lib/stream-fd.c:157 (0%
CPU usage)
2024-01-03T14:47:42.980Z|00166|poll_loop(stopwatch0)|DBG|wakeup due to
[POLLIN] on fd 10 (FIFO pipe:[79138091]) at lib/stopwatch.c:456
2024-01-03T14:47:42.980Z|05557|jsonrpc|DBG|tcp:10.89.0.4:6642:
received notification, method="update3",
params=[["monid","OVN_Southbound"],"00000000-0000-0000-0000-000000000000",{"SB_Global":{"cb81dba5-4bff-49e0-ae9f-107a43616b77":{"modify":{"options":["map",[["e2e_timestamp","1704293262"]]]}}}}]
2024-01-03T14:47:42.980Z|05558|jsonrpc|DBG|tcp:10.89.0.4:6642:
received reply, result=[{"count":1},{},{}], id=228
# recompute again here
----
> Could you be more specific what recompute are avoided by this patch? I can
> see for example, IPSec option change is handled with I-P, but these are
> really rare changes. It seems most other option changes and chassis feature
> changes would still trigger recompute with this patch.
Right now with the present main, we fall back to full recompute twice
1. CMS writes some key-value to NB_Global.options
2. ovn-northd does a full recompute
3. ovn-northd syncs NB_Global.options to SB_Global.options
4. ovn-northd again does a full recompute when it receives the
update for the SB_Global.options transaction.
>
> > >
> > > Signed-off-by: Numan Siddique <num...@ovn.org>
> > > ---
> > > northd/aging.c | 21 +-
> > > northd/automake.mk | 2 +
> > > northd/en-global-config.c | 588 ++++++++++++++++++++++++++++++++++++++
> > > northd/en-global-config.h | 65 +++++
> > > northd/en-lflow.c | 11 +-
> > > northd/en-northd.c | 52 ++--
> > > northd/en-northd.h | 2 +-
> > > northd/en-sync-sb.c | 22 +-
> > > northd/inc-proc-northd.c | 38 ++-
> > > northd/northd.c | 230 +++------------
> > > northd/northd.h | 24 +-
> > > tests/ovn-northd.at | 256 +++++++++++++++--
> > > 12 files changed, 1014 insertions(+), 297 deletions(-)
> >
> > I think most of the options values we interpret in the the NB_Global and
> > SB_Global tables don't usually change (or don't change often).
> >
> > Doesn't it make sense to not have "proper I-P" for these tables and
> > instead enhance northd_nb_nb_global_handler() to:
> >
> > - if one of the well known NB_Global/SB_Global options change trigger a
> > recompute of the northd node
> > - if one of the other options change then do a plain NB -> SB options sync
> >
> > I hope that can be done in way less than "1014 insertions(+), 297
> > deletions(-)".
> >
> > What do you think?
I did think of this. But even to do this, we need to store the
config smap in some engine node data
(maybe northd engine node) and compare each config option (which we
are interested in) with the
stored one and see if it has changed or not. The function
check_nb_options_out_of_sync() in this patch
already does that.
And even to sync from NB_Global.options to SB_Global.options would
require some amount of code
even if we go with your suggested approach.
Right now with the present main, we fall back to full recompute twice
1. CMS writes some key-value to NB_Global.options
2. ovn-northd does a full recompute
3. ovn-northd syncs NB_Global.options to SB_Global.options
4. ovn-northd again does a full recompute when it receives the
update for the SB_Global.options transaction.
With your suggested approach, we still need to solve (4). That's the
reason I thought of having a separate
engine to handle all these. And it made sense to also handle the
Chassis changes for the chassis_features.
> >
>
> +1. And I am even questioning "well known NB_Global/SB_Global options
> change". Are there really such changes that need to be done frequently in
> production?
They don't change frequently. But the issue is that CMS can write any
key-value to NB_Global.options
and we fall back to recompute twice. Please see above.
>
> Besides, I have a comment to the function check_nb_options_out_of_sync():
> why not simply check all options by comparing two SMAPs?
We do check the smaps first. If no change, then it would be a no-op.
But if the NB_Global.options gets modified due to any foo=bar key
value, we resort
to full recompute and this patch tries to solve this.
This patch checks for all the supported options/keys which we care
about in the NB_Global.options
and if any of these changes we update the newly added engine node and
in the tracked data
we if the NB options (which we care about changed) or SB chassis
features changed.
The nodes which depend on this global node data, can either ignore
these changes, handle these changes
or fall back to full recompute.
Otherwise it would
> be easy to miss the check for a newly added option. The function name also
> looks like it checks all options instead of some selected ones. And the
> criteria for the selection is not clear.
The criteria is simple. We check for all the supported options in
NB_Global.options.
In the future if we add new options we need to enhance this function
to add something like
---
if (config_out_of_sync(&nb->options, &config_data->nb_options,
"newly_added_foo_option", false)) {
return true;
}
---
And I think that's reasonable to expect the author to add this code.
Thanks
Numan
>
> Thanks,
> Han
>
>
> > Thanks,
> > Dumitru
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
