[ovs-dev] [PATCH ovn v8 1/4] ovn-controller: Split logical flow and physical flow processing.

Han Zhou hzhou at ovn.org
Wed Jun 2 06:21:59 UTC 2021


On Tue, Jun 1, 2021 at 5:10 PM Numan Siddique <numans at ovn.org> wrote:
>
> On Sun, May 30, 2021 at 11:45 PM <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <numans at ovn.org>
> >
> > Presently, the 'flow_output' engine node recomputes physical
> > flows by calling physical_run() in the 'physical_flow_changes'
> > handler in some scenarios.  Because of this, an engine run can
> > do a full recompute of physical flows but not full recompute
> > of logical flows.  Although this works now, it is problematic
> > as the same desired flow table is used for both physical and
> > logical flows.
> >
> > This patch now separates the handling of logical flows and
> > physical flows and removes the 'physical_flow_changes' engine
> > node.  Two separate engine nodes are added - lflow_output and
> > pflow_output with their own flow tables and these two nodes are
> > now inputs to the main engine node - flow_output.  This separation
> > reflects the data dependency more clearly.
> >
> > CC: Han Zhou <hzhou at ovn.org>
> > Signed-off-by: Numan Siddique <numans at ovn.org>
>
> Hi Han,
>
> Gentle ping.  Wondering if you got the chance to take a look at the
> first patch of the series.
>

Hi Numan,

Thanks for the revision and sorry for the slow response. I see that you
updated some change handlers from noop to NULL for pflow_output, which
makes more sense now. However, I see there are still several noop handlers
for lflow_output node.
When we use a noop handler, it means that the input is used for compute the
output (otherwise it wouldn't be a dependency), but we know that the
related change is handled *indirectly* when handling another input. I think
this should be the only situation we use noop handler, and when using it we
need to document where is the change handled. Please see my comments
inlined for each of the input below.

> It would be great if the first patch can be considered before we
> branch (or before the 21.06 release).
>
I understand this patch has been hanging around for quite long, but since
it is more of a refactoring than feature or bug-fix, I'd in fact prefer
merging it *after* the release, together with the related patches in the
series, because of the experiences we had before for big changes in I-P
(including many of my patches) that caused regressions. When we are
confident enough we could still backport it to 21.06 when necessary. What
do you think?


> > @@ -2731,58 +2691,71 @@ main(int argc, char *argv[])
> >      engine_add_input(&en_port_groups, &en_runtime_data,
> >                       port_groups_runtime_data_handler);
> >
> > -    /* Engine node physical_flow_changes indicates whether
> > -     * we can recompute only physical flows or we can
> > -     * incrementally process the physical flows.
> > -     *
> > -     * Note: The order of inputs is important, all OVS interface
changes must
> > +    /* Note: The order of inputs is important, all OVS interface
changes must
> >       * be handled before any ct_zone changes.
> >       */
> > -    engine_add_input(&en_physical_flow_changes, &en_ovs_interface,
> > -                     physical_flow_changes_ovs_iface_handler);
> > -    engine_add_input(&en_physical_flow_changes, &en_ct_zones,
> > -                     physical_flow_changes_ct_zones_handler);
> > -
> > -    engine_add_input(&en_flow_output, &en_addr_sets,
> > -                     flow_output_addr_sets_handler);
> > -    engine_add_input(&en_flow_output, &en_port_groups,
> > -                     flow_output_port_groups_handler);
> > -    engine_add_input(&en_flow_output, &en_runtime_data,
> > -                     flow_output_runtime_data_handler);
> > -    engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL);
> > -    engine_add_input(&en_flow_output, &en_physical_flow_changes,
> > -                     flow_output_physical_flow_changes_handler);
> > +    engine_add_input(&en_pflow_output, &en_ovs_interface,
> > +                     pflow_output_ovs_iface_handler);
> > +    engine_add_input(&en_pflow_output, &en_ct_zones,
> > +                     NULL);
> > +    engine_add_input(&en_pflow_output, &en_sb_chassis,
> > +                     pflow_output_sb_chassis_handler);
> > +    engine_add_input(&en_pflow_output, &en_sb_port_binding,
> > +                     pflow_output_sb_port_binding_handler);
> > +    engine_add_input(&en_pflow_output, &en_sb_multicast_group,
> > +                     pflow_output_sb_multicast_group_handler);
> > +
> > +    engine_add_input(&en_pflow_output, &en_runtime_data,
> > +                     NULL);
> > +    engine_add_input(&en_pflow_output, &en_sb_encap, NULL);
> > +    engine_add_input(&en_pflow_output, &en_mff_ovn_geneve, NULL);
> > +    engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL);
> > +    engine_add_input(&en_pflow_output, &en_ovs_bridge, NULL);
> > +
> > +    engine_add_input(&en_lflow_output, &en_addr_sets,
> > +                     lflow_output_addr_sets_handler);
> > +    engine_add_input(&en_lflow_output, &en_port_groups,
> > +                     lflow_output_port_groups_handler);
> > +    engine_add_input(&en_lflow_output, &en_runtime_data,
> > +                     lflow_output_runtime_data_handler);
> >
> >      /* We need this input nodes for only data. Hence the noop handler.
*/
> > -    engine_add_input(&en_flow_output, &en_ct_zones,
engine_noop_handler);
> > -    engine_add_input(&en_flow_output, &en_ovs_interface,
engine_noop_handler);
> > -
> > -    engine_add_input(&en_flow_output, &en_ovs_open_vswitch, NULL);
> > -    engine_add_input(&en_flow_output, &en_ovs_bridge, NULL);
> > -
> > -    engine_add_input(&en_flow_output, &en_sb_chassis, NULL);
> > -    engine_add_input(&en_flow_output, &en_sb_encap, NULL);
> > -    engine_add_input(&en_flow_output, &en_sb_multicast_group,
> > -                     flow_output_sb_multicast_group_handler);
> > -    engine_add_input(&en_flow_output, &en_sb_port_binding,
> > -                     flow_output_sb_port_binding_handler);
> > -    engine_add_input(&en_flow_output, &en_sb_mac_binding,
> > -                     flow_output_sb_mac_binding_handler);
> > -    engine_add_input(&en_flow_output, &en_sb_logical_flow,
> > -                     flow_output_sb_logical_flow_handler);
> > +    engine_add_input(&en_lflow_output, &en_ct_zones,
> > +                     engine_noop_handler);

