On 24 July 2016 at 10:07, Ryan Moats <rmo...@us.ibm.com> wrote: > In the physical processing of ovn-controller, there are two > sets of OF flows that are still fully recalculated every cycle: > > Flows that aren't associated with any logical flow, and > Flows calculated based on multicast groups > > Because these flows are recalculated fully each cycle, full > duplicates of existing OF flows are created and the OF management > code in ovn-controller pollutes the logs with false positive > warnings about repeated duplicates. > > As a short term measure, ignore full duplicates for both of > these types of flows, but still warn if the action changes > (as that is not expected and may be indicative of a problem). > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com> >
Even with this patch, I get consistent unit test failures when I run OVN system tests via : make check-kernel TESTSUITEFLAGS="-k ovn" The test that fails has the following warning: +2016-07-26T09:58:04.535Z|00013|ofctrl|WARN|duplicate flow with modified action for parent a356d28e-84f1-4984-94b2-3ee5a3db2b9b: table_id=32, priority=100, reg15=0xffff,metadata=0x6, actions=set_field:0x1->reg15,resubmit(,34),set_field:0xffff->reg15,resubmit(,33) > --- > ovn/controller/ofctrl.c | 26 +++++++++++++++++++++----- > ovn/controller/ofctrl.h | 3 +++ > ovn/controller/physical.c | 28 +++++++++++++++++++--------- > 3 files changed, 43 insertions(+), 14 deletions(-) > > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c > index f0451b7..2b26f2d 100644 > --- a/ovn/controller/ofctrl.c > +++ b/ovn/controller/ofctrl.c > @@ -550,10 +550,10 @@ log_ovn_flow_rl(struct vlog_rate_limit *rl, enum > vlog_level level, > * > * This just assembles the desired flow tables in memory. Nothing is > actually > * sent to the switch until a later call to ofctrl_run(). */ > -void > -ofctrl_add_flow(uint8_t table_id, uint16_t priority, > +static void > +_ofctrl_add_flow(uint8_t table_id, uint16_t priority, > const struct match *match, const struct ofpbuf *actions, > - const struct uuid *uuid) > + const struct uuid *uuid, bool dupwarn) > { > /* Structure that uses table_id+priority+various things as hashes. */ > struct ovn_flow *f = xmalloc(sizeof *f); > @@ -591,8 +591,10 @@ ofctrl_add_flow(uint8_t table_id, uint16_t priority, > */ > if (ofpacts_equal(f->ofpacts, f->ofpacts_len, > d->ofpacts, d->ofpacts_len)) { > - static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 1); > - log_ovn_flow_rl(&rl, VLL_INFO, f, "duplicate flow"); > + if (dupwarn) { > + static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 1); > + log_ovn_flow_rl(&rl, VLL_INFO, f, "duplicate flow"); > + } > } else { > static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 1); > log_ovn_flow_rl(&rl, VLL_WARN, f, > @@ -617,6 +619,20 @@ ofctrl_add_flow(uint8_t table_id, uint16_t priority, > f->uuid_hindex_node.hash); > } > > +void > +ofctrl_add_flow(uint8_t table_id, uint16_t priority, > + const struct match *match, const struct ofpbuf *actions, > + const struct uuid *uuid) { > + _ofctrl_add_flow(table_id, priority, match, actions, uuid, true); > +} > + > +void > +ofctrl_add_flow_no_warn(uint8_t table_id, uint16_t priority, > + const struct match *match, const struct ofpbuf > *actions, > + const struct uuid *uuid) { > + _ofctrl_add_flow(table_id, priority, match, actions, uuid, false); > +} > + > /* Removes a bundles of flows from the flow table. */ > void > ofctrl_remove_flows(const struct uuid *uuid) > diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h > index 49b95b0..b591e82 100644 > --- a/ovn/controller/ofctrl.h > +++ b/ovn/controller/ofctrl.h > @@ -42,6 +42,9 @@ struct ovn_flow *ofctrl_dup_flow(struct ovn_flow > *source); > void ofctrl_add_flow(uint8_t table_id, uint16_t priority, > const struct match *, const struct ofpbuf *ofpacts, > const struct uuid *uuid); > +void ofctrl_add_flow_no_warn(uint8_t table_id, uint16_t priority, > + const struct match *, const struct ofpbuf > *ofpacts, > + const struct uuid *uuid); > > void ofctrl_remove_flows(const struct uuid *uuid); > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index a104e33..9e6dff4 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -549,8 +549,9 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, > * group as the logical output port. */ > put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, ofpacts_p); > > - ofctrl_add_flow(OFTABLE_LOCAL_OUTPUT, 100, > - &match, ofpacts_p, &mc->header_.uuid); > + /* Add flow, allowing expected full duplication to be ignored. */ > + ofctrl_add_flow_no_warn(OFTABLE_LOCAL_OUTPUT, 100, > + &match, ofpacts_p, &mc->header_.uuid); > } > > /* Table 32, priority 100. > @@ -587,8 +588,10 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, > if (local_ports) { > put_resubmit(OFTABLE_LOCAL_OUTPUT, remote_ofpacts_p); > } > - ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 100, > - &match, remote_ofpacts_p, &mc->header_.uuid); > + /* Add flow, allowing expected full duplication to be > ignored. */ > + ofctrl_add_flow_no_warn(OFTABLE_REMOTE_OUTPUT, 100, > + &match, remote_ofpacts_p, > + &mc->header_.uuid); > } > } > sset_destroy(&remote_chassis); > @@ -794,8 +797,9 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > > put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts); > > - ofctrl_add_flow(OFTABLE_PHY_TO_LOG, 100, &match, &ofpacts, > - hc_uuid); > + /* Add flow, allowing expected full duplication to be ignored. */ > + ofctrl_add_flow_no_warn(OFTABLE_PHY_TO_LOG, 100, &match, &ofpacts, > + hc_uuid); > } > > /* Add flows for VXLAN encapsulations. Due to the limited amount of > @@ -828,7 +832,9 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > put_load(binding->tunnel_key, MFF_LOG_INPORT, 0, 15, > &ofpacts); > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts); > > - ofctrl_add_flow(OFTABLE_PHY_TO_LOG, 100, &match, &ofpacts, > hc_uuid); > + /* Add flow, allowing expected full duplication to be > ignored. */ > + ofctrl_add_flow_no_warn(OFTABLE_PHY_TO_LOG, 100, &match, > &ofpacts, > + hc_uuid); > } > } > > @@ -841,7 +847,9 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > match_init_catchall(&match); > ofpbuf_clear(&ofpacts); > put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts); > - ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 0, &match, &ofpacts, hc_uuid); > + /* Add flow, allowing expected full duplication to be ignored. */ > + ofctrl_add_flow_no_warn(OFTABLE_REMOTE_OUTPUT, 0, &match, &ofpacts, > + hc_uuid); > > /* Table 34, Priority 0. > * ======================= > @@ -855,7 +863,9 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > MFF_LOG_REGS; > #undef MFF_LOG_REGS > put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, &ofpacts); > - ofctrl_add_flow(OFTABLE_DROP_LOOPBACK, 0, &match, &ofpacts, hc_uuid); > + /* Add flow, allowing expected full duplication to be ignored. */ > + ofctrl_add_flow_no_warn(OFTABLE_DROP_LOOPBACK, 0, &match, &ofpacts, > + hc_uuid); > > ofpbuf_uninit(&ofpacts); > HMAP_FOR_EACH_POP (tun, hmap_node, &tunnels) { > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev