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

Han Zhou hzhou at ovn.org
Thu May 21 05:43:47 UTC 2020


Hi Numan, I think this patch is not harmful, but not necessary for the I-P
series. And for (2) in the commit message I want to clarify. Please see my
comments below.

On Wed, May 20, 2020 at 12:40 PM <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
>    (1) If F and F' share the same 'sb_uuid', this function doesn't update
the
>        existing flow F with new actions A2.
>
>    (2) If F and F' don't share the same 'sb_uuid', this function adds F'
>        also into the flow table with F and F' having the same hash. While
installing
>        the flows only one will be considered.
>
> This patch fixes (1) by updating the F with the new actions A2.
> This 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). This patch is
> required to update such flows. Otherwise we will have incomplete actions
installed.
>
I think (1) shouldn't happen normally if we follow the principle of
handling deletions first and handle updates by deleting old entries and
then handle as additions. If it happens, it means something is wrong (e.g.
there are multiple flows generated by same lflow but with same match
condition) and we can't handle it anyway in OVS flows. It should be logged
as warning or error instead of debug. I think the log level is a miss when
I submit the initial I-P patches.

> We should also address (2) and it should be fixed
>   - by logging the duplicate flow F'
>   - and discarding F'.
>
> Scenario (2) can happen when
>  - either ovn-northd installs 2 logical flows with same match but with
>    different actions due to some bug in ovn-northd
>
>  - CMS adds 2 ACLs with the same match and priority, but with different
>    actions.
>
(2) was on purpose and we can't drop the F'. If there are 2 lflows with
same match condition, we need to have both in the table, so that later if
one of them is deleted, we know which one to delete. In reality, this can
be a misconfig in ACL and then it is corrected by removing one of them. In
this case we want the final action remained to be the correct one. Consider
below steps:
a) 2 ACLs with same match condition are added: ACL1 has action drop, ACL2
has action allow.
b) flow1 for ACL1 is added with action drop with sb_uuid1,  and flow2 for
ACL2 is added with action allow with sb_uuid2
c) flow1 is installed to OVS with action drop, because we don't know the
real intention of the user so has to choose which ever comes first.
d) ACL1 is deleted by user because the user figured out the problem and
action allow is desired for the ACL, and the flow1 is removed according
sb_uuid1
e) ofctrl_run figures out the difference of the installed flow1 and desired
flow2, they have same match but different actions, so the OVS flow is
updated with action allow.

> However this patch doesn't attempt to fix (2).
>
> Acked-by: Mark Michelson <mmichels at redhat.com>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
>  controller/ofctrl.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index b8a9c2da8..edc22824f 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -642,6 +642,19 @@ ofctrl_recv(const struct ofp_header *oh, enum
ofptype type)
>      }
>  }
>
> +static void
> +ofctrl_swap_flow_actions(struct ovn_flow *a, struct ovn_flow *b)
> +{
> +    struct ofpact *tmp = a->ofpacts;
> +    size_t tmp_len = a->ofpacts_len;
> +
> +    a->ofpacts = b->ofpacts;
> +    a->ofpacts_len = b->ofpacts_len;
> +
> +    b->ofpacts = tmp;
> +    b->ofpacts_len = tmp_len;
> +}
> +
>  /* Flow table interfaces to the rest of ovn-controller. */
>
>  /* Adds a flow to 'desired_flows' with the specified 'match' and
'actions' to
> @@ -667,14 +680,21 @@ 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 {
> +            ofctrl_swap_flow_actions(f, existing_f);
>          }
>          ovn_flow_destroy(f);
>          return;
> --
> 2.26.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list