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

Han Zhou hzhou at ovn.org
Wed Jun 3 06:13:16 UTC 2020


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)

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


More information about the dev mailing list