On 5/24/22 01:10, Han Zhou wrote:
> 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:
Hi Han,
Oh, I see what happened.
> ---------------------------------------------------------------------------
> 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
I did cherry pick from upstream/main but then, when fixing conflicts, I
took the tests/ovn-controller.at change from patchwork. My bad, sorry
for the confusion.
> 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.
This I added manually and you're right I shouldn't have.
> 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).
>
The incremental diff looks good to me. I think it's fine to apply it.
> Thanks,
> Han
>
Thanks a lot for the careful review!
Regards,
Dumiru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
