| Message ID | 169228593571.3284801.16345967697151576426.stgit@dceara.remote.csb |
|---|---|
| State | Accepted |
| Headers | show |
| Series | northd: Meter incremental processing. | 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 | success | github build: passed |
Hi Dumitru, I have one finding in the test code, but other than that it looks good. On 8/17/23 11:25, Dumitru Ceara wrote: > This is beneficial in a few ways: > - first, it reduces the number of different types of data the northd > I-P node has to process. > - it turns out the northd I-P node (whose recompute is rather costly) > doesn't really depend on meters or ACLs. > - prepares the ground for a pure I-P implementation for ACLs, with a > simple/clear dependency between NB.ACL and the lflow I-P node. > > From a meters synchronization perspective this commit doesn't change any > of the existing behavior and logic. It mostly just moves the meters > related code out of northd.c and into en-meters.c. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > lib/stopwatch-names.h | 1 > northd/automake.mk | 2 > northd/en-lflow.c | 5 + > northd/en-meters.c | 281 ++++++++++++++++++++++++++++++++++++++++++++++ > northd/en-meters.h | 44 +++++++ > northd/en-northd.c | 6 - > northd/inc-proc-northd.c | 14 ++ > northd/northd.c | 235 +------------------------------------- > northd/northd.h | 4 - > northd/ovn-northd.c | 3 > tests/ovn-northd.at | 36 ++++++ > 11 files changed, 390 insertions(+), 241 deletions(-) > create mode 100644 northd/en-meters.c > create mode 100644 northd/en-meters.h > > diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h > index de6fca4ccc..98535fff5a 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 SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run" > > #endif > diff --git a/northd/automake.mk b/northd/automake.mk > index b17f1fdb54..f52766de0c 100644 > --- a/northd/automake.mk > +++ b/northd/automake.mk > @@ -12,6 +12,8 @@ northd_ovn_northd_SOURCES = \ > northd/en-northd.h \ > northd/en-lflow.c \ > northd/en-lflow.h \ > + northd/en-meters.c \ > + northd/en-meters.h \ > northd/en-northd-output.c \ > northd/en-northd-output.h \ > northd/en-sync-sb.c \ > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index 28ab1c67fb..0886d4c5ca 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -20,6 +20,7 @@ > > #include "en-lflow.h" > #include "en-northd.h" > +#include "en-meters.h" > > #include "lib/inc-proc-eng.h" > #include "northd.h" > @@ -35,6 +36,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 sync_meters_data *sync_meters_data = > + engine_get_input_data("sync_meters", node); > lflow_input->nbrec_bfd_table = > EN_OVSDB_GET(engine_get_input("NB_bfd", node)); > lflow_input->sbrec_bfd_table = > @@ -56,7 +59,7 @@ lflow_get_input_data(struct engine_node *node, > lflow_input->ls_ports = &northd_data->ls_ports; > lflow_input->lr_ports = &northd_data->lr_ports; > lflow_input->port_groups = &northd_data->port_groups; > - lflow_input->meter_groups = &northd_data->meter_groups; > + lflow_input->meter_groups = &sync_meters_data->meter_groups; > lflow_input->lbs = &northd_data->lbs; > lflow_input->features = &northd_data->features; > lflow_input->ovn_internal_version_changed = > diff --git a/northd/en-meters.c b/northd/en-meters.c > new file mode 100644 > index 0000000000..aabd002b62 > --- /dev/null > +++ b/northd/en-meters.c > @@ -0,0 +1,281 @@ > +/* > + * Copyright (c) 2023, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > + > +#include "openvswitch/vlog.h" > +#include "stopwatch.h" > + > +#include "en-meters.h" > +#include "lib/stopwatch-names.h" > + > +VLOG_DEFINE_THIS_MODULE(en_meters); > + > +static void build_meter_groups(struct shash *meter_group, > + const struct nbrec_meter_table *); > +static void sync_meters(struct ovsdb_idl_txn *ovnsb_txn, > + const struct nbrec_meter_table *, > + const struct nbrec_acl_table *, > + const struct sbrec_meter_table *, > + struct shash *meter_groups); > + > +void > +*en_sync_meters_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct sync_meters_data *data = xmalloc(sizeof *data); > + > + *data = (struct sync_meters_data) { > + .meter_groups = SHASH_INITIALIZER(&data->meter_groups), > + }; > + return data; > +} > + > +void > +en_sync_meters_cleanup(void *data_) > +{ > + struct sync_meters_data *data = data_; > + > + shash_destroy(&data->meter_groups); > +} > + > +void > +en_sync_meters_run(struct engine_node *node, void *data_) > +{ > + struct sync_meters_data *data = data_; > + > + const struct nbrec_acl_table *acl_table = > + EN_OVSDB_GET(engine_get_input("NB_acl", node)); > + > + const struct nbrec_meter_table *nb_meter_table = > + EN_OVSDB_GET(engine_get_input("NB_meter", node)); > + > + const struct sbrec_meter_table *sb_meter_table = > + EN_OVSDB_GET(engine_get_input("SB_meter", node)); > + > + const struct engine_context *eng_ctx = engine_get_context(); > + > + stopwatch_start(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec()); > + > + build_meter_groups(&data->meter_groups, nb_meter_table); > + > + sync_meters(eng_ctx->ovnsb_idl_txn, nb_meter_table, acl_table, > + sb_meter_table, &data->meter_groups); > + > + stopwatch_stop(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec()); > + engine_set_node_state(node, EN_UPDATED); > +} > + > +const struct nbrec_meter* > +fair_meter_lookup_by_name(const struct shash *meter_groups, > + const char *meter_name) > +{ > + const struct nbrec_meter *nb_meter = > + meter_name ? shash_find_data(meter_groups, meter_name) : NULL; > + if (nb_meter) { > + return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; > + } > + return NULL; > +} > + > +struct band_entry { > + int64_t rate; > + int64_t burst_size; > + const char *action; > +}; > + > +static int > +band_cmp(const void *band1_, const void *band2_) > +{ > + const struct band_entry *band1p = band1_; > + const struct band_entry *band2p = band2_; > + > + if (band1p->rate != band2p->rate) { > + return band1p->rate - band2p->rate; > + } else if (band1p->burst_size != band2p->burst_size) { > + return band1p->burst_size - band2p->burst_size; > + } else { > + return strcmp(band1p->action, band2p->action); > + } > +} > + > +static bool > +bands_need_update(const struct nbrec_meter *nb_meter, > + const struct sbrec_meter *sb_meter) > +{ > + if (nb_meter->n_bands != sb_meter->n_bands) { > + return true; > + } > + > + /* Place the Northbound entries in sorted order. */ > + struct band_entry *nb_bands; > + nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands); > + for (size_t i = 0; i < nb_meter->n_bands; i++) { > + struct nbrec_meter_band *nb_band = nb_meter->bands[i]; > + > + nb_bands[i].rate = nb_band->rate; > + nb_bands[i].burst_size = nb_band->burst_size; > + nb_bands[i].action = nb_band->action; > + } > + qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp); > + > + /* Place the Southbound entries in sorted order. */ > + struct band_entry *sb_bands; > + sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands); > + for (size_t i = 0; i < sb_meter->n_bands; i++) { > + struct sbrec_meter_band *sb_band = sb_meter->bands[i]; > + > + sb_bands[i].rate = sb_band->rate; > + sb_bands[i].burst_size = sb_band->burst_size; > + sb_bands[i].action = sb_band->action; > + } > + qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp); > + > + bool need_update = false; > + for (size_t i = 0; i < nb_meter->n_bands; i++) { > + if (band_cmp(&nb_bands[i], &sb_bands[i])) { > + need_update = true; > + break; > + } > + } > + > + free(nb_bands); > + free(sb_bands); > + > + return need_update; > +} > + > +static void > +sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn, > + const char *meter_name, > + const struct nbrec_meter *nb_meter, > + struct shash *sb_meters, > + struct sset *used_sb_meters) > +{ > + const struct sbrec_meter *sb_meter; > + bool new_sb_meter = false; > + > + sb_meter = shash_find_data(sb_meters, meter_name); > + if (!sb_meter) { > + sb_meter = sbrec_meter_insert(ovnsb_txn); > + sbrec_meter_set_name(sb_meter, meter_name); > + shash_add(sb_meters, sb_meter->name, sb_meter); > + new_sb_meter = true; > + } > + sset_add(used_sb_meters, meter_name); > + > + if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { > + struct sbrec_meter_band **sb_bands; > + sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); > + for (size_t i = 0; i < nb_meter->n_bands; i++) { > + const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; > + > + sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn); > + > + sbrec_meter_band_set_action(sb_bands[i], nb_band->action); > + sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); > + sbrec_meter_band_set_burst_size(sb_bands[i], > + nb_band->burst_size); > + } > + sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); > + free(sb_bands); > + } > + > + sbrec_meter_set_unit(sb_meter, nb_meter->unit); > +} > + > +static void > +sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, > + struct shash *meter_groups, > + const struct nbrec_acl *acl, struct shash *sb_meters, > + struct sset *used_sb_meters) > +{ > + const struct nbrec_meter *nb_meter = > + fair_meter_lookup_by_name(meter_groups, acl->meter); > + > + if (!nb_meter) { > + return; > + } > + > + char *meter_name = alloc_acl_log_unique_meter_name(acl); > + sync_meters_iterate_nb_meter(ovnsb_txn, meter_name, nb_meter, sb_meters, > + used_sb_meters); > + free(meter_name); > +} > + > +static void > +build_meter_groups(struct shash *meter_groups, > + const struct nbrec_meter_table *nb_meter_table) > +{ > + const struct nbrec_meter *nb_meter; > + > + shash_clear(meter_groups); > + NBREC_METER_TABLE_FOR_EACH (nb_meter, nb_meter_table) { > + shash_add(meter_groups, nb_meter->name, nb_meter); > + } > +} > + > +/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have > + * a corresponding entries in the Meter and Meter_Band tables in > + * OVN_Southbound. Additionally, ACL logs that use fair meters have > + * a private copy of its meter in the SB table. > + */ > +static void > +sync_meters(struct ovsdb_idl_txn *ovnsb_txn, > + const struct nbrec_meter_table *nbrec_meter_table, > + const struct nbrec_acl_table *nbrec_acl_table, > + const struct sbrec_meter_table *sbrec_meter_table, > + struct shash *meter_groups) > +{ > + struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); > + struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters); > + > + const struct sbrec_meter *sb_meter; > + SBREC_METER_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) { > + shash_add(&sb_meters, sb_meter->name, sb_meter); > + } > + > + const struct nbrec_meter *nb_meter; > + NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { > + sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name, nb_meter, > + &sb_meters, &used_sb_meters); > + } > + > + /* > + * In addition to creating Meters in the SB from the block above, check > + * and see if additional rows are needed to get ACLs logs individually > + * rate-limited. > + */ > + const struct nbrec_acl *acl; > + NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) { > + sync_acl_fair_meter(ovnsb_txn, meter_groups, acl, > + &sb_meters, &used_sb_meters); > + } > + > + const char *used_meter; > + SSET_FOR_EACH_SAFE (used_meter, &used_sb_meters) { > + shash_find_and_delete(&sb_meters, used_meter); > + sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter)); > + } > + sset_destroy(&used_sb_meters); > + > + struct shash_node *node; > + SHASH_FOR_EACH_SAFE (node, &sb_meters) { > + sbrec_meter_delete(node->data); > + shash_delete(&sb_meters, node); > + } > + shash_destroy(&sb_meters); > +} > diff --git a/northd/en-meters.h b/northd/en-meters.h > new file mode 100644 > index 0000000000..a1743282e3 > --- /dev/null > +++ b/northd/en-meters.h > @@ -0,0 +1,44 @@ > +/* > + * Copyright (c) 2023, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > +#ifndef EN_METERS_H > +#define EN_METERS_H 1 > + > +#include "openvswitch/shash.h" > + > +#include "lib/inc-proc-eng.h" > +#include "lib/ovn-nb-idl.h" > +#include "lib/ovn-sb-idl.h" > + > +struct sync_meters_data { > + struct shash meter_groups; > +}; > + > +void *en_sync_meters_init(struct engine_node *, struct engine_arg *); > +void en_sync_meters_cleanup(void *data); > +void en_sync_meters_run(struct engine_node *, void *data); > + > +const struct nbrec_meter *fair_meter_lookup_by_name( > + const struct shash *meter_groups, > + const char *meter_name); > + > +static inline char* > +alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) > +{ > + return xasprintf("%s__" UUID_FMT, > + acl->meter, UUID_ARGS(&acl->header_.uuid)); > +} > + > +#endif /* EN_ACL_H */ > diff --git a/northd/en-northd.c b/northd/en-northd.c > index 044fa70190..cc05efdbbc 100644 > --- a/northd/en-northd.c > +++ b/northd/en-northd.c > @@ -76,10 +76,6 @@ northd_get_input_data(struct engine_node *node, > 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 = > - EN_OVSDB_GET(engine_get_input("NB_acl", node)); > input_data->nbrec_static_mac_binding_table = > EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node)); > input_data->nbrec_chassis_template_var_table = > @@ -107,8 +103,6 @@ northd_get_input_data(struct engine_node *node, > 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 = > EN_OVSDB_GET(engine_get_input("SB_dns", node)); > input_data->sbrec_ip_multicast_table = > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index d328deb222..1de0551b3d 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -33,6 +33,7 @@ > #include "en-northd.h" > #include "en-lflow.h" > #include "en-northd-output.h" > +#include "en-meters.h" > #include "en-sync-sb.h" > #include "en-sync-from-sb.h" > #include "unixctl.h" > @@ -135,6 +136,7 @@ static ENGINE_NODE(lflow, "lflow"); > static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); > static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); > static ENGINE_NODE(northd_output, "northd_output"); > +static ENGINE_NODE(sync_meters, "sync_meters"); > 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(fdb_aging, "fdb_aging"); > @@ -148,10 +150,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > 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); > engine_add_input(&en_northd, &en_nb_logical_router, NULL); > engine_add_input(&en_northd, &en_nb_mirror, NULL); > - engine_add_input(&en_northd, &en_nb_meter, NULL); > engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL); > engine_add_input(&en_northd, &en_nb_chassis_template_var, NULL); > > @@ -188,7 +188,13 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_fdb_aging, &en_northd, NULL); > engine_add_input(&en_fdb_aging, &en_fdb_aging_waker, NULL); > > + engine_add_input(&en_sync_meters, &en_nb_acl, NULL); > + engine_add_input(&en_sync_meters, &en_nb_meter, NULL); > + engine_add_input(&en_sync_meters, &en_sb_meter, NULL); > + > engine_add_input(&en_lflow, &en_nb_bfd, NULL); > + engine_add_input(&en_lflow, &en_nb_acl, NULL); > + engine_add_input(&en_lflow, &en_sync_meters, NULL); > engine_add_input(&en_lflow, &en_sb_bfd, NULL); > engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); > engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); > @@ -204,9 +210,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > /* 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 table and > + * SB Meter/Meter_Band tables. > */ > engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL); > + engine_add_input(&en_sync_to_sb, &en_sync_meters, 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 13f2ae0565..48fb44a8be 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -21,6 +21,7 @@ > #include "bitmap.h" > #include "coverage.h" > #include "dirs.h" > +#include "en-meters.h" > #include "ipam.h" > #include "openvswitch/dynamic-string.h" > #include "hash.h" > @@ -4980,25 +4981,22 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports, > } > > static bool > -check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls) > +check_ls_changes_other_than_lsp_acl(const struct nbrec_logical_switch *ls) > { > /* Check if the columns are changed in this row. */ > enum nbrec_logical_switch_column_id col; > for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) { > - if (nbrec_logical_switch_is_updated(ls, col) && > - col != NBREC_LOGICAL_SWITCH_COL_PORTS) { > + if (nbrec_logical_switch_is_updated(ls, col)) { > + if (col == NBREC_LOGICAL_SWITCH_COL_ACLS || > + col == NBREC_LOGICAL_SWITCH_COL_PORTS) { > + continue; > + } > return true; > } > } > > /* Check if the referenced rows are changed. > XXX: Need a better OVSDB IDL interface for this check. */ > - for (size_t i = 0; i < ls->n_acls; i++) { > - if (nbrec_acl_row_get_seqno(ls->acls[i], > - OVSDB_IDL_CHANGE_MODIFY) > 0) { > - return true; > - } > - } > if (ls->copp && nbrec_copp_row_get_seqno(ls->copp, > OVSDB_IDL_CHANGE_MODIFY) > 0) { > return true; > @@ -5106,7 +5104,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > } > > /* Now only able to handle lsp changes. */ > - if (check_ls_changes_other_than_lsp(changed_ls)) { > + if (check_ls_changes_other_than_lsp_acl(changed_ls)) { > goto fail; > } > > @@ -6987,25 +6985,6 @@ build_acl_hints(struct ovn_datapath *od, > } > } > > -static const struct nbrec_meter* > -fair_meter_lookup_by_name(const struct shash *meter_groups, > - const char *meter_name) > -{ > - const struct nbrec_meter *nb_meter = > - meter_name ? shash_find_data(meter_groups, meter_name) : NULL; > - if (nb_meter) { > - return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; > - } > - return NULL; > -} > - > -static char* > -alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) > -{ > - return xasprintf("%s__" UUID_FMT, > - acl->meter, UUID_ARGS(&acl->header_.uuid)); > -} > - > static void > build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, > const struct shash *meter_groups) > @@ -16608,183 +16587,6 @@ sync_port_groups(struct ovsdb_idl_txn *ovnsb_txn, > shash_destroy(&sb_port_groups); > } > > -struct band_entry { > - int64_t rate; > - int64_t burst_size; > - const char *action; > -}; > - > -static int > -band_cmp(const void *band1_, const void *band2_) > -{ > - const struct band_entry *band1p = band1_; > - const struct band_entry *band2p = band2_; > - > - if (band1p->rate != band2p->rate) { > - return band1p->rate - band2p->rate; > - } else if (band1p->burst_size != band2p->burst_size) { > - return band1p->burst_size - band2p->burst_size; > - } else { > - return strcmp(band1p->action, band2p->action); > - } > -} > - > -static bool > -bands_need_update(const struct nbrec_meter *nb_meter, > - const struct sbrec_meter *sb_meter) > -{ > - if (nb_meter->n_bands != sb_meter->n_bands) { > - return true; > - } > - > - /* Place the Northbound entries in sorted order. */ > - struct band_entry *nb_bands; > - nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands); > - for (size_t i = 0; i < nb_meter->n_bands; i++) { > - struct nbrec_meter_band *nb_band = nb_meter->bands[i]; > - > - nb_bands[i].rate = nb_band->rate; > - nb_bands[i].burst_size = nb_band->burst_size; > - nb_bands[i].action = nb_band->action; > - } > - qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp); > - > - /* Place the Southbound entries in sorted order. */ > - struct band_entry *sb_bands; > - sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands); > - for (size_t i = 0; i < sb_meter->n_bands; i++) { > - struct sbrec_meter_band *sb_band = sb_meter->bands[i]; > - > - sb_bands[i].rate = sb_band->rate; > - sb_bands[i].burst_size = sb_band->burst_size; > - sb_bands[i].action = sb_band->action; > - } > - qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp); > - > - bool need_update = false; > - for (size_t i = 0; i < nb_meter->n_bands; i++) { > - if (band_cmp(&nb_bands[i], &sb_bands[i])) { > - need_update = true; > - break; > - } > - } > - > - free(nb_bands); > - free(sb_bands); > - > - return need_update; > -} > - > -static void > -sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn, > - const char *meter_name, > - const struct nbrec_meter *nb_meter, > - struct shash *sb_meters, > - struct sset *used_sb_meters) > -{ > - const struct sbrec_meter *sb_meter; > - bool new_sb_meter = false; > - > - sb_meter = shash_find_data(sb_meters, meter_name); > - if (!sb_meter) { > - sb_meter = sbrec_meter_insert(ovnsb_txn); > - sbrec_meter_set_name(sb_meter, meter_name); > - shash_add(sb_meters, sb_meter->name, sb_meter); > - new_sb_meter = true; > - } > - sset_add(used_sb_meters, meter_name); > - > - if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { > - struct sbrec_meter_band **sb_bands; > - sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); > - for (size_t i = 0; i < nb_meter->n_bands; i++) { > - const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; > - > - sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn); > - > - sbrec_meter_band_set_action(sb_bands[i], nb_band->action); > - sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); > - sbrec_meter_band_set_burst_size(sb_bands[i], > - nb_band->burst_size); > - } > - sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); > - free(sb_bands); > - } > - > - sbrec_meter_set_unit(sb_meter, nb_meter->unit); > -} > - > -static void > -sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, > - struct shash *meter_groups, > - const struct nbrec_acl *acl, struct shash *sb_meters, > - struct sset *used_sb_meters) > -{ > - const struct nbrec_meter *nb_meter = > - fair_meter_lookup_by_name(meter_groups, acl->meter); > - > - if (!nb_meter) { > - return; > - } > - > - char *meter_name = alloc_acl_log_unique_meter_name(acl); > - sync_meters_iterate_nb_meter(ovnsb_txn, meter_name, nb_meter, sb_meters, > - used_sb_meters); > - free(meter_name); > -} > - > -/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have > - * a corresponding entries in the Meter and Meter_Band tables in > - * OVN_Southbound. Additionally, ACL logs that use fair meters have > - * a private copy of its meter in the SB table. > - */ > -static void > -sync_meters(struct ovsdb_idl_txn *ovnsb_txn, > - const struct nbrec_meter_table *nbrec_meter_table, > - const struct nbrec_acl_table *nbrec_acl_table, > - const struct sbrec_meter_table *sbrec_meter_table, > - struct shash *meter_groups) > -{ > - struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); > - struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters); > - > - const struct sbrec_meter *sb_meter; > - SBREC_METER_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) { > - shash_add(&sb_meters, sb_meter->name, sb_meter); > - } > - > - const struct nbrec_meter *nb_meter; > - NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { > - sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name, nb_meter, > - &sb_meters, &used_sb_meters); > - } > - > - /* > - * In addition to creating Meters in the SB from the block above, check > - * and see if additional rows are needed to get ACLs logs individually > - * rate-limited. > - */ > - const struct nbrec_acl *acl; > - NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) { > - sync_acl_fair_meter(ovnsb_txn, meter_groups, acl, > - &sb_meters, &used_sb_meters); > - } > - > - const char *used_meter; > - SSET_FOR_EACH_SAFE (used_meter, &used_sb_meters) { > - shash_find_and_delete(&sb_meters, used_meter); > - sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter)); > - } > - sset_destroy(&used_sb_meters); > - > - struct shash_node *node; > - SHASH_FOR_EACH_SAFE (node, &sb_meters) { > - sbrec_meter_delete(node->data); > - shash_delete(&sb_meters, node); > - } > - shash_destroy(&sb_meters); > -} > - > static bool > mirror_needs_update(const struct nbrec_mirror *nb_mirror, > const struct sbrec_mirror *sb_mirror) > @@ -17254,16 +17056,6 @@ build_mcast_groups(const struct sbrec_igmp_group_table *sbrec_igmp_group_table, > } > } > > -static void > -build_meter_groups(const struct nbrec_meter_table *nbrec_meter_table, > - struct shash *meter_groups) > -{ > - const struct nbrec_meter *nb_meter; > - NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { > - shash_add(meter_groups, nb_meter->name, nb_meter); > - } > -} > - > static const struct nbrec_static_mac_binding * > static_mac_binding_by_port_ip( > const struct nbrec_static_mac_binding_table *nbrec_static_mb_table, > @@ -17405,7 +17197,6 @@ northd_init(struct northd_data *data) > hmap_init(&data->ls_ports); > hmap_init(&data->lr_ports); > hmap_init(&data->port_groups); > - shash_init(&data->meter_groups); > hmap_init(&data->lbs); > hmap_init(&data->lb_groups); > ovs_list_init(&data->lr_list); > @@ -17443,12 +17234,6 @@ northd_destroy(struct northd_data *data) > > hmap_destroy(&data->port_groups); > > - struct shash_node *node; > - SHASH_FOR_EACH_SAFE (node, &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 > @@ -17587,7 +17372,6 @@ ovnnb_db_run(struct northd_input *input_data, > build_ip_mcast(ovnsb_txn, input_data->sbrec_ip_multicast_table, > input_data->sbrec_ip_mcast_by_dp, > &data->ls_datapaths.datapaths); > - build_meter_groups(input_data->nbrec_meter_table, &data->meter_groups); > build_static_mac_binding_table(ovnsb_txn, > input_data->nbrec_static_mac_binding_table, > input_data->sbrec_static_mac_binding_table, > @@ -17602,9 +17386,6 @@ ovnnb_db_run(struct northd_input *input_data, > &data->ls_datapaths, &data->lbs); > sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table, > &data->port_groups); > - sync_meters(ovnsb_txn, input_data->nbrec_meter_table, > - input_data->nbrec_acl_table, input_data->sbrec_meter_table, > - &data->meter_groups); > sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, > input_data->sbrec_mirror_table); > sync_dns_entries(ovnsb_txn, input_data->sbrec_dns_table, > diff --git a/northd/northd.h b/northd/northd.h > index f3e63b1e1a..dcc3acabd5 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -31,8 +31,6 @@ struct northd_input { > 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 > *nbrec_static_mac_binding_table; > const struct nbrec_chassis_template_var_table > @@ -50,7 +48,6 @@ struct northd_input { > 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; > const struct sbrec_static_mac_binding_table > @@ -109,7 +106,6 @@ struct northd_data { > struct hmap ls_ports; > struct hmap lr_ports; > struct hmap port_groups; > - struct shash meter_groups; > struct hmap lbs; > struct hmap lb_groups; > struct ovs_list lr_list; > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 4fa1b039ea..45362f4f93 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -836,6 +836,9 @@ main(int argc, char *argv[]) > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > &sbrec_multicast_group_columns[i]); > } > + for (size_t i = 0; i < SBREC_METER_N_COLUMNS; i++) { > + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_meter_columns[i]); > + } > > unixctl_command_register("sb-connection-status", "", 0, 0, > ovn_conn_show, ovnsb_idl_loop.idl); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index d5be3be75b..dc8bf253f1 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9,6 +9,8 @@ m4_define([_DUMP_DB_TABLES], [ > ovn-sbctl list logical_flow >> $1 > ovn-sbctl list port_binding >> $1 > ovn-sbctl list address_set >> $1 > + ovn-sbctl list meter >> $1 > + ovn-sbctl list meter_band >> $1 > ]) > > # CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -9679,6 +9681,40 @@ OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ACL/Meter incremental processing - no northd recompute]) > +ovn_start > + > +check_recompute_counter() { > + northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) > + AT_CHECK([test x$northd_recomp = x$1]) > + > + northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) > + AT_CHECK([test x$northd_recomp = x$1]) > + > + sync_meters_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_meters recompute) > + AT_CHECK([test x$sync_meters_recomp = x$3]) > +} In check_recompute_counter(), the same comparison is performed twice for the $1 parameter. Then the $3 parameter is used in the third comparison. The $2 parameter is never used. I assume $2 was supposed to be used for something, but it's not clear what. > + > +check ovn-nbctl --wait=sb ls-add ls > + > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > +check ovn-nbctl --wait=sb meter-add m drop 1 pktps > +check ovn-nbctl --wait=sb acl-add ls from-lport 1 1 allow > +dnl Only triggers recompute of the sync_meters and lflow nodes. > +check_recompute_counter 0 2 2 > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > +check ovn-nbctl --wait=sb meter-del m > +check ovn-nbctl --wait=sb acl-del ls > +dnl Only triggers recompute of the sync_meters and lflow nodes. > +check_recompute_counter 0 2 2 > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([check fip and lb flows]) > AT_KEYWORDS([fip-lb-flows]) > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 8/21/23 19:33, Mark Michelson wrote: > Hi Dumitru, > > I have one finding in the test code, but other than that it looks good. > Hi Mark, Thanks for the review! > On 8/17/23 11:25, Dumitru Ceara wrote: >> This is beneficial in a few ways: >> - first, it reduces the number of different types of data the northd >> I-P node has to process. >> - it turns out the northd I-P node (whose recompute is rather costly) >> doesn't really depend on meters or ACLs. >> - prepares the ground for a pure I-P implementation for ACLs, with a >> simple/clear dependency between NB.ACL and the lflow I-P node. >> >> From a meters synchronization perspective this commit doesn't change any >> of the existing behavior and logic. It mostly just moves the meters >> related code out of northd.c and into en-meters.c. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> lib/stopwatch-names.h | 1 >> northd/automake.mk | 2 >> northd/en-lflow.c | 5 + >> northd/en-meters.c | 281 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> northd/en-meters.h | 44 +++++++ >> northd/en-northd.c | 6 - >> northd/inc-proc-northd.c | 14 ++ >> northd/northd.c | 235 +------------------------------------- >> northd/northd.h | 4 - >> northd/ovn-northd.c | 3 >> tests/ovn-northd.at | 36 ++++++ >> 11 files changed, 390 insertions(+), 241 deletions(-) >> create mode 100644 northd/en-meters.c >> create mode 100644 northd/en-meters.h >> >> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h >> index de6fca4ccc..98535fff5a 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 SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run" >> #endif >> diff --git a/northd/automake.mk b/northd/automake.mk >> index b17f1fdb54..f52766de0c 100644 >> --- a/northd/automake.mk >> +++ b/northd/automake.mk >> @@ -12,6 +12,8 @@ northd_ovn_northd_SOURCES = \ >> northd/en-northd.h \ >> northd/en-lflow.c \ >> northd/en-lflow.h \ >> + northd/en-meters.c \ >> + northd/en-meters.h \ >> northd/en-northd-output.c \ >> northd/en-northd-output.h \ >> northd/en-sync-sb.c \ >> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >> index 28ab1c67fb..0886d4c5ca 100644 >> --- a/northd/en-lflow.c >> +++ b/northd/en-lflow.c >> @@ -20,6 +20,7 @@ >> #include "en-lflow.h" >> #include "en-northd.h" >> +#include "en-meters.h" >> #include "lib/inc-proc-eng.h" >> #include "northd.h" >> @@ -35,6 +36,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 sync_meters_data *sync_meters_data = >> + engine_get_input_data("sync_meters", node); >> lflow_input->nbrec_bfd_table = >> EN_OVSDB_GET(engine_get_input("NB_bfd", node)); >> lflow_input->sbrec_bfd_table = >> @@ -56,7 +59,7 @@ lflow_get_input_data(struct engine_node *node, >> lflow_input->ls_ports = &northd_data->ls_ports; >> lflow_input->lr_ports = &northd_data->lr_ports; >> lflow_input->port_groups = &northd_data->port_groups; >> - lflow_input->meter_groups = &northd_data->meter_groups; >> + lflow_input->meter_groups = &sync_meters_data->meter_groups; >> lflow_input->lbs = &northd_data->lbs; >> lflow_input->features = &northd_data->features; >> lflow_input->ovn_internal_version_changed = >> diff --git a/northd/en-meters.c b/northd/en-meters.c >> new file mode 100644 >> index 0000000000..aabd002b62 >> --- /dev/null >> +++ b/northd/en-meters.c >> @@ -0,0 +1,281 @@ >> +/* >> + * Copyright (c) 2023, Red Hat, Inc. >> + * >> + * Licensed under the Apache License, Version 2.0 (the "License"); >> + * you may not use this file except in compliance with the License. >> + * You may obtain a copy of the License at: >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, software >> + * distributed under the License is distributed on an "AS IS" BASIS, >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >> implied. >> + * See the License for the specific language governing permissions and >> + * limitations under the License. >> + */ >> + >> +#include <config.h> >> + >> +#include "openvswitch/vlog.h" >> +#include "stopwatch.h" >> + >> +#include "en-meters.h" >> +#include "lib/stopwatch-names.h" >> + >> +VLOG_DEFINE_THIS_MODULE(en_meters); >> + >> +static void build_meter_groups(struct shash *meter_group, >> + const struct nbrec_meter_table *); >> +static void sync_meters(struct ovsdb_idl_txn *ovnsb_txn, >> + const struct nbrec_meter_table *, >> + const struct nbrec_acl_table *, >> + const struct sbrec_meter_table *, >> + struct shash *meter_groups); >> + >> +void >> +*en_sync_meters_init(struct engine_node *node OVS_UNUSED, >> + struct engine_arg *arg OVS_UNUSED) >> +{ >> + struct sync_meters_data *data = xmalloc(sizeof *data); >> + >> + *data = (struct sync_meters_data) { >> + .meter_groups = SHASH_INITIALIZER(&data->meter_groups), >> + }; >> + return data; >> +} >> + >> +void >> +en_sync_meters_cleanup(void *data_) >> +{ >> + struct sync_meters_data *data = data_; >> + >> + shash_destroy(&data->meter_groups); >> +} >> + >> +void >> +en_sync_meters_run(struct engine_node *node, void *data_) >> +{ >> + struct sync_meters_data *data = data_; >> + >> + const struct nbrec_acl_table *acl_table = >> + EN_OVSDB_GET(engine_get_input("NB_acl", node)); >> + >> + const struct nbrec_meter_table *nb_meter_table = >> + EN_OVSDB_GET(engine_get_input("NB_meter", node)); >> + >> + const struct sbrec_meter_table *sb_meter_table = >> + EN_OVSDB_GET(engine_get_input("SB_meter", node)); >> + >> + const struct engine_context *eng_ctx = engine_get_context(); >> + >> + stopwatch_start(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec()); >> + >> + build_meter_groups(&data->meter_groups, nb_meter_table); >> + >> + sync_meters(eng_ctx->ovnsb_idl_txn, nb_meter_table, acl_table, >> + sb_meter_table, &data->meter_groups); >> + >> + stopwatch_stop(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec()); >> + engine_set_node_state(node, EN_UPDATED); >> +} >> + >> +const struct nbrec_meter* >> +fair_meter_lookup_by_name(const struct shash *meter_groups, >> + const char *meter_name) >> +{ >> + const struct nbrec_meter *nb_meter = >> + meter_name ? shash_find_data(meter_groups, meter_name) : NULL; >> + if (nb_meter) { >> + return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; >> + } >> + return NULL; >> +} >> + >> +struct band_entry { >> + int64_t rate; >> + int64_t burst_size; >> + const char *action; >> +}; >> + >> +static int >> +band_cmp(const void *band1_, const void *band2_) >> +{ >> + const struct band_entry *band1p = band1_; >> + const struct band_entry *band2p = band2_; >> + >> + if (band1p->rate != band2p->rate) { >> + return band1p->rate - band2p->rate; >> + } else if (band1p->burst_size != band2p->burst_size) { >> + return band1p->burst_size - band2p->burst_size; >> + } else { >> + return strcmp(band1p->action, band2p->action); >> + } >> +} >> + >> +static bool >> +bands_need_update(const struct nbrec_meter *nb_meter, >> + const struct sbrec_meter *sb_meter) >> +{ >> + if (nb_meter->n_bands != sb_meter->n_bands) { >> + return true; >> + } >> + >> + /* Place the Northbound entries in sorted order. */ >> + struct band_entry *nb_bands; >> + nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands); >> + for (size_t i = 0; i < nb_meter->n_bands; i++) { >> + struct nbrec_meter_band *nb_band = nb_meter->bands[i]; >> + >> + nb_bands[i].rate = nb_band->rate; >> + nb_bands[i].burst_size = nb_band->burst_size; >> + nb_bands[i].action = nb_band->action; >> + } >> + qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp); >> + >> + /* Place the Southbound entries in sorted order. */ >> + struct band_entry *sb_bands; >> + sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands); >> + for (size_t i = 0; i < sb_meter->n_bands; i++) { >> + struct sbrec_meter_band *sb_band = sb_meter->bands[i]; >> + >> + sb_bands[i].rate = sb_band->rate; >> + sb_bands[i].burst_size = sb_band->burst_size; >> + sb_bands[i].action = sb_band->action; >> + } >> + qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp); >> + >> + bool need_update = false; >> + for (size_t i = 0; i < nb_meter->n_bands; i++) { >> + if (band_cmp(&nb_bands[i], &sb_bands[i])) { >> + need_update = true; >> + break; >> + } >> + } >> + >> + free(nb_bands); >> + free(sb_bands); >> + >> + return need_update; >> +} >> + >> +static void >> +sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn, >> + const char *meter_name, >> + const struct nbrec_meter *nb_meter, >> + struct shash *sb_meters, >> + struct sset *used_sb_meters) >> +{ >> + const struct sbrec_meter *sb_meter; >> + bool new_sb_meter = false; >> + >> + sb_meter = shash_find_data(sb_meters, meter_name); >> + if (!sb_meter) { >> + sb_meter = sbrec_meter_insert(ovnsb_txn); >> + sbrec_meter_set_name(sb_meter, meter_name); >> + shash_add(sb_meters, sb_meter->name, sb_meter); >> + new_sb_meter = true; >> + } >> + sset_add(used_sb_meters, meter_name); >> + >> + if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { >> + struct sbrec_meter_band **sb_bands; >> + sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); >> + for (size_t i = 0; i < nb_meter->n_bands; i++) { >> + const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; >> + >> + sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn); >> + >> + sbrec_meter_band_set_action(sb_bands[i], nb_band->action); >> + sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); >> + sbrec_meter_band_set_burst_size(sb_bands[i], >> + nb_band->burst_size); >> + } >> + sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); >> + free(sb_bands); >> + } >> + >> + sbrec_meter_set_unit(sb_meter, nb_meter->unit); >> +} >> + >> +static void >> +sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, >> + struct shash *meter_groups, >> + const struct nbrec_acl *acl, struct shash >> *sb_meters, >> + struct sset *used_sb_meters) >> +{ >> + const struct nbrec_meter *nb_meter = >> + fair_meter_lookup_by_name(meter_groups, acl->meter); >> + >> + if (!nb_meter) { >> + return; >> + } >> + >> + char *meter_name = alloc_acl_log_unique_meter_name(acl); >> + sync_meters_iterate_nb_meter(ovnsb_txn, meter_name, nb_meter, >> sb_meters, >> + used_sb_meters); >> + free(meter_name); >> +} >> + >> +static void >> +build_meter_groups(struct shash *meter_groups, >> + const struct nbrec_meter_table *nb_meter_table) >> +{ >> + const struct nbrec_meter *nb_meter; >> + >> + shash_clear(meter_groups); >> + NBREC_METER_TABLE_FOR_EACH (nb_meter, nb_meter_table) { >> + shash_add(meter_groups, nb_meter->name, nb_meter); >> + } >> +} >> + >> +/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have >> + * a corresponding entries in the Meter and Meter_Band tables in >> + * OVN_Southbound. Additionally, ACL logs that use fair meters have >> + * a private copy of its meter in the SB table. >> + */ >> +static void >> +sync_meters(struct ovsdb_idl_txn *ovnsb_txn, >> + const struct nbrec_meter_table *nbrec_meter_table, >> + const struct nbrec_acl_table *nbrec_acl_table, >> + const struct sbrec_meter_table *sbrec_meter_table, >> + struct shash *meter_groups) >> +{ >> + struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); >> + struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters); >> + >> + const struct sbrec_meter *sb_meter; >> + SBREC_METER_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) { >> + shash_add(&sb_meters, sb_meter->name, sb_meter); >> + } >> + >> + const struct nbrec_meter *nb_meter; >> + NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { >> + sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name, >> nb_meter, >> + &sb_meters, &used_sb_meters); >> + } >> + >> + /* >> + * In addition to creating Meters in the SB from the block above, >> check >> + * and see if additional rows are needed to get ACLs logs >> individually >> + * rate-limited. >> + */ >> + const struct nbrec_acl *acl; >> + NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) { >> + sync_acl_fair_meter(ovnsb_txn, meter_groups, acl, >> + &sb_meters, &used_sb_meters); >> + } >> + >> + const char *used_meter; >> + SSET_FOR_EACH_SAFE (used_meter, &used_sb_meters) { >> + shash_find_and_delete(&sb_meters, used_meter); >> + sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter)); >> + } >> + sset_destroy(&used_sb_meters); >> + >> + struct shash_node *node; >> + SHASH_FOR_EACH_SAFE (node, &sb_meters) { >> + sbrec_meter_delete(node->data); >> + shash_delete(&sb_meters, node); >> + } >> + shash_destroy(&sb_meters); >> +} >> diff --git a/northd/en-meters.h b/northd/en-meters.h >> new file mode 100644 >> index 0000000000..a1743282e3 >> --- /dev/null >> +++ b/northd/en-meters.h >> @@ -0,0 +1,44 @@ >> +/* >> + * Copyright (c) 2023, Red Hat, Inc. >> + * >> + * Licensed under the Apache License, Version 2.0 (the "License"); >> + * you may not use this file except in compliance with the License. >> + * You may obtain a copy of the License at: >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, software >> + * distributed under the License is distributed on an "AS IS" BASIS, >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >> implied. >> + * See the License for the specific language governing permissions and >> + * limitations under the License. >> + */ >> +#ifndef EN_METERS_H >> +#define EN_METERS_H 1 >> + >> +#include "openvswitch/shash.h" >> + >> +#include "lib/inc-proc-eng.h" >> +#include "lib/ovn-nb-idl.h" >> +#include "lib/ovn-sb-idl.h" >> + >> +struct sync_meters_data { >> + struct shash meter_groups; >> +}; >> + >> +void *en_sync_meters_init(struct engine_node *, struct engine_arg *); >> +void en_sync_meters_cleanup(void *data); >> +void en_sync_meters_run(struct engine_node *, void *data); >> + >> +const struct nbrec_meter *fair_meter_lookup_by_name( >> + const struct shash *meter_groups, >> + const char *meter_name); >> + >> +static inline char* >> +alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) >> +{ >> + return xasprintf("%s__" UUID_FMT, >> + acl->meter, UUID_ARGS(&acl->header_.uuid)); >> +} >> + >> +#endif /* EN_ACL_H */ >> diff --git a/northd/en-northd.c b/northd/en-northd.c >> index 044fa70190..cc05efdbbc 100644 >> --- a/northd/en-northd.c >> +++ b/northd/en-northd.c >> @@ -76,10 +76,6 @@ northd_get_input_data(struct engine_node *node, >> 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 = >> - EN_OVSDB_GET(engine_get_input("NB_acl", node)); >> input_data->nbrec_static_mac_binding_table = >> EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node)); >> input_data->nbrec_chassis_template_var_table = >> @@ -107,8 +103,6 @@ northd_get_input_data(struct engine_node *node, >> 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 = >> EN_OVSDB_GET(engine_get_input("SB_dns", node)); >> input_data->sbrec_ip_multicast_table = >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >> index d328deb222..1de0551b3d 100644 >> --- a/northd/inc-proc-northd.c >> +++ b/northd/inc-proc-northd.c >> @@ -33,6 +33,7 @@ >> #include "en-northd.h" >> #include "en-lflow.h" >> #include "en-northd-output.h" >> +#include "en-meters.h" >> #include "en-sync-sb.h" >> #include "en-sync-from-sb.h" >> #include "unixctl.h" >> @@ -135,6 +136,7 @@ static ENGINE_NODE(lflow, "lflow"); >> static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); >> static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); >> static ENGINE_NODE(northd_output, "northd_output"); >> +static ENGINE_NODE(sync_meters, "sync_meters"); >> 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(fdb_aging, "fdb_aging"); >> @@ -148,10 +150,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> 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); >> engine_add_input(&en_northd, &en_nb_logical_router, NULL); >> engine_add_input(&en_northd, &en_nb_mirror, NULL); >> - engine_add_input(&en_northd, &en_nb_meter, NULL); >> engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL); >> engine_add_input(&en_northd, &en_nb_chassis_template_var, NULL); >> @@ -188,7 +188,13 @@ void inc_proc_northd_init(struct ovsdb_idl_loop >> *nb, >> engine_add_input(&en_fdb_aging, &en_northd, NULL); >> engine_add_input(&en_fdb_aging, &en_fdb_aging_waker, NULL); >> + engine_add_input(&en_sync_meters, &en_nb_acl, NULL); >> + engine_add_input(&en_sync_meters, &en_nb_meter, NULL); >> + engine_add_input(&en_sync_meters, &en_sb_meter, NULL); >> + >> engine_add_input(&en_lflow, &en_nb_bfd, NULL); >> + engine_add_input(&en_lflow, &en_nb_acl, NULL); >> + engine_add_input(&en_lflow, &en_sync_meters, NULL); >> engine_add_input(&en_lflow, &en_sb_bfd, NULL); >> engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); >> engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); >> @@ -204,9 +210,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> /* 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 table and >> + * SB Meter/Meter_Band tables. >> */ >> engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL); >> + engine_add_input(&en_sync_to_sb, &en_sync_meters, 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 13f2ae0565..48fb44a8be 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -21,6 +21,7 @@ >> #include "bitmap.h" >> #include "coverage.h" >> #include "dirs.h" >> +#include "en-meters.h" >> #include "ipam.h" >> #include "openvswitch/dynamic-string.h" >> #include "hash.h" >> @@ -4980,25 +4981,22 @@ ls_port_create(struct ovsdb_idl_txn >> *ovnsb_txn, struct hmap *ls_ports, >> } >> static bool >> -check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls) >> +check_ls_changes_other_than_lsp_acl(const struct nbrec_logical_switch >> *ls) >> { >> /* Check if the columns are changed in this row. */ >> enum nbrec_logical_switch_column_id col; >> for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) { >> - if (nbrec_logical_switch_is_updated(ls, col) && >> - col != NBREC_LOGICAL_SWITCH_COL_PORTS) { >> + if (nbrec_logical_switch_is_updated(ls, col)) { >> + if (col == NBREC_LOGICAL_SWITCH_COL_ACLS || >> + col == NBREC_LOGICAL_SWITCH_COL_PORTS) { >> + continue; >> + } >> return true; >> } >> } >> /* Check if the referenced rows are changed. >> XXX: Need a better OVSDB IDL interface for this check. */ >> - for (size_t i = 0; i < ls->n_acls; i++) { >> - if (nbrec_acl_row_get_seqno(ls->acls[i], >> - OVSDB_IDL_CHANGE_MODIFY) > 0) { >> - return true; >> - } >> - } >> if (ls->copp && nbrec_copp_row_get_seqno(ls->copp, >> OVSDB_IDL_CHANGE_MODIFY) > 0) { >> return true; >> @@ -5106,7 +5104,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >> } >> /* Now only able to handle lsp changes. */ >> - if (check_ls_changes_other_than_lsp(changed_ls)) { >> + if (check_ls_changes_other_than_lsp_acl(changed_ls)) { >> goto fail; >> } >> @@ -6987,25 +6985,6 @@ build_acl_hints(struct ovn_datapath *od, >> } >> } >> -static const struct nbrec_meter* >> -fair_meter_lookup_by_name(const struct shash *meter_groups, >> - const char *meter_name) >> -{ >> - const struct nbrec_meter *nb_meter = >> - meter_name ? shash_find_data(meter_groups, meter_name) : NULL; >> - if (nb_meter) { >> - return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; >> - } >> - return NULL; >> -} >> - >> -static char* >> -alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) >> -{ >> - return xasprintf("%s__" UUID_FMT, >> - acl->meter, UUID_ARGS(&acl->header_.uuid)); >> -} >> - >> static void >> build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, >> const struct shash *meter_groups) >> @@ -16608,183 +16587,6 @@ sync_port_groups(struct ovsdb_idl_txn >> *ovnsb_txn, >> shash_destroy(&sb_port_groups); >> } >> -struct band_entry { >> - int64_t rate; >> - int64_t burst_size; >> - const char *action; >> -}; >> - >> -static int >> -band_cmp(const void *band1_, const void *band2_) >> -{ >> - const struct band_entry *band1p = band1_; >> - const struct band_entry *band2p = band2_; >> - >> - if (band1p->rate != band2p->rate) { >> - return band1p->rate - band2p->rate; >> - } else if (band1p->burst_size != band2p->burst_size) { >> - return band1p->burst_size - band2p->burst_size; >> - } else { >> - return strcmp(band1p->action, band2p->action); >> - } >> -} >> - >> -static bool >> -bands_need_update(const struct nbrec_meter *nb_meter, >> - const struct sbrec_meter *sb_meter) >> -{ >> - if (nb_meter->n_bands != sb_meter->n_bands) { >> - return true; >> - } >> - >> - /* Place the Northbound entries in sorted order. */ >> - struct band_entry *nb_bands; >> - nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands); >> - for (size_t i = 0; i < nb_meter->n_bands; i++) { >> - struct nbrec_meter_band *nb_band = nb_meter->bands[i]; >> - >> - nb_bands[i].rate = nb_band->rate; >> - nb_bands[i].burst_size = nb_band->burst_size; >> - nb_bands[i].action = nb_band->action; >> - } >> - qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp); >> - >> - /* Place the Southbound entries in sorted order. */ >> - struct band_entry *sb_bands; >> - sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands); >> - for (size_t i = 0; i < sb_meter->n_bands; i++) { >> - struct sbrec_meter_band *sb_band = sb_meter->bands[i]; >> - >> - sb_bands[i].rate = sb_band->rate; >> - sb_bands[i].burst_size = sb_band->burst_size; >> - sb_bands[i].action = sb_band->action; >> - } >> - qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp); >> - >> - bool need_update = false; >> - for (size_t i = 0; i < nb_meter->n_bands; i++) { >> - if (band_cmp(&nb_bands[i], &sb_bands[i])) { >> - need_update = true; >> - break; >> - } >> - } >> - >> - free(nb_bands); >> - free(sb_bands); >> - >> - return need_update; >> -} >> - >> -static void >> -sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn, >> - const char *meter_name, >> - const struct nbrec_meter *nb_meter, >> - struct shash *sb_meters, >> - struct sset *used_sb_meters) >> -{ >> - const struct sbrec_meter *sb_meter; >> - bool new_sb_meter = false; >> - >> - sb_meter = shash_find_data(sb_meters, meter_name); >> - if (!sb_meter) { >> - sb_meter = sbrec_meter_insert(ovnsb_txn); >> - sbrec_meter_set_name(sb_meter, meter_name); >> - shash_add(sb_meters, sb_meter->name, sb_meter); >> - new_sb_meter = true; >> - } >> - sset_add(used_sb_meters, meter_name); >> - >> - if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { >> - struct sbrec_meter_band **sb_bands; >> - sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); >> - for (size_t i = 0; i < nb_meter->n_bands; i++) { >> - const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; >> - >> - sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn); >> - >> - sbrec_meter_band_set_action(sb_bands[i], nb_band->action); >> - sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); >> - sbrec_meter_band_set_burst_size(sb_bands[i], >> - nb_band->burst_size); >> - } >> - sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); >> - free(sb_bands); >> - } >> - >> - sbrec_meter_set_unit(sb_meter, nb_meter->unit); >> -} >> - >> -static void >> -sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, >> - struct shash *meter_groups, >> - const struct nbrec_acl *acl, struct shash >> *sb_meters, >> - struct sset *used_sb_meters) >> -{ >> - const struct nbrec_meter *nb_meter = >> - fair_meter_lookup_by_name(meter_groups, acl->meter); >> - >> - if (!nb_meter) { >> - return; >> - } >> - >> - char *meter_name = alloc_acl_log_unique_meter_name(acl); >> - sync_meters_iterate_nb_meter(ovnsb_txn, meter_name, nb_meter, >> sb_meters, >> - used_sb_meters); >> - free(meter_name); >> -} >> - >> -/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have >> - * a corresponding entries in the Meter and Meter_Band tables in >> - * OVN_Southbound. Additionally, ACL logs that use fair meters have >> - * a private copy of its meter in the SB table. >> - */ >> -static void >> -sync_meters(struct ovsdb_idl_txn *ovnsb_txn, >> - const struct nbrec_meter_table *nbrec_meter_table, >> - const struct nbrec_acl_table *nbrec_acl_table, >> - const struct sbrec_meter_table *sbrec_meter_table, >> - struct shash *meter_groups) >> -{ >> - struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); >> - struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters); >> - >> - const struct sbrec_meter *sb_meter; >> - SBREC_METER_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) { >> - shash_add(&sb_meters, sb_meter->name, sb_meter); >> - } >> - >> - const struct nbrec_meter *nb_meter; >> - NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { >> - sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name, >> nb_meter, >> - &sb_meters, &used_sb_meters); >> - } >> - >> - /* >> - * In addition to creating Meters in the SB from the block above, >> check >> - * and see if additional rows are needed to get ACLs logs >> individually >> - * rate-limited. >> - */ >> - const struct nbrec_acl *acl; >> - NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) { >> - sync_acl_fair_meter(ovnsb_txn, meter_groups, acl, >> - &sb_meters, &used_sb_meters); >> - } >> - >> - const char *used_meter; >> - SSET_FOR_EACH_SAFE (used_meter, &used_sb_meters) { >> - shash_find_and_delete(&sb_meters, used_meter); >> - sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter)); >> - } >> - sset_destroy(&used_sb_meters); >> - >> - struct shash_node *node; >> - SHASH_FOR_EACH_SAFE (node, &sb_meters) { >> - sbrec_meter_delete(node->data); >> - shash_delete(&sb_meters, node); >> - } >> - shash_destroy(&sb_meters); >> -} >> - >> static bool >> mirror_needs_update(const struct nbrec_mirror *nb_mirror, >> const struct sbrec_mirror *sb_mirror) >> @@ -17254,16 +17056,6 @@ build_mcast_groups(const struct >> sbrec_igmp_group_table *sbrec_igmp_group_table, >> } >> } >> -static void >> -build_meter_groups(const struct nbrec_meter_table *nbrec_meter_table, >> - struct shash *meter_groups) >> -{ >> - const struct nbrec_meter *nb_meter; >> - NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { >> - shash_add(meter_groups, nb_meter->name, nb_meter); >> - } >> -} >> - >> static const struct nbrec_static_mac_binding * >> static_mac_binding_by_port_ip( >> const struct nbrec_static_mac_binding_table *nbrec_static_mb_table, >> @@ -17405,7 +17197,6 @@ northd_init(struct northd_data *data) >> hmap_init(&data->ls_ports); >> hmap_init(&data->lr_ports); >> hmap_init(&data->port_groups); >> - shash_init(&data->meter_groups); >> hmap_init(&data->lbs); >> hmap_init(&data->lb_groups); >> ovs_list_init(&data->lr_list); >> @@ -17443,12 +17234,6 @@ northd_destroy(struct northd_data *data) >> hmap_destroy(&data->port_groups); >> - struct shash_node *node; >> - SHASH_FOR_EACH_SAFE (node, &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 >> @@ -17587,7 +17372,6 @@ ovnnb_db_run(struct northd_input *input_data, >> build_ip_mcast(ovnsb_txn, input_data->sbrec_ip_multicast_table, >> input_data->sbrec_ip_mcast_by_dp, >> &data->ls_datapaths.datapaths); >> - build_meter_groups(input_data->nbrec_meter_table, >> &data->meter_groups); >> build_static_mac_binding_table(ovnsb_txn, >> input_data->nbrec_static_mac_binding_table, >> input_data->sbrec_static_mac_binding_table, >> @@ -17602,9 +17386,6 @@ ovnnb_db_run(struct northd_input *input_data, >> &data->ls_datapaths, &data->lbs); >> sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table, >> &data->port_groups); >> - sync_meters(ovnsb_txn, input_data->nbrec_meter_table, >> - input_data->nbrec_acl_table, >> input_data->sbrec_meter_table, >> - &data->meter_groups); >> sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, >> input_data->sbrec_mirror_table); >> sync_dns_entries(ovnsb_txn, input_data->sbrec_dns_table, >> diff --git a/northd/northd.h b/northd/northd.h >> index f3e63b1e1a..dcc3acabd5 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -31,8 +31,6 @@ struct northd_input { >> 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 >> *nbrec_static_mac_binding_table; >> const struct nbrec_chassis_template_var_table >> @@ -50,7 +48,6 @@ struct northd_input { >> 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; >> const struct sbrec_static_mac_binding_table >> @@ -109,7 +106,6 @@ struct northd_data { >> struct hmap ls_ports; >> struct hmap lr_ports; >> struct hmap port_groups; >> - struct shash meter_groups; >> struct hmap lbs; >> struct hmap lb_groups; >> struct ovs_list lr_list; >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index 4fa1b039ea..45362f4f93 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -836,6 +836,9 @@ main(int argc, char *argv[]) >> ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, >> &sbrec_multicast_group_columns[i]); >> } >> + for (size_t i = 0; i < SBREC_METER_N_COLUMNS; i++) { >> + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, >> &sbrec_meter_columns[i]); >> + } >> unixctl_command_register("sb-connection-status", "", 0, 0, >> ovn_conn_show, ovnsb_idl_loop.idl); >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index d5be3be75b..dc8bf253f1 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -9,6 +9,8 @@ m4_define([_DUMP_DB_TABLES], [ >> ovn-sbctl list logical_flow >> $1 >> ovn-sbctl list port_binding >> $1 >> ovn-sbctl list address_set >> $1 >> + ovn-sbctl list meter >> $1 >> + ovn-sbctl list meter_band >> $1 >> ]) >> # CHECK_NO_CHANGE_AFTER_RECOMPUTE >> @@ -9679,6 +9681,40 @@ OVN_CLEANUP([hv1]) >> AT_CLEANUP >> ]) >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> +AT_SETUP([ACL/Meter incremental processing - no northd recompute]) >> +ovn_start >> + >> +check_recompute_counter() { >> + northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE >> inc-engine/show-stats northd recompute) >> + AT_CHECK([test x$northd_recomp = x$1]) >> + >> + northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE >> inc-engine/show-stats northd recompute) >> + AT_CHECK([test x$northd_recomp = x$1]) >> + >> + sync_meters_recomp=$(as northd ovn-appctl -t NORTHD_TYPE >> inc-engine/show-stats sync_meters recompute) >> + AT_CHECK([test x$sync_meters_recomp = x$3]) >> +} > > In check_recompute_counter(), the same comparison is performed twice for > the $1 parameter. Then the $3 parameter is used in the third comparison. > The $2 parameter is never used. I assume $2 was supposed to be used for > something, but it's not clear what. > Oops, it was supposed to ensure we don't recompute the lflow node. Does the following incremental change look OK to you? If so, I can fold it in and apply the series. Thanks, Dumitru diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index dc8bf253f1..9df02b422f 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -9689,8 +9689,8 @@ check_recompute_counter() { northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) AT_CHECK([test x$northd_recomp = x$1]) - northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) - AT_CHECK([test x$northd_recomp = x$1]) + lflow_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute) + AT_CHECK([test x$lflow_recomp = x$2]) sync_meters_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_meters recompute) AT_CHECK([test x$sync_meters_recomp = x$3]) --- >> + >> +check ovn-nbctl --wait=sb ls-add ls >> + >> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats >> +check ovn-nbctl --wait=sb meter-add m drop 1 pktps >> +check ovn-nbctl --wait=sb acl-add ls from-lport 1 1 allow >> +dnl Only triggers recompute of the sync_meters and lflow nodes. >> +check_recompute_counter 0 2 2 >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats >> +check ovn-nbctl --wait=sb meter-del m >> +check ovn-nbctl --wait=sb acl-del ls >> +dnl Only triggers recompute of the sync_meters and lflow nodes. >> +check_recompute_counter 0 2 2 >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +AT_CLEANUP >> +]) >> + >> OVN_FOR_EACH_NORTHD_NO_HV([ >> AT_SETUP([check fip and lb flows]) >> AT_KEYWORDS([fip-lb-flows]) >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
On 8/29/23 04:49, Dumitru Ceara wrote: > On 8/21/23 19:33, Mark Michelson wrote: >> Hi Dumitru, >> >> I have one finding in the test code, but other than that it looks good. >> > > Hi Mark, > > Thanks for the review! > >> On 8/17/23 11:25, Dumitru Ceara wrote: >>> This is beneficial in a few ways: >>> - first, it reduces the number of different types of data the northd >>> I-P node has to process. >>> - it turns out the northd I-P node (whose recompute is rather costly) >>> doesn't really depend on meters or ACLs. >>> - prepares the ground for a pure I-P implementation for ACLs, with a >>> simple/clear dependency between NB.ACL and the lflow I-P node. >>> >>> From a meters synchronization perspective this commit doesn't change any >>> of the existing behavior and logic. It mostly just moves the meters >>> related code out of northd.c and into en-meters.c. >>> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> --- >>> lib/stopwatch-names.h | 1 >>> northd/automake.mk | 2 >>> northd/en-lflow.c | 5 + >>> northd/en-meters.c | 281 >>> ++++++++++++++++++++++++++++++++++++++++++++++ >>> northd/en-meters.h | 44 +++++++ >>> northd/en-northd.c | 6 - >>> northd/inc-proc-northd.c | 14 ++ >>> northd/northd.c | 235 +------------------------------------- >>> northd/northd.h | 4 - >>> northd/ovn-northd.c | 3 >>> tests/ovn-northd.at | 36 ++++++ >>> 11 files changed, 390 insertions(+), 241 deletions(-) >>> create mode 100644 northd/en-meters.c >>> create mode 100644 northd/en-meters.h >>> >>> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h >>> index de6fca4ccc..98535fff5a 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 SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run" >>> #endif >>> diff --git a/northd/automake.mk b/northd/automake.mk >>> index b17f1fdb54..f52766de0c 100644 >>> --- a/northd/automake.mk >>> +++ b/northd/automake.mk >>> @@ -12,6 +12,8 @@ northd_ovn_northd_SOURCES = \ >>> northd/en-northd.h \ >>> northd/en-lflow.c \ >>> northd/en-lflow.h \ >>> + northd/en-meters.c \ >>> + northd/en-meters.h \ >>> northd/en-northd-output.c \ >>> northd/en-northd-output.h \ >>> northd/en-sync-sb.c \ >>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >>> index 28ab1c67fb..0886d4c5ca 100644 >>> --- a/northd/en-lflow.c >>> +++ b/northd/en-lflow.c >>> @@ -20,6 +20,7 @@ >>> #include "en-lflow.h" >>> #include "en-northd.h" >>> +#include "en-meters.h" >>> #include "lib/inc-proc-eng.h" >>> #include "northd.h" >>> @@ -35,6 +36,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 sync_meters_data *sync_meters_data = >>> + engine_get_input_data("sync_meters", node); >>> lflow_input->nbrec_bfd_table = >>> EN_OVSDB_GET(engine_get_input("NB_bfd", node)); >>> lflow_input->sbrec_bfd_table = >>> @@ -56,7 +59,7 @@ lflow_get_input_data(struct engine_node *node, >>> lflow_input->ls_ports = &northd_data->ls_ports; >>> lflow_input->lr_ports = &northd_data->lr_ports; >>> lflow_input->port_groups = &northd_data->port_groups; >>> - lflow_input->meter_groups = &northd_data->meter_groups; >>> + lflow_input->meter_groups = &sync_meters_data->meter_groups; >>> lflow_input->lbs = &northd_data->lbs; >>> lflow_input->features = &northd_data->features; >>> lflow_input->ovn_internal_version_changed = >>> diff --git a/northd/en-meters.c b/northd/en-meters.c >>> new file mode 100644 >>> index 0000000000..aabd002b62 >>> --- /dev/null >>> +++ b/northd/en-meters.c >>> @@ -0,0 +1,281 @@ >>> +/* >>> + * Copyright (c) 2023, Red Hat, Inc. >>> + * >>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>> + * you may not use this file except in compliance with the License. >>> + * You may obtain a copy of the License at: >>> + * >>> + * http://www.apache.org/licenses/LICENSE-2.0 >>> + * >>> + * Unless required by applicable law or agreed to in writing, software >>> + * distributed under the License is distributed on an "AS IS" BASIS, >>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>> implied. >>> + * See the License for the specific language governing permissions and >>> + * limitations under the License. >>> + */ >>> + >>> +#include <config.h> >>> + >>> +#include "openvswitch/vlog.h" >>> +#include "stopwatch.h" >>> + >>> +#include "en-meters.h" >>> +#include "lib/stopwatch-names.h" >>> + >>> +VLOG_DEFINE_THIS_MODULE(en_meters); >>> + >>> +static void build_meter_groups(struct shash *meter_group, >>> + const struct nbrec_meter_table *); >>> +static void sync_meters(struct ovsdb_idl_txn *ovnsb_txn, >>> + const struct nbrec_meter_table *, >>> + const struct nbrec_acl_table *, >>> + const struct sbrec_meter_table *, >>> + struct shash *meter_groups); >>> + >>> +void >>> +*en_sync_meters_init(struct engine_node *node OVS_UNUSED, >>> + struct engine_arg *arg OVS_UNUSED) >>> +{ >>> + struct sync_meters_data *data = xmalloc(sizeof *data); >>> + >>> + *data = (struct sync_meters_data) { >>> + .meter_groups = SHASH_INITIALIZER(&data->meter_groups), >>> + }; >>> + return data; >>> +} >>> + >>> +void >>> +en_sync_meters_cleanup(void *data_) >>> +{ >>> + struct sync_meters_data *data = data_; >>> + >>> + shash_destroy(&data->meter_groups); >>> +} >>> + >>> +void >>> +en_sync_meters_run(struct engine_node *node, void *data_) >>> +{ >>> + struct sync_meters_data *data = data_; >>> + >>> + const struct nbrec_acl_table *acl_table = >>> + EN_OVSDB_GET(engine_get_input("NB_acl", node)); >>> + >>> + const struct nbrec_meter_table *nb_meter_table = >>> + EN_OVSDB_GET(engine_get_input("NB_meter", node)); >>> + >>> + const struct sbrec_meter_table *sb_meter_table = >>> + EN_OVSDB_GET(engine_get_input("SB_meter", node)); >>> + >>> + const struct engine_context *eng_ctx = engine_get_context(); >>> + >>> + stopwatch_start(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec()); >>> + >>> + build_meter_groups(&data->meter_groups, nb_meter_table); >>> + >>> + sync_meters(eng_ctx->ovnsb_idl_txn, nb_meter_table, acl_table, >>> + sb_meter_table, &data->meter_groups); >>> + >>> + stopwatch_stop(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec()); >>> + engine_set_node_state(node, EN_UPDATED); >>> +} >>> + >>> +const struct nbrec_meter* >>> +fair_meter_lookup_by_name(const struct shash *meter_groups, >>> + const char *meter_name) >>> +{ >>> + const struct nbrec_meter *nb_meter = >>> + meter_name ? shash_find_data(meter_groups, meter_name) : NULL; >>> + if (nb_meter) { >>> + return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; >>> + } >>> + return NULL; >>> +} >>> + >>> +struct band_entry { >>> + int64_t rate; >>> + int64_t burst_size; >>> + const char *action; >>> +}; >>> + >>> +static int >>> +band_cmp(const void *band1_, const void *band2_) >>> +{ >>> + const struct band_entry *band1p = band1_; >>> + const struct band_entry *band2p = band2_; >>> + >>> + if (band1p->rate != band2p->rate) { >>> + return band1p->rate - band2p->rate; >>> + } else if (band1p->burst_size != band2p->burst_size) { >>> + return band1p->burst_size - band2p->burst_size; >>> + } else { >>> + return strcmp(band1p->action, band2p->action); >>> + } >>> +} >>> + >>> +static bool >>> +bands_need_update(const struct nbrec_meter *nb_meter, >>> + const struct sbrec_meter *sb_meter) >>> +{ >>> + if (nb_meter->n_bands != sb_meter->n_bands) { >>> + return true; >>> + } >>> + >>> + /* Place the Northbound entries in sorted order. */ >>> + struct band_entry *nb_bands; >>> + nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands); >>> + for (size_t i = 0; i < nb_meter->n_bands; i++) { >>> + struct nbrec_meter_band *nb_band = nb_meter->bands[i]; >>> + >>> + nb_bands[i].rate = nb_band->rate; >>> + nb_bands[i].burst_size = nb_band->burst_size; >>> + nb_bands[i].action = nb_band->action; >>> + } >>> + qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp); >>> + >>> + /* Place the Southbound entries in sorted order. */ >>> + struct band_entry *sb_bands; >>> + sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands); >>> + for (size_t i = 0; i < sb_meter->n_bands; i++) { >>> + struct sbrec_meter_band *sb_band = sb_meter->bands[i]; >>> + >>> + sb_bands[i].rate = sb_band->rate; >>> + sb_bands[i].burst_size = sb_band->burst_size; >>> + sb_bands[i].action = sb_band->action; >>> + } >>> + qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp); >>> + >>> + bool need_update = false; >>> + for (size_t i = 0; i < nb_meter->n_bands; i++) { >>> + if (band_cmp(&nb_bands[i], &sb_bands[i])) { >>> + need_update = true; >>> + break; >>> + } >>> + } >>> + >>> + free(nb_bands); >>> + free(sb_bands); >>> + >>> + return need_update; >>> +} >>> + >>> +static void >>> +sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn, >>> + const char *meter_name, >>> + const struct nbrec_meter *nb_meter, >>> + struct shash *sb_meters, >>> + struct sset *used_sb_meters) >>> +{ >>> + const struct sbrec_meter *sb_meter; >>> + bool new_sb_meter = false; >>> + >>> + sb_meter = shash_find_data(sb_meters, meter_name); >>> + if (!sb_meter) { >>> + sb_meter = sbrec_meter_insert(ovnsb_txn); >>> + sbrec_meter_set_name(sb_meter, meter_name); >>> + shash_add(sb_meters, sb_meter->name, sb_meter); >>> + new_sb_meter = true; >>> + } >>> + sset_add(used_sb_meters, meter_name); >>> + >>> + if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { >>> + struct sbrec_meter_band **sb_bands; >>> + sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); >>> + for (size_t i = 0; i < nb_meter->n_bands; i++) { >>> + const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; >>> + >>> + sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn); >>> + >>> + sbrec_meter_band_set_action(sb_bands[i], nb_band->action); >>> + sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); >>> + sbrec_meter_band_set_burst_size(sb_bands[i], >>> + nb_band->burst_size); >>> + } >>> + sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); >>> + free(sb_bands); >>> + } >>> + >>> + sbrec_meter_set_unit(sb_meter, nb_meter->unit); >>> +} >>> + >>> +static void >>> +sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, >>> + struct shash *meter_groups, >>> + const struct nbrec_acl *acl, struct shash >>> *sb_meters, >>> + struct sset *used_sb_meters) >>> +{ >>> + const struct nbrec_meter *nb_meter = >>> + fair_meter_lookup_by_name(meter_groups, acl->meter); >>> + >>> + if (!nb_meter) { >>> + return; >>> + } >>> + >>> + char *meter_name = alloc_acl_log_unique_meter_name(acl); >>> + sync_meters_iterate_nb_meter(ovnsb_txn, meter_name, nb_meter, >>> sb_meters, >>> + used_sb_meters); >>> + free(meter_name); >>> +} >>> + >>> +static void >>> +build_meter_groups(struct shash *meter_groups, >>> + const struct nbrec_meter_table *nb_meter_table) >>> +{ >>> + const struct nbrec_meter *nb_meter; >>> + >>> + shash_clear(meter_groups); >>> + NBREC_METER_TABLE_FOR_EACH (nb_meter, nb_meter_table) { >>> + shash_add(meter_groups, nb_meter->name, nb_meter); >>> + } >>> +} >>> + >>> +/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have >>> + * a corresponding entries in the Meter and Meter_Band tables in >>> + * OVN_Southbound. Additionally, ACL logs that use fair meters have >>> + * a private copy of its meter in the SB table. >>> + */ >>> +static void >>> +sync_meters(struct ovsdb_idl_txn *ovnsb_txn, >>> + const struct nbrec_meter_table *nbrec_meter_table, >>> + const struct nbrec_acl_table *nbrec_acl_table, >>> + const struct sbrec_meter_table *sbrec_meter_table, >>> + struct shash *meter_groups) >>> +{ >>> + struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); >>> + struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters); >>> + >>> + const struct sbrec_meter *sb_meter; >>> + SBREC_METER_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) { >>> + shash_add(&sb_meters, sb_meter->name, sb_meter); >>> + } >>> + >>> + const struct nbrec_meter *nb_meter; >>> + NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { >>> + sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name, >>> nb_meter, >>> + &sb_meters, &used_sb_meters); >>> + } >>> + >>> + /* >>> + * In addition to creating Meters in the SB from the block above, >>> check >>> + * and see if additional rows are needed to get ACLs logs >>> individually >>> + * rate-limited. >>> + */ >>> + const struct nbrec_acl *acl; >>> + NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) { >>> + sync_acl_fair_meter(ovnsb_txn, meter_groups, acl, >>> + &sb_meters, &used_sb_meters); >>> + } >>> + >>> + const char *used_meter; >>> + SSET_FOR_EACH_SAFE (used_meter, &used_sb_meters) { >>> + shash_find_and_delete(&sb_meters, used_meter); >>> + sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter)); >>> + } >>> + sset_destroy(&used_sb_meters); >>> + >>> + struct shash_node *node; >>> + SHASH_FOR_EACH_SAFE (node, &sb_meters) { >>> + sbrec_meter_delete(node->data); >>> + shash_delete(&sb_meters, node); >>> + } >>> + shash_destroy(&sb_meters); >>> +} >>> diff --git a/northd/en-meters.h b/northd/en-meters.h >>> new file mode 100644 >>> index 0000000000..a1743282e3 >>> --- /dev/null >>> +++ b/northd/en-meters.h >>> @@ -0,0 +1,44 @@ >>> +/* >>> + * Copyright (c) 2023, Red Hat, Inc. >>> + * >>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>> + * you may not use this file except in compliance with the License. >>> + * You may obtain a copy of the License at: >>> + * >>> + * http://www.apache.org/licenses/LICENSE-2.0 >>> + * >>> + * Unless required by applicable law or agreed to in writing, software >>> + * distributed under the License is distributed on an "AS IS" BASIS, >>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>> implied. >>> + * See the License for the specific language governing permissions and >>> + * limitations under the License. >>> + */ >>> +#ifndef EN_METERS_H >>> +#define EN_METERS_H 1 >>> + >>> +#include "openvswitch/shash.h" >>> + >>> +#include "lib/inc-proc-eng.h" >>> +#include "lib/ovn-nb-idl.h" >>> +#include "lib/ovn-sb-idl.h" >>> + >>> +struct sync_meters_data { >>> + struct shash meter_groups; >>> +}; >>> + >>> +void *en_sync_meters_init(struct engine_node *, struct engine_arg *); >>> +void en_sync_meters_cleanup(void *data); >>> +void en_sync_meters_run(struct engine_node *, void *data); >>> + >>> +const struct nbrec_meter *fair_meter_lookup_by_name( >>> + const struct shash *meter_groups, >>> + const char *meter_name); >>> + >>> +static inline char* >>> +alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) >>> +{ >>> + return xasprintf("%s__" UUID_FMT, >>> + acl->meter, UUID_ARGS(&acl->header_.uuid)); >>> +} >>> + >>> +#endif /* EN_ACL_H */ >>> diff --git a/northd/en-northd.c b/northd/en-northd.c >>> index 044fa70190..cc05efdbbc 100644 >>> --- a/northd/en-northd.c >>> +++ b/northd/en-northd.c >>> @@ -76,10 +76,6 @@ northd_get_input_data(struct engine_node *node, >>> 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 = >>> - EN_OVSDB_GET(engine_get_input("NB_acl", node)); >>> input_data->nbrec_static_mac_binding_table = >>> EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node)); >>> input_data->nbrec_chassis_template_var_table = >>> @@ -107,8 +103,6 @@ northd_get_input_data(struct engine_node *node, >>> 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 = >>> EN_OVSDB_GET(engine_get_input("SB_dns", node)); >>> input_data->sbrec_ip_multicast_table = >>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >>> index d328deb222..1de0551b3d 100644 >>> --- a/northd/inc-proc-northd.c >>> +++ b/northd/inc-proc-northd.c >>> @@ -33,6 +33,7 @@ >>> #include "en-northd.h" >>> #include "en-lflow.h" >>> #include "en-northd-output.h" >>> +#include "en-meters.h" >>> #include "en-sync-sb.h" >>> #include "en-sync-from-sb.h" >>> #include "unixctl.h" >>> @@ -135,6 +136,7 @@ static ENGINE_NODE(lflow, "lflow"); >>> static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); >>> static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); >>> static ENGINE_NODE(northd_output, "northd_output"); >>> +static ENGINE_NODE(sync_meters, "sync_meters"); >>> 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(fdb_aging, "fdb_aging"); >>> @@ -148,10 +150,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >>> 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); >>> engine_add_input(&en_northd, &en_nb_logical_router, NULL); >>> engine_add_input(&en_northd, &en_nb_mirror, NULL); >>> - engine_add_input(&en_northd, &en_nb_meter, NULL); >>> engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL); >>> engine_add_input(&en_northd, &en_nb_chassis_template_var, NULL); >>> @@ -188,7 +188,13 @@ void inc_proc_northd_init(struct ovsdb_idl_loop >>> *nb, >>> engine_add_input(&en_fdb_aging, &en_northd, NULL); >>> engine_add_input(&en_fdb_aging, &en_fdb_aging_waker, NULL); >>> + engine_add_input(&en_sync_meters, &en_nb_acl, NULL); >>> + engine_add_input(&en_sync_meters, &en_nb_meter, NULL); >>> + engine_add_input(&en_sync_meters, &en_sb_meter, NULL); >>> + >>> engine_add_input(&en_lflow, &en_nb_bfd, NULL); >>> + engine_add_input(&en_lflow, &en_nb_acl, NULL); >>> + engine_add_input(&en_lflow, &en_sync_meters, NULL); >>> engine_add_input(&en_lflow, &en_sb_bfd, NULL); >>> engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); >>> engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); >>> @@ -204,9 +210,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >>> /* 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 table and >>> + * SB Meter/Meter_Band tables. >>> */ >>> engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL); >>> + engine_add_input(&en_sync_to_sb, &en_sync_meters, 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 13f2ae0565..48fb44a8be 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -21,6 +21,7 @@ >>> #include "bitmap.h" >>> #include "coverage.h" >>> #include "dirs.h" >>> +#include "en-meters.h" >>> #include "ipam.h" >>> #include "openvswitch/dynamic-string.h" >>> #include "hash.h" >>> @@ -4980,25 +4981,22 @@ ls_port_create(struct ovsdb_idl_txn >>> *ovnsb_txn, struct hmap *ls_ports, >>> } >>> static bool >>> -check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls) >>> +check_ls_changes_other_than_lsp_acl(const struct nbrec_logical_switch >>> *ls) >>> { >>> /* Check if the columns are changed in this row. */ >>> enum nbrec_logical_switch_column_id col; >>> for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) { >>> - if (nbrec_logical_switch_is_updated(ls, col) && >>> - col != NBREC_LOGICAL_SWITCH_COL_PORTS) { >>> + if (nbrec_logical_switch_is_updated(ls, col)) { >>> + if (col == NBREC_LOGICAL_SWITCH_COL_ACLS || >>> + col == NBREC_LOGICAL_SWITCH_COL_PORTS) { >>> + continue; >>> + } >>> return true; >>> } >>> } >>> /* Check if the referenced rows are changed. >>> XXX: Need a better OVSDB IDL interface for this check. */ >>> - for (size_t i = 0; i < ls->n_acls; i++) { >>> - if (nbrec_acl_row_get_seqno(ls->acls[i], >>> - OVSDB_IDL_CHANGE_MODIFY) > 0) { >>> - return true; >>> - } >>> - } >>> if (ls->copp && nbrec_copp_row_get_seqno(ls->copp, >>> OVSDB_IDL_CHANGE_MODIFY) > 0) { >>> return true; >>> @@ -5106,7 +5104,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn >>> *ovnsb_idl_txn, >>> } >>> /* Now only able to handle lsp changes. */ >>> - if (check_ls_changes_other_than_lsp(changed_ls)) { >>> + if (check_ls_changes_other_than_lsp_acl(changed_ls)) { >>> goto fail; >>> } >>> @@ -6987,25 +6985,6 @@ build_acl_hints(struct ovn_datapath *od, >>> } >>> } >>> -static const struct nbrec_meter* >>> -fair_meter_lookup_by_name(const struct shash *meter_groups, >>> - const char *meter_name) >>> -{ >>> - const struct nbrec_meter *nb_meter = >>> - meter_name ? shash_find_data(meter_groups, meter_name) : NULL; >>> - if (nb_meter) { >>> - return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; >>> - } >>> - return NULL; >>> -} >>> - >>> -static char* >>> -alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) >>> -{ >>> - return xasprintf("%s__" UUID_FMT, >>> - acl->meter, UUID_ARGS(&acl->header_.uuid)); >>> -} >>> - >>> static void >>> build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, >>> const struct shash *meter_groups) >>> @@ -16608,183 +16587,6 @@ sync_port_groups(struct ovsdb_idl_txn >>> *ovnsb_txn, >>> shash_destroy(&sb_port_groups); >>> } >>> -struct band_entry { >>> - int64_t rate; >>> - int64_t burst_size; >>> - const char *action; >>> -}; >>> - >>> -static int >>> -band_cmp(const void *band1_, const void *band2_) >>> -{ >>> - const struct band_entry *band1p = band1_; >>> - const struct band_entry *band2p = band2_; >>> - >>> - if (band1p->rate != band2p->rate) { >>> - return band1p->rate - band2p->rate; >>> - } else if (band1p->burst_size != band2p->burst_size) { >>> - return band1p->burst_size - band2p->burst_size; >>> - } else { >>> - return strcmp(band1p->action, band2p->action); >>> - } >>> -} >>> - >>> -static bool >>> -bands_need_update(const struct nbrec_meter *nb_meter, >>> - const struct sbrec_meter *sb_meter) >>> -{ >>> - if (nb_meter->n_bands != sb_meter->n_bands) { >>> - return true; >>> - } >>> - >>> - /* Place the Northbound entries in sorted order. */ >>> - struct band_entry *nb_bands; >>> - nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands); >>> - for (size_t i = 0; i < nb_meter->n_bands; i++) { >>> - struct nbrec_meter_band *nb_band = nb_meter->bands[i]; >>> - >>> - nb_bands[i].rate = nb_band->rate; >>> - nb_bands[i].burst_size = nb_band->burst_size; >>> - nb_bands[i].action = nb_band->action; >>> - } >>> - qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp); >>> - >>> - /* Place the Southbound entries in sorted order. */ >>> - struct band_entry *sb_bands; >>> - sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands); >>> - for (size_t i = 0; i < sb_meter->n_bands; i++) { >>> - struct sbrec_meter_band *sb_band = sb_meter->bands[i]; >>> - >>> - sb_bands[i].rate = sb_band->rate; >>> - sb_bands[i].burst_size = sb_band->burst_size; >>> - sb_bands[i].action = sb_band->action; >>> - } >>> - qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp); >>> - >>> - bool need_update = false; >>> - for (size_t i = 0; i < nb_meter->n_bands; i++) { >>> - if (band_cmp(&nb_bands[i], &sb_bands[i])) { >>> - need_update = true; >>> - break; >>> - } >>> - } >>> - >>> - free(nb_bands); >>> - free(sb_bands); >>> - >>> - return need_update; >>> -} >>> - >>> -static void >>> -sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn, >>> - const char *meter_name, >>> - const struct nbrec_meter *nb_meter, >>> - struct shash *sb_meters, >>> - struct sset *used_sb_meters) >>> -{ >>> - const struct sbrec_meter *sb_meter; >>> - bool new_sb_meter = false; >>> - >>> - sb_meter = shash_find_data(sb_meters, meter_name); >>> - if (!sb_meter) { >>> - sb_meter = sbrec_meter_insert(ovnsb_txn); >>> - sbrec_meter_set_name(sb_meter, meter_name); >>> - shash_add(sb_meters, sb_meter->name, sb_meter); >>> - new_sb_meter = true; >>> - } >>> - sset_add(used_sb_meters, meter_name); >>> - >>> - if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { >>> - struct sbrec_meter_band **sb_bands; >>> - sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); >>> - for (size_t i = 0; i < nb_meter->n_bands; i++) { >>> - const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; >>> - >>> - sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn); >>> - >>> - sbrec_meter_band_set_action(sb_bands[i], nb_band->action); >>> - sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); >>> - sbrec_meter_band_set_burst_size(sb_bands[i], >>> - nb_band->burst_size); >>> - } >>> - sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); >>> - free(sb_bands); >>> - } >>> - >>> - sbrec_meter_set_unit(sb_meter, nb_meter->unit); >>> -} >>> - >>> -static void >>> -sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, >>> - struct shash *meter_groups, >>> - const struct nbrec_acl *acl, struct shash >>> *sb_meters, >>> - struct sset *used_sb_meters) >>> -{ >>> - const struct nbrec_meter *nb_meter = >>> - fair_meter_lookup_by_name(meter_groups, acl->meter); >>> - >>> - if (!nb_meter) { >>> - return; >>> - } >>> - >>> - char *meter_name = alloc_acl_log_unique_meter_name(acl); >>> - sync_meters_iterate_nb_meter(ovnsb_txn, meter_name, nb_meter, >>> sb_meters, >>> - used_sb_meters); >>> - free(meter_name); >>> -} >>> - >>> -/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have >>> - * a corresponding entries in the Meter and Meter_Band tables in >>> - * OVN_Southbound. Additionally, ACL logs that use fair meters have >>> - * a private copy of its meter in the SB table. >>> - */ >>> -static void >>> -sync_meters(struct ovsdb_idl_txn *ovnsb_txn, >>> - const struct nbrec_meter_table *nbrec_meter_table, >>> - const struct nbrec_acl_table *nbrec_acl_table, >>> - const struct sbrec_meter_table *sbrec_meter_table, >>> - struct shash *meter_groups) >>> -{ >>> - struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); >>> - struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters); >>> - >>> - const struct sbrec_meter *sb_meter; >>> - SBREC_METER_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) { >>> - shash_add(&sb_meters, sb_meter->name, sb_meter); >>> - } >>> - >>> - const struct nbrec_meter *nb_meter; >>> - NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { >>> - sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name, >>> nb_meter, >>> - &sb_meters, &used_sb_meters); >>> - } >>> - >>> - /* >>> - * In addition to creating Meters in the SB from the block above, >>> check >>> - * and see if additional rows are needed to get ACLs logs >>> individually >>> - * rate-limited. >>> - */ >>> - const struct nbrec_acl *acl; >>> - NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) { >>> - sync_acl_fair_meter(ovnsb_txn, meter_groups, acl, >>> - &sb_meters, &used_sb_meters); >>> - } >>> - >>> - const char *used_meter; >>> - SSET_FOR_EACH_SAFE (used_meter, &used_sb_meters) { >>> - shash_find_and_delete(&sb_meters, used_meter); >>> - sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter)); >>> - } >>> - sset_destroy(&used_sb_meters); >>> - >>> - struct shash_node *node; >>> - SHASH_FOR_EACH_SAFE (node, &sb_meters) { >>> - sbrec_meter_delete(node->data); >>> - shash_delete(&sb_meters, node); >>> - } >>> - shash_destroy(&sb_meters); >>> -} >>> - >>> static bool >>> mirror_needs_update(const struct nbrec_mirror *nb_mirror, >>> const struct sbrec_mirror *sb_mirror) >>> @@ -17254,16 +17056,6 @@ build_mcast_groups(const struct >>> sbrec_igmp_group_table *sbrec_igmp_group_table, >>> } >>> } >>> -static void >>> -build_meter_groups(const struct nbrec_meter_table *nbrec_meter_table, >>> - struct shash *meter_groups) >>> -{ >>> - const struct nbrec_meter *nb_meter; >>> - NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { >>> - shash_add(meter_groups, nb_meter->name, nb_meter); >>> - } >>> -} >>> - >>> static const struct nbrec_static_mac_binding * >>> static_mac_binding_by_port_ip( >>> const struct nbrec_static_mac_binding_table *nbrec_static_mb_table, >>> @@ -17405,7 +17197,6 @@ northd_init(struct northd_data *data) >>> hmap_init(&data->ls_ports); >>> hmap_init(&data->lr_ports); >>> hmap_init(&data->port_groups); >>> - shash_init(&data->meter_groups); >>> hmap_init(&data->lbs); >>> hmap_init(&data->lb_groups); >>> ovs_list_init(&data->lr_list); >>> @@ -17443,12 +17234,6 @@ northd_destroy(struct northd_data *data) >>> hmap_destroy(&data->port_groups); >>> - struct shash_node *node; >>> - SHASH_FOR_EACH_SAFE (node, &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 >>> @@ -17587,7 +17372,6 @@ ovnnb_db_run(struct northd_input *input_data, >>> build_ip_mcast(ovnsb_txn, input_data->sbrec_ip_multicast_table, >>> input_data->sbrec_ip_mcast_by_dp, >>> &data->ls_datapaths.datapaths); >>> - build_meter_groups(input_data->nbrec_meter_table, >>> &data->meter_groups); >>> build_static_mac_binding_table(ovnsb_txn, >>> input_data->nbrec_static_mac_binding_table, >>> input_data->sbrec_static_mac_binding_table, >>> @@ -17602,9 +17386,6 @@ ovnnb_db_run(struct northd_input *input_data, >>> &data->ls_datapaths, &data->lbs); >>> sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table, >>> &data->port_groups); >>> - sync_meters(ovnsb_txn, input_data->nbrec_meter_table, >>> - input_data->nbrec_acl_table, >>> input_data->sbrec_meter_table, >>> - &data->meter_groups); >>> sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, >>> input_data->sbrec_mirror_table); >>> sync_dns_entries(ovnsb_txn, input_data->sbrec_dns_table, >>> diff --git a/northd/northd.h b/northd/northd.h >>> index f3e63b1e1a..dcc3acabd5 100644 >>> --- a/northd/northd.h >>> +++ b/northd/northd.h >>> @@ -31,8 +31,6 @@ struct northd_input { >>> 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 >>> *nbrec_static_mac_binding_table; >>> const struct nbrec_chassis_template_var_table >>> @@ -50,7 +48,6 @@ struct northd_input { >>> 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; >>> const struct sbrec_static_mac_binding_table >>> @@ -109,7 +106,6 @@ struct northd_data { >>> struct hmap ls_ports; >>> struct hmap lr_ports; >>> struct hmap port_groups; >>> - struct shash meter_groups; >>> struct hmap lbs; >>> struct hmap lb_groups; >>> struct ovs_list lr_list; >>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >>> index 4fa1b039ea..45362f4f93 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -836,6 +836,9 @@ main(int argc, char *argv[]) >>> ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, >>> &sbrec_multicast_group_columns[i]); >>> } >>> + for (size_t i = 0; i < SBREC_METER_N_COLUMNS; i++) { >>> + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, >>> &sbrec_meter_columns[i]); >>> + } >>> unixctl_command_register("sb-connection-status", "", 0, 0, >>> ovn_conn_show, ovnsb_idl_loop.idl); >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index d5be3be75b..dc8bf253f1 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >>> @@ -9,6 +9,8 @@ m4_define([_DUMP_DB_TABLES], [ >>> ovn-sbctl list logical_flow >> $1 >>> ovn-sbctl list port_binding >> $1 >>> ovn-sbctl list address_set >> $1 >>> + ovn-sbctl list meter >> $1 >>> + ovn-sbctl list meter_band >> $1 >>> ]) >>> # CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> @@ -9679,6 +9681,40 @@ OVN_CLEANUP([hv1]) >>> AT_CLEANUP >>> ]) >>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>> +AT_SETUP([ACL/Meter incremental processing - no northd recompute]) >>> +ovn_start >>> + >>> +check_recompute_counter() { >>> + northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE >>> inc-engine/show-stats northd recompute) >>> + AT_CHECK([test x$northd_recomp = x$1]) >>> + >>> + northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE >>> inc-engine/show-stats northd recompute) >>> + AT_CHECK([test x$northd_recomp = x$1]) >>> + >>> + sync_meters_recomp=$(as northd ovn-appctl -t NORTHD_TYPE >>> inc-engine/show-stats sync_meters recompute) >>> + AT_CHECK([test x$sync_meters_recomp = x$3]) >>> +} >> >> In check_recompute_counter(), the same comparison is performed twice for >> the $1 parameter. Then the $3 parameter is used in the third comparison. >> The $2 parameter is never used. I assume $2 was supposed to be used for >> something, but it's not clear what. >> > > Oops, it was supposed to ensure we don't recompute the lflow node. > > Does the following incremental change look OK to you? > If so, I can fold it in and apply the series. Yes, this looks good to me. Acked-by: Mark Michelson <mmichels@redhat.com> > > Thanks, > Dumitru > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index dc8bf253f1..9df02b422f 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9689,8 +9689,8 @@ check_recompute_counter() { > northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) > AT_CHECK([test x$northd_recomp = x$1]) > > - northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) > - AT_CHECK([test x$northd_recomp = x$1]) > + lflow_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute) > + AT_CHECK([test x$lflow_recomp = x$2]) > > sync_meters_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_meters recompute) > AT_CHECK([test x$sync_meters_recomp = x$3]) > --- > >>> + >>> +check ovn-nbctl --wait=sb ls-add ls >>> + >>> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats >>> +check ovn-nbctl --wait=sb meter-add m drop 1 pktps >>> +check ovn-nbctl --wait=sb acl-add ls from-lport 1 1 allow >>> +dnl Only triggers recompute of the sync_meters and lflow nodes. >>> +check_recompute_counter 0 2 2 >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> + >>> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats >>> +check ovn-nbctl --wait=sb meter-del m >>> +check ovn-nbctl --wait=sb acl-del ls >>> +dnl Only triggers recompute of the sync_meters and lflow nodes. >>> +check_recompute_counter 0 2 2 >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> + >>> +AT_CLEANUP >>> +]) >>> + >>> OVN_FOR_EACH_NORTHD_NO_HV([ >>> AT_SETUP([check fip and lb flows]) >>> AT_KEYWORDS([fip-lb-flows]) >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> >
diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h index de6fca4ccc..98535fff5a 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 SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run" #endif diff --git a/northd/automake.mk b/northd/automake.mk index b17f1fdb54..f52766de0c 100644 --- a/northd/automake.mk +++ b/northd/automake.mk @@ -12,6 +12,8 @@ northd_ovn_northd_SOURCES = \ northd/en-northd.h \ northd/en-lflow.c \ northd/en-lflow.h \ + northd/en-meters.c \ + northd/en-meters.h \ northd/en-northd-output.c \ northd/en-northd-output.h \ northd/en-sync-sb.c \ diff --git a/northd/en-lflow.c b/northd/en-lflow.c index 28ab1c67fb..0886d4c5ca 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -20,6 +20,7 @@ #include "en-lflow.h" #include "en-northd.h" +#include "en-meters.h" #include "lib/inc-proc-eng.h" #include "northd.h" @@ -35,6 +36,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 sync_meters_data *sync_meters_data = + engine_get_input_data("sync_meters", node); lflow_input->nbrec_bfd_table = EN_OVSDB_GET(engine_get_input("NB_bfd", node)); lflow_input->sbrec_bfd_table = @@ -56,7 +59,7 @@ lflow_get_input_data(struct engine_node *node, lflow_input->ls_ports = &northd_data->ls_ports; lflow_input->lr_ports = &northd_data->lr_ports; lflow_input->port_groups = &northd_data->port_groups; - lflow_input->meter_groups = &northd_data->meter_groups; + lflow_input->meter_groups = &sync_meters_data->meter_groups; lflow_input->lbs = &northd_data->lbs; lflow_input->features = &northd_data->features; lflow_input->ovn_internal_version_changed = diff --git a/northd/en-meters.c b/northd/en-meters.c new file mode 100644 index 0000000000..aabd002b62 --- /dev/null +++ b/northd/en-meters.c @@ -0,0 +1,281 @@ +/* + * Copyright (c) 2023, Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <config.h> + +#include "openvswitch/vlog.h" +#include "stopwatch.h" + +#include "en-meters.h" +#include "lib/stopwatch-names.h" + +VLOG_DEFINE_THIS_MODULE(en_meters); + +static void build_meter_groups(struct shash *meter_group, + const struct nbrec_meter_table *); +static void sync_meters(struct ovsdb_idl_txn *ovnsb_txn, + const struct nbrec_meter_table *, + const struct nbrec_acl_table *, + const struct sbrec_meter_table *, + struct shash *meter_groups); + +void +*en_sync_meters_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + struct sync_meters_data *data = xmalloc(sizeof *data); + + *data = (struct sync_meters_data) { + .meter_groups = SHASH_INITIALIZER(&data->meter_groups), + }; + return data; +} + +void +en_sync_meters_cleanup(void *data_) +{ + struct sync_meters_data *data = data_; + + shash_destroy(&data->meter_groups); +} + +void +en_sync_meters_run(struct engine_node *node, void *data_) +{ + struct sync_meters_data *data = data_; + + const struct nbrec_acl_table *acl_table = + EN_OVSDB_GET(engine_get_input("NB_acl", node)); + + const struct nbrec_meter_table *nb_meter_table = + EN_OVSDB_GET(engine_get_input("NB_meter", node)); + + const struct sbrec_meter_table *sb_meter_table = + EN_OVSDB_GET(engine_get_input("SB_meter", node)); + + const struct engine_context *eng_ctx = engine_get_context(); + + stopwatch_start(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec()); + + build_meter_groups(&data->meter_groups, nb_meter_table); + + sync_meters(eng_ctx->ovnsb_idl_txn, nb_meter_table, acl_table, + sb_meter_table, &data->meter_groups); + + stopwatch_stop(SYNC_METERS_RUN_STOPWATCH_NAME, time_msec()); + engine_set_node_state(node, EN_UPDATED); +} + +const struct nbrec_meter* +fair_meter_lookup_by_name(const struct shash *meter_groups, + const char *meter_name) +{ + const struct nbrec_meter *nb_meter = + meter_name ? shash_find_data(meter_groups, meter_name) : NULL; + if (nb_meter) { + return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; + } + return NULL; +} + +struct band_entry { + int64_t rate; + int64_t burst_size; + const char *action; +}; + +static int +band_cmp(const void *band1_, const void *band2_) +{ + const struct band_entry *band1p = band1_; + const struct band_entry *band2p = band2_; + + if (band1p->rate != band2p->rate) { + return band1p->rate - band2p->rate; + } else if (band1p->burst_size != band2p->burst_size) { + return band1p->burst_size - band2p->burst_size; + } else { + return strcmp(band1p->action, band2p->action); + } +} + +static bool +bands_need_update(const struct nbrec_meter *nb_meter, + const struct sbrec_meter *sb_meter) +{ + if (nb_meter->n_bands != sb_meter->n_bands) { + return true; + } + + /* Place the Northbound entries in sorted order. */ + struct band_entry *nb_bands; + nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands); + for (size_t i = 0; i < nb_meter->n_bands; i++) { + struct nbrec_meter_band *nb_band = nb_meter->bands[i]; + + nb_bands[i].rate = nb_band->rate; + nb_bands[i].burst_size = nb_band->burst_size; + nb_bands[i].action = nb_band->action; + } + qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp); + + /* Place the Southbound entries in sorted order. */ + struct band_entry *sb_bands; + sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands); + for (size_t i = 0; i < sb_meter->n_bands; i++) { + struct sbrec_meter_band *sb_band = sb_meter->bands[i]; + + sb_bands[i].rate = sb_band->rate; + sb_bands[i].burst_size = sb_band->burst_size; + sb_bands[i].action = sb_band->action; + } + qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp); + + bool need_update = false; + for (size_t i = 0; i < nb_meter->n_bands; i++) { + if (band_cmp(&nb_bands[i], &sb_bands[i])) { + need_update = true; + break; + } + } + + free(nb_bands); + free(sb_bands); + + return need_update; +} + +static void +sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn, + const char *meter_name, + const struct nbrec_meter *nb_meter, + struct shash *sb_meters, + struct sset *used_sb_meters) +{ + const struct sbrec_meter *sb_meter; + bool new_sb_meter = false; + + sb_meter = shash_find_data(sb_meters, meter_name); + if (!sb_meter) { + sb_meter = sbrec_meter_insert(ovnsb_txn); + sbrec_meter_set_name(sb_meter, meter_name); + shash_add(sb_meters, sb_meter->name, sb_meter); + new_sb_meter = true; + } + sset_add(used_sb_meters, meter_name); + + if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { + struct sbrec_meter_band **sb_bands; + sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); + for (size_t i = 0; i < nb_meter->n_bands; i++) { + const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; + + sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn); + + sbrec_meter_band_set_action(sb_bands[i], nb_band->action); + sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); + sbrec_meter_band_set_burst_size(sb_bands[i], + nb_band->burst_size); + } + sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); + free(sb_bands); + } + + sbrec_meter_set_unit(sb_meter, nb_meter->unit); +} + +static void +sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, + struct shash *meter_groups, + const struct nbrec_acl *acl, struct shash *sb_meters, + struct sset *used_sb_meters) +{ + const struct nbrec_meter *nb_meter = + fair_meter_lookup_by_name(meter_groups, acl->meter); + + if (!nb_meter) { + return; + } + + char *meter_name = alloc_acl_log_unique_meter_name(acl); + sync_meters_iterate_nb_meter(ovnsb_txn, meter_name, nb_meter, sb_meters, + used_sb_meters); + free(meter_name); +} + +static void +build_meter_groups(struct shash *meter_groups, + const struct nbrec_meter_table *nb_meter_table) +{ + const struct nbrec_meter *nb_meter; + + shash_clear(meter_groups); + NBREC_METER_TABLE_FOR_EACH (nb_meter, nb_meter_table) { + shash_add(meter_groups, nb_meter->name, nb_meter); + } +} + +/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have + * a corresponding entries in the Meter and Meter_Band tables in + * OVN_Southbound. Additionally, ACL logs that use fair meters have + * a private copy of its meter in the SB table. + */ +static void +sync_meters(struct ovsdb_idl_txn *ovnsb_txn, + const struct nbrec_meter_table *nbrec_meter_table, + const struct nbrec_acl_table *nbrec_acl_table, + const struct sbrec_meter_table *sbrec_meter_table, + struct shash *meter_groups) +{ + struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); + struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters); + + const struct sbrec_meter *sb_meter; + SBREC_METER_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) { + shash_add(&sb_meters, sb_meter->name, sb_meter); + } + + const struct nbrec_meter *nb_meter; + NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { + sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name, nb_meter, + &sb_meters, &used_sb_meters); + } + + /* + * In addition to creating Meters in the SB from the block above, check + * and see if additional rows are needed to get ACLs logs individually + * rate-limited. + */ + const struct nbrec_acl *acl; + NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) { + sync_acl_fair_meter(ovnsb_txn, meter_groups, acl, + &sb_meters, &used_sb_meters); + } + + const char *used_meter; + SSET_FOR_EACH_SAFE (used_meter, &used_sb_meters) { + shash_find_and_delete(&sb_meters, used_meter); + sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter)); + } + sset_destroy(&used_sb_meters); + + struct shash_node *node; + SHASH_FOR_EACH_SAFE (node, &sb_meters) { + sbrec_meter_delete(node->data); + shash_delete(&sb_meters, node); + } + shash_destroy(&sb_meters); +} diff --git a/northd/en-meters.h b/northd/en-meters.h new file mode 100644 index 0000000000..a1743282e3 --- /dev/null +++ b/northd/en-meters.h @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2023, Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef EN_METERS_H +#define EN_METERS_H 1 + +#include "openvswitch/shash.h" + +#include "lib/inc-proc-eng.h" +#include "lib/ovn-nb-idl.h" +#include "lib/ovn-sb-idl.h" + +struct sync_meters_data { + struct shash meter_groups; +}; + +void *en_sync_meters_init(struct engine_node *, struct engine_arg *); +void en_sync_meters_cleanup(void *data); +void en_sync_meters_run(struct engine_node *, void *data); + +const struct nbrec_meter *fair_meter_lookup_by_name( + const struct shash *meter_groups, + const char *meter_name); + +static inline char* +alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) +{ + return xasprintf("%s__" UUID_FMT, + acl->meter, UUID_ARGS(&acl->header_.uuid)); +} + +#endif /* EN_ACL_H */ diff --git a/northd/en-northd.c b/northd/en-northd.c index 044fa70190..cc05efdbbc 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -76,10 +76,6 @@ northd_get_input_data(struct engine_node *node, 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 = - EN_OVSDB_GET(engine_get_input("NB_acl", node)); input_data->nbrec_static_mac_binding_table = EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node)); input_data->nbrec_chassis_template_var_table = @@ -107,8 +103,6 @@ northd_get_input_data(struct engine_node *node, 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 = EN_OVSDB_GET(engine_get_input("SB_dns", node)); input_data->sbrec_ip_multicast_table = diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index d328deb222..1de0551b3d 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -33,6 +33,7 @@ #include "en-northd.h" #include "en-lflow.h" #include "en-northd-output.h" +#include "en-meters.h" #include "en-sync-sb.h" #include "en-sync-from-sb.h" #include "unixctl.h" @@ -135,6 +136,7 @@ static ENGINE_NODE(lflow, "lflow"); static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); static ENGINE_NODE(northd_output, "northd_output"); +static ENGINE_NODE(sync_meters, "sync_meters"); 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(fdb_aging, "fdb_aging"); @@ -148,10 +150,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, 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); engine_add_input(&en_northd, &en_nb_logical_router, NULL); engine_add_input(&en_northd, &en_nb_mirror, NULL); - engine_add_input(&en_northd, &en_nb_meter, NULL); engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL); engine_add_input(&en_northd, &en_nb_chassis_template_var, NULL); @@ -188,7 +188,13 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_fdb_aging, &en_northd, NULL); engine_add_input(&en_fdb_aging, &en_fdb_aging_waker, NULL); + engine_add_input(&en_sync_meters, &en_nb_acl, NULL); + engine_add_input(&en_sync_meters, &en_nb_meter, NULL); + engine_add_input(&en_sync_meters, &en_sb_meter, NULL); + engine_add_input(&en_lflow, &en_nb_bfd, NULL); + engine_add_input(&en_lflow, &en_nb_acl, NULL); + engine_add_input(&en_lflow, &en_sync_meters, NULL); engine_add_input(&en_lflow, &en_sb_bfd, NULL); engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); @@ -204,9 +210,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, /* 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 table and + * SB Meter/Meter_Band tables. */ engine_add_input(&en_sync_to_sb, &en_sync_to_sb_addr_set, NULL); + engine_add_input(&en_sync_to_sb, &en_sync_meters, 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 13f2ae0565..48fb44a8be 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -21,6 +21,7 @@ #include "bitmap.h" #include "coverage.h" #include "dirs.h" +#include "en-meters.h" #include "ipam.h" #include "openvswitch/dynamic-string.h" #include "hash.h" @@ -4980,25 +4981,22 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports, } static bool -check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls) +check_ls_changes_other_than_lsp_acl(const struct nbrec_logical_switch *ls) { /* Check if the columns are changed in this row. */ enum nbrec_logical_switch_column_id col; for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) { - if (nbrec_logical_switch_is_updated(ls, col) && - col != NBREC_LOGICAL_SWITCH_COL_PORTS) { + if (nbrec_logical_switch_is_updated(ls, col)) { + if (col == NBREC_LOGICAL_SWITCH_COL_ACLS || + col == NBREC_LOGICAL_SWITCH_COL_PORTS) { + continue; + } return true; } } /* Check if the referenced rows are changed. XXX: Need a better OVSDB IDL interface for this check. */ - for (size_t i = 0; i < ls->n_acls; i++) { - if (nbrec_acl_row_get_seqno(ls->acls[i], - OVSDB_IDL_CHANGE_MODIFY) > 0) { - return true; - } - } if (ls->copp && nbrec_copp_row_get_seqno(ls->copp, OVSDB_IDL_CHANGE_MODIFY) > 0) { return true; @@ -5106,7 +5104,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, } /* Now only able to handle lsp changes. */ - if (check_ls_changes_other_than_lsp(changed_ls)) { + if (check_ls_changes_other_than_lsp_acl(changed_ls)) { goto fail; } @@ -6987,25 +6985,6 @@ build_acl_hints(struct ovn_datapath *od, } } -static const struct nbrec_meter* -fair_meter_lookup_by_name(const struct shash *meter_groups, - const char *meter_name) -{ - const struct nbrec_meter *nb_meter = - meter_name ? shash_find_data(meter_groups, meter_name) : NULL; - if (nb_meter) { - return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; - } - return NULL; -} - -static char* -alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) -{ - return xasprintf("%s__" UUID_FMT, - acl->meter, UUID_ARGS(&acl->header_.uuid)); -} - static void build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, const struct shash *meter_groups) @@ -16608,183 +16587,6 @@ sync_port_groups(struct ovsdb_idl_txn *ovnsb_txn, shash_destroy(&sb_port_groups); } -struct band_entry { - int64_t rate; - int64_t burst_size; - const char *action; -}; - -static int -band_cmp(const void *band1_, const void *band2_) -{ - const struct band_entry *band1p = band1_; - const struct band_entry *band2p = band2_; - - if (band1p->rate != band2p->rate) { - return band1p->rate - band2p->rate; - } else if (band1p->burst_size != band2p->burst_size) { - return band1p->burst_size - band2p->burst_size; - } else { - return strcmp(band1p->action, band2p->action); - } -} - -static bool -bands_need_update(const struct nbrec_meter *nb_meter, - const struct sbrec_meter *sb_meter) -{ - if (nb_meter->n_bands != sb_meter->n_bands) { - return true; - } - - /* Place the Northbound entries in sorted order. */ - struct band_entry *nb_bands; - nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands); - for (size_t i = 0; i < nb_meter->n_bands; i++) { - struct nbrec_meter_band *nb_band = nb_meter->bands[i]; - - nb_bands[i].rate = nb_band->rate; - nb_bands[i].burst_size = nb_band->burst_size; - nb_bands[i].action = nb_band->action; - } - qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp); - - /* Place the Southbound entries in sorted order. */ - struct band_entry *sb_bands; - sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands); - for (size_t i = 0; i < sb_meter->n_bands; i++) { - struct sbrec_meter_band *sb_band = sb_meter->bands[i]; - - sb_bands[i].rate = sb_band->rate; - sb_bands[i].burst_size = sb_band->burst_size; - sb_bands[i].action = sb_band->action; - } - qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp); - - bool need_update = false; - for (size_t i = 0; i < nb_meter->n_bands; i++) { - if (band_cmp(&nb_bands[i], &sb_bands[i])) { - need_update = true; - break; - } - } - - free(nb_bands); - free(sb_bands); - - return need_update; -} - -static void -sync_meters_iterate_nb_meter(struct ovsdb_idl_txn *ovnsb_txn, - const char *meter_name, - const struct nbrec_meter *nb_meter, - struct shash *sb_meters, - struct sset *used_sb_meters) -{ - const struct sbrec_meter *sb_meter; - bool new_sb_meter = false; - - sb_meter = shash_find_data(sb_meters, meter_name); - if (!sb_meter) { - sb_meter = sbrec_meter_insert(ovnsb_txn); - sbrec_meter_set_name(sb_meter, meter_name); - shash_add(sb_meters, sb_meter->name, sb_meter); - new_sb_meter = true; - } - sset_add(used_sb_meters, meter_name); - - if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) { - struct sbrec_meter_band **sb_bands; - sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands); - for (size_t i = 0; i < nb_meter->n_bands; i++) { - const struct nbrec_meter_band *nb_band = nb_meter->bands[i]; - - sb_bands[i] = sbrec_meter_band_insert(ovnsb_txn); - - sbrec_meter_band_set_action(sb_bands[i], nb_band->action); - sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate); - sbrec_meter_band_set_burst_size(sb_bands[i], - nb_band->burst_size); - } - sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); - free(sb_bands); - } - - sbrec_meter_set_unit(sb_meter, nb_meter->unit); -} - -static void -sync_acl_fair_meter(struct ovsdb_idl_txn *ovnsb_txn, - struct shash *meter_groups, - const struct nbrec_acl *acl, struct shash *sb_meters, - struct sset *used_sb_meters) -{ - const struct nbrec_meter *nb_meter = - fair_meter_lookup_by_name(meter_groups, acl->meter); - - if (!nb_meter) { - return; - } - - char *meter_name = alloc_acl_log_unique_meter_name(acl); - sync_meters_iterate_nb_meter(ovnsb_txn, meter_name, nb_meter, sb_meters, - used_sb_meters); - free(meter_name); -} - -/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have - * a corresponding entries in the Meter and Meter_Band tables in - * OVN_Southbound. Additionally, ACL logs that use fair meters have - * a private copy of its meter in the SB table. - */ -static void -sync_meters(struct ovsdb_idl_txn *ovnsb_txn, - const struct nbrec_meter_table *nbrec_meter_table, - const struct nbrec_acl_table *nbrec_acl_table, - const struct sbrec_meter_table *sbrec_meter_table, - struct shash *meter_groups) -{ - struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); - struct sset used_sb_meters = SSET_INITIALIZER(&used_sb_meters); - - const struct sbrec_meter *sb_meter; - SBREC_METER_TABLE_FOR_EACH (sb_meter, sbrec_meter_table) { - shash_add(&sb_meters, sb_meter->name, sb_meter); - } - - const struct nbrec_meter *nb_meter; - NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { - sync_meters_iterate_nb_meter(ovnsb_txn, nb_meter->name, nb_meter, - &sb_meters, &used_sb_meters); - } - - /* - * In addition to creating Meters in the SB from the block above, check - * and see if additional rows are needed to get ACLs logs individually - * rate-limited. - */ - const struct nbrec_acl *acl; - NBREC_ACL_TABLE_FOR_EACH (acl, nbrec_acl_table) { - sync_acl_fair_meter(ovnsb_txn, meter_groups, acl, - &sb_meters, &used_sb_meters); - } - - const char *used_meter; - SSET_FOR_EACH_SAFE (used_meter, &used_sb_meters) { - shash_find_and_delete(&sb_meters, used_meter); - sset_delete(&used_sb_meters, SSET_NODE_FROM_NAME(used_meter)); - } - sset_destroy(&used_sb_meters); - - struct shash_node *node; - SHASH_FOR_EACH_SAFE (node, &sb_meters) { - sbrec_meter_delete(node->data); - shash_delete(&sb_meters, node); - } - shash_destroy(&sb_meters); -} - static bool mirror_needs_update(const struct nbrec_mirror *nb_mirror, const struct sbrec_mirror *sb_mirror) @@ -17254,16 +17056,6 @@ build_mcast_groups(const struct sbrec_igmp_group_table *sbrec_igmp_group_table, } } -static void -build_meter_groups(const struct nbrec_meter_table *nbrec_meter_table, - struct shash *meter_groups) -{ - const struct nbrec_meter *nb_meter; - NBREC_METER_TABLE_FOR_EACH (nb_meter, nbrec_meter_table) { - shash_add(meter_groups, nb_meter->name, nb_meter); - } -} - static const struct nbrec_static_mac_binding * static_mac_binding_by_port_ip( const struct nbrec_static_mac_binding_table *nbrec_static_mb_table, @@ -17405,7 +17197,6 @@ northd_init(struct northd_data *data) hmap_init(&data->ls_ports); hmap_init(&data->lr_ports); hmap_init(&data->port_groups); - shash_init(&data->meter_groups); hmap_init(&data->lbs); hmap_init(&data->lb_groups); ovs_list_init(&data->lr_list); @@ -17443,12 +17234,6 @@ northd_destroy(struct northd_data *data) hmap_destroy(&data->port_groups); - struct shash_node *node; - SHASH_FOR_EACH_SAFE (node, &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 @@ -17587,7 +17372,6 @@ ovnnb_db_run(struct northd_input *input_data, build_ip_mcast(ovnsb_txn, input_data->sbrec_ip_multicast_table, input_data->sbrec_ip_mcast_by_dp, &data->ls_datapaths.datapaths); - build_meter_groups(input_data->nbrec_meter_table, &data->meter_groups); build_static_mac_binding_table(ovnsb_txn, input_data->nbrec_static_mac_binding_table, input_data->sbrec_static_mac_binding_table, @@ -17602,9 +17386,6 @@ ovnnb_db_run(struct northd_input *input_data, &data->ls_datapaths, &data->lbs); sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table, &data->port_groups); - sync_meters(ovnsb_txn, input_data->nbrec_meter_table, - input_data->nbrec_acl_table, input_data->sbrec_meter_table, - &data->meter_groups); sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, input_data->sbrec_mirror_table); sync_dns_entries(ovnsb_txn, input_data->sbrec_dns_table, diff --git a/northd/northd.h b/northd/northd.h index f3e63b1e1a..dcc3acabd5 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -31,8 +31,6 @@ struct northd_input { 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 *nbrec_static_mac_binding_table; const struct nbrec_chassis_template_var_table @@ -50,7 +48,6 @@ struct northd_input { 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; const struct sbrec_static_mac_binding_table @@ -109,7 +106,6 @@ struct northd_data { struct hmap ls_ports; struct hmap lr_ports; struct hmap port_groups; - struct shash meter_groups; struct hmap lbs; struct hmap lb_groups; struct ovs_list lr_list; diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 4fa1b039ea..45362f4f93 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -836,6 +836,9 @@ main(int argc, char *argv[]) ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_multicast_group_columns[i]); } + for (size_t i = 0; i < SBREC_METER_N_COLUMNS; i++) { + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_meter_columns[i]); + } unixctl_command_register("sb-connection-status", "", 0, 0, ovn_conn_show, ovnsb_idl_loop.idl); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index d5be3be75b..dc8bf253f1 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -9,6 +9,8 @@ m4_define([_DUMP_DB_TABLES], [ ovn-sbctl list logical_flow >> $1 ovn-sbctl list port_binding >> $1 ovn-sbctl list address_set >> $1 + ovn-sbctl list meter >> $1 + ovn-sbctl list meter_band >> $1 ]) # CHECK_NO_CHANGE_AFTER_RECOMPUTE @@ -9679,6 +9681,40 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([ACL/Meter incremental processing - no northd recompute]) +ovn_start + +check_recompute_counter() { + northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) + AT_CHECK([test x$northd_recomp = x$1]) + + northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) + AT_CHECK([test x$northd_recomp = x$1]) + + sync_meters_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_meters recompute) + AT_CHECK([test x$sync_meters_recomp = x$3]) +} + +check ovn-nbctl --wait=sb ls-add ls + +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb meter-add m drop 1 pktps +check ovn-nbctl --wait=sb acl-add ls from-lport 1 1 allow +dnl Only triggers recompute of the sync_meters and lflow nodes. +check_recompute_counter 0 2 2 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb meter-del m +check ovn-nbctl --wait=sb acl-del ls +dnl Only triggers recompute of the sync_meters and lflow nodes. +check_recompute_counter 0 2 2 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD_NO_HV([ AT_SETUP([check fip and lb flows]) AT_KEYWORDS([fip-lb-flows])
This is beneficial in a few ways: - first, it reduces the number of different types of data the northd I-P node has to process. - it turns out the northd I-P node (whose recompute is rather costly) doesn't really depend on meters or ACLs. - prepares the ground for a pure I-P implementation for ACLs, with a simple/clear dependency between NB.ACL and the lflow I-P node. From a meters synchronization perspective this commit doesn't change any of the existing behavior and logic. It mostly just moves the meters related code out of northd.c and into en-meters.c. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/stopwatch-names.h | 1 northd/automake.mk | 2 northd/en-lflow.c | 5 + northd/en-meters.c | 281 ++++++++++++++++++++++++++++++++++++++++++++++ northd/en-meters.h | 44 +++++++ northd/en-northd.c | 6 - northd/inc-proc-northd.c | 14 ++ northd/northd.c | 235 +------------------------------------- northd/northd.h | 4 - northd/ovn-northd.c | 3 tests/ovn-northd.at | 36 ++++++ 11 files changed, 390 insertions(+), 241 deletions(-) create mode 100644 northd/en-meters.c create mode 100644 northd/en-meters.h