On Fri, Apr 14, 2023 at 12:26 PM Ales Musil <[email protected]> wrote:
> There was a chance for race condition when
> idl_txn wasn't available and the current_br_int_name
> changed before that old tunnels were cleaned up.
> To prevent the race condition update
> the curent_br_int_name only after old tunnel cleanup.
>
>
I forgot to add:
Fixes: 2afc0984d13b ("controller: Clear tunnels from old integration
bridge")
> Signed-off-by: Ales Musil <[email protected]>
> ---
> controller/encaps.c | 21 ++++++++++++++++-----
> controller/encaps.h | 5 +++--
> controller/ovn-controller.c | 26 ++------------------------
> 3 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/controller/encaps.c b/controller/encaps.c
> index 32cd0882c..b69d72584 100644
> --- a/controller/encaps.c
> +++ b/controller/encaps.c
> @@ -36,6 +36,8 @@ VLOG_DEFINE_THIS_MODULE(encaps);
> */
> #define OVN_MVTEP_CHASSISID_DELIM '@'
>
> +static char *current_br_int_name = NULL;
> +
> void
> encaps_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> {
> @@ -409,14 +411,13 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct sbrec_sb_global *sbg,
> const struct ovsrec_open_vswitch_table *ovs_table,
> const struct sset *transport_zones,
> - const struct ovsrec_bridge_table *bridge_table,
> - const char *br_int_name)
> + const struct ovsrec_bridge_table *bridge_table)
> {
> if (!ovs_idl_txn || !br_int) {
> return;
> }
>
> - if (!br_int_name) {
> + if (!current_br_int_name) {
> /* The controller has just started, we need to look through all
> * bridges for old tunnel ports. */
> char *tunnel_prefix = xasprintf("ovn%s-",
> get_chassis_idx(ovs_table));
> @@ -431,14 +432,18 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
> }
>
> free(tunnel_prefix);
> - } else if (strcmp(br_int_name, br_int->name)) {
> + current_br_int_name = xstrdup(br_int->name);
> + } else if (strcmp(current_br_int_name, br_int->name)) {
> /* The integration bridge was changed, clear tunnel ports from
> * the old one. */
> const struct ovsrec_bridge *old_br_int =
> - get_bridge(bridge_table, br_int_name);
> + get_bridge(bridge_table, current_br_int_name);
> if (old_br_int) {
> clear_old_tunnels(old_br_int, "", 0);
> }
> +
> + free(current_br_int_name);
> + current_br_int_name = xstrdup(br_int->name);
> }
>
> const struct sbrec_chassis *chassis_rec;
> @@ -553,3 +558,9 @@ encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn,
>
> return !any_changes;
> }
> +
> +void
> +encaps_destroy(void)
> +{
> + free(current_br_int_name);
> +}
> diff --git a/controller/encaps.h b/controller/encaps.h
> index cf38dac1a..3e58b3c82 100644
> --- a/controller/encaps.h
> +++ b/controller/encaps.h
> @@ -36,8 +36,7 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct sbrec_sb_global *,
> const struct ovsrec_open_vswitch_table *,
> const struct sset *transport_zones,
> - const struct ovsrec_bridge_table *bridge_table,
> - const char *br_int_name);
> + const struct ovsrec_bridge_table *bridge_table);
>
> bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct ovsrec_bridge *br_int);
> @@ -48,4 +47,6 @@ bool encaps_tunnel_id_parse(const char *tunnel_id, char
> **chassis_id,
> bool encaps_tunnel_id_match(const char *tunnel_id, const char
> *chassis_id,
> const char *encap_ip);
>
> +void encaps_destroy(void);
> +
> #endif /* controller/encaps.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index e170e9262..c094cb74d 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -536,21 +536,6 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
> *br_int_ = br_int;
> }
>
> -static void
> -consider_br_int_change(const struct ovsrec_bridge *br_int, char
> **current_name)
> -{
> - ovs_assert(current_name);
> -
> - if (!*current_name) {
> - *current_name = xstrdup(br_int->name);
> - }
> -
> - if (strcmp(*current_name, br_int->name)) {
> - free(*current_name);
> - *current_name = xstrdup(br_int->name);
> - }
> -}
> -
> static void
> update_ssl_config(const struct ovsrec_ssl_table *ssl_table)
> {
> @@ -4933,8 +4918,6 @@ main(int argc, char *argv[])
> char *ovn_version = ovn_get_internal_version();
> VLOG_INFO("OVN internal version is : [%s]", ovn_version);
>
> - char *current_br_int_name = NULL;
> -
> /* Main loop. */
> exiting = false;
> restart = false;
> @@ -5088,8 +5071,7 @@ main(int argc, char *argv[])
> sbrec_sb_global_first(ovnsb_idl_loop.idl),
> ovs_table,
> &transport_zones,
> - bridge_table,
> - current_br_int_name);
> + bridge_table);
>
> stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
> time_msec());
> @@ -5276,10 +5258,6 @@ main(int argc, char *argv[])
> stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
> time_msec());
> }
> - /* The name needs to be reflected at the end of the block.
> - * This allows us to detect br-int changes and act
> - * accordingly. */
> - consider_br_int_change(br_int, ¤t_br_int_name);
> }
>
> if (!engine_has_run()) {
> @@ -5463,7 +5441,6 @@ loop_done:
> }
>
> free(ovn_version);
> - free(current_br_int_name);
> unixctl_server_destroy(unixctl);
> lflow_destroy();
> ofctrl_destroy();
> @@ -5471,6 +5448,7 @@ loop_done:
> binding_destroy();
> patch_destroy();
> mirror_destroy();
> + encaps_destroy();
> if_status_mgr_destroy(if_mgr);
> shash_destroy(&vif_plug_deleted_iface_ids);
> shash_destroy(&vif_plug_changed_iface_ids);
> --
> 2.39.2
>
>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
