[ovs-dev] [PATCH v2 3/3] ovn-controller: Consider only port binding changes of normal type for run_time dataengine

[Numan Siddique]

On Tue, Jun 25, 2019 at 1:23 AM Han Zhou <zhouhan at gmail.com> wrote:

>
>
> On Mon, Jun 24, 2019 at 9:59 AM Han Zhou <zhouhan at gmail.com> wrote:
> >
> >
> >
> > On Mon, Jun 24, 2019 at 4:00 AM <nusiddiq at redhat.com> wrote:
> > >
> > > From: Numan Siddique <nusiddiq at redhat.com>
> > >
> > > Any changes for Port_Bindings rows of type - "chassisredirect",
> "patch", "l3gateway" etc
> > > which are not related to the chassis can be ignored in the function
> > > 'binding_evaluate_port_binding_changes()'. Presently this returns true
> and this results
> > > in unnecessary flow computation on a chassis.
> > >
> > > Changes to these Port_Bindings (of type != "") will be handled by the
> engine handler
> > > flow_output_sb_port_binding_handler() for the engine node
> 'en_sb_port_binding' (which
> > > is input to 'en_flow_output'.
> > >
> > > For example, if a chassisredirect port is claimed by a gateway
> chassis, the compute
> > > nodes only need to update the flow in table 32 in the bundle action.
> Where as presently
> > > flow computation is triggered and this causes wastage of CPU.
> > >
> > > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> > > ---
> > >  ovn/controller/binding.c | 16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> > > index 87d0b6d88..7d287ece9 100644
> > > --- a/ovn/controller/binding.c
> > > +++ b/ovn/controller/binding.c
> > > @@ -715,11 +715,21 @@ binding_evaluate_port_binding_changes(
> > >           * - If a regular VIF is unbound from this chassis, the local
> ovsdb
> > >           *   interface table will be updated, which will trigger
> recompute.
> > >           *
> > > -         * - If the port is not a regular VIF, always trigger
> recompute. */
> > > +         * If the port is not a regular VIF, then ignore it. */
> > >          if (binding_rec->chassis == chassis_rec
> > >              || is_our_chassis(chassis_rec, binding_rec,
> > > -                              active_tunnels, &lport_to_iface,
> local_lports)
> > > -            || strcmp(binding_rec->type, "")) {
> > > +                              active_tunnels, &lport_to_iface,
> > > +                              local_lports)) {
> > > +            changed = true;
> > > +            break;
> > > +        }
> > > +
> > > +        /* Any changes to chassisredirect port (not claimed by this
> chassis),
> > > +         * doesn't require any logical flow computation. It only
> requires
> > > +         * physical flow update and thiss will be handled by
> > > +         * flow_output_sb_port_binding_handler() in ovn-controller.c
> */
> > > +        if (strcmp(binding_rec->type, "") &&
> > > +            strcmp(binding_rec->type, "chassisredirect")) {
> > >              changed = true;
> > >              break;
> > >          }
> > > --
> > > 2.21.0
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > Makes sense. Thanks for the improvement!
> >
> > Acked-by: Han Zhou <hzhou8 at ebay.com>
>
> Sorry, I should not ack. I realized that this patch might cause problem.
> There is a "XXX" need to be addressed before applying this patch. Please
> read the "XXX" in ovn-controller.c L1401. Specifically this part:
> ---
>      *  - When is_chassis_resident(<lport>) is used in lflow. In this case
> the
>      *    port binding is not a regular VIF. It can be either "patch" or
>      *    "external", with ha-chassis-group assigned.  In current
>      *    "runtime_data" handling, port-binding changes for these types
> always
>      *    trigger recomputing. So it is fine even if we do not handle it
> here.
>      *    (due to the ovsdb tracking support for referenced table changes,
>      *    ha-chassis-group changes will appear as port-binding change).
> ---
> I guess it will cause fail-over not taking effect in the old active node,
> thus it may continue sending GARP etc. It should be solved if we addressed
> this "XXX" as mentioned:
> ... One approach is to maintain a mapping between lport
>      * names and the lflows that uses them, and reprocess the related
> lflows
>      * when related port-bindings change.
>
>
Thanks for pointing this out. You are right.


> However, I wonder if it is really valuable to do this optimization just
> for the "chassisredirect" case. How often would "chassisredirect" port
> binding change happen in real world scenarios?
>

In real deployments, I would say this would be rare. But if you see the
scale tests run by Daniel and Lucas, I noticed that this caused
recomputations.
However when I tested with these patches there were no improvements at all.

Thanks
Numan