[ovs-dev] [PATCH ovn 2/2] ovn-controller: Access OVS Datapath table only if supported.

[Flavio Fernandes]

> On Sep 1, 2021, at 3:13 PM, Dumitru Ceara <dceara at redhat.com> wrote:
> 
> Use the newly added IDL API, ovsdb_idl_server_has_table(), through the
> auto generated ovsrec_server_has_datapath_table() to determine if it's
> OK to read and potentially create a Datapath record in the local OVS DB.
> 
> In order to do that we also need to bump the OVS submodule to include
> 7502849e9581 ("ovsdb-idl: Add APIs to query if a table and a column is
> present.").
> 
> Fixes: 56e2cd3a2f06 ("ovn-controller: Detect OVS datapath capabilities.")
> Reported-at: https://bugzilla.redhat.com/1992705
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
> controller/ovn-controller.c |   32 +++++++++++++++++++-------------
> lib/features.c              |    5 +++++
> ovs                         |    2 +-
> 3 files changed, 25 insertions(+), 14 deletions(-)


Tested-by: Flavio Fernandes <flavio at flaviof.com>


> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 739048cf8..0031a1035 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -439,12 +439,11 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>                const struct ovsrec_bridge_table *bridge_table,
>                const struct ovsrec_open_vswitch_table *ovs_table,
>                const struct ovsrec_bridge **br_int_,
> -               const struct ovsrec_datapath **br_int_dp_)
> +               const struct ovsrec_datapath **br_int_dp)
> {
>     const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
> -    const struct ovsrec_datapath *br_int_dp = NULL;
> 
> -    ovs_assert(br_int_ && br_int_dp_);
> +    ovs_assert(br_int_);
>     if (ovs_idl_txn) {
>         if (!br_int) {
>             br_int = create_br_int(ovs_idl_txn, ovs_table);
> @@ -476,15 +475,16 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>                 ovsrec_bridge_set_fail_mode(br_int, "secure");
>                 VLOG_WARN("Integration bridge fail-mode changed to 'secure'.");
>             }
> -            br_int_dp = get_br_datapath(cfg, datapath_type);
> -            if (!br_int_dp) {
> -                br_int_dp = create_br_datapath(ovs_idl_txn, cfg,
> -                                               datapath_type);
> +            if (br_int_dp) {
> +                *br_int_dp = get_br_datapath(cfg, datapath_type);
> +                if (!(*br_int_dp)) {
> +                    *br_int_dp = create_br_datapath(ovs_idl_txn, cfg,
> +                                                    datapath_type);
> +                }
>             }
>         }
>     }
>     *br_int_ = br_int;
> -    *br_int_dp_ = br_int_dp;
> }
> 
> static const char *
> @@ -3519,8 +3519,10 @@ main(int argc, char *argv[])
>             ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
>         const struct ovsrec_bridge *br_int = NULL;
>         const struct ovsrec_datapath *br_int_dp = NULL;
> -        process_br_int(ovs_idl_txn, bridge_table, ovs_table,
> -                       &br_int, &br_int_dp);
> +        process_br_int(ovs_idl_txn, bridge_table, ovs_table, &br_int,
> +                       ovsrec_server_has_datapath_table(ovs_idl_loop.idl)
> +                       ? &br_int_dp
> +                       : NULL);
> 
>         /* Enable ACL matching for double tagged traffic. */
>         if (ovs_idl_txn) {
> @@ -3563,9 +3565,13 @@ main(int argc, char *argv[])
>                                       &chassis_private);
>             }
> 
> -            /* If any OVS feature support changed, force a full recompute. */
> -            if (br_int_dp
> -                    && ovs_feature_support_update(&br_int_dp->capabilities)) {
> +            /* If any OVS feature support changed, force a full recompute.
> +             * 'br_int_dp' is valid only if an OVS transaction is possible.
> +             */
> +            if (ovs_idl_txn
> +                && ovs_feature_support_update(br_int_dp
> +                                              ? &br_int_dp->capabilities
> +                                              : NULL)) {
>                 VLOG_INFO("OVS feature set changed, force recompute.");
>                 engine_set_force_recompute(true);
>             }
> diff --git a/lib/features.c b/lib/features.c
> index 87d04ee3f..fddf4c450 100644
> --- a/lib/features.c
> +++ b/lib/features.c
> @@ -62,8 +62,13 @@ ovs_feature_is_supported(enum ovs_feature_value feature)
> bool
> ovs_feature_support_update(const struct smap *ovs_capabilities)
> {
> +    static struct smap empty_caps = SMAP_INITIALIZER(&empty_caps);
>     bool updated = false;
> 
> +    if (!ovs_capabilities) {
> +        ovs_capabilities = &empty_caps;
> +    }
> +
>     for (size_t i = 0; i < ARRAY_SIZE(all_ovs_features); i++) {
>         enum ovs_feature_value value = all_ovs_features[i].value;
>         const char *name = all_ovs_features[i].name;
> diff --git a/ovs b/ovs
> index daf627f45..7502849e9 160000
> --- a/ovs
> +++ b/ovs
> @@ -1 +1 @@
> -Subproject commit daf627f459ffbc7171d42a2c01f80754bfd54edc
> +Subproject commit 7502849e9581a1dabe53eac51fd34521126c7b3f
> 

I really like git submodules !

One side effect of this change is that deployments that try to build their own flavor of
ovs instead of using this will possibly break. But I think that is fair, as OVN cannot
control OVS releases anyways.

I verified that the code works when using the right OVS version and see build break
when attempting to use OVS without ovsrec_server_has_datapath_table symbol.

-- flaviof