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

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@redhat.com>
---
 controller/ovn-controller.c |   32 +++++++++++++++++++-------------
 lib/features.c              |    5 +++++
 ovs                         |    2 +-
 3 files changed, 25 insertions(+), 14 deletions(-)

> On Sep 1, 2021, at 3:13 PM, Dumitru Ceara <dceara@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@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@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

On 9/1/21 10:57 PM, Flavio Fernandes wrote:
> 
> 
>> On Sep 1, 2021, at 3:13 PM, Dumitru Ceara <dceara@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@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@flaviof.com>
> 

Thanks!

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

This is not the first time we added code that uses new OVS APIs.  It was
actually one of the reasons to move to OVS as a submodule IIRC, to make
it easier for users to build OVN.

This doesn't, however, impose restrictions (or set guarantees) about
what OVS versions should be used at runtime (e.g., for ovsdb-server and
ovs-vswitchd).

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

Thanks again!

> -- flaviof
> 

Regards,
Dumitru