On 8/17/23 12:43, Ales Musil wrote:
> On Wed, Aug 16, 2023 at 10:42 PM Dumitru Ceara <dce...@redhat.com> 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 <dce...@redhat.com>
>>
> ---
>>
>
> Hi Dumitru,
>
Hi Ales,
> I have a couple of suggestions.
>
Thanks for your review!
>
>> lib/stopwatch-names.h | 1 +
>> northd/automake.mk | 2 +
>> northd/en-lflow.c | 5 +-
>> northd/en-meters.c | 295 +++++++++++++++++++++++++++++++++++++++
>> northd/en-meters.h | 44 ++++++
>> northd/en-northd.c | 6 -
>> northd/inc-proc-northd.c | 14 +-
>> northd/northd.c | 249 ++-------------------------------
>> northd/northd.h | 4 -
>> northd/ovn-northd.c | 3 +
>> tests/ovn-northd.at | 36 +++++
>> 11 files changed, 404 insertions(+), 255 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..3d3b22368f
>> --- /dev/null
>> +++ b/northd/en-meters.c
>> @@ -0,0 +1,295 @@
>> +/*
>> + * 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 ? -1 : 1;
>>
>
> We could spare the extra comparison by doing "return band1p->rate >
> band2p->rate" instead.
>
We clarified this offline; it's actually supposed to be:
return band1p->rate - band2p->rate;
I changed it accordingly.
>
>> + } else if (band1p->burst_size != band2p->burst_size) {
>> + return band1p->burst_size > band2p->burst_size ? -1 : 1;
>>
>
> Same here.
>
Here too.
>
>> + } 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;
>> + }
>> +
>> + /* A single band is the most common scenario, so speed up that
>> + * check. */
>> + if (nb_meter->n_bands == 1) {
>> + struct nbrec_meter_band *nb_band = nb_meter->bands[0];
>> + struct sbrec_meter_band *sb_band = sb_meter->bands[0];
>> +
>> + return !(nb_band->rate == sb_band->rate
>> + && nb_band->burst_size == sb_band->burst_size
>> + && !strcmp(sb_band->action, nb_band->action));
>>
>
> I know that it's effectively the same, but actually more readable to use
> "return !band_cmp".
>
There's a catch. band_cmp() expects struct band_entry as arguments.
However, I disagree with the micro-optimization done in this "if" so I
just removed it all together in v2.
>
>> + }
>> +
>> + /* 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 (nb_bands[i].rate != sb_bands[i].rate
>> + || nb_bands[i].burst_size != sb_bands[i].burst_size
>> + || strcmp(nb_bands[i].action, sb_bands[i].action)) {
>>
>
> Same here we could just use the band_cmp.
>
Done.
>
>> + need_update = true;
>> + goto done;
>>
>
> Done label is not needed.
>
True, I added a break here.
>
>> + }
>> + }
>> +
>> +done:
>> + 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 9a12a94ae2..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,197 +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 ? -1 : 1;
>> - } else if (band1p->burst_size != band2p->burst_size) {
>> - return band1p->burst_size > band2p->burst_size ? -1 : 1;
>> - } 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;
>> - }
>> -
>> - /* A single band is the most common scenario, so speed up that
>> - * check. */
>> - if (nb_meter->n_bands == 1) {
>> - struct nbrec_meter_band *nb_band = nb_meter->bands[0];
>> - struct sbrec_meter_band *sb_band = sb_meter->bands[0];
>> -
>> - return !(nb_band->rate == sb_band->rate
>> - && nb_band->burst_size == sb_band->burst_size
>> - && !strcmp(sb_band->action, nb_band->action));
>> - }
>> -
>> - /* 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 (nb_bands[i].rate != sb_bands[i].rate
>> - || nb_bands[i].burst_size != sb_bands[i].burst_size
>> - || strcmp(nb_bands[i].action, sb_bands[i].action)) {
>> - need_update = true;
>> - goto done;
>> - }
>> - }
>> -
>> -done:
>> - 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)
>> @@ -17268,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,
>> @@ -17419,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);
>> @@ -17457,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
>> @@ -17601,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,
>> @@ -17616,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])
>> --
>> 2.39.3
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Other than that it looks good.
>
> Thanks,
> Ales
>
V2 posted:
https://patchwork.ozlabs.org/project/ovn/list/?series=369253&state=*
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
