On Thu, Aug 10, 2023 at 5:45 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> For now it's still a node recompute for every port group change. It
> doesn't trigger northd recompute anymore though.
>
> A follow up commit will add incremental processing of NB.Port_Group
> changes.
>
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
> lib/stopwatch-names.h | 1 +
> northd/en-lflow.c | 4 ++-
> northd/en-northd.c | 4 ---
> northd/en-port-group.c | 56
++++++++++++++++++++++++++++++++++++++++++++++
> northd/en-port-group.h | 21 +++++++++++++++++
> northd/inc-proc-northd.c | 15 ++++++++++--
> northd/northd.c | 9 -------
> northd/northd.h | 3 --
> tests/ovn-northd.at | 8 ++-----
> 9 files changed, 96 insertions(+), 25 deletions(-)
>
> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> index de6fca4ccc..08cb0159a7 100644
> --- a/lib/stopwatch-names.h
> +++ b/lib/stopwatch-names.h
> @@ -30,5 +30,6 @@
> #define LFLOWS_IGMP_STOPWATCH_NAME "lflows_igmp"
> #define LFLOWS_DP_GROUPS_STOPWATCH_NAME "lflows_dp_groups"
> #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
> +#define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
>
> #endif
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 7187cf959f..7f6a7872b2 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -35,6 +35,8 @@ lflow_get_input_data(struct engine_node *node,
> struct lflow_input *lflow_input)
> {
> struct northd_data *northd_data = engine_get_input_data("northd",
node);
> + struct port_group_data *pg_data =
> + engine_get_input_data("port_group", node);
> lflow_input->nbrec_bfd_table =
> EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> lflow_input->sbrec_bfd_table =
> @@ -55,7 +57,7 @@ lflow_get_input_data(struct engine_node *node,
> lflow_input->lr_datapaths = &northd_data->lr_datapaths;
> lflow_input->ls_ports = &northd_data->ls_ports;
> lflow_input->lr_ports = &northd_data->lr_ports;
> - lflow_input->ls_port_groups = &northd_data->ls_port_groups;
> + lflow_input->ls_port_groups = &pg_data->ls_port_groups;
> lflow_input->meter_groups = &northd_data->meter_groups;
> lflow_input->lbs = &northd_data->lbs;
> lflow_input->features = &northd_data->features;
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 044fa70190..6fb0597144 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -74,8 +74,6 @@ northd_get_input_data(struct engine_node *node,
> EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> input_data->nbrec_load_balancer_group_table =
> EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> - input_data->nbrec_port_group_table =
> - EN_OVSDB_GET(engine_get_input("NB_port_group", node));
> input_data->nbrec_meter_table =
> EN_OVSDB_GET(engine_get_input("NB_meter", node));
> input_data->nbrec_acl_table =
> @@ -105,8 +103,6 @@ northd_get_input_data(struct engine_node *node,
> 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_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 =
> diff --git a/northd/en-port-group.c b/northd/en-port-group.c
> index b83926c351..2c36410246 100644
> --- a/northd/en-port-group.c
> +++ b/northd/en-port-group.c
> @@ -17,8 +17,10 @@
> #include <config.h>
>
> #include "openvswitch/vlog.h"
> +#include "stopwatch.h"
>
> #include "en-port-group.h"
> +#include "lib/stopwatch-names.h"
> #include "northd.h"
>
> VLOG_DEFINE_THIS_MODULE(en_port_group);
> @@ -235,3 +237,57 @@ ls_port_group_record_destroy(struct ls_port_group
*ls_pg,
> }
> }
>
> +/* Incremental processing implementation. */
> +static struct port_group_input
> +port_group_get_input_data(struct engine_node *node)
> +{
> + struct northd_data *northd_data = engine_get_input_data("northd",
node);
> +
> + return (struct port_group_input) {
> + .nbrec_port_group_table =
> + EN_OVSDB_GET(engine_get_input("NB_port_group", node)),
> + .sbrec_port_group_table =
> + EN_OVSDB_GET(engine_get_input("SB_port_group", node)),
> + .ls_ports = &northd_data->ls_ports,
> + };
> +}
> +
> +void *
> +en_port_group_init(struct engine_node *node OVS_UNUSED,
> + struct engine_arg *arg OVS_UNUSED)
> +{
> + struct port_group_data *pg_data = xmalloc(sizeof *pg_data);
> +
> + ls_port_group_table_init(&pg_data->ls_port_groups);
> + return pg_data;
> +}
> +
> +void
> +en_port_group_cleanup(void *data_)
> +{
> + struct port_group_data *data = data_;
> +
> + ls_port_group_table_destroy(&data->ls_port_groups);
> +}
> +
> +void
> +en_port_group_run(struct engine_node *node, void *data_)
> +{
> + const struct engine_context *eng_ctx = engine_get_context();
> + struct port_group_input input_data = port_group_get_input_data(node);
> + struct port_group_data *data = data_;
> +
> + stopwatch_start(PORT_GROUP_RUN_STOPWATCH_NAME, time_msec());
> +
> + ls_port_group_table_clear(&data->ls_port_groups);
> + ls_port_group_table_build(&data->ls_port_groups,
> + input_data.nbrec_port_group_table,
> + input_data.ls_ports);
> +
> + ls_port_group_table_sync(&data->ls_port_groups,
> + input_data.sbrec_port_group_table,
> + eng_ctx->ovnsb_idl_txn);
> +
> + stopwatch_stop(PORT_GROUP_RUN_STOPWATCH_NAME, time_msec());
> + engine_set_node_state(node, EN_UPDATED);
> +}
> diff --git a/northd/en-port-group.h b/northd/en-port-group.h
> index 2c8e01f51f..5cbf6c6c4a 100644
> --- a/northd/en-port-group.h
> +++ b/northd/en-port-group.h
> @@ -60,4 +60,25 @@ void ls_port_group_table_build(struct
ls_port_group_table *ls_port_groups,
> void ls_port_group_table_sync(const struct ls_port_group_table
*ls_port_groups,
> const struct sbrec_port_group_table *,
> struct ovsdb_idl_txn *ovnsb_txn);
> +
> +/* Incremental processing implementation. */
> +struct port_group_input {
> + /* Northbound table references. */
> + const struct nbrec_port_group_table *nbrec_port_group_table;
> +
> + /* Southbound table references. */
> + const struct sbrec_port_group_table *sbrec_port_group_table;
> +
> + /* northd node references. */
> + const struct hmap *ls_ports;
> +};
> +
> +struct port_group_data {
> + struct ls_port_group_table ls_port_groups;
> +};
> +
> +void *en_port_group_init(struct engine_node *, struct engine_arg *);
> +void en_port_group_cleanup(void *data);
> +void en_port_group_run(struct engine_node *, void *data);
> +
> #endif /* EN_PORT_GROUP_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index d328deb222..6d5f9e8d16 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -137,6 +137,7 @@ static ENGINE_NODE(mac_binding_aging_waker,
"mac_binding_aging_waker");
> static ENGINE_NODE(northd_output, "northd_output");
> static ENGINE_NODE(sync_to_sb, "sync_to_sb");
> static ENGINE_NODE(sync_to_sb_addr_set, "sync_to_sb_addr_set");
> +static ENGINE_NODE(port_group, "port_group");
> static ENGINE_NODE(fdb_aging, "fdb_aging");
> static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
>
> @@ -145,7 +146,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> {
> /* Define relationships between nodes where first argument is
dependent
> * on the second argument */
> - engine_add_input(&en_northd, &en_nb_port_group, NULL);
> engine_add_input(&en_northd, &en_nb_load_balancer, NULL);
> engine_add_input(&en_northd, &en_nb_load_balancer_group, NULL);
> engine_add_input(&en_northd, &en_nb_acl, NULL);
> @@ -157,7 +157,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>
> engine_add_input(&en_northd, &en_sb_sb_global, NULL);
> engine_add_input(&en_northd, &en_sb_chassis, NULL);
> - engine_add_input(&en_northd, &en_sb_port_group, NULL);
> engine_add_input(&en_northd, &en_sb_mirror, NULL);
> engine_add_input(&en_northd, &en_sb_meter, NULL);
> engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> @@ -194,6 +193,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_lflow, &en_sb_multicast_group, NULL);
> engine_add_input(&en_lflow, &en_sb_igmp_group, NULL);
> engine_add_input(&en_lflow, &en_northd, lflow_northd_handler);
> + engine_add_input(&en_lflow, &en_port_group, NULL);
>
> engine_add_input(&en_sync_to_sb_addr_set, &en_nb_address_set,
> sync_to_sb_addr_set_nb_address_set_handler);
> @@ -202,11 +202,20 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_sync_to_sb_addr_set, &en_northd, NULL);
> engine_add_input(&en_sync_to_sb_addr_set, &en_sb_address_set, NULL);
>
> + engine_add_input(&en_port_group, &en_nb_port_group, NULL);
> + engine_add_input(&en_port_group, &en_sb_port_group, NULL);
> + /* No need for an explicit handler for northd changes. Port changes
> + * that affect port_groups trigger updates to the NB.Port_Group
> + * table too (because of the explicit dependency in the schema). */
> + engine_add_input(&en_port_group, &en_northd, engine_noop_handler);
> +
> /* en_sync_to_sb engine node syncs the SB database tables from
> * the NB database tables.
> - * Right now this engine only syncs the SB Address_Set table.
> + * Right now this engine syncs the SB Address_Set and Port_Group
> + * tables.
> */
> engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL);
> + engine_add_input(&en_sync_to_sb, &en_port_group, NULL);
>
> engine_add_input(&en_sync_from_sb, &en_northd,
> sync_from_sb_northd_handler);
> diff --git a/northd/northd.c b/northd/northd.c
> index 04da75fa96..b695153805 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -17238,7 +17238,6 @@ northd_init(struct northd_data *data)
> ovn_datapaths_init(&data->lr_datapaths);
> hmap_init(&data->ls_ports);
> hmap_init(&data->lr_ports);
> - ls_port_group_table_init(&data->ls_port_groups);
> shash_init(&data->meter_groups);
> hmap_init(&data->lbs);
> hmap_init(&data->lb_groups);
> @@ -17270,8 +17269,6 @@ northd_destroy(struct northd_data *data)
> }
> hmap_destroy(&data->lb_groups);
>
> - ls_port_group_table_destroy(&data->ls_port_groups);
> -
> struct shash_node *node;
> SHASH_FOR_EACH_SAFE (node, &data->meter_groups) {
> shash_delete(&data->meter_groups, node);
> @@ -17410,9 +17407,6 @@ ovnnb_db_run(struct northd_input *input_data,
> ods_size(&data->ls_datapaths),
> ods_size(&data->lr_datapaths));
> build_ipam(&data->ls_datapaths.datapaths, &data->ls_ports);
> - ls_port_group_table_build(&data->ls_port_groups,
> - input_data->nbrec_port_group_table,
> - &data->ls_ports);
> build_lrouter_groups(&data->lr_ports, &data->lr_list);
> build_ip_mcast(ovnsb_txn, input_data->sbrec_ip_multicast_table,
> input_data->sbrec_ip_mcast_by_dp,
> @@ -17430,9 +17424,6 @@ ovnnb_db_run(struct northd_input *input_data,
>
> sync_lbs(ovnsb_txn, input_data->sbrec_load_balancer_table,
> &data->ls_datapaths, &data->lbs);
> - ls_port_group_table_sync(&data->ls_port_groups,
> - input_data->sbrec_port_group_table,
> - ovnsb_txn);
> sync_meters(ovnsb_txn, input_data->nbrec_meter_table,
> input_data->nbrec_acl_table,
input_data->sbrec_meter_table,
> &data->meter_groups);
> diff --git a/northd/northd.h b/northd/northd.h
> index da93a7c6a5..ba28ec63af 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -31,7 +31,6 @@ struct northd_input {
> const struct nbrec_load_balancer_table *nbrec_load_balancer_table;
> const struct nbrec_load_balancer_group_table
> *nbrec_load_balancer_group_table;
> - const struct nbrec_port_group_table *nbrec_port_group_table;
> const struct nbrec_meter_table *nbrec_meter_table;
> const struct nbrec_acl_table *nbrec_acl_table;
> const struct nbrec_static_mac_binding_table
> @@ -50,7 +49,6 @@ struct northd_input {
> const struct sbrec_fdb_table *sbrec_fdb_table;
> const struct sbrec_load_balancer_table *sbrec_load_balancer_table;
> const struct sbrec_service_monitor_table
*sbrec_service_monitor_table;
> - const struct sbrec_port_group_table *sbrec_port_group_table;
> const struct sbrec_meter_table *sbrec_meter_table;
> const struct sbrec_dns_table *sbrec_dns_table;
> const struct sbrec_ip_multicast_table *sbrec_ip_multicast_table;
> @@ -109,7 +107,6 @@ struct northd_data {
> struct ovn_datapaths lr_datapaths;
> struct hmap ls_ports;
> struct hmap lr_ports;
> - struct ls_port_group_table ls_port_groups;
> struct shash meter_groups;
> struct hmap lbs;
> struct hmap lb_groups;
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index d5be3be75b..1a12513d7a 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -8923,11 +8923,9 @@ check as northd ovn-appctl -t NORTHD_TYPE
inc-engine/clear-stats
> check ovn-nbctl add port_group pg1 ports ${p1_uuid}
> wait_column '20.0.0.4' Address_Set addresses name=pg1_ip4
>
> -# There should be recompute of the sync_to_sb_addr_set engine node since
northd engine changes.
> -# There will be another recompute when the update message is received
from the sb ovsdb-server.
> -# Once we add I-P for Port_Groups, there should be no recompute here.
> -recompute_stat=$(as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats sync_to_sb_addr_set recompute)
> -AT_CHECK([test $recompute_stat -ge 1])
> +# There should be no recompute of the sync_to_sb_addr_set engine node.
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats
sync_to_sb_addr_set recompute], [0], [0
> +])
>
> # No change, no recompute
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>
Acked-by: Han Zhou <hz...@ovn.org>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
