> When a router is created or destroyed add the ability to en-lr-nat
> engine node to compute the new state of the lr_nat_table instead of
> recalculating the lr_nat_table for the whole northbound datapabase.
>
> I needed to change lr_nat_table_find_by_index_() function to
> lr_nat_table_find_by_uuid_() because the ovn_datapath indexes are no
> longer guaranteed sequential starting from zero there needed to be a way
> to determine if the entry lr_nat_table->array[index] is initialized or
> not. This can be determined by using the hmap of ovn_datapaths.
>
> Reported-by: Dumitru Ceara <[email protected]>
> Reported-at: https://issues.redhat.com/browse/FDP-757
> Signed-off-by: Jacob Tanenbaum <[email protected]>
Hi Jacob,
Some comments inline.
Regards,
Lorenzo
>
> diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c
> index 0583e34a5..85ecfb72f 100644
> --- a/northd/en-lr-nat.c
> +++ b/northd/en-lr-nat.c
> @@ -48,6 +48,9 @@ static void lr_nat_table_build(struct lr_nat_table *,
> static struct lr_nat_record *lr_nat_table_find_by_index_(
> const struct lr_nat_table *, size_t od_index);
>
> +static struct lr_nat_record *lr_nat_table_find_by_uuid_(
> + const struct lr_nat_table *, struct uuid nb_uuid);
> +
> static struct lr_nat_record *lr_nat_record_create(struct lr_nat_table *,
> const struct ovn_datapath
> *,
> const struct hmap
> *lr_ports);
> @@ -83,6 +86,7 @@ en_lr_nat_init(struct engine_node *node OVS_UNUSED,
> struct ed_type_lr_nat_data *data = xzalloc(sizeof *data);
> lr_nat_table_init(&data->lr_nats);
> hmapx_init(&data->trk_data.crupdated);
> + hmapx_init(&data->trk_data.deleted);
> return data;
> }
>
> @@ -92,6 +96,7 @@ en_lr_nat_cleanup(void *data_)
> struct ed_type_lr_nat_data *data = (struct ed_type_lr_nat_data *) data_;
> lr_nat_table_destroy(&data->lr_nats);
> hmapx_destroy(&data->trk_data.crupdated);
> + hmapx_destroy(&data->trk_data.deleted);
I guess we need to run lr_nat_record_destroy() here, right?
> }
>
> void
> @@ -99,6 +104,11 @@ en_lr_nat_clear_tracked_data(void *data_)
> {
> struct ed_type_lr_nat_data *data = (struct ed_type_lr_nat_data *) data_;
> hmapx_clear(&data->trk_data.crupdated);
> + struct hmapx_node *hmapx_node;
> + HMAPX_FOR_EACH_SAFE (hmapx_node, &data->trk_data.deleted) {
> + lr_nat_record_destroy(hmapx_node->data);
> + hmapx_delete(&data->trk_data.deleted, hmapx_node);
> + }
> }
>
> enum engine_node_state
> @@ -125,33 +135,49 @@ lr_nat_northd_handler(struct engine_node *node, void
> *data_)
> return EN_UNHANDLED;
> }
>
> - if (northd_has_lrouters_in_tracked_data(&northd_data->trk_data)) {
> - return EN_UNHANDLED;
> - }
> -
> - if (!northd_has_lr_nats_in_tracked_data(&northd_data->trk_data)) {
> + if (!northd_has_lr_nats_in_tracked_data(&northd_data->trk_data) &&
> + hmapx_count(&northd_data->trk_data.trk_routers.deleted) == 0) {
> return EN_HANDLED_UNCHANGED;
> }
>
> struct ed_type_lr_nat_data *data = data_;
> struct lr_nat_record *lrnat_rec;
> +
nit: please remove new-line here.
> const struct ovn_datapath *od;
> struct hmapx_node *hmapx_node;
> + struct lr_nat_table *table = &data->lr_nats;
> +
> + if (northd_has_lrouters_in_tracked_data(&northd_data->trk_data)) {
> + if (hmap_count(&northd_data->lr_datapaths.datapaths) >
> + hmap_count(&table->entries)) {
> + table->array = xrealloc(table->array,
> + ods_size(&northd_data->lr_datapaths) *
> + sizeof *table->array);
> + }
> + }
>
> HMAPX_FOR_EACH (hmapx_node, &northd_data->trk_data.trk_nat_lrs) {
> od = hmapx_node->data;
> - lrnat_rec = lr_nat_table_find_by_index_(&data->lr_nats, od->index);
> - ovs_assert(lrnat_rec);
> - lr_nat_record_reinit(lrnat_rec, od, &northd_data->lr_ports);
> + lrnat_rec = lr_nat_table_find_by_uuid_(&data->lr_nats, od->key);
> + if (lrnat_rec) {
> + lr_nat_record_reinit(lrnat_rec, od, &northd_data->lr_ports);
> + } else {
> + lrnat_rec = lr_nat_record_create(table, od,
> + &northd_data->lr_ports);
> + }
>
> /* Add the lrnet rec to the tracking data. */
> hmapx_add(&data->trk_data.crupdated, lrnat_rec);
> }
> -
> + HMAPX_FOR_EACH (hmapx_node, &northd_data->trk_data.trk_routers.deleted) {
> + od = hmapx_node->data;
> + lrnat_rec = lr_nat_table_find_by_index_(table, od->index);
Can lrnat_rec be NULL here?
> + hmap_remove(&table->entries, &lrnat_rec->key_node);
> + hmapx_add(&data->trk_data.deleted, lrnat_rec);
> + }
> if (lr_nat_has_tracked_data(&data->trk_data)) {
> return EN_HANDLED_UPDATED;
> }
> -
nit: unnecessary change
> return EN_HANDLED_UNCHANGED;
> }
>
> @@ -206,6 +232,19 @@ lr_nat_table_find_by_index_(const struct lr_nat_table
> *table,
> return table->array[od_index];
> }
>
> +static struct lr_nat_record *
> +lr_nat_table_find_by_uuid_(const struct lr_nat_table *table,
> + struct uuid nb_uuid)
> +{
> + struct lr_nat_record *record;
> + HMAP_FOR_EACH_WITH_HASH (record, key_node,
> + uuid_hash(&nb_uuid),
> + &table->entries){
> + return record;
> + }
> + return NULL;
> +}
> +
> static struct lr_nat_record *
> lr_nat_record_create(struct lr_nat_table *table,
> const struct ovn_datapath *od,
> diff --git a/northd/en-lr-nat.h b/northd/en-lr-nat.h
> index 495e24042..d18ca56c3 100644
> --- a/northd/en-lr-nat.h
> +++ b/northd/en-lr-nat.h
> @@ -97,6 +97,8 @@ struct lr_nat_record {
> struct lr_nat_tracked_data {
> /* Created or updated logical router with NAT data. */
> struct hmapx crupdated;
> + /* Deleted logical router with NAT data. */
> + struct hmapx deleted;
> };
>
> struct lr_nat_table {
> @@ -134,7 +136,7 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry)
>
> static inline bool
> lr_nat_has_tracked_data(struct lr_nat_tracked_data *trk_data) {
> - return !hmapx_is_empty(&trk_data->crupdated);
> + return !hmapx_is_empty(&trk_data->crupdated) ||
> !hmapx_is_empty(&trk_data->deleted);
> }
>
> #endif /* EN_LR_NAT_H */
> diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
> index c80a49819..2b6c0d0ff 100644
> --- a/northd/en-lr-stateful.c
> +++ b/northd/en-lr-stateful.c
> @@ -135,7 +135,8 @@ enum engine_input_handler_result
> lr_stateful_northd_handler(struct engine_node *node, void *data OVS_UNUSED)
> {
> struct northd_data *northd_data = engine_get_input_data("northd", node);
> - if (!northd_has_tracked_data(&northd_data->trk_data)) {
> + if (!northd_has_tracked_data(&northd_data->trk_data) ||
> + northd_has_lrouters_in_tracked_data(&northd_data->trk_data)) {
> return EN_UNHANDLED;
> }
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 2d62a2399..3c358c232 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -12521,7 +12521,7 @@ check ovn-nbctl --wait=sb lsp-add sw0 sw0p1 --
> lsp-set-addresses sw0p1 "00:00:20
> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> check ovn-nbctl --wait=sb lr-add lr0
> check_engine_stats northd norecompute compute
> -check_engine_stats lr_nat recompute nocompute
> +check_engine_stats lr_nat norecompute compute
> check_engine_stats lr_stateful recompute nocompute
> check_engine_stats sync_to_sb_pb recompute nocompute
> check_engine_stats sync_to_sb_lb norecompute compute
> @@ -12532,7 +12532,7 @@ check as northd ovn-appctl -t ovn-northd
> inc-engine/clear-stats
> check ovn-nbctl --wait=sb lr-del lr0
>
> check_engine_stats northd norecompute compute
> -check_engine_stats lr_nat recompute nocompute
> +check_engine_stats lr_nat norecompute compute
> check_engine_stats lr_stateful recompute nocompute
> check_engine_stats sync_to_sb_pb recompute nocompute
> check_engine_stats sync_to_sb_lb norecompute compute
> --
> 2.51.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
