On Tue, Nov 9, 2021 at 11:36 AM Mark Gray <[email protected]> wrote:
>
> From: Mark Gray <[email protected]>
>
> 'struct northd_data' is used to hold the global state data for the
> incremental processing node 'en_northd'. This structure will also hold
> 'struct northd_input' which will hold references to output data from
> its input nodes. In particular, this will hold references to database
tables
> and indexes. In order to achieve this, we refactor in the following way:
>
> * Introduce northd_init() which initializes this data.
> * Introduce northd_destroy() which clears this data for a new iteration.
> * Remove 'ovn_internal_version' from the context passed to northd
> as it can be determined within northd using ovn_get_internal_version.
> * Remove 'use_parallel_build' from the context as it is read from DB as
> suggested by Han.
> * Refactor northd.c to use 'struct northd_data' and 'struct northd_input'
> where applicable.
>
> Signed-off-by: Mark Gray <[email protected]>
> ---
> lib/inc-proc-eng.h | 3 +
> northd/en-northd.c | 107 ++++++-
> northd/inc-proc-northd.c | 42 ++-
> northd/inc-proc-northd.h | 6 +-
> northd/northd.c | 632 ++++++++++++++++++++++-----------------
> northd/northd.h | 73 ++++-
> northd/ovn-northd.c | 87 ++----
> 7 files changed, 596 insertions(+), 354 deletions(-)
>
> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> index f89a40bd54ca..1823750c814c 100644
> --- a/lib/inc-proc-eng.h
> +++ b/lib/inc-proc-eng.h
> @@ -72,6 +72,9 @@ struct engine_context {
> struct ovsdb_idl_txn *ovs_idl_txn;
> struct ovsdb_idl_txn *ovnsb_idl_txn;
> struct ovsdb_idl_txn *ovnnb_idl_txn;
> +
> + struct ovsdb_idl_loop *ovnsb_idl_loop;
> +
> void *client_ctx;
> };
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index d310fa4dd31f..f39dbd9da3e0 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -20,26 +20,123 @@
>
> #include "en-northd.h"
> #include "lib/inc-proc-eng.h"
> +#include "openvswitch/list.h" /* TODO This is needed for
ovn-parallel-hmap.h.
> + * lib/ovn-parallel-hmap.h should be
updated
> + * to include this dependency itself */
> +#include "lib/ovn-parallel-hmap.h"
> #include "northd.h"
> +#include "lib/util.h"
> #include "openvswitch/vlog.h"
>
> VLOG_DEFINE_THIS_MODULE(en_northd);
>
> -void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
> +void en_northd_run(struct engine_node *node, void *data)
> {
> const struct engine_context *eng_ctx = engine_get_context();
> - struct northd_context *ctx = eng_ctx->client_ctx;
> - ovn_db_run(ctx);
>
> + struct northd_input input_data;
> +
> + northd_destroy(data);
> + northd_init(data);
> +
> + input_data.sbrec_chassis_by_name =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_chassis", node),
> + "sbrec_chassis_by_name");
> + input_data.sbrec_chassis_by_hostname =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_chassis", node),
> + "sbrec_chassis_by_hostname");
> + input_data.sbrec_ha_chassis_grp_by_name =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_ha_chassis_group", node),
> + "sbrec_ha_chassis_grp_by_name");
> + input_data.sbrec_mcast_group_by_name_dp =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_multicast_group", node),
> + "sbrec_mcast_group_by_name");
> + input_data.sbrec_ip_mcast_by_dp =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_ip_multicast", node),
> + "sbrec_ip_mcast_by_dp");
> +
> + input_data.nbrec_nb_global_table =
> + EN_OVSDB_GET(engine_get_input("NB_nb_global", node));
> + input_data.nbrec_logical_switch =
> + EN_OVSDB_GET(engine_get_input("NB_logical_switch", node));
> + input_data.nbrec_logical_router =
> + EN_OVSDB_GET(engine_get_input("NB_logical_router", node));
> + input_data.nbrec_load_balancer_table =
> + EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> + input_data.nbrec_port_group_table =
> + EN_OVSDB_GET(engine_get_input("NB_port_group", node));
> + input_data.nbrec_bfd_table =
> + EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> + input_data.nbrec_address_set_table =
> + EN_OVSDB_GET(engine_get_input("NB_address_set", node));
> + input_data.nbrec_meter_table =
> + EN_OVSDB_GET(engine_get_input("NB_meter", node));
> + input_data.nbrec_acl_table =
> + EN_OVSDB_GET(engine_get_input("NB_acl", node));
> +
> + input_data.sbrec_sb_global_table =
> + EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> + input_data.sbrec_datapath_binding_table =
> + EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
> + input_data.sbrec_port_binding_table =
> + EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
> + input_data.sbrec_mac_binding_table =
> + EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> + input_data.sbrec_ha_chassis_group_table =
> + EN_OVSDB_GET(engine_get_input("SB_ha_chassis_group", node));
> + input_data.sbrec_chassis =
> + EN_OVSDB_GET(engine_get_input("SB_chassis", node));
> + input_data.sbrec_fdb_table =
> + EN_OVSDB_GET(engine_get_input("SB_fdb", node));
> + input_data.sbrec_load_balancer_table =
> + EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
> + input_data.sbrec_service_monitor_table =
> + EN_OVSDB_GET(engine_get_input("SB_service_monitor", node));
> + input_data.sbrec_bfd_table =
> + EN_OVSDB_GET(engine_get_input("SB_bfd", node));
> + input_data.sbrec_logical_flow_table =
> + EN_OVSDB_GET(engine_get_input("SB_logical_flow", node));
> + input_data.sbrec_multicast_group_table =
> + EN_OVSDB_GET(engine_get_input("SB_multicast_group", node));
> + input_data.sbrec_address_set_table =
> + EN_OVSDB_GET(engine_get_input("SB_address_set", node));
> + input_data.sbrec_port_group_table =
> + EN_OVSDB_GET(engine_get_input("SB_port_group", node));
> + input_data.sbrec_meter_table =
> + EN_OVSDB_GET(engine_get_input("SB_meter", node));
> + input_data.sbrec_dns_table =
> + EN_OVSDB_GET(engine_get_input("SB_dns", node));
> + input_data.sbrec_ip_multicast_table =
> + EN_OVSDB_GET(engine_get_input("SB_ip_multicast", node));
> + input_data.sbrec_igmp_group_table =
> + EN_OVSDB_GET(engine_get_input("SB_igmp_group", node));
> + input_data.sbrec_chassis_private_table =
> + EN_OVSDB_GET(engine_get_input("SB_chassis_private", node));
> +
> + northd_run(&input_data, data,
> + eng_ctx->ovnnb_idl_txn,
> + eng_ctx->ovnsb_idl_txn,
> + eng_ctx->ovnsb_idl_loop);
> engine_set_node_state(node, EN_UPDATED);
>
> }
> void *en_northd_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> {
> - return NULL;
> + struct northd_data *data = xmalloc(sizeof *data);
> +
> + northd_init(data);
> +
> + return data;
> }
>
> -void en_northd_cleanup(void *data OVS_UNUSED)
> +void en_northd_cleanup(void *data)
> {
> + northd_destroy(data);
> + free(data);
This will cause double free. I saw that you removed it in the next patch,
but it shouldn't be here in the first place. I don't think it needs another
revision, and I can just remove this line before merging.
...
> +void
> +northd_destroy(struct northd_data *data)
> +{
> + struct ovn_northd_lb *lb;
> + HMAP_FOR_EACH_POP (lb, hmap_node, &data->lbs) {
> + ovn_northd_lb_destroy(lb);
> + }
> + hmap_destroy(&data->lbs);
> +
> + struct ovn_igmp_group *igmp_group, *next_igmp_group;
> +
> + HMAP_FOR_EACH_SAFE (igmp_group, next_igmp_group, hmap_node,
> + &data->igmp_groups) {
> + ovn_igmp_group_destroy(&data->igmp_groups, igmp_group);
> + }
> +
> + struct ovn_port_group *pg, *next_pg;
> + HMAP_FOR_EACH_SAFE (pg, next_pg, key_node, &data->port_groups) {
> + ovn_port_group_destroy(&data->port_groups, pg);
> + }
> +
> + hmap_destroy(&data->igmp_groups);
> + hmap_destroy(&data->mcast_groups);
> + hmap_destroy(&data->port_groups);
> + hmap_destroy(&data->bfd_connections);
> +
> + struct shash_node *node, *next;
> + SHASH_FOR_EACH_SAFE (node, next, &data->meter_groups) {
> + shash_delete(&data->meter_groups, node);
> + }
> + shash_destroy(&data->meter_groups);
> +
> + /* XXX Having to explicitly clean up macam here
> + * is a bit strange. We don't explicitly initialize
> + * macam in this module, but this is the logical place
> + * to clean it up. Ideally, more IPAM logic can be factored
> + * out of ovn-northd and this can be taken care of there
> + * as well.
> + */
> + cleanup_macam();
Yes, it does look weird here since macam is a static member of the ipam
module but it is updated in the I-P engine. We should either avoid updating
it in I-P engine, or move the data as part of the I-P engine. But I think
it is ok to leave it as a follow-up patch whichever method is appropriate.
Thanks,
Han
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
