| Message ID | 20220328053943.1606715-6-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Use ct_mark for masked access to make flows HW-offloading friendly. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Mon, Mar 28, 2022 at 1:48 AM Han Zhou <hzhou@ovn.org> 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 <hzhou@ovn.org> > --- > 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. 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Mar 29, 2022 at 2:19 PM Numan Siddique <numans@ovn.org> wrote: > > On Mon, Mar 28, 2022 at 1:48 AM Han Zhou <hzhou@ovn.org> 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 <hzhou@ovn.org> > > --- > > 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 -------------------- 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Tue, Mar 29, 2022 at 5:43 PM Han Zhou <hzhou@ovn.org> wrote: > > On Tue, Mar 29, 2022 at 2:19 PM Numan Siddique <numans@ovn.org> wrote: > > > > On Mon, Mar 28, 2022 at 1:48 AM Han Zhou <hzhou@ovn.org> 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 <hzhou@ovn.org> > > > --- > > > 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 <numans@ovn.org> 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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 +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
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 <hzhou@ovn.org> --- controller/ovn-controller.c | 66 +++++++++++++++++++++++++++++++++++++ tests/ovn-controller.at | 46 ++++++++++++++++++++++++++ 2 files changed, 112 insertions(+)