On Mon, May 23, 2022 at 12:39 PM Dumitru Ceara <[email protected]> wrote:
>
> From: Han Zhou <[email protected]>
>
> 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]>
> Acked-by: Numan Siddique <[email protected]>
> (cherry picked from commit c2eeb2c98ea860dbbc7eee5e9bae8a65769b0da3)
> Signed-off-by: Dumitru Ceara <[email protected]>
> ---
> controller/ovn-controller.c | 66
+++++++++++++++++++++++++++++++++++++++++++
> tests/ovn-controller.at | 48 +++++++++++++++++++++++++++++++
> 2 files changed, 114 insertions(+)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index b7272d3ec..1457bb04d 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -953,6 +953,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") \
> @@ -2161,6 +2162,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;
> };
> @@ -3252,6 +3312,7 @@ main(int argc, char *argv[])
> ENGINE_NODE(flow_output, "flow_output");
> ENGINE_NODE(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
> @@ -3300,6 +3361,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);
> +
> engine_add_input(&en_lflow_output, &en_addr_sets,
> lflow_output_addr_sets_handler);
> engine_add_input(&en_lflow_output, &en_port_groups,
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 2f39e5f3e..2c6e6e492 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -853,3 +853,51 @@ OVS_WAIT_UNTIL([as hv1 ovs-ofctl dump-flows br-int |
grep table=38 | grep -q "re
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +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
> +])
>
Thanks Dumitru! I noticed a difference of this patch compared with the
upstream one:
---------------------------------------------------------------------------
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 2c6e6e492..d24f3435e 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -854,7 +854,6 @@ OVN_CLEANUP([hv1])
AT_CLEANUP
])
-OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn-controller - I-P handle northd_internal_version change])
ovn_start --backup-northd=none
@@ -881,23 +880,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
-])
---------------------------------------------------------------
It looks like you were using the old patches from patchwork to backport
instead of the upstream? There was this small difference discussed in an
email without sending a new version:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-March/392866.html
The diff here is almost the same as the one discussed in the email, except
the OVN_FOR_EACH_NORTHD macro. Did you add the OVN_FOR_EACH_NORTHD manually
when backporting? I didn't add it because this is a test for ovn-controller
and the northd (whether it is C or DDlog, or the DP group=true/false)
shouldn't matter.
Please take a look and if it is ok, I will apply to the branch-21.12 with
the above diff on top of your backport series (the rest of the series LGTM).
Thanks,
Han
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
