[ovs-dev] [PATCH ovn v9 8/8] ovn-controller: Handle sbrec_chassis changes incrementally.

Numan Siddique numans at ovn.org
Wed Jun 3 07:44:15 UTC 2020


On Wed, Jun 3, 2020 at 11:43 AM Han Zhou <hzhou at ovn.org> wrote:

> On Thu, May 28, 2020 at 4:06 AM <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <numans at ovn.org>
> >
> > When ovn-controller updates the nb_cfg column of its chassis,
> > this results in full recomputation on all the nodes. This results
> > in wastage of CPU cycles. To address this, this patch handles
> > sbrec_chassis changes incrementally.
> >
> > We don't need to handle any sbrec_chassis changes during runtime_data
> > stage, because before engine_run() is called, encaps_run() is called
> > which will handle any chassis/encap changes.
> >
> > For new chassis addition and deletion, we need to add/delete flows for
> > the tunnel ports created/deleted. So physical_run() is called for this.
> >
> > For any chassis updates, we can ignore this for flow computation.
> >
> > This patch handles all these.
> >
> > Acked-by: Mark Michelson <mmichels at redhat.com>
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >  controller/ovn-controller.c | 50 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 41 insertions(+), 9 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 2a177ef9b..56744a39e 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1461,6 +1461,7 @@ static void init_physical_ctx(struct engine_node
> *node,
> >          engine_ovsdb_node_get_index(
> >                  engine_get_input("SB_chassis", node),
> >                  "name");
> > +
> >      if (chassis_id) {
> >          chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
> chassis_id);
> >      }
> > @@ -1536,8 +1537,8 @@ static void init_lflow_ctx(struct engine_node
> *node,
> >      const struct sbrec_chassis *chassis = NULL;
> >      struct ovsdb_idl_index *sbrec_chassis_by_name =
> >          engine_ovsdb_node_get_index(
> > -                engine_get_input("SB_chassis", node),
> > -                "name");
> > +            engine_get_input("SB_chassis", node),
> > +            "name");
> >      if (chassis_id) {
> >          chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
> chassis_id);
> >      }
> > @@ -1617,8 +1618,8 @@ en_flow_output_run(struct engine_node *node, void
> *data)
> >
> >      struct ovsdb_idl_index *sbrec_chassis_by_name =
> >          engine_ovsdb_node_get_index(
> > -                engine_get_input("SB_chassis", node),
> > -                "name");
> > +            engine_get_input("SB_chassis", node),
> > +            "name");
> >
> >      const struct sbrec_chassis *chassis = NULL;
> >      if (chassis_id) {
> > @@ -1775,8 +1776,8 @@ _flow_output_resource_ref_handler(struct
> engine_node *node, void *data,
> >
> >      struct ovsdb_idl_index *sbrec_chassis_by_name =
> >          engine_ovsdb_node_get_index(
> > -                engine_get_input("SB_chassis", node),
> > -                "name");
> > +            engine_get_input("SB_chassis", node),
> > +            "name");
> >      const struct sbrec_chassis *chassis = NULL;
> >      if (chassis_id) {
> >          chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
> chassis_id);
> > @@ -1948,6 +1949,31 @@ physical_flow_changes_ovs_iface_handler(struct
> engine_node *node,
> >      return true;
> >  }
> >
> > +/* Handles sbrec_chassis changes.
> > + * If a new chassis is added or removed return false, so that
> > + * physical flows are programmed.
> > + * For any updates, there is no need for any flow computation.
>
> Are we sure about this? At least I saw chassis->hostname, and
> chassis->other_config are used in physical_run().
> For example:
> put_replace_router_port_mac_flows() -> chassis_get_mac() uses
> other_config:ovn-chassis-mac-mappings. If this is updated, the flows should
> change.
>
> For hostname, it is used here.
>         if (ofport && requested_chassis && requested_chassis[0] &&
>             strcmp(requested_chassis, chassis->name) &&
>             strcmp(requested_chassis, chassis->hostname)) {
>             /* Even though there is an ofport for this port_binding, it is
>              * requested on a different chassis. So ignore this ofport.
>              */
>             ofport = 0;
>         }
> So if chassis->hostname changed, the flow change may be needed as well.
>
> There are also many places using chassis->name, but I assume name should
> never be changed so that should be ok.
>
> For every input, we need to check how they are used in full compute and
> then will know how to handle/ignore in change handlers. (Current test cases
> can't be relied because there are many corner cases not covered)
>
> (sorry that I didn't spot this when reviewing earlier versions)
>

No worries. Thanks for the review.
Great catch. I totally missed that. I'll explore further on this. If it is
too complicated to
handle, I'll just drop this patch from the series.

Thanks
Numan


>
> Thanks,
> Han
>
> > + * Encap changes will also result in sbrec_chassis changes,
> > + * but we handle encap changes separately.
> > + */
> > +static bool
> > +physical_flow_changes_sb_chassis_handler(struct engine_node *node
> OVS_UNUSED,
> > +                                         void *data OVS_UNUSED)
> > +{
> > +    struct sbrec_chassis_table *chassis_table =
> > +        (struct sbrec_chassis_table *)EN_OVSDB_GET(
> > +            engine_get_input("SB_chassis", node));
> > +
> > +    const struct sbrec_chassis *ch;
> > +    SBREC_CHASSIS_TABLE_FOR_EACH_TRACKED (ch, chassis_table) {
> > +        if (sbrec_chassis_is_deleted(ch) || sbrec_chassis_is_new(ch)) {
> > +            return false;
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  static bool
> >  flow_output_physical_flow_changes_handler(struct engine_node *node, void
> *data)
> >  {
> > @@ -2200,6 +2226,10 @@ main(int argc, char *argv[])
> >                       NULL);
> >      engine_add_input(&en_physical_flow_changes, &en_ovs_interface,
> >                       physical_flow_changes_ovs_iface_handler);
> > +    engine_add_input(&en_physical_flow_changes, &en_sb_chassis,
> > +                     physical_flow_changes_sb_chassis_handler);
> > +    engine_add_input(&en_physical_flow_changes, &en_sb_encap,
> > +                     NULL);
> >
> >      engine_add_input(&en_flow_output, &en_addr_sets,
> >                       flow_output_addr_sets_handler);
> > @@ -2218,9 +2248,10 @@ main(int argc, char *argv[])
> >
> >      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,
> > +                     flow_output_noop_handler);
> > +    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,
> > @@ -2247,7 +2278,8 @@ main(int argc, char *argv[])
> >                       runtime_data_ovs_interface_handler);
> >      engine_add_input(&en_runtime_data, &en_ovs_qos, NULL);
> >
> > -    engine_add_input(&en_runtime_data, &en_sb_chassis, NULL);
> > +    engine_add_input(&en_runtime_data, &en_sb_chassis,
> > +                     runtime_data_noop_handler);
> >      engine_add_input(&en_runtime_data, &en_sb_datapath_binding,
> >                       runtime_data_sb_datapath_binding_handler);
> >      engine_add_input(&en_runtime_data, &en_sb_port_binding,
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list