From: Han Zhou <[email protected]> Date: Wednesday, December 14, 2016 at 11:53 AM To: Darrell Ball <[email protected]> Cc: "[email protected]" <[email protected]> Subject: Re: [ovs-dev] [PATCH v2] ovn-controller: Fix duplicated flows in table 32.
On Wed, Dec 14, 2016 at 9:36 AM, Darrell Ball <[email protected]<mailto:[email protected]>> wrote: > > > > On 12/13/16, 11:15 PM, > "[email protected]<mailto:[email protected]> on > behalf of Han Zhou" > <[email protected]<mailto:[email protected]> on > behalf of [email protected]<mailto:[email protected]>> wrote: > > In commit 475f0a2c it introduced a priority 150 flow for filtering > the sending of traffic received from vxlan tunnels back out tunnels. > However, it added the flow for every remote port processing, which > results in continuous logs about duplicated flows. We only need to > install this flow once per physical_run() loop iteration. > > All flows are attempted to be installed each iteration of physical_run() > We filter those by keeping context (installed_flows) across physical_run() > iterations. > In a given iteration of physical_run(), we also check for duplicated flow > attempts (desired_flows) and treat those as info logs. > > Fix duplicated flows in table32. > should be > Fix duplicated flow add attempts in table 32. > > However, it added the flow for every remote port processing, which > results in continuous logs about duplicated flows. We only need to > install this flow once per physical_run() loop iteration. > > should be > > However, it attempted to add the flow for every remote port processing, > which results in continuous logs about duplicated flow add attempts. We > only need to attempt to install this flow once per physical_run() loop > iteration. > Agree. This is more accurate. > I remember at some point we had lots of duplicated flow logs and hence > maybe ignored them. At least for some tests we don’t see those anymore. > > Perhaps we should update one test to check for duplicated flow ovn-controller > logs ? > The vtep test could be one such test since those logs seem clean now ? > What do you think ? > Thanks for your suggestion. I agree it is better to have it covered in automation. I just wonder how can we tell whether the duplication is unexpected or just some normal case, if not all duplicates are unexpected. If we don't expect any duplicated flows at all, we should change the log level to ERROR, and then in test cases we just need to make sure there is no error log. ERROR log level is not appropriate since we always filter adding the extra flows anyways and it amount to extra processing and filtering. We anyways attempt to re-add every flow for every iteration of the ovn-controller main loop, not just physical flows; we filter these attempts with “installed_flows”, so we are doing lots of extra processing normally, at this time. With the vtep test, the non-physical flows are not as many and diverse. So checking for attempting to add the same flow in the same iteration of ovn-controller main loop where the desired_flows log check is made (which is before the installed_flows check) should help to catch the physical flow duplicate attempts in a given iteration of the main loop. If we added the check there, we would be trying to catch some portion of the unnecessary extra processing and filtering which seems valid and avoid noisy logs, which could obscure other logs ? > > Signed-off-by: Han Zhou <[email protected]<mailto:[email protected]>> > Acked-by: Darrell Ball <[email protected]<mailto:[email protected]>> > --- > > Notes: > v1 -> v2: update commit message according to Darrell's comments. > > ovn/controller/physical.c | 47 > ++++++++++++++++++++++++----------------------- > 1 file changed, 24 insertions(+), 23 deletions(-) > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index 48adb78..3b653dd 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -465,33 +465,16 @@ consider_port_binding(enum mf_field_id > mff_ovn_geneve, > } else { > /* Remote port connected by tunnel */ > > - /* Table 32, priority 150 and 100. > + /* Table 32, priority 100. > * =============================== > * > - * Priority 150 is for packets received from a VXLAN tunnel > - * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to > - * lack of needed metadata in VXLAN, explicitly skip sending > - * back out any tunnels and resubmit to table 33 for local > - * delivery. > - * > - * Priority 100 is for all other traffic which need to be sent > - * to a remote hypervisor. Each flow matches an output port > - * that includes a logical port on a remote hypervisor, and > - * tunnels the packet to that hypervisor. > + * Priority 100 is for traffic that needs to be sent to a remote > + * hypervisor. Each flow matches an output port that includes a > + * logical port on a remote hypervisor, and tunnels the packet to > + * that hypervisor. > */ > match_init_catchall(&match); > ofpbuf_clear(ofpacts_p); > - match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0, > - MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN); > - > - /* Resubmit to table 33. */ > - put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p); > - ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, > - ofpacts_p); > - > - > - match_init_catchall(&match); > - ofpbuf_clear(ofpacts_p); > > /* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */ > match_set_metadata(&match, htonll(dp_key)); > @@ -870,12 +853,30 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > } > } > > + /* Table 32, priority 150. > + * =============================== > + * > + * Priority 150 is for packets received from a VXLAN tunnel > + * which get resubmitted to OFTABLE_LOG_INGRESS_PIPELINE due to > + * lack of needed metadata in VXLAN, explicitly skip sending > + * back out any tunnels and resubmit to table 33 for local > + * delivery. > + */ > + struct match match; > + match_init_catchall(&match); > + ofpbuf_clear(&ofpacts); > + match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0, > + MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN); > + > + /* Resubmit to table 33. */ > + put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts); > + ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, > &ofpacts); > + > /* Table 32, Priority 0. > * ======================= > * > * Resubmit packets that are not directed at tunnels or part of a > * multicast group to the local output table. */ > - struct match match; > match_init_catchall(&match); > ofpbuf_clear(&ofpacts); > put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts); > -- > 2.1.0 > > _______________________________________________ > dev mailing list > [email protected]<mailto:[email protected]> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DgICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=NLjhIOe9VMcn_AAkZej-qQONYdl5dAaLeWYKgo-E0x8&s=rdkMYzTllwepoHwyV7D2X3uDk7Xw7dUMMWjOZ2f1hPY&e= > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
