On Tue, Mar 29, 2022 at 5:43 PM Han Zhou <[email protected]> wrote: > > On Tue, Mar 29, 2022 at 2:19 PM Numan Siddique <[email protected]> wrote: > > > > On Mon, Mar 28, 2022 at 1:48 AM Han Zhou <[email protected]> wrote: > > > > > > Add an engine node en_northd_internal_version as an input to > > > en_lflow_output. When this node is updated, it triggers a recompute for > > > en_lflow_output. This node adds SB_Global as its only input, and it is > > > updated only when SB_Global's options:northd_internal_version is > updated. > > > > > > In a later patch the northd_internal_version will be used in > > > en_lflow_output and impact flow generation. > > > > > > Signed-off-by: Han Zhou <[email protected]> > > > --- > > > controller/ovn-controller.c | 66 +++++++++++++++++++++++++++++++++++++ > > > tests/ovn-controller.at | 46 ++++++++++++++++++++++++++ > > > 2 files changed, 112 insertions(+) > > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index ea5e9df41..26ca97825 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -954,6 +954,7 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > > } > > > > > > #define SB_NODES \ > > > + SB_NODE(sb_global, "sb_global") \ > > > SB_NODE(chassis, "chassis") \ > > > SB_NODE(encap, "encap") \ > > > SB_NODE(address_set, "address_set") \ > > > @@ -2194,6 +2195,65 @@ non_vif_data_ovs_iface_handler(struct > engine_node *node, void *data OVS_UNUSED) > > > return local_nonvif_data_handle_ovs_iface_changes(iface_table); > > > } > > > > > > +struct ed_type_northd_internal_version { > > > + char *ver; > > > +}; > > > + > > > + > > > +static void * > > > +en_northd_internal_version_init(struct engine_node *node OVS_UNUSED, > > > + struct engine_arg *arg OVS_UNUSED) > > > +{ > > > + struct ed_type_northd_internal_version *n_ver = xzalloc(sizeof > *n_ver); > > > + n_ver->ver = xstrdup(""); > > > + return n_ver; > > > +} > > > + > > > +static void > > > +en_northd_internal_version_cleanup(void *data) > > > +{ > > > + struct ed_type_northd_internal_version *n_ver = data; > > > + free(n_ver->ver); > > > +} > > > + > > > +static void > > > +en_northd_internal_version_run(struct engine_node *node, void *data) > > > +{ > > > + struct ed_type_northd_internal_version *n_ver = data; > > > + struct sbrec_sb_global_table *sb_global_table = > > > + (struct sbrec_sb_global_table *)EN_OVSDB_GET( > > > + engine_get_input("SB_sb_global", node)); > > > + const struct sbrec_sb_global *sb_global = > > > + sbrec_sb_global_table_first(sb_global_table); > > > + free(n_ver->ver); > > > + n_ver->ver = > > > + xstrdup(sb_global ? smap_get_def(&sb_global->options, > > > + "northd_internal_version", > "") : ""); > > > + engine_set_node_state(node, EN_UPDATED); > > > +} > > > + > > > +static bool > > > +en_northd_internal_version_sb_sb_global_handler(struct engine_node > *node, > > > + void *data) > > > +{ > > > + struct ed_type_northd_internal_version *n_ver = data; > > > + struct sbrec_sb_global_table *sb_global_table = > > > + (struct sbrec_sb_global_table *)EN_OVSDB_GET( > > > + engine_get_input("SB_sb_global", node)); > > > + const struct sbrec_sb_global *sb_global = > > > + sbrec_sb_global_table_first(sb_global_table); > > > + > > > + const char *new_ver = > > > + sb_global ? smap_get_def(&sb_global->options, > > > + "northd_internal_version", "") : ""; > > > + if (strcmp(new_ver, n_ver->ver)) { > > > + free(n_ver->ver); > > > + n_ver->ver = xstrdup(new_ver); > > > + engine_set_node_state(node, EN_UPDATED); > > > + } > > > + return true; > > > +} > > > + > > > struct lflow_output_persistent_data { > > > struct lflow_cache *lflow_cache; > > > }; > > > @@ -3243,6 +3303,7 @@ main(int argc, char *argv[]) > > > ENGINE_NODE(flow_output, "flow_output"); > > > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets"); > > > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); > > > + ENGINE_NODE(northd_internal_version, "northd_internal_version"); > > > > > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > > > SB_NODES > > > @@ -3291,6 +3352,11 @@ main(int argc, char *argv[]) > > > engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL); > > > engine_add_input(&en_pflow_output, &en_ovs_bridge, NULL); > > > > > > + engine_add_input(&en_northd_internal_version, &en_sb_sb_global, > > > + en_northd_internal_version_sb_sb_global_handler); > > > + > > > + engine_add_input(&en_lflow_output, &en_northd_internal_version, > NULL); > > > + > > > /* Keep en_addr_sets before en_runtime_data because > > > * lflow_output_runtime_data_handler may *partially* reprocess a > lflow when > > > * the lflow is attached to a DP group and a new DP in that DP > group is > > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > > index 6e4c24f0f..236f1c015 100644 > > > --- a/tests/ovn-controller.at > > > +++ b/tests/ovn-controller.at > > > @@ -2057,3 +2057,49 @@ AT_CHECK([echo $(($reprocess_count_new - > $reprocess_count_old))], [0], [2 > > > > > > OVN_CLEANUP([hv1]) > > > AT_CLEANUP > > > + > > > +AT_SETUP([ovn-controller - I-P handle northd_internal_version change]) > > > + > > > +ovn_start --backup-northd=none > > > + > > > +net_add n1 > > > +sim_add hv1 > > > +as hv1 > > > +check ovs-vsctl add-br br-phys > > > +ovn_attach n1 br-phys 192.168.0.1 > > > +check ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > > + set interface hv1-vif1 external-ids:iface-id=ls1-lp1 > > > + > > > +check ovn-nbctl ls-add ls1 > > > + > > > +check ovn-nbctl lsp-add ls1 ls1-lp1 \ > > > +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01" > > > + > > > +wait_for_ports_up > > > +ovn-appctl -t ovn-controller vlog/set file:dbg > > > + > > > +read_counter() { > > > + ovn-appctl -t ovn-controller coverage/read-counter $1 > > > +} > > > + > > > +# nb_cfg update in sb_global shouldn't trigger lflow_run. > > > +lflow_run_old=$(read_counter lflow_run) > > > +ovn-nbctl --wait=hv sync > > > +lflow_run_new=$(read_counter lflow_run) > > > +AT_CHECK([echo $(($lflow_run_new - $lflow_run_old))], [0], [0 > > > +]) > > > + > > > +# northd_internal_version update in sb_global:options should trigger > lflow_run. > > > +as northd > > > +OVS_APP_EXIT_AND_WAIT(ovn-northd) > > > +as hv1 > > > +lflow_run_old=$(read_counter lflow_run) > > > +check ovn-sbctl set SB_Global . options:northd_internal_version=foo > > > +sleep 0.1 > > > > Thanks for the patch series. > > > > I've just one minor comment here. Instead of sleep, I'd suggest > > using OVS_WAIT_UNTIL() to avoid flaky test failure in case CI is slow. > > Thanks Numan for the comment. I didn't use OVS_WAIT_UNTIL because I wonder > if we wait long enough there can be other events triggering lflow_run(), > which would pass this test even if the internal-version change didn't > trigger the lflow_run(). > I thought about a cleaner way to test it without using sleep, and also > avoid killing ovn-northd. Please take a look at the below diff, and if it > is ok I will apply it with this minor change: > > -------------------- 8>< > -------------------------------------------------------------- ><8 > ------------------------ > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 236f1c015..88f230541 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -2084,22 +2084,21 @@ read_counter() { > > # nb_cfg update in sb_global shouldn't trigger lflow_run. > lflow_run_old=$(read_counter lflow_run) > -ovn-nbctl --wait=hv sync > +check ovn-nbctl --wait=hv sync > lflow_run_new=$(read_counter lflow_run) > AT_CHECK([echo $(($lflow_run_new - $lflow_run_old))], [0], [0 > ]) > > # northd_internal_version update in sb_global:options should trigger > lflow_run. > -as northd > -OVS_APP_EXIT_AND_WAIT(ovn-northd) > -as hv1 > +# The below steps should cause northd_internal_version change twice. One by > +# ovn-sbctl, and the other by ovn-northd to change it back. > + > lflow_run_old=$(read_counter lflow_run) > check ovn-sbctl set SB_Global . options:northd_internal_version=foo > -sleep 0.1 > +check ovn-nbctl --wait=hv sync > lflow_run_new=$(read_counter lflow_run) > -AT_CHECK([echo $(($lflow_run_new - $lflow_run_old))], [0], [1 > +AT_CHECK([echo $(($lflow_run_new - $lflow_run_old))], [0], [2 > ]) > > -as northd start_daemon ovn-northd > OVN_CLEANUP([hv1]) > AT_CLEANUP
This looks much cleaner. LGTM Acked-by: Numan Siddique <[email protected]> Numan > -------------------- 8>< > -------------------------------------------------------------- ><8 > ------------------------ > > Thanks, > Han > > > > > I've acked the entire series in patch 0. > > > > Thanks > > Numan > > > > > > > +lflow_run_new=$(read_counter lflow_run) > > > +AT_CHECK([echo $(($lflow_run_new - $lflow_run_old))], [0], [1 > > > +]) > > > + > > > +as northd start_daemon ovn-northd > > > +OVN_CLEANUP([hv1]) > > > +AT_CLEANUP > > > -- > > > 2.30.2 > > > > > > _______________________________________________ > > > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
