[ovs-dev] [PATCH ovn v2 5/5] ovn-controller: Fix incremental processing for logical port references.

[Dumitru Ceara]

On 6/11/21 9:35 PM, Han Zhou wrote:
> If a lflow has an lport name in the match, but when the lflow is
> processed the port-binding is not seen by ovn-controller, the
> corresponding openflow will not be created. Later if the port-binding is
> created/monitored by ovn-controller, the lflow is not reprocessed
> because the lflow didn't change and ovn-controller doesn't know that the
> port-binding affects the lflow. This patch fixes the problem by tracking
> the references when parsing the lflow, even if the port-binding is not
> found when the lflow is firstly parsed. A test case is also added to
> cover the scenario.
> 
> Signed-off-by: Han Zhou <hzhou at ovn.org>
> ---
>  controller/lflow.c          | 64 +++++++++++++++++++++++++++----------
>  controller/lflow.h          |  3 ++
>  controller/ovn-controller.c | 17 ++++++++--
>  tests/ovn.at                | 47 +++++++++++++++++++++++++++
>  4 files changed, 113 insertions(+), 18 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 34eca135a..7ae0ed15e 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -61,6 +61,7 @@ struct lookup_port_aux {
>  
>  struct condition_aux {
>      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> +    const struct sbrec_datapath_binding *dp;
>      const struct sbrec_chassis *chassis;
>      const struct sset *active_tunnels;
>      const struct sbrec_logical_flow *lflow;
> @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp)
>  
>      const struct lookup_port_aux *aux = aux_;
>  
> +    /* Store the name that used to lookup the lport to lflow reference, so that
> +     * in the future when the lport's port binding changes, the logical flow
> +     * that references this lport can be reprocessed. */
> +    lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> +                       &aux->lflow->header_.uuid);
> +
>      const struct sbrec_port_binding *pb
>          = lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name);
>      if (pb && pb->datapath == aux->dp) {
> @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name)
>  {
>      const struct condition_aux *c_aux = c_aux_;
>  
> +    /* Store the port name that used to lookup the lport to lflow reference, so
> +     * that in the future when the lport's port-binding changes the logical
> +     * flow that references this lport can be reprocessed. */
> +    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> +                       &c_aux->lflow->header_.uuid);
> +
>      const struct sbrec_port_binding *pb
>          = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name);
>      if (!pb) {
>          return false;
>      }
>  
> -    /* Store the port_name to lflow reference. */
> -    int64_t dp_id = pb->datapath->tunnel_key;
> -    char buf[16];
> -    get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
> -    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> -                       &c_aux->lflow->header_.uuid);
> -
>      if (strcmp(pb->type, "chassisredirect")) {
>          /* for non-chassisredirect ports */
>          return pb->chassis && pb->chassis == c_aux->chassis;
> @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
>                  int64_t dp_id = dp->tunnel_key;
>                  char buf[16];
>                  get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
> -                lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf,
> -                                   &lflow->header_.uuid);
>                  if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
>                      VLOG_DBG("lflow "UUID_FMT
>                               " port %s in match is not local, skip",
> @@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
>      };
>      struct condition_aux cond_aux = {
>          .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
> +        .dp = dp,
>          .chassis = l_ctx_in->chassis,
>          .active_tunnels = l_ctx_in->active_tunnels,
>          .lflow = lflow,
> @@ -883,7 +888,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
>  
>          /* Cache new entry if caching is enabled. */
>          if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> -            if (cached_expr && !is_cr_cond_present) {
> +            if (cached_expr
> +                && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
> +                                     &lflow->header_.uuid)) {

Why is "!is_cr_cond_present" not OK here?  It seems to me that the
result is the same except that lflow_ref_lookup() will do more work.

>                  lflow_cache_add_matches(l_ctx_out->lflow_cache,
>                                          &lflow->header_.uuid, matches,
>                                          matches_size);
> @@ -1746,19 +1753,44 @@ lflow_processing_end:
>      return handled;
>  }
>  
> +/* Handles a port-binding change that is possibly related to a lport's
> + * residence status on this chassis. */
>  bool
>  lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
>                               struct lflow_ctx_in *l_ctx_in,
>                               struct lflow_ctx_out *l_ctx_out)
>  {
> +    bool ret = true;
>      bool changed;
> -    int64_t dp_id = pb->datapath->tunnel_key;
> -    char pb_ref_name[16];
> -    get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name,
> -                         sizeof(pb_ref_name));
>  
> -    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
> -                                    l_ctx_in, l_ctx_out, &changed);
> +    if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port,
> +                                  l_ctx_in, l_ctx_out, &changed)) {
> +        ret = false;
> +    }
> +    return ret;

This can be simplified:

return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port,
                                l_ctx_in, l_ctx_out, &changed);


Regards,
Dumitru