It seems ct_zones is for physical flow computing, and we handles it by
recomputing physical flows. I didn't see it used for logical flow
computing. So can we remove it from the dependency?

> > +    engine_add_input(&en_lflow_output, &en_ovs_interface,
> > +                     engine_noop_handler);

Same for ovs_interface, I didn't see it used for lflow_output. Shall we
remove it?

> > +    engine_add_input(&en_lflow_output, &en_sb_chassis,
> > +                     engine_noop_handler);

This one doesn't seem to justify a noop handler. It is used for lflow
computing and we cannot handle the changes incrementally (for now), so we
should use NULL just to trigger recompute.

> > +    engine_add_input(&en_lflow_output, &en_sb_multicast_group,
> > +                     engine_noop_handler);
> >
> > +    /* Any changes to the port binding, need not be handled
> > +     * for lflow_outout engine.  We still need sb_port_binding
> > +     * as input to access the port binding data in lflow.c and
> > +     * hence the noop handler. */
> > +    engine_add_input(&en_lflow_output, &en_sb_port_binding,
> > +                     engine_noop_handler);

For multicast_group and port_binding, we didn't handle it previously, but
it is not correct. I submitted a patch series last week. Please help review
it and probably it can be incorporated to this patch later if it looks ok.
https://patchwork.ozlabs.org/project/ovn/list/?series=246286

Thanks,
Han


More information about the dev mailing list