| Message ID | 20200520194001.2351927-1-numans@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Incremental processing improvements. | expand |
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@ovn.org> wrote: > > From: Numan Siddique <numans@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@redhat.com> > Signed-off-by: Numan Siddique <numans@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, May 21, 2020 at 11:14 AM Han Zhou <hzhou@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 <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@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 <mmichels@redhat.com> > > Signed-off-by: Numan Siddique <numans@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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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;