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. 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. > > > > 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? > +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? Besides, I have a comment to the function check_nb_options_out_of_sync(): why not simply check all options by comparing two SMAPs? 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. 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
