[ovs-dev] [PATCH ovn v5 4/9] ofctrl_check_and_add_flow: Replace the actions of an existing flow if actions have changed.

[Mark Michelson]

On 5/11/20 5:46 AM, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
> 
> If ofctrl_check_and_add_flow(F') is called where flow F' has match-actions (M, A2)
> and if there already exists a flow F with match-actions (M, A1) in the desired flow
> table, then this function  doesn't update the existing flow F with new actions
> actions A2.
> 
> This patch is required for the upcoming patch in this series which
> adds incremental processing for OVS interface in the flow output stage.
> Since we will be not be clearing the flow output data if these changes
> are handled incrementally, some of the existing flows will be updated
> with new actions. One such example would be flows in physical
> table OFTABLE_LOCAL_OUTPUT (table 33). And this patch is required to
> update such flows. Otherwise we will have incomplete actions installed.

I understand the explanation for this patch, but I'm wondering if this 
now makes it possible to do silly things like define ACLs with duplicate 
matches but different verdicts. Previously, if you did this, the first 
ACL would get installed, and you'd see a debug message about dropping 
the duplicate flow. With this change, whichever ACL is processed last 
wins and there's no debug message.

Maybe we could store a value in the ovn_flow that indicates when the 
flow was added to the desired flow table. Perhaps each time the 
incremental engine runs, an int is incremented. This way, you could have 
logic like:

existing_f = ovn_flow_lookup();
if (existing_f) {
     if (existing_f->age == f->age || ofpacts_equal(existing_f, f)) {
         // This is a duplicate flow
     } else {
         // replace existing_f's actions with f's actions
     }
}

> 
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
>   controller/ofctrl.c | 23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 4b51cd86e..8f2f13767 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table,
>   
>       ovn_flow_log(f, "ofctrl_add_flow");
>   
> -    if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) {
> -        if (log_duplicate_flow) {
> -            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> -            if (!VLOG_DROP_DBG(&rl)) {
> -                char *s = ovn_flow_to_string(f);
> -                VLOG_DBG("dropping duplicate flow: %s", s);
> -                free(s);
> +    struct ovn_flow *existing_f =
> +        ovn_flow_lookup(&flow_table->match_flow_table, f, true);
> +    if (existing_f) {
> +        if (ofpacts_equal(f->ofpacts, f->ofpacts_len,
> +                          existing_f->ofpacts, existing_f->ofpacts_len)) {
> +            if (log_duplicate_flow) {
> +                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> +                if (!VLOG_DROP_DBG(&rl)) {
> +                    char *s = ovn_flow_to_string(f);
> +                    VLOG_DBG("dropping duplicate flow: %s", s);
> +                    free(s);
> +                }
>               }
> +        } else {
> +            free(existing_f->ofpacts);
> +            existing_f->ofpacts = xmemdup(f->ofpacts, f->ofpacts_len);
> +            existing_f->ofpacts_len = f->ofpacts_len;
>           }
>           ovn_flow_destroy(f);
>           return;
>