[ovs-dev] [PATCH ovn v2] ovn-controller: Handle changes in requested-tnl-key

[Dumitru Ceara]

On 9/12/20 9:10 AM, Mark Gray wrote:
> runtime_data_sb_datapath_binding_handler() only handles deletion of rows
> in the Datapath_Binding table in OVN-SB. The user can update the
> requested-tnl-key by the following command:
> 
> ovn-nbctl set logical_switch sw0 other_config:requested-tnl-key=<key>
> 
> This command modifies the tunnel_key column. This patch
> ensures that an update of this column is handled by the
> incremental processing engine by forcing a recompute
> of the runtime_data node.
> 
> As it is expected that changing the requested-tnl-key is not updated
> frequently, we do not attempt to make this update incrementally.
> 
> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> ---
>  controller/ovn-controller.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

Hi Mark,

Would you mind adding a test case for this in ovn.at?

> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 995805470..106f8eae1 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1362,8 +1362,8 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data)
>  }
>  
>  static bool
> -runtime_data_sb_datapath_binding_handler(struct engine_node *node OVS_UNUSED,
> -                                         void *data OVS_UNUSED)
> +runtime_data_sb_datapath_binding_handler(struct engine_node *node,
> +                                         void *data)
>  {
>      struct sbrec_datapath_binding_table *dp_table =
>          (struct sbrec_datapath_binding_table *)EN_OVSDB_GET(
> @@ -1378,6 +1378,18 @@ runtime_data_sb_datapath_binding_handler(struct engine_node *node OVS_UNUSED,
>                  return false;
>              }
>          }
> +
> +        /* Force recompute when the tunnel_key is updated. However,
> +           don't update if the external_id column is updated as this
> +           can be done when a logical switch or logical router is
> +           added which does not recquire a recompute.
> +        */
> +        if (sbrec_datapath_binding_is_updated(dp,
> +                 SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY) &&
> +            !sbrec_datapath_binding_is_updated(dp,
> +                 SBREC_DATAPATH_BINDING_COL_EXTERNAL_IDS)) {
> +            return false;
> +        }
>      }
>  
>      return true;
> 

I'm not sure whether this is completely correct.

What if the sequence of operations is:

ovn-nbctl set logical_switch sw other_config:requested-tnl-key=42
# ovn-northd writes to datapath_binding.tunnel_key

ovn-sbctl set datapath_binding sw external_ids:foo=bar

# It can happen that ovn-controller gets both SB updates at the
# same time and we end up returning true.

That would mean we successfully "incrementally processed" the datapath
binding update but we actually didn't update the flows, right?

Or am I missing something?

Thanks,
Dumitru