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
-------------------- 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
