On Sep 18, Lorenzo Bianconi wrote:
> > 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?
as patch 1/4, we do not need to run lr_nat_record_destroy(). Please ignore this
comment.
Regards,
Lorenzo
>
> > }
> >
> > 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
