On Fri, May 15, 2020 at 6:50 AM Mark Michelson <[email protected]> wrote:
> On 5/11/20 5:46 AM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > 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 > } > } > > I don't think we really need to maintain an age param for this. I did some testing with the present master.. I added the below ACLs to a logical switch ovn-nbctl acl-add sw0 from-lport 1002 "ip4.src == 10.0.0.4" "drop" ovn-nbctl acl-add sw0 from-lport 1003 "ip4.src == 10.0.0.4" "allow" And the corresponding OF flows are: ** cookie=0xd23a2018, duration=6.057s, table=14, n_packets=0, n_bytes=0, priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=drop cookie=0x9bb3ead1, duration=3.218s, table=14, n_packets=0, n_bytes=0, priority=2003,ip,metadata=0x1,nw_src=10.0.0.4 actions=resubmit(,15) *** And then changed the second ACL with priority 1003 to 1002 (ovn-nbctl actl-add doesn't allow to add duplicate ACLs) ovn-nbctl set acl $acl_id priority=1002 After this I see the below flows: cookie=0x8515a20e, duration=15.873s, table=14, n_packets=0, n_bytes=0, priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=resubmit(,15) I was expecting to see a debug warning message in ovn-controller. But It doesn't show up. This happens because right now ovn_flow_lookup() is called here [1] with cmp_sb_uuid set to true. Since both the above ACLs will have different sb_uuid, ovn_flow_lookup() fails. I think we should pass 'cmp_sb_uuid' to false. I think we should consider a flow as duplicate (with the same match) and ignore it if the sb_uuid is different (as in the case of above ACLs) And if sb_uuid matches, then we should replace the existing actions and this is what this patch does. Examples of such flows are flows with MC_Flood, MC_Unknown. Thanks Numan > > > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > 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; > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
