| Message ID | 169167150658.2447623.16064515466507285336.stgit@dceara.remote.csb |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add port group incremental processing in northd. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Thu, Aug 10, 2023 at 2:45 PM Dumitru Ceara <dceara@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 <dceara@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 > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On Thu, Aug 10, 2023 at 5:45 AM Dumitru Ceara <dceara@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 <dceara@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 <hzhou@ovn.org>
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
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 <dceara@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(-)