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>
>
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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
