| Message ID | 1469380073-27333-1-git-send-email-rmoats@us.ibm.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@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 >
On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: Add incremental processing to lflow_run and physical_run)" causes load balancing system unit tests to fail. A little debugging shows that groups are getting deleted when new flows are added. My hunch is that this is likely because 'desired_groups' in ofctl_put gets deleted in every run. But in the next run, it does not get updated as we no longer process all flows. --- > 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 >
Guru Shetty <guru@ovn.org> wrote on 07/26/2016 03:54:29 PM: > From: Guru Shetty <guru@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: ovs dev <dev@openvswitch.org> > Date: 07/26/2016 03:54 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > duplicate flow warnings > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: > Add incremental processing to lflow_run and physical_run)" causes > load balancing system unit tests to fail. A little debugging shows > that groups are getting deleted when new flows are added. My hunch > is that this is likely because 'desired_groups' in ofctl_put gets > deleted in every run. But in the next run, it does not get updated > as we no longer process all flows. That's going to take persisting the desired_groups data. I can take a shot if you'd like, just give me the link to the patch set that includes the load balancing system unit tests and I'll see what I can do to make it right ...
On 26 July 2016 at 15:54, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > Guru Shetty <guru@ovn.org> wrote on 07/26/2016 03:54:29 PM: > > > From: Guru Shetty <guru@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: ovs dev <dev@openvswitch.org> > > Date: 07/26/2016 03:54 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > duplicate flow warnings > > > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: > > Add incremental processing to lflow_run and physical_run)" causes > > load balancing system unit tests to fail. A little debugging shows > > that groups are getting deleted when new flows are added. My hunch > > is that this is likely because 'desired_groups' in ofctl_put gets > > deleted in every run. But in the next run, it does not get updated > > as we no longer process all flows. > > That's going to take persisting the desired_groups data. > > I can take a shot if you'd like, just give me the link to the > patch set that includes the load balancing system unit tests > and I'll see what I can do to make it right ... > It already exists in the OVN repo. tests/system-ovn.at > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
Guru Shetty <guru@ovn.org> wrote on 07/26/2016 06:05:47 PM: > From: Guru Shetty <guru@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: ovs dev <dev@openvswitch.org> > Date: 07/26/2016 06:06 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > duplicate flow warnings > > On 26 July 2016 at 15:54, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > Guru Shetty <guru@ovn.org> wrote on 07/26/2016 03:54:29 PM: > > > From: Guru Shetty <guru@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: ovs dev <dev@openvswitch.org> > > Date: 07/26/2016 03:54 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > duplicate flow warnings > > > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: > > Add incremental processing to lflow_run and physical_run)" causes > > load balancing system unit tests to fail. A little debugging shows > > that groups are getting deleted when new flows are added. My hunch > > is that this is likely because 'desired_groups' in ofctl_put gets > > deleted in every run. But in the next run, it does not get updated > > as we no longer process all flows. > > That's going to take persisting the desired_groups data. > > I can take a shot if you'd like, just give me the link to the > patch set that includes the load balancing system unit tests > and I'll see what I can do to make it right ... > > It already exists in the OVN repo. tests/system-ovn.at Ack and verified that it is failing - I'll take a deeper look later tonight/tomorrow and see what I can make work.
> On Jul 26, 2016, at 5:30 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > > Guru Shetty <guru@ovn.org> wrote on 07/26/2016 06:05:47 PM: > > > From: Guru Shetty <guru@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: ovs dev <dev@openvswitch.org> > > Date: 07/26/2016 06:06 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > duplicate flow warnings > > > > On 26 July 2016 at 15:54, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > > > > > Guru Shetty <guru@ovn.org> wrote on 07/26/2016 03:54:29 PM: > > > > > From: Guru Shetty <guru@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: ovs dev <dev@openvswitch.org> > > > Date: 07/26/2016 03:54 PM > > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > > duplicate flow warnings > > > > > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: > > > Add incremental processing to lflow_run and physical_run)" causes > > > load balancing system unit tests to fail. A little debugging shows > > > that groups are getting deleted when new flows are added. My hunch > > > is that this is likely because 'desired_groups' in ofctl_put gets > > > deleted in every run. But in the next run, it does not get updated > > > as we no longer process all flows. > > > > That's going to take persisting the desired_groups data. > > > > I can take a shot if you'd like, just give me the link to the > > patch set that includes the load balancing system unit tests > > and I'll see what I can do to make it right ... > > > > It already exists in the OVN repo. tests/system-ovn.at > > Ack and verified that it is failing - I'll take a deeper look > later tonight/tomorrow and see what I can make work. > Thanks much. (Just to make sure you have the environment right, you should have the right kernel modules with conntrack support installed on your machine. On master, it will only work on pre 4.6 kernels if there is no ovs kernel module already instslled from upstream kernel. To make it work, you should either remove upstream kernel modules or install a /etc/depmod.d/openvswitch.conf to override upstream one. On 4.6 and above it should not matter as upstream kernel module has conntrack support. You can make sure that you get the tests working before the said commit so that you dont go on a wild goose chase.) >
Guru Shetty <guru.ovn@gmail.com> wrote on 07/26/2016 10:22:30 PM: > From: Guru Shetty <guru.ovn@gmail.com> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: Guru Shetty <guru@ovn.org>, ovs dev <dev@openvswitch.org> > Date: 07/26/2016 10:22 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > duplicate flow warnings > > > On Jul 26, 2016, at 5:30 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > Guru Shetty <guru@ovn.org> wrote on 07/26/2016 06:05:47 PM: > > > From: Guru Shetty <guru@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: ovs dev <dev@openvswitch.org> > > Date: 07/26/2016 06:06 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > duplicate flow warnings > > > > On 26 July 2016 at 15:54, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > > > > > Guru Shetty <guru@ovn.org> wrote on 07/26/2016 03:54:29 PM: > > > > > From: Guru Shetty <guru@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: ovs dev <dev@openvswitch.org> > > > Date: 07/26/2016 03:54 PM > > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > > duplicate flow warnings > > > > > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: > > > Add incremental processing to lflow_run and physical_run)" causes > > > load balancing system unit tests to fail. A little debugging shows > > > that groups are getting deleted when new flows are added. My hunch > > > is that this is likely because 'desired_groups' in ofctl_put gets > > > deleted in every run. But in the next run, it does not get updated > > > as we no longer process all flows. > > > > That's going to take persisting the desired_groups data. > > > > I can take a shot if you'd like, just give me the link to the > > patch set that includes the load balancing system unit tests > > and I'll see what I can do to make it right ... > > > > It already exists in the OVN repo. tests/system-ovn.at > > Ack and verified that it is failing - I'll take a deeper look > later tonight/tomorrow and see what I can make work. > > Thanks much. > > (Just to make sure you have the environment right, you should have > the right kernel modules with conntrack support installed on your > machine. On master, it will only work on pre 4.6 kernels if there is > no ovs kernel module already instslled from upstream kernel. To make > it work, you should either remove upstream kernel modules or install > a /etc/depmod.d/openvswitch.conf to override upstream one. On 4.6 > and above it should not matter as upstream kernel module has > conntrack support. > > You can make sure that you get the tests working before the said > commit so that you dont go on a wild goose chase.) No worries - I've been running on a 4.6 kernel for a while now, so I've already verified that the test worked with if logical flows are recalculated every cycle. I'm almost to the point of having a patch set ready to go - I'm just putting the path to remove item(s) from desired_groups when the logical_flow that created them is removed - that's not strictly correct, because it doesn't catch if the flow is modified to not call a LB conntrack action, but it's a first approximation.
Guru Shetty <guru.ovn@gmail.com> wrote on 07/26/2016 10:22:30 PM: > From: Guru Shetty <guru.ovn@gmail.com> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: Guru Shetty <guru@ovn.org>, ovs dev <dev@openvswitch.org> > Date: 07/26/2016 10:22 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > duplicate flow warnings > > > On Jul 26, 2016, at 5:30 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > Guru Shetty <guru@ovn.org> wrote on 07/26/2016 06:05:47 PM: > > > From: Guru Shetty <guru@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: ovs dev <dev@openvswitch.org> > > Date: 07/26/2016 06:06 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > duplicate flow warnings > > > > On 26 July 2016 at 15:54, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > > > > > Guru Shetty <guru@ovn.org> wrote on 07/26/2016 03:54:29 PM: > > > > > From: Guru Shetty <guru@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: ovs dev <dev@openvswitch.org> > > > Date: 07/26/2016 03:54 PM > > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > > duplicate flow warnings > > > > > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: > > > Add incremental processing to lflow_run and physical_run)" causes > > > load balancing system unit tests to fail. A little debugging shows > > > that groups are getting deleted when new flows are added. My hunch > > > is that this is likely because 'desired_groups' in ofctl_put gets > > > deleted in every run. But in the next run, it does not get updated > > > as we no longer process all flows. > > > > That's going to take persisting the desired_groups data. > > > > I can take a shot if you'd like, just give me the link to the > > patch set that includes the load balancing system unit tests > > and I'll see what I can do to make it right ... > > > > It already exists in the OVN repo. tests/system-ovn.at > > Ack and verified that it is failing - I'll take a deeper look > later tonight/tomorrow and see what I can make work. > > Thanks much. > > (Just to make sure you have the environment right, you should have > the right kernel modules with conntrack support installed on your > machine. On master, it will only work on pre 4.6 kernels if there is > no ovs kernel module already instslled from upstream kernel. To make > it work, you should either remove upstream kernel modules or install > a /etc/depmod.d/openvswitch.conf to override upstream one. On 4.6 > and above it should not matter as upstream kernel module has > conntrack support. > > You can make sure that you get the tests working before the said > commit so that you dont go on a wild goose chase.) Mitigation patch is at http://patchwork.ozlabs.org/patch/653068/ for review. In my previous message, I incorrectly stated that the above patch didn't handle flow modifications correctly. It actually does.
On Tue, Jul 26, 2016 at 01:54:29PM -0700, Guru Shetty wrote: > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: Add > incremental processing to lflow_run and physical_run)" causes load > balancing system unit tests to fail. A little debugging shows that groups > are getting deleted when new flows are added. My hunch is that this is > likely because 'desired_groups' in ofctl_put gets deleted in every run. But > in the next run, it does not get updated as we no longer process all flows. It's unclear to me from the discussion of this patch whether it's beneficial. Can someone clarify? Thanks, Ben.
Ben Pfaff <blp@ovn.org> wrote on 07/27/2016 03:53:56 PM: > From: Ben Pfaff <blp@ovn.org> > To: Guru Shetty <guru@ovn.org> > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev <dev@openvswitch.org> > Date: 07/27/2016 03:54 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > duplicate flow warnings > > On Tue, Jul 26, 2016 at 01:54:29PM -0700, Guru Shetty wrote: > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: Add > > incremental processing to lflow_run and physical_run)" causes load > > balancing system unit tests to fail. A little debugging shows that groups > > are getting deleted when new flows are added. My hunch is that this is > > likely because 'desired_groups' in ofctl_put gets deleted in every run. But > > in the next run, it does not get updated as we no longer process all flows. > > It's unclear to me from the discussion of this patch whether it's > beneficial. Can someone clarify? > > Thanks, > > Ben. Unfortunately, I don't think we maintained reasonable email thread discipline on this discussion - for me, the vast majority of the conversation was about the CPU cycle bug that Guru mentions and not the patch itself. The conditions stated in the commit message still apply even after fixing Guru's issue and so I still see the squelching as being useful to avoid polluting the ovn-controller logs with false positive messages... Ryan
On Wed, Jul 27, 2016 at 09:13:56PM -0500, Ryan Moats wrote: > > > Ben Pfaff <blp@ovn.org> wrote on 07/27/2016 03:53:56 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Guru Shetty <guru@ovn.org> > > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev <dev@openvswitch.org> > > Date: 07/27/2016 03:54 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > duplicate flow warnings > > > > On Tue, Jul 26, 2016 at 01:54:29PM -0700, Guru Shetty wrote: > > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > > > > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: > Add > > > incremental processing to lflow_run and physical_run)" causes load > > > balancing system unit tests to fail. A little debugging shows that > groups > > > are getting deleted when new flows are added. My hunch is that this is > > > likely because 'desired_groups' in ofctl_put gets deleted in every run. > But > > > in the next run, it does not get updated as we no longer process all > flows. > > > > It's unclear to me from the discussion of this patch whether it's > > beneficial. Can someone clarify? > > > > Thanks, > > > > Ben. > > Unfortunately, I don't think we maintained reasonable email thread > discipline > on this discussion - for me, the vast majority of the conversation was > about > the CPU cycle bug that Guru mentions and not the patch itself. > > The conditions stated in the commit message still apply even after fixing > Guru's issue and so I still see the squelching as being useful to avoid > polluting the ovn-controller logs with false positive messages... I think that you said in IRC that you're dropping this patch. Let me know if I'm wrong. Thanks, Ben.
Ben Pfaff <blp@ovn.org> wrote on 07/28/2016 04:28:31 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: ovs dev <dev@openvswitch.org>, Guru Shetty <guru@ovn.org> > Date: 07/28/2016 04:29 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > duplicate flow warnings > > On Wed, Jul 27, 2016 at 09:13:56PM -0500, Ryan Moats wrote: > > > > > > Ben Pfaff <blp@ovn.org> wrote on 07/27/2016 03:53:56 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: Guru Shetty <guru@ovn.org> > > > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev <dev@openvswitch.org> > > > Date: 07/27/2016 03:54 PM > > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected > > > duplicate flow warnings > > > > > > On Tue, Jul 26, 2016 at 01:54:29PM -0700, Guru Shetty wrote: > > > > On 24 July 2016 at 10:07, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > > > > > > > > > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller: > > Add > > > > incremental processing to lflow_run and physical_run)" causes load > > > > balancing system unit tests to fail. A little debugging shows that > > groups > > > > are getting deleted when new flows are added. My hunch is that this is > > > > likely because 'desired_groups' in ofctl_put gets deleted in every run. > > But > > > > in the next run, it does not get updated as we no longer process all > > flows. > > > > > > It's unclear to me from the discussion of this patch whether it's > > > beneficial. Can someone clarify? > > > > > > Thanks, > > > > > > Ben. > > > > Unfortunately, I don't think we maintained reasonable email thread > > discipline > > on this discussion - for me, the vast majority of the conversation was > > about > > the CPU cycle bug that Guru mentions and not the patch itself. > > > > The conditions stated in the commit message still apply even after fixing > > Guru's issue and so I still see the squelching as being useful to avoid > > polluting the ovn-controller logs with false positive messages... > > I think that you said in IRC that you're dropping this patch. Let me > know if I'm wrong. > > Thanks, > > Ben. Yes, I've marked this as not applicable because I'm dropping it - it is not necessary. Ryan
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) {
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 <rmoats@us.ibm.com> --- ovn/controller/ofctrl.c | 26 +++++++++++++++++++++----- ovn/controller/ofctrl.h | 3 +++ ovn/controller/physical.c | 28 +++++++++++++++++++--------- 3 files changed, 43 insertions(+), 14 deletions(-)