On Wed, Apr 30, 2025 at 2:19 PM Mark Michelson <mmich...@redhat.com> wrote: > > Hi Numan, > > The patch looks good to me. My only suggestion is to change the option > so it is positively-worded instead of negatively-worded. In other words, > instead of "disable_chassis_nb_cfg_update", call it > "enable_chassis_nb_cfg_update" and have it true by default. It tends to > be harder to follow options when they are worded negatively, especially > when they are false by default. > > Thanks! >
Thanks Mark for the review comments. It makes total sense. I addressed your comments and submitted v2. Please take a look - https://patchwork.ozlabs.org/project/ovn/patch/20250430210727.2939034-1-num...@ovn.org/ Numan > On 4/30/25 00:29, num...@ovn.org wrote: > > From: Numan Siddique <num...@ovn.org> > > > > This commit adds a new nb_global option "disable_chassis_nb_cfg_update" > > and if this is set to true, ovn-controller will no longer update > > the nb_cfg in its chassis_private record. It would still update the > > external_ids:ovn-nb-cfg in the local OVS integration bridge. > > > > This option will be useful for large scaled deployments which wants > > to ensure and monitor that southbound db changes are handled by > > ovn-controllers and yet don't want ovn-controllers to update their > > chassis private records. > > > > Signed-off-by: Numan Siddique <num...@ovn.org> > > --- > > controller/ovn-controller.c | 29 ++++++++++++++++++++++++++--- > > ovn-nb.xml | 11 +++++++++++ > > ovn-sb.xml | 11 +++++++++++ > > tests/ovn-controller.at | 31 +++++++++++++++++++++++++++++++ > > 4 files changed, 79 insertions(+), 3 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 37406f9235..415008287c 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -827,7 +827,8 @@ static void > > store_nb_cfg(struct ovsdb_idl_txn *sb_txn, struct ovsdb_idl_txn *ovs_txn, > > const struct sbrec_chassis_private *chassis, > > const struct ovsrec_bridge *br_int, > > - unsigned int delay_nb_cfg_report) > > + unsigned int delay_nb_cfg_report, > > + bool disable_ch_nb_cfg_update) > > { > > struct ofctrl_acked_seqnos *acked_nb_cfg_seqnos = > > ofctrl_acked_seqnos_get(ofctrl_seq_type_nb_cfg); > > @@ -849,7 +850,8 @@ store_nb_cfg(struct ovsdb_idl_txn *sb_txn, struct > > ovsdb_idl_txn *ovs_txn, > > > > long long ts_now = time_wall_msec(); > > > > - if (sb_txn && chassis && cur_cfg != chassis->nb_cfg) { > > + if (sb_txn && chassis && cur_cfg != chassis->nb_cfg > > + && !disable_ch_nb_cfg_update) { > > sbrec_chassis_private_set_nb_cfg(chassis, cur_cfg); > > sbrec_chassis_private_set_nb_cfg_timestamp(chassis, ts_now); > > > > @@ -3560,6 +3562,7 @@ struct ed_type_northd_options { > > * be tunnelled or sent via the localnet > > * port. Default value is 'false'. */ > > bool register_consolidation; > > + bool disable_ch_nb_cfg_update; > > }; > > > > > > @@ -3603,6 +3606,12 @@ en_northd_options_run(struct engine_node *node, void > > *data) > > false) > > : false; > > > > + n_opts->disable_ch_nb_cfg_update = > > + sb_global > > + ? smap_get_bool(&sb_global->options, > > "disable_chassis_nb_cfg_update", > > + false) > > + : false; > > + > > engine_set_node_state(node, EN_UPDATED); > > } > > > > @@ -3648,6 +3657,17 @@ en_northd_options_sb_sb_global_handler(struct > > engine_node *node, void *data) > > engine_set_node_state(node, EN_UPDATED); > > } > > > > + bool disable_ch_nb_cfg_update = > > + sb_global > > + ? smap_get_bool(&sb_global->options, > > "disable_chassis_nb_cfg_update", > > + false) > > + : false; > > + > > + if (disable_ch_nb_cfg_update != n_opts->disable_ch_nb_cfg_update) { > > + n_opts->disable_ch_nb_cfg_update = disable_ch_nb_cfg_update; > > + engine_set_node_state(node, EN_UPDATED); > > + } > > + > > return true; > > } > > > > @@ -6462,8 +6482,11 @@ main(int argc, char *argv[]) > > engine_clear_force_recompute(); > > } > > > > + struct ed_type_northd_options *n_opts = > > + engine_get_data(&en_northd_options); > > store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private, > > - br_int, delay_nb_cfg_report); > > + br_int, delay_nb_cfg_report, > > + n_opts ? n_opts->disable_ch_nb_cfg_update : > > false); > > > > if (pending_pkt.conn) { > > struct ed_type_addr_sets *as_data = > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 17346f2284..7878d0cbb1 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -429,6 +429,17 @@ > > </p> > > </column> > > > > + <column name="options" key="disable_chassis_nb_cfg_update"> > > + <p> > > + If set to true, ovn-controllers will no longer update the > > + <ref column="nb_cfg" table="Chassis_Private" > > db="OVN_Southbound"/> > > + column in the <ref table="Chassis_Private" db="OVN_Southbound"/> > > + table of the <ref db="OVN_Southbound"/> database. They will > > still > > + update the <code>external_ids:ovn-nb-cfg</code> in the local > > + OVS integration bridge. > > + </p> > > + </column> > > + > > <group title="Options for configuring interconnection route > > advertisement"> > > <p> > > These options control how routes are advertised between OVN > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index 9777cc64ad..9223019be0 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -173,6 +173,17 @@ > > options are described individually below. > > </column> > > > > + <column name="options" key="disable_chassis_nb_cfg_update"> > > + <p> > > + If set to true, ovn-controller will no longer update the > > + <ref column="nb_cfg" table="Chassis_Private" > > db="OVN_Southbound"/> > > + column in the <ref table="Chassis_Private" db="OVN_Southbound"/> > > + table of the <ref db="OVN_Southbound"/> database. It will still > > + update the <code>external_ids:ovn-nb-cfg</code> in the local > > + OVS integration bridge. > > + </p> > > + </column> > > + > > <group title="Options for configuring BFD"> > > <p> > > These options apply when <code>ovn-controller</code> configures > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > index fdd574c206..ef469d5e13 100644 > > --- a/tests/ovn-controller.at > > +++ b/tests/ovn-controller.at > > @@ -3813,3 +3813,34 @@ start_controller > > > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > + > > +AT_SETUP([ovn-cntroller options:disable_chassis_nb_cfg_update]) > > +ovn_start > > + > > +net_add n1 > > +sim_add hv1 > > +as hv1 > > +check ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.11 24 geneve > > +check ovn-nbctl --wait=hv sync > > + > > +check ovn-nbctl set nb_global . nb_cfg=100 > > +wait_column 100 chassis_priv nb_cfg name=hv1 > > +AT_CHECK([as hv1 ovs-vsctl get bridge br-int external_ids:ovn-nb-cfg], > > [0], ["100" > > +]) > > + > > +check ovn-nbctl set nb_global . options:disable_chassis_nb_cfg_update=true > > +check ovn-nbctl set nb_global . nb_cfg=200 > > +OVS_WAIT_UNTIL([ > > + ovs_nb_cfg=$(as hv1 ovs-vsctl get bridge br-int > > external_ids:ovn-nb-cfg | sed 's/"//g') > > + test "$ovs_nb_cfg" = "200" > > +]) > > + > > +check_column 100 chassis_priv nb_cfg name=hv1 > > +check ovn-nbctl set nb_global . options:disable_chassis_nb_cfg_update=false > > +wait_column 200 chassis_priv nb_cfg name=hv1 > > + > > +check ovn-nbctl --wait=hv sync > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
