On Thu, May 21, 2020 at 11:14 AM Han Zhou <hz...@ovn.org> wrote:
> 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.
>
You're right. This patch is not needed. It was earlier needed when the
series
was not handling run time data changes.
My bad that I didn't test without this patch. In v8, I've dropped this
patch.
Thanks for the review and comments.
>
> On Wed, May 20, 2020 at 12:40 PM <num...@ovn.org> wrote:
> >
> > From: Numan Siddique <num...@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.
>
Ok. Thanks for the explanation.
Numan
>
> > However this patch doesn't attempt to fix (2).
> >
> > Acked-by: Mark Michelson <mmich...@redhat.com>
> > Signed-off-by: Numan Siddique <num...@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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
