Hi! Thank you so much for the review. I'd be glad if you merged it after the squash. Thank you so much for the edits you made. There really are a lot of them. I'll take a look at them and keep them in mind for the future.
On 28.11.2025 14:40, Dumitru Ceara wrote: > On 11/23/25 12:11 PM, Alexandra Rukomoinikova wrote: >> From: Aleksandr Smirnov <[email protected]> >> >> The northd creates flows that restrict processing ARP requests: >> If ARP request comes from lswitch that has either localnet or >> l2gateway ports and router's attachment port is L3 dgw port such >> ARP will be allowed only on resident chassis. >> >> However, same lswitch could also have 'normal' aka virtual lports. >> For these lports restrictions described above should not be applied. >> >> The fix moves is_chassis_resident check from lrouter to lswitch level. >> The residence check is only applied for ARP requests that were >> originated from localnet/l2gateway ports. If such check is succeeded >> ARP request is allowed otherwise ARP request packet is dropped. >> For ARP requests coming from virtual ports no residence restrictions >> are applied. >> >> Co-authored-by: Alexandra Rukomoinikova <[email protected]> >> Signed-off-by: Aleksandr Smirnov <[email protected]> >> Signed-off-by: Alexandra Rukomoinikova <[email protected]> >> --- >> v4 --> v5: >> 1) Addressed Dumitru's comments: >> - Eliminated O(n) search by implementing uuid based lookup within ls_arp >> nodes >> - Retained ls_index in ls_arp_record structure for exclusive use in >> northd.c - It seems that this is the best option for searching for a datapass >> >> 2) Optimized ls_arp_record creation: >> - Previous approach: Iterated through all NATs for each ls_arp_record >> - New approach: Iterate through logical switch router ports to get >> required NAT records - i think it should reduce number of iterations >> >> 3) Restricted ls_arp_record creation scope: Records now created only for >> logical switches with physical ports - there shouldn't be many of them >> >> 4) Added ovn-northd documentation and ovn-northd tests for lflow >> functionality > Hi Alexandra, Aleksandr, > > Thanks for the new revision! Hi! Thanks for the review! I'll answer below >> --- >> lib/stopwatch-names.h | 1 + >> northd/automake.mk | 2 + >> northd/en-lflow.c | 29 ++++ >> northd/en-lflow.h | 2 + >> northd/en-ls-arp.c | 355 +++++++++++++++++++++++++++++++++++++++ >> northd/en-ls-arp.h | 86 ++++++++++ >> northd/inc-proc-northd.c | 8 + >> northd/northd.c | 191 +++++++++++++++++++-- >> northd/northd.h | 10 ++ >> northd/ovn-northd.8.xml | 16 ++ >> northd/ovn-northd.c | 1 + >> tests/ovn-northd.at | 79 ++++++++- >> tests/ovn.at | 153 +++++++++++++++++ >> 13 files changed, 918 insertions(+), 15 deletions(-) >> create mode 100644 northd/en-ls-arp.c >> create mode 100644 northd/en-ls-arp.h >> >> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h >> index f3a931c40..b912e813c 100644 >> --- a/lib/stopwatch-names.h >> +++ b/lib/stopwatch-names.h >> @@ -34,6 +34,7 @@ >> #define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run" >> #define LR_STATEFUL_RUN_STOPWATCH_NAME "lr_stateful" >> #define LS_STATEFUL_RUN_STOPWATCH_NAME "ls_stateful" >> +#define LS_ARP_RUN_STOPWATCH_NAME "ls_arp" >> #define ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME "advertised_route_sync" >> #define LEARNED_ROUTE_SYNC_RUN_STOPWATCH_NAME "learned_route_sync" >> #define DYNAMIC_ROUTES_RUN_STOPWATCH_NAME "dynamic_routes" >> diff --git a/northd/automake.mk b/northd/automake.mk >> index 45ca0337f..8cd4fb3a1 100644 >> --- a/northd/automake.mk >> +++ b/northd/automake.mk >> @@ -44,6 +44,8 @@ northd_ovn_northd_SOURCES = \ >> northd/en-lr-stateful.h \ >> northd/en-ls-stateful.c \ >> northd/en-ls-stateful.h \ >> + northd/en-ls-arp.c \ >> + northd/en-ls-arp.h \ >> northd/en-sampling-app.c \ >> northd/en-sampling-app.h \ >> northd/en-acl-ids.c \ >> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >> index 30bf7e35c..21a244a1b 100644 >> --- a/northd/en-lflow.c >> +++ b/northd/en-lflow.c >> @@ -23,6 +23,7 @@ >> #include "en-lr-nat.h" >> #include "en-lr-stateful.h" >> #include "en-ls-stateful.h" >> +#include "en-ls-arp.h" >> #include "en-multicast.h" >> #include "en-northd.h" >> #include "en-meters.h" >> @@ -62,6 +63,8 @@ lflow_get_input_data(struct engine_node *node, >> engine_get_input_data("lr_stateful", node); >> struct ed_type_ls_stateful *ls_stateful_data = >> engine_get_input_data("ls_stateful", node); >> + struct ed_type_ls_arp *ls_arp_data = >> + engine_get_input_data("ls_arp", node); >> struct multicast_igmp_data *multicat_igmp_data = >> engine_get_input_data("multicast_igmp", node); >> struct ic_learned_svc_monitors_data *ic_learned_svc_monitors_data = >> @@ -86,6 +89,7 @@ lflow_get_input_data(struct engine_node *node, >> lflow_input->ls_port_groups = &pg_data->ls_port_groups; >> lflow_input->lr_stateful_table = &lr_stateful_data->table; >> lflow_input->ls_stateful_table = &ls_stateful_data->table; >> + lflow_input->ls_arp_table = &ls_arp_data->table; >> lflow_input->meter_groups = &sync_meters_data->meter_groups; >> lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map; >> lflow_input->local_svc_monitors_map = >> @@ -229,6 +233,31 @@ lflow_ls_stateful_handler(struct engine_node *node, >> void *data) >> return EN_HANDLED_UPDATED; >> } >> >> +enum engine_input_handler_result >> +lflow_ls_arp_handler(struct engine_node *node, void *data) >> +{ >> + struct ed_type_ls_arp *ls_arp_data = >> + engine_get_input_data("ls_arp", node); >> + >> + if (!ls_arp_has_tracked_data(&ls_arp_data->trk_data)) { >> + return EN_UNHANDLED; >> + } >> + >> + const struct engine_context *eng_ctx = engine_get_context(); >> + struct lflow_data *lflow_data = data; >> + struct lflow_input lflow_input; >> + >> + lflow_get_input_data(node, &lflow_input); >> + if (!lflow_handle_ls_arp_changes(eng_ctx->ovnsb_idl_txn, >> + &ls_arp_data->trk_data, >> + &lflow_input, >> + lflow_data->lflow_table)) { >> + return EN_UNHANDLED; >> + } >> + >> + return EN_HANDLED_UPDATED; >> +} >> + >> enum engine_input_handler_result >> lflow_multicast_igmp_handler(struct engine_node *node, void *data) >> { >> diff --git a/northd/en-lflow.h b/northd/en-lflow.h >> index 99bcfda15..d2a92e49f 100644 >> --- a/northd/en-lflow.h >> +++ b/northd/en-lflow.h >> @@ -25,6 +25,8 @@ lflow_lr_stateful_handler(struct engine_node *, void >> *data); >> enum engine_input_handler_result >> lflow_ls_stateful_handler(struct engine_node *node, void *data); >> enum engine_input_handler_result >> +lflow_ls_arp_handler(struct engine_node *, void *); >> +enum engine_input_handler_result >> lflow_multicast_igmp_handler(struct engine_node *node, void *data); >> enum engine_input_handler_result >> lflow_group_ecmp_route_change_handler(struct engine_node *node, void >> *data); >> diff --git a/northd/en-ls-arp.c b/northd/en-ls-arp.c >> new file mode 100644 >> index 000000000..69d76ef3e >> --- /dev/null >> +++ b/northd/en-ls-arp.c >> @@ -0,0 +1,355 @@ >> +/* >> + * 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> >> + >> +/* OVS includes */ >> +#include "include/openvswitch/hmap.h" >> +#include "openvswitch/util.h" >> +#include "openvswitch/vlog.h" >> +#include "stopwatch.h" >> + >> +/* OVN includes */ >> +#include "en-lr-nat.h" >> +#include "en-ls-arp.h" >> +#include "lib/inc-proc-eng.h" >> +#include "lib/ovn-nb-idl.h" >> +#include "lib/ovn-sb-idl.h" >> +#include "lib/ovn-util.h" >> +#include "lib/stopwatch-names.h" >> +#include "lflow-mgr.h" >> +#include "northd.h" >> + >> +VLOG_DEFINE_THIS_MODULE(en_ls_arp); >> + >> +/* Static functions. */ >> +struct ls_arp_input { >> + const struct ovn_datapaths *ls_datapaths; >> + const struct lr_nat_table *lr_nats; >> +}; >> + >> +static struct ls_arp_input >> +ls_arp_get_input_data(struct engine_node *node) >> +{ >> + const struct northd_data *northd_data = >> + engine_get_input_data("northd", node); >> + struct ed_type_lr_nat_data *lr_nat_data = >> + engine_get_input_data("lr_nat", node); >> + >> + return (struct ls_arp_input) { >> + .ls_datapaths = &northd_data->ls_datapaths, >> + .lr_nats = &lr_nat_data->lr_nats, >> + }; >> +} >> + >> +static void >> +ls_arp_record_clear(struct ls_arp_record *ls_arp_record) >> +{ >> + lflow_ref_destroy(ls_arp_record->lflow_ref); >> + hmapx_destroy(&ls_arp_record->nat_records); >> + free(ls_arp_record); >> +} >> + >> +static void >> +ls_arp_table_clear(struct ls_arp_table *table) >> +{ >> + struct ls_arp_record *ls_arp_record; >> + HMAP_FOR_EACH_POP (ls_arp_record, key_node, &table->entries) { >> + ls_arp_record_clear(ls_arp_record); >> + } >> +} >> + >> +static inline bool >> +is_centralized_nat_record(const struct ovn_nat *nat_entry) >> +{ >> + return nat_entry->is_valid >> + && nat_entry->l3dgw_port >> + && nat_entry->l3dgw_port->peer >> + && nat_entry->l3dgw_port->peer->od >> + && !nat_entry->is_distributed; >> +} >> + >> +static void >> +nat_record_data_create(struct ls_arp_record *ls_arp_record, >> + const struct ovn_datapath *od, >> + const struct lr_nat_table *lr_nats) >> +{ >> + struct ovn_port *op; >> + VECTOR_FOR_EACH (&od->router_ports, op) { >> + const struct ovn_datapath *lr_od = op->peer->od; >> + const struct lr_nat_record *lrnat_rec = >> + lr_nat_table_find_by_uuid(lr_nats, lr_od->key); >> + >> + if (!lrnat_rec) { >> + continue; >> + } >> + >> + for (size_t i = 0; i < lrnat_rec->n_nat_entries; i++) { >> + const struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i]; >> + >> + if (is_centralized_nat_record(nat_entry)) { >> + hmapx_add(&ls_arp_record->nat_records, >> + (struct lrnat_rec *) lrnat_rec); > Nit: indentation. > >> + } >> + } >> + } >> +} >> + >> +static struct ls_arp_record* > Nit: missing space before '*'. > >> +ls_arp_record_lookup_by_od_(const struct ls_arp_table *table, >> + const struct ovn_datapath *od) >> +{ >> + struct ls_arp_record *ls_arp_record; >> + HMAP_FOR_EACH_WITH_HASH (ls_arp_record, key_node, >> + uuid_hash(&od->nbs->header_.uuid), >> + &table->entries) { >> + if (uuid_equals(&ls_arp_record->nbs_uuid, >> + &od->nbs->header_.uuid)) { >> + return ls_arp_record; >> + } >> + } >> + >> + return NULL; >> +} >> + >> +static struct ls_arp_record * >> +ls_arp_record_create(struct ls_arp_table *table, >> + const struct ovn_datapath *od, >> + const struct lr_nat_table *lr_nats) >> +{ >> + struct ls_arp_record *ls_arp_record = xzalloc(sizeof *ls_arp_record); >> + >> + ls_arp_record->ls_index = od->index; >> + ls_arp_record->nbs_uuid = od->nbs->header_.uuid; >> + >> + hmapx_init(&ls_arp_record->nat_records); >> + nat_record_data_create(ls_arp_record, od, lr_nats); >> + >> + ls_arp_record->lflow_ref = lflow_ref_create(); >> + >> + hmap_insert(&table->entries, &ls_arp_record->key_node, >> + uuid_hash(&od->nbs->header_.uuid)); >> + >> + return ls_arp_record; >> +} >> + >> +/* Public functions. */ >> +void* >> +en_ls_arp_init(struct engine_node *node OVS_UNUSED, >> + struct engine_arg *arg OVS_UNUSED) >> +{ >> + struct ed_type_ls_arp *data = xzalloc(sizeof *data); >> + >> + hmap_init(&data->table.entries); >> + hmapx_init(&data->trk_data.crupdated); >> + hmapx_init(&data->trk_data.deleted); >> + >> + return data; >> +} >> + >> +void >> +en_ls_arp_clear_tracked_data(void *data_) >> +{ >> + struct ed_type_ls_arp *data = data_; >> + hmapx_clear(&data->trk_data.crupdated); >> + >> + struct hmapx_node *n; >> + HMAPX_FOR_EACH_SAFE (n, &data->trk_data.deleted) { >> + ls_arp_record_clear(n->data); >> + hmapx_delete(&data->trk_data.deleted, n); >> + } >> + hmapx_clear(&data->trk_data.deleted); >> +} >> + >> +void >> +en_ls_arp_cleanup(void *data_) >> +{ >> + struct ed_type_ls_arp *data = data_; >> + >> + ls_arp_table_clear(&data->table); >> + hmap_destroy(&data->table.entries); >> + hmapx_destroy(&data->trk_data.crupdated); >> + >> + struct hmapx_node *n; >> + HMAPX_FOR_EACH_SAFE (n, &data->trk_data.deleted) { >> + ls_arp_record_clear(n->data); >> + hmapx_delete(&data->trk_data.deleted, n); >> + } >> + hmapx_destroy(&data->trk_data.deleted); >> +} >> + >> +enum engine_node_state >> +en_ls_arp_run(struct engine_node *node, void *data_) >> +{ >> + struct ls_arp_input input_data = ls_arp_get_input_data(node); >> + struct ed_type_ls_arp *data = data_; >> + >> + stopwatch_start(LS_ARP_RUN_STOPWATCH_NAME, time_msec()); >> + >> + ls_arp_table_clear(&data->table); >> + >> + const struct ovn_datapath *od; >> + HMAP_FOR_EACH (od, key_node, &input_data.ls_datapaths->datapaths) { >> + /* Filtering ARP entries at logical switch works >> + * when there are physical ports on the switch. */ > Nit: indendation. > >> + if (hmapx_is_empty(&od->ph_ports)) { >> + continue; >> + } >> + >> + ls_arp_record_create(&data->table, od, input_data.lr_nats); >> + } >> + >> + stopwatch_stop(LS_ARP_RUN_STOPWATCH_NAME, time_msec()); >> + >> + return EN_UPDATED; >> +} >> + >> +/* Handler functions. */ >> +enum engine_input_handler_result >> +ls_arp_northd_handler(struct engine_node *node, void *data_) >> +{ >> + struct northd_data *northd_data = engine_get_input_data("northd", node); >> + if (!northd_has_tracked_data(&northd_data->trk_data)) { >> + return EN_UNHANDLED; >> + } >> + >> + if (!northd_has_lswitches_in_tracked_data(&northd_data->trk_data)) { >> + return EN_HANDLED_UNCHANGED; >> + } >> + >> + struct northd_tracked_data *nd_changes = &northd_data->trk_data; >> + struct ls_arp_input input_data = ls_arp_get_input_data(node); >> + struct ed_type_ls_arp *data = data_; >> + struct hmapx_node *hmapx_node; >> + struct ls_arp_record *ls_arp_record; >> + >> + HMAPX_FOR_EACH (hmapx_node, &nd_changes->trk_switches.crupdated) { >> + const struct ovn_datapath *od = hmapx_node->data; >> + >> + ls_arp_record = ls_arp_record_lookup_by_od_(&data->table, od); >> + >> + if (!ls_arp_record) { >> + /* Filtering ARP entries at logical switch works >> + * when there are physical ports on the switch. */ >> + if (hmapx_is_empty(&od->ph_ports)) { >> + continue; > This continue here made me spend quite some time looking at the code and > wondering why it works fine for the case when a switch used to have a > localnet/l2gw port but that got removed in the current iteration. > > It turns out we then cause a lr_nat node recompute (because the northd > node didn't have any NAT tracked data) which in turn causes the > ls_arp_lr_nat_handler() below to fail incremental processing. > > I think it would be good to have a note about this and also to update > the ovn-northd.at test to cover this scenario. > >> + } >> + ls_arp_record = ls_arp_record_create(&data->table, >> + od, input_data.lr_nats); >> + } else { >> + nat_record_data_create(ls_arp_record, od, input_data.lr_nats); >> + } >> + >> + hmapx_add(&data->trk_data.crupdated, ls_arp_record); >> + } >> + >> + HMAPX_FOR_EACH (hmapx_node, &nd_changes->trk_switches.deleted) { >> + const struct ovn_datapath *od = hmapx_node->data; >> + >> + ls_arp_record = ls_arp_record_lookup_by_od_(&data->table, od); >> + if (ls_arp_record) { >> + hmap_remove(&data->table.entries, &ls_arp_record->key_node); >> + hmapx_add(&data->trk_data.deleted, ls_arp_record); >> + } >> + } >> + >> + if (ls_arp_has_tracked_data(&data->trk_data)) { >> + return EN_HANDLED_UPDATED; >> + } >> + >> + return EN_HANDLED_UNCHANGED; >> +} >> + >> +static void >> +nat_odmap_create(struct lr_nat_record *lrnat_rec, >> + struct hmapx *odmap) >> +{ >> + for (size_t i = 0; i < lrnat_rec->n_nat_entries; i++) { >> + const struct ovn_nat *nat_entry = &lrnat_rec->nat_entries[i]; >> + >> + if (is_centralized_nat_record(nat_entry)) { >> + hmapx_add(odmap, nat_entry->l3dgw_port->peer->od); >> + } >> + } >> +} >> + >> +enum engine_input_handler_result >> +ls_arp_lr_nat_handler(struct engine_node *node, void *data_) >> +{ >> + struct ed_type_lr_nat_data *lr_nat_data = >> + engine_get_input_data("lr_nat", node); >> + struct ls_arp_input input_data = ls_arp_get_input_data(node); >> + >> + if (!lr_nat_has_tracked_data(&lr_nat_data->trk_data)) { >> + return EN_UNHANDLED; >> + } >> + >> + struct ed_type_ls_arp *data = data_; >> + >> + struct hmapx_node *hmapx_node; >> + struct ls_arp_record *ls_arp_record; >> + HMAPX_FOR_EACH (hmapx_node, &lr_nat_data->trk_data.crupdated) { >> + struct lr_nat_record *nat_record_p = hmapx_node->data; >> + >> + struct hmapx ls_links_map = HMAPX_INITIALIZER(&ls_links_map); >> + nat_odmap_create(nat_record_p, &ls_links_map); >> + >> + LS_ARP_TABLE_FOR_EACH (ls_arp_record, &data->table) { >> + struct hmapx_node *nr_node = >> + hmapx_find(&ls_arp_record->nat_records, nat_record_p); >> + >> + if (nr_node) { >> + hmapx_add(&data->trk_data.crupdated, ls_arp_record); >> + hmapx_delete(&ls_arp_record->nat_records, nr_node); >> + } >> + } >> + >> + struct hmapx_node *crupdated_ls_hmapx; >> + HMAPX_FOR_EACH (crupdated_ls_hmapx, &ls_links_map) { >> + struct ovn_datapath *crupdated_ls = crupdated_ls_hmapx->data; >> + ls_arp_record = >> + ls_arp_record_lookup_by_od_(&data->table, crupdated_ls); >> + >> + if (!ls_arp_record) { >> + ls_arp_record = ls_arp_record_create(&data->table, >> + crupdated_ls, >> + input_data.lr_nats); >> + } >> + >> + hmapx_add(&data->trk_data.crupdated, ls_arp_record); >> + hmapx_add(&ls_arp_record->nat_records, nat_record_p); >> + } >> + hmapx_destroy(&ls_links_map); >> + } >> + >> + HMAPX_FOR_EACH (hmapx_node, &lr_nat_data->trk_data.deleted) { >> + struct lr_nat_record *nr_cur = hmapx_node->data; >> + >> + struct ls_arp_record *ar; >> + LS_ARP_TABLE_FOR_EACH (ar, &data->table) { >> + struct hmapx_node *nr_node = hmapx_find(&ar->nat_records, >> nr_cur); >> + >> + if (nr_node) { >> + hmapx_add(&data->trk_data.crupdated, ar); >> + hmapx_delete(&ar->nat_records, nr_node); >> + } >> + } >> + } >> + >> + if (ls_arp_has_tracked_data(&data->trk_data)) { >> + return EN_HANDLED_UPDATED; >> + } >> + >> + return EN_HANDLED_UNCHANGED; >> +} >> diff --git a/northd/en-ls-arp.h b/northd/en-ls-arp.h >> new file mode 100644 >> index 000000000..5eaf913bb >> --- /dev/null >> +++ b/northd/en-ls-arp.h >> @@ -0,0 +1,86 @@ >> +/* >> + * 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_LS_ARP_H >> +#define EN_LS_ARP_H 1 >> + >> +/* OVS includes. */ >> +#include "lib/hmapx.h" >> +#include "openvswitch/hmap.h" >> + >> +/* OVN includes. */ >> +#include "lib/inc-proc-eng.h" >> +#include "lib/ovn-nb-idl.h" >> +#include "lib/ovn-sb-idl.h" >> +#include "lib/ovn-util.h" >> +#include "lib/stopwatch-names.h" >> + >> +struct lflow_ref; >> + >> +struct ls_arp_record { >> + struct hmap_node key_node; >> + >> + /* UUID of the NB Logical switch. */ >> + struct uuid nbs_uuid; >> + >> + /* Index of logical switch item in northd. */ >> + size_t ls_index; >> + >> + /* 'lflow_ref' is used to reference logical flows generated for >> + * this ls_arp record. */ >> + struct lflow_ref *lflow_ref; >> + >> + /* lr_nat_record ptrs that trigger this od to rebuild lflow. */ >> + struct hmapx nat_records; >> +}; >> + >> +struct ls_arp_table { >> + struct hmap entries; >> +}; >> + >> +#define LS_ARP_TABLE_FOR_EACH(LS_ARP_REC, TABLE) \ >> + HMAP_FOR_EACH (LS_ARP_REC, key_node, \ >> + &(TABLE)->entries) >> + >> +#define LS_ARP_TABLE_FOR_EACH_IN_P(LS_ARP_REC, JOBID, TABLE) \ >> + HMAP_FOR_EACH_IN_PARALLEL (LS_ARP_REC, key_node, JOBID, \ >> + &(TABLE)->entries) >> + >> +struct ls_arp_tracked_data { >> + struct hmapx crupdated; >> + struct hmapx deleted; >> +}; >> + >> +struct ed_type_ls_arp { >> + struct ls_arp_table table; >> + struct ls_arp_tracked_data trk_data; >> +}; >> + >> +void *en_ls_arp_init(struct engine_node *, struct engine_arg *); >> +void en_ls_arp_cleanup(void *); >> +void en_ls_arp_clear_tracked_data(void *); >> +enum engine_node_state en_ls_arp_run(struct engine_node *, void *); >> + >> +enum engine_input_handler_result >> +ls_arp_lr_nat_handler(struct engine_node *, void *); >> +enum engine_input_handler_result >> +ls_arp_northd_handler(struct engine_node *, void *); >> + >> +static inline bool >> +ls_arp_has_tracked_data(struct ls_arp_tracked_data *trk_data) { >> + return !hmapx_is_empty(&trk_data->crupdated) || >> + !hmapx_is_empty(&trk_data->deleted); >> +} >> + >> +#endif /* EN_LS_ARP_H */ >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >> index 94199de12..748e09769 100644 >> --- a/northd/inc-proc-northd.c >> +++ b/northd/inc-proc-northd.c >> @@ -34,6 +34,7 @@ >> #include "en-lr-stateful.h" >> #include "en-lr-nat.h" >> #include "en-ls-stateful.h" >> +#include "en-ls-arp.h" >> #include "en-multicast.h" >> #include "en-northd.h" >> #include "en-lflow.h" >> @@ -174,6 +175,7 @@ static ENGINE_NODE(lb_data, CLEAR_TRACKED_DATA); >> static ENGINE_NODE(lr_nat, CLEAR_TRACKED_DATA); >> static ENGINE_NODE(lr_stateful, CLEAR_TRACKED_DATA); >> static ENGINE_NODE(ls_stateful, CLEAR_TRACKED_DATA); >> +static ENGINE_NODE(ls_arp, CLEAR_TRACKED_DATA); >> static ENGINE_NODE(route_policies); >> static ENGINE_NODE(routes); >> static ENGINE_NODE(bfd); >> @@ -305,6 +307,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> ls_stateful_port_group_handler); >> engine_add_input(&en_ls_stateful, &en_nb_acl, ls_stateful_acl_handler); >> >> + engine_add_input(&en_ls_arp, &en_lr_nat, ls_arp_lr_nat_handler); >> + engine_add_input(&en_ls_arp, &en_northd, ls_arp_northd_handler); >> + >> engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL); >> engine_add_input(&en_mac_binding_aging, &en_northd, NULL); >> engine_add_input(&en_mac_binding_aging, &en_mac_binding_aging_waker, >> NULL); >> @@ -409,6 +414,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_lflow, &en_port_group, engine_noop_handler); >> engine_add_input(&en_lflow, &en_lr_stateful, >> lflow_lr_stateful_handler); >> engine_add_input(&en_lflow, &en_ls_stateful, >> lflow_ls_stateful_handler); >> + >> + engine_add_input(&en_lflow, &en_ls_arp, lflow_ls_arp_handler); >> + > Nit: I would remove the newlines before and after this one. > >> engine_add_input(&en_lflow, &en_multicast_igmp, >> lflow_multicast_igmp_handler); >> engine_add_input(&en_lflow, &en_sb_acl_id, NULL); >> diff --git a/northd/northd.c b/northd/northd.c >> index e978b66c2..a0db6f226 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -50,6 +50,7 @@ >> #include "en-lr-nat.h" >> #include "en-lr-stateful.h" >> #include "en-ls-stateful.h" >> +#include "en-ls-arp.h" >> #include "en-multicast.h" >> #include "en-sampling-app.h" >> #include "en-datapath-logical-switch.h" >> @@ -136,6 +137,7 @@ static bool vxlan_mode; >> #define REGBIT_IP_FRAG "reg0[19]" >> #define REGBIT_ACL_PERSIST_ID "reg0[20]" >> #define REGBIT_ACL_HINT_ALLOW_PERSISTED "reg0[21]" >> +#define REGBIT_EXT_ARP "reg0[22]" >> >> /* Register definitions for switches and routers. */ >> >> @@ -547,6 +549,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct >> uuid *key, >> hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key)); >> od->lr_group = NULL; >> hmap_init(&od->ports); >> + hmapx_init(&od->ph_ports); >> sset_init(&od->router_ips); >> od->ls_peers = VECTOR_EMPTY_INITIALIZER(struct ovn_datapath *); >> od->router_ports = VECTOR_EMPTY_INITIALIZER(struct ovn_port *); >> @@ -584,6 +587,7 @@ ovn_datapath_destroy(struct ovn_datapath *od) >> vector_destroy(&od->l3dgw_ports); >> destroy_mcast_info_for_datapath(od); >> destroy_ports_for_datapath(od); >> + hmapx_destroy(&od->ph_ports); >> sset_destroy(&od->router_ips); >> lflow_ref_destroy(od->datapath_lflows); >> free(od); >> @@ -1218,6 +1222,12 @@ lsp_is_vtep(const struct nbrec_logical_switch_port >> *nbsp) >> return !strcmp(nbsp->type, "vtep"); >> } >> >> +static bool >> +lsp_is_l2gw(const struct nbrec_logical_switch_port *nbsp) >> +{ >> + return !strcmp(nbsp->type, "l2gateway"); >> +} >> + >> static bool >> localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp) >> { >> @@ -1604,6 +1614,10 @@ join_logical_ports_lsp(struct hmap *ports, >> od->has_vtep_lports = true; >> } >> >> + if (lsp_is_localnet(nbsp) || lsp_is_l2gw(nbsp)) { >> + hmapx_add(&od->ph_ports, op); >> + } >> + >> parse_lsp_addrs(op); >> >> op->od = od; >> @@ -1997,6 +2011,15 @@ join_logical_ports(const struct >> sbrec_port_binding_table *sbrec_pb_table, >> } >> } >> >> +static bool >> +is_nat_distributed(const struct nbrec_nat *nat, >> + const struct ovn_datapath *od) >> +{ >> + return !vector_is_empty(&od->l3dgw_ports) >> + && nat->logical_port && nat->external_mac >> + && !strcmp(nat->type, "dnat_and_snat"); >> +} >> + >> /* Returns an array of strings, each consisting of a MAC address followed >> * by one or more IP addresses, and if the port is a distributed gateway >> * port, followed by 'is_chassis_resident("LPORT_NAME")', where the >> @@ -2055,9 +2078,7 @@ get_nat_addresses(const struct ovn_port *op, size_t >> *n, bool routable_only, >> >> /* Determine whether this NAT rule satisfies the conditions for >> * distributed NAT processing. */ >> - if (!vector_is_empty(&op->od->l3dgw_ports) && >> - !strcmp(nat->type, "dnat_and_snat") && >> - nat->logical_port && nat->external_mac) { >> + if (is_nat_distributed(nat, op->od)) { >> /* Distributed NAT rule. */ >> if (eth_addr_from_string(nat->external_mac, &mac)) { >> struct ds address = DS_EMPTY_INITIALIZER; >> @@ -9676,6 +9697,99 @@ >> build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op, >> ds_destroy(&match); >> } >> >> +/* >> + * Create ARP filtering flow for od, assumed logical switch, >> + * for the following condition: >> + * Given lswitch has either localnet or l2gateway ports and >> + * router connection ports that requires chassis residence. >> + * ARP requests coming from localnet/l2gateway ports >> + * allowed for processing on resident chassis only. >> + */ >> +static void >> +build_lswitch_arp_chassis_resident(const struct ovn_datapath *od, >> + struct lflow_table *lflows, >> + const struct ls_arp_record *ar) >> +{ >> + struct sset inports; >> + struct sset resident_ports; >> + struct sset distributed_nat_ports; > Nit: reverse xmas tree and we can use SSET_INITIALIZER() directly. > >> + >> + sset_init(&inports); >> + sset_init(&resident_ports); >> + sset_init(&distributed_nat_ports); >> + >> + struct ds match = DS_EMPTY_INITIALIZER; >> + >> + struct hmapx_node *node; >> + HMAPX_FOR_EACH (node, &od->ph_ports) { >> + struct ovn_port *op = node->data; >> + sset_add(&inports, op->json_key); >> + } >> + >> + struct ovn_port *op; >> + VECTOR_FOR_EACH (&od->router_ports, op) { >> + struct ovn_port *op_r = op->peer; >> + >> + if (lrp_is_l3dgw(op_r)) { >> + sset_add(&resident_ports, op_r->cr_port->json_key); >> + } >> + } >> + >> + struct hmapx_node *hmapx_node; >> + HMAPX_FOR_EACH (hmapx_node, &ar->nat_records) { >> + struct lr_nat_record *nr = hmapx_node->data; >> + >> + for (size_t i = 0; i < nr->n_nat_entries; i++) { >> + struct ovn_nat *ent = &nr->nat_entries[i]; >> + if (ent->is_valid && ent->is_distributed) { >> + sset_add(&distributed_nat_ports, ent->nb->logical_port); >> + } >> + } >> + } >> + >> + >> + if (!sset_is_empty(&inports) && !sset_is_empty(&resident_ports)) { >> + const char *item; >> + >> + SSET_FOR_EACH (item, &inports) { >> + ds_clear(&match); >> + ds_put_format(&match, >> + "(arp.op == 1 || arp.op == 2) && inport == %s", >> + item); >> + ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 75, >> + ds_cstr(&match), REGBIT_EXT_ARP " = 1; next;", >> + ar->lflow_ref); >> + } >> + >> + SSET_FOR_EACH (item, &resident_ports) { >> + ds_clear(&match); >> + ds_put_format(&match, >> + REGBIT_EXT_ARP" == 1 && is_chassis_resident(%s)", >> + item); >> + ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 75, >> + ds_cstr(&match), "next;", ar->lflow_ref); >> + } >> + >> + SSET_FOR_EACH (item, &distributed_nat_ports) { >> + ds_clear(&match); >> + ds_put_format(&match, >> + REGBIT_EXT_ARP >> + " == 1 && is_chassis_resident(\"%s\")", >> + item); >> + ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 75, >> + ds_cstr(&match), "next;", ar->lflow_ref); >> + } >> + >> + ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 70, >> + REGBIT_EXT_ARP" == 1", "drop;", ar->lflow_ref); >> + } >> + >> + ds_destroy(&match); >> + sset_destroy(&inports); >> + sset_destroy(&resident_ports); >> + sset_destroy(&distributed_nat_ports); >> +} >> + >> static bool >> is_vlan_transparent(const struct ovn_datapath *od) >> { >> @@ -14117,10 +14231,6 @@ build_neigh_learning_flows_for_lrouter_port( >> op->lrp_networks.ipv4_addrs[i].network_s, >> op->lrp_networks.ipv4_addrs[i].plen, >> op->lrp_networks.ipv4_addrs[i].addr_s); >> - if (lrp_is_l3dgw(op)) { >> - ds_put_format(match, " && is_chassis_resident(%s)", >> - op->cr_port->json_key); >> - } >> const char *actions_s = REGBIT_LOOKUP_NEIGHBOR_RESULT >> " = lookup_arp(inport, arp.spa, arp.sha); " >> REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1;" >> @@ -14137,10 +14247,6 @@ build_neigh_learning_flows_for_lrouter_port( >> op->json_key, >> op->lrp_networks.ipv4_addrs[i].network_s, >> op->lrp_networks.ipv4_addrs[i].plen); >> - if (lrp_is_l3dgw(op)) { >> - ds_put_format(match, " && is_chassis_resident(%s)", >> - op->cr_port->json_key); >> - } >> ds_clear(actions); >> ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT >> " = lookup_arp(inport, arp.spa, arp.sha); %snext;", >> @@ -18552,6 +18658,7 @@ struct lswitch_flow_build_info { >> const struct ls_port_group_table *ls_port_groups; >> const struct lr_stateful_table *lr_stateful_table; >> const struct ls_stateful_table *ls_stateful_table; >> + const struct ls_arp_table *ls_arp_table; >> struct lflow_table *lflows; >> const struct shash *meter_groups; >> const struct hmap *lb_dps_map; >> @@ -18744,6 +18851,7 @@ build_lflows_thread(void *arg) >> struct worker_control *control = (struct worker_control *) arg; >> const struct lr_stateful_record *lr_stateful_rec; >> const struct ls_stateful_record *ls_stateful_rec; >> + const struct ls_arp_record *ls_arp_rec; >> struct lswitch_flow_build_info *lsi; >> struct ovn_lb_datapaths *lb_dps; >> struct ovn_datapath *od; >> @@ -18900,6 +19008,19 @@ build_lflows_thread(void *arg) >> } >> } >> >> + for (bnum = control->id; >> + bnum <= lsi->ls_arp_table->entries.mask; >> + bnum += control->pool->size) >> + { >> + LS_ARP_TABLE_FOR_EACH_IN_P (ls_arp_rec, bnum, >> + lsi->ls_arp_table) { >> + od = ovn_datapaths_find_by_index( >> + lsi->ls_datapaths, ls_arp_rec->ls_index); >> + build_lswitch_arp_chassis_resident(od, lsi->lflows, >> + ls_arp_rec); >> + } >> + } >> + >> lsi->thread_lflow_counter = thread_lflow_counter; >> } >> post_completed_work(control); >> @@ -18948,6 +19069,7 @@ build_lswitch_and_lrouter_flows( >> const struct ls_port_group_table *ls_pgs, >> const struct lr_stateful_table *lr_stateful_table, >> const struct ls_stateful_table *ls_stateful_table, >> + const struct ls_arp_table *ls_arp_table, >> struct lflow_table *lflows, >> const struct shash *meter_groups, >> const struct hmap *lb_dps_map, >> @@ -18984,6 +19106,7 @@ build_lswitch_and_lrouter_flows( >> lsiv[index].ls_port_groups = ls_pgs; >> lsiv[index].lr_stateful_table = lr_stateful_table; >> lsiv[index].ls_stateful_table = ls_stateful_table; >> + lsiv[index].ls_arp_table = ls_arp_table; >> lsiv[index].meter_groups = meter_groups; >> lsiv[index].lb_dps_map = lb_dps_map; >> lsiv[index].local_svc_monitor_map = >> @@ -19020,6 +19143,7 @@ build_lswitch_and_lrouter_flows( >> } else { >> const struct lr_stateful_record *lr_stateful_rec; >> const struct ls_stateful_record *ls_stateful_rec; >> + const struct ls_arp_record *ls_arp_rec; >> struct ovn_lb_datapaths *lb_dps; >> struct ovn_datapath *od; >> struct ovn_port *op; >> @@ -19032,6 +19156,7 @@ build_lswitch_and_lrouter_flows( >> .ls_port_groups = ls_pgs, >> .lr_stateful_table = lr_stateful_table, >> .ls_stateful_table = ls_stateful_table, >> + .ls_arp_table = ls_arp_table, >> .lflows = lflows, >> .meter_groups = meter_groups, >> .lb_dps_map = lb_dps_map, >> @@ -19127,6 +19252,12 @@ build_lswitch_and_lrouter_flows( >> lsi.sbrec_acl_id_table); >> } >> >> + LS_ARP_TABLE_FOR_EACH (ls_arp_rec, ls_arp_table) { >> + od = ovn_datapaths_find_by_index(lsi.ls_datapaths, >> + ls_arp_rec->ls_index); >> + build_lswitch_arp_chassis_resident(od, lsi.lflows, ls_arp_rec); >> + } >> + >> ds_destroy(&lsi.match); >> ds_destroy(&lsi.actions); >> } >> @@ -19210,6 +19341,7 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, >> input_data->ls_port_groups, >> input_data->lr_stateful_table, >> input_data->ls_stateful_table, >> + input_data->ls_arp_table, >> lflows, >> input_data->meter_groups, >> input_data->lb_datapaths_map, >> @@ -19712,6 +19844,43 @@ lflow_handle_ls_stateful_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> return true; >> } >> >> +bool >> +lflow_handle_ls_arp_changes(struct ovsdb_idl_txn *ovnsb_txn, >> + struct ls_arp_tracked_data *trk_data, >> + struct lflow_input *lflow_input, >> + struct lflow_table *lflows) >> +{ >> + struct hmapx_node *hmapx_node; >> + >> + HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { >> + const struct ls_arp_record *ls_arp_record = hmapx_node->data; >> + const struct ovn_datapath *od = >> + ovn_datapaths_find_by_index(lflow_input->ls_datapaths, >> + ls_arp_record->ls_index); >> + lflow_ref_unlink_lflows(ls_arp_record->lflow_ref); >> + >> + build_lswitch_arp_chassis_resident(od, lflows, ls_arp_record); >> + >> + bool handled = lflow_ref_sync_lflows( >> + ls_arp_record->lflow_ref, lflows, ovnsb_txn, >> + lflow_input->ls_datapaths, >> + lflow_input->lr_datapaths, >> + lflow_input->ovn_internal_version_changed, >> + lflow_input->sbrec_logical_flow_table, >> + lflow_input->sbrec_logical_dp_group_table); >> + if (!handled) { >> + return false; >> + } >> + } >> + >> + HMAPX_FOR_EACH (hmapx_node, &trk_data->deleted) { >> + struct ls_arp_record *ls_arp_record = hmapx_node->data; >> + lflow_ref_unlink_lflows(ls_arp_record->lflow_ref); >> + } >> + >> + return true; >> +} >> + >> static bool >> mirror_needs_update(const struct nbrec_mirror *nb_mirror, >> const struct sbrec_mirror *sb_mirror) >> diff --git a/northd/northd.h b/northd/northd.h >> index 32134d36e..c41c41e00 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -20,6 +20,7 @@ >> #include "lib/ovn-util.h" >> #include "lib/ovs-atomic.h" >> #include "lib/sset.h" >> +#include "lib/hmapx.h" >> #include "northd/en-port-group.h" >> #include "northd/ipam.h" >> #include "northd/lb.h" >> @@ -27,6 +28,7 @@ >> #include "simap.h" >> #include "ovs-thread.h" >> #include "en-lr-stateful.h" >> +#include "en-ls-arp.h" >> #include "vec.h" >> #include "datapath-sync.h" >> >> @@ -272,6 +274,7 @@ struct lflow_input { >> const struct ls_port_group_table *ls_port_groups; >> const struct lr_stateful_table *lr_stateful_table; >> const struct ls_stateful_table *ls_stateful_table; >> + const struct ls_arp_table *ls_arp_table; >> const struct shash *meter_groups; >> const struct hmap *lb_datapaths_map; >> const struct sset *bfd_ports; >> @@ -470,6 +473,9 @@ struct ovn_datapath { >> * Valid only if it is logical router datapath. NULL otherwise. */ >> struct lrouter_group *lr_group; >> >> + /* Set of localnet or l2gw ports. */ >> + struct hmapx ph_ports; > Nit: I think a more explicit name is a bit better, e.g., "phys_ports". > >> + >> /* Map of ovn_port objects belonging to this datapath. >> * This map doesn't include derived ports. */ >> struct hmap ports; >> @@ -957,6 +963,10 @@ bool lflow_handle_ls_stateful_changes(struct >> ovsdb_idl_txn *, >> struct ls_stateful_tracked_data *, >> struct lflow_input *, >> struct lflow_table *lflows); >> +bool lflow_handle_ls_arp_changes(struct ovsdb_idl_txn *, >> + struct ls_arp_tracked_data *, >> + struct lflow_input *, >> + struct lflow_table *lflows); >> bool northd_handle_sb_port_binding_changes( >> const struct sbrec_port_binding_table *, struct hmap *ls_ports, >> struct hmap *lr_ports); >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >> index 005fd87d1..0f6693b2f 100644 >> --- a/northd/ovn-northd.8.xml >> +++ b/northd/ovn-northd.8.xml >> @@ -310,6 +310,15 @@ >> </p> >> </li> >> >> + <li> >> + For each logical switch that has connected physical ports >> + (localnet or l2gateway) and is also connected to a distributed >> router, >> + filtering rules are added for ARP requests coming from localnet or >> + l2gateway ports, allowed for processing on gateway chassis. >> + The <code>REGBIT_EXT_ARP</code> register is set for all ARP requests >> + originating from physical ports with priority 75 flow. >> + </li> >> + >> <li> >> For each (enabled) vtep logical port, a priority 70 flow is added >> which >> matches on all packets and applies the action >> @@ -396,6 +405,13 @@ >> One priority-0 fallback flow that matches all packets and advances >> to >> the next table. >> </li> >> + >> + <li> >> + Priority 75: Allows <code>REGBIT_EXT_ARP</code> packets only on >> gateway >> + chassis and chassis with distributed NAT entries. >> + Priority 70: Drops <code>REGBIT_EXT_ARP</code> packets on >> non-gateway >> + chassis (complements the priority 75 flow). >> + </li> >> </ul> >> >> <h3>Ingress Table 2: Mirror </h3> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index 52a3c7883..25677d73e 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -1016,6 +1016,7 @@ main(int argc, char *argv[]) >> stopwatch_create(LR_NAT_RUN_STOPWATCH_NAME, SW_MS); >> stopwatch_create(LR_STATEFUL_RUN_STOPWATCH_NAME, SW_MS); >> stopwatch_create(LS_STATEFUL_RUN_STOPWATCH_NAME, SW_MS); >> + stopwatch_create(LS_ARP_RUN_STOPWATCH_NAME, SW_MS); >> stopwatch_create(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME, SW_MS); >> stopwatch_create(LEARNED_ROUTE_SYNC_RUN_STOPWATCH_NAME, SW_MS); >> stopwatch_create(DYNAMIC_ROUTES_RUN_STOPWATCH_NAME, SW_MS); >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index f7ec6a33a..1533405b2 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -7335,9 +7335,6 @@ AT_CHECK([grep lr_in_admission lrflows | grep cr-DR | >> ovn_strip_lflows], [0], [d >> ]) >> # Check the flows in lr_in_lookup_neighbor stage >> AT_CHECK([grep lr_in_lookup_neighbor lrflows | grep cr-DR | >> ovn_strip_lflows], [0], [dnl >> - table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S1" >> && arp.spa == 172.16.1.0/24 && arp.op == 1 && >> is_chassis_resident("cr-DR-S1")), action=(reg9[[2]] = lookup_arp(inport, >> arp.spa, arp.sha); next;) >> - table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S2" >> && arp.spa == 172.16.2.0/24 && arp.op == 1 && >> is_chassis_resident("cr-DR-S2")), action=(reg9[[2]] = lookup_arp(inport, >> arp.spa, arp.sha); next;) >> - table=??(lr_in_lookup_neighbor), priority=100 , match=(inport == "DR-S3" >> && arp.spa == 172.16.3.0/24 && arp.op == 1 && >> is_chassis_resident("cr-DR-S3")), action=(reg9[[2]] = lookup_arp(inport, >> arp.spa, arp.sha); next;) >> table=??(lr_in_lookup_neighbor), priority=120 , match=(inport == >> "DR-S1" && (nd_na || nd_ns) && eth.mcast && >> !is_chassis_resident("cr-DR-S1")), action=(reg9[[2]] = 1; next;) >> table=??(lr_in_lookup_neighbor), priority=120 , match=(inport == >> "DR-S2" && (nd_na || nd_ns) && eth.mcast && >> !is_chassis_resident("cr-DR-S2")), action=(reg9[[2]] = 1; next;) >> table=??(lr_in_lookup_neighbor), priority=120 , match=(inport == >> "DR-S3" && (nd_na || nd_ns) && eth.mcast && >> !is_chassis_resident("cr-DR-S3")), action=(reg9[[2]] = 1; next;) >> @@ -14031,7 +14028,7 @@ AT_CHECK([grep -Fe "172.168.0.110" -e >> "172.168.0.120" -e "10.0.0.3" -e "20.0.0.3 >> table=??(lr_out_undnat ), priority=100 , match=(ip && ip4.src == >> 20.0.0.3 && outport == "lr0-public" && >> is_chassis_resident("cr-lr0-public")), action=(ct_dnat;) >> ]) >> >> -AT_CHECK([grep -Fe "172.168.0.110" -e "172.168.0.120" -e "10.0.0.3" -e >> "20.0.0.3" -e "30:54:00:00:00:03" -e "sw0-port1" publicflows | >> ovn_strip_lflows], [0], [dnl >> +AT_CHECK([grep -Fe "172.168.0.110" -e "172.168.0.120" -e "10.0.0.3" -e >> "20.0.0.3" -e "30:54:00:00:00:03" -e "sw0-port1" publicflows | >> ovn_strip_lflows | grep -v "reg0.*22"], [0], [dnl >> table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >> 30:54:00:00:00:03 && is_chassis_resident("sw0-port1")), action=(outport = >> "public-lr0"; output;) >> table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src == >> {00:00:00:00:ff:02, 30:54:00:00:00:03} && eth.dst == ff:ff:ff:ff:ff:ff && >> (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport = "_MC_flood_l2"; >> output;) >> table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 && >> arp.op == 1 && arp.tpa == 172.168.0.110), action=(clone {outport = >> "public-lr0"; output; }; outport = "_MC_flood_l2"; output;) >> @@ -18760,3 +18757,77 @@ check_row_count Advertised_MAC_Binding 0 >> OVN_CLEANUP_NORTHD >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> +AT_SETUP([Logical Switch ARP filtering]) >> +ovn_start >> + >> +check ovn-nbctl lr-add lr1 >> +check ovn-nbctl lrp-add lr1 down_link f0:00:00:00:00:f1 192.168.1.1/24 >> + >> +check ovn-nbctl ls-add ls1 >> +check ovn-nbctl lsp-add ls1 up_link >> +check ovn-nbctl lsp-add ls1 down_vif1 >> +check ovn-nbctl lsp-add ls1 down_vif2 >> +check ovn-nbctl lsp-add ls1 down_ext >> + >> +check ovn-nbctl set Logical_Switch_Port up_link \ >> + type=router \ >> + options:router-port=down_link \ >> + addresses=router >> + >> +check ovn-nbctl lsp-set-addresses down_vif1 'f0:00:00:00:00:01 >> 192.168.1.101' >> +check ovn-nbctl lsp-set-addresses down_vif2 'f0:00:00:00:00:02 >> 192.168.1.102' >> +check ovn-nbctl lrp-set-gateway-chassis down_link hv1 >> + > There's a race here, we need --wait=sb sync (also in a bunch of other places > in this test). > >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | >> ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) >> + table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), >> action=(drop;) >> +]) >> + >> +# Check localnet port addings trigger ls-arp flow >> +check ovn-nbctl lsp-set-type down_ext localnet >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | >> ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) >> + table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), >> action=(drop;) >> + table=??(ls_in_apply_port_sec), priority=70 , match=(reg0[[22]] == 1), >> action=(drop;) >> + table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && >> is_chassis_resident("cr-down_link")), action=(next;) >> +]) >> + >> +# Check nat adding to dgr attached to logical switch trigger ls-arp flow >> +check ovn-nbctl lr-nat-add lr1 dnat_and_snat 192.168.0.4 10.0.0.4 >> +check ovn-nbctl lr-nat-add lr1 dnat_and_snat 192.168.0.3 10.0.0.3 down_vif1 >> f0:00:00:00:00:03 >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | >> ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) >> + table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), >> action=(drop;) >> + table=??(ls_in_apply_port_sec), priority=70 , match=(reg0[[22]] == 1), >> action=(drop;) >> + table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && >> is_chassis_resident("cr-down_link")), action=(next;) >> + table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && >> is_chassis_resident("down_vif1")), action=(next;) >> +]) >> + >> +check ovn-nbctl lr-nat-del lr1 dnat_and_snat 192.168.0.3 >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | >> ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) >> + table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), >> action=(drop;) >> + table=??(ls_in_apply_port_sec), priority=70 , match=(reg0[[22]] == 1), >> action=(drop;) >> + table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && >> is_chassis_resident("cr-down_link")), action=(next;) >> +]) >> + >> +# Check changinf logical port type >> +check ovn-nbctl lsp-set-type down_ext l2gateway >> + >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | >> ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) >> + table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), >> action=(drop;) >> + table=??(ls_in_apply_port_sec), priority=70 , match=(reg0[[22]] == 1), >> action=(drop;) >> + table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && >> is_chassis_resident("cr-down_link")), action=(next;) >> +]) >> + >> +check ovn-nbctl lsp-del down_ext >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | >> ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) >> + table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), >> action=(drop;) >> +]) >> + >> +AT_CLEANUP >> +]) >> \ No newline at end of file > No newline at end of file. :) > >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 30560f883..113d64229 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -43885,3 +43885,156 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD([ >> +AT_SETUP([GARP delivery: gw and external ports]) >> +AT_SKIP_IF([test $HAVE_SCAPY = no]) >> +ovn_start >> + >> +# Configure initial environment >> +# LR1: down_link <-> LS1: up_link >> +# set lr_down: gateway port (chassis redirect) bound to hv1 >> +# LS1: down_vif1 - vif port bound to hv1 >> +# down_vif2 - vif port bound to hv2 >> +# down_ext - outher port will be iteratred as localnet, l2gateway >> +# >> +# Test: throw ARP annonce request from vitrual ports (down_vif1, down_vif2) >> +# ensure mac_binding is always updated. >> +# (Fixing the issue: mac_binding is only updated for packets came from >> +# down_link's resident chassis) >> +# throw ARP annonce request from from localnet. >> +# ensure mac_binding is updated only if localnet bound to same hv as >> l3dgw >> + >> +check ovn-nbctl lr-add lr1 >> +check ovn-nbctl lrp-add lr1 down_link f0:00:00:00:00:f1 192.168.1.1/24 >> + >> +check ovn-nbctl ls-add ls1 >> +check ovn-nbctl lsp-add ls1 up_link >> +check ovn-nbctl lsp-add ls1 down_vif1 >> +check ovn-nbctl lsp-add ls1 down_vif2 >> +check ovn-nbctl lsp-add ls1 down_ext >> + >> +check ovn-nbctl set Logical_Switch_Port up_link \ >> + type=router \ >> + options:router-port=down_link \ >> + addresses=router >> + >> +check ovn-nbctl lsp-set-addresses down_vif1 'f0:00:00:00:00:01 >> 192.168.1.101' >> +check ovn-nbctl lsp-set-addresses down_vif2 'f0:00:00:00:00:02 >> 192.168.1.102' >> + >> +check ovn-nbctl lsp-set-type down_ext localnet >> +check ovn-nbctl lsp-set-options down_ext network_name=physnet1 >> +check ovn-nbctl lrp-set-gateway-chassis down_link hv1 >> + >> +net_add n1 >> + >> +# Create hypervisor hv1 connected to n1 >> +sim_add hv1 >> +as hv1 >> +ovs-vsctl add-br br-phys >> +ovn_attach n1 br-phys 192.168.0.1 >> +ovs-vsctl add-port br-int vif1 -- \ >> + set Interface vif1 external-ids:iface-id=down_vif1 \ >> + options:tx_pcap=hv1/vif1-tx.pcap options:rxq_pcap=hv1/vif1-rx.pcap >> + >> +# Create hypervisor hv2 connected to n1, add localnet here >> +sim_add hv2 >> +as hv2 >> +ovs-vsctl add-br br-phys >> +ovs-vsctl add-br br-eth0 >> +ovn_attach n1 br-phys 192.168.0.2 >> +ovs-vsctl add-port br-int vif2 -- \ >> + set Interface vif2 external-ids:iface-id=down_vif2 \ >> + options:tx_pcap=hv2/vif2-tx.pcap options:rxq_pcap=hv2/vif2-rx.pcap >> + >> +ovs-vsctl set Open_vSwitch . >> external_ids:ovn-bridge-mappings="physnet1:br-eth0" >> + >> +ovs-vsctl add-port br-eth0 vif_ext -- \ >> + set Interface vif_ext options:tx_pcap=hv2/vif_ext-tx.pcap \ >> + options:rxq_pcap=hv2/vif_ext-rx.pcap >> + >> +# Pre-populate the hypervisors' ARP tables so that we don't lose any >> +# packets for ARP resolution (native tunneling doesn't queue packets >> +# for ARP resolution). >> +OVN_POPULATE_ARP >> + >> +wait_for_ports_up >> +check ovn-nbctl --wait=hv sync >> + >> +# Annonce 192.168.1.222 from localnet in hv2 >> +# result: drop, hv2 is not gateway chassis for down_link >> +sha=02:00:00:00:00:ee >> +tha=00:00:00:00:00:00 >> +spa=192.168.1.222 >> +tpa=$spa >> +garp=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \ >> + ARP(hwsrc='${sha}', hwdst='${tha}', psrc='${spa}', >> pdst='${tpa}')") >> +as hv2 ovs-appctl netdev-dummy/receive vif_ext $garp >> + >> +# Make hv2 gateway chassis >> +# Annonce 192.168.1.223 from localnet in hv2 >> +# result: ok, hv2 is gateway chassis for down_link >> +# >> +check ovn-nbctl lrp-set-gateway-chassis down_link hv2 >> + >> +wait_row_count Port_Binding 1 logical_port=cr-down_link 'chassis!=[[]]' > I think to be completely sure we don't race here we should add a: > check ovn-nbctl --wait=hv sync > >> + >> +sha=02:00:00:00:00:ee >> +tha=00:00:00:00:00:00 >> +spa=192.168.1.223 >> +tpa=$spa >> +garp=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \ >> + ARP(hwsrc='${sha}', hwdst='${tha}', psrc='${spa}', >> pdst='${tpa}')") >> +as hv2 ovs-appctl netdev-dummy/receive vif_ext $garp >> + >> +# Annonce 192.168.1.111, 112 from vif1, vif2 in hv1, hv2 >> +# result: ok, vif1, vif2 are virtual ports, restrictions are not applied. >> +sha=f0:00:00:00:00:01 >> +tha=00:00:00:00:00:00 >> +spa=192.168.1.111 >> +tpa=0.0.0.0 >> +garp=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \ >> + ARP(hwsrc='${sha}', hwdst='${tha}', psrc='${spa}', >> pdst='${tpa}')") >> +as hv1 ovs-appctl netdev-dummy/receive vif1 $garp >> + >> +sha=f0:00:00:00:00:02 >> +tha=00:00:00:00:00:00 >> +spa=192.168.1.112 >> +tpa=0.0.0.0 >> +garp=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \ >> + ARP(hwsrc='${sha}', hwdst='${tha}', psrc='${spa}', >> pdst='${tpa}')") >> +as hv2 ovs-appctl netdev-dummy/receive vif2 $garp >> + >> +# Set down_ext type to l2gateway >> +# Annonce 192.168.1.113, 114 from vif1, vif2 in hv1, hv2 >> +# result: ok, vif1, vif2 are virtual ports, restrictions are not applied. >> +check ovn-nbctl --wait=hv lsp-set-type down_ext l2gateway >> + >> +sha=f0:00:00:00:00:01 >> +tha=00:00:00:00:00:00 >> +spa=192.168.1.113 >> +tpa=0.0.0.0 >> +garp=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \ >> + ARP(hwsrc='${sha}', hwdst='${tha}', psrc='${spa}', >> pdst='${tpa}')") >> +as hv1 ovs-appctl netdev-dummy/receive vif1 $garp >> + >> +sha=f0:00:00:00:00:02 >> +tha=00:00:00:00:00:00 >> +spa=192.168.1.114 >> +tpa=0.0.0.0 >> +garp=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \ >> + ARP(hwsrc='${sha}', hwdst='${tha}', psrc='${spa}', >> pdst='${tpa}')") >> +as hv2 ovs-appctl netdev-dummy/receive vif2 $garp >> + >> +wait_row_count MAC_Binding 1 ip="192.168.1.111" mac='"f0:00:00:00:00:01"' >> logical_port='"down_link"' >> +wait_row_count MAC_Binding 1 ip="192.168.1.112" mac='"f0:00:00:00:00:02"' >> logical_port='"down_link"' >> +wait_row_count MAC_Binding 1 ip="192.168.1.113" mac='"f0:00:00:00:00:01"' >> logical_port='"down_link"' >> +wait_row_count MAC_Binding 1 ip="192.168.1.114" mac='"f0:00:00:00:00:02"' >> logical_port='"down_link"' >> +wait_row_count MAC_Binding 1 ip="192.168.1.223" mac='"02:00:00:00:00:ee"' >> logical_port='"down_link"' >> +wait_row_count MAC_Binding 0 ip="192.168.1.222" mac='"02:00:00:00:00:ee"' >> logical_port='"down_link"' >> + >> +check ovn-nbctl --wait=hv lsp-set-type down_ext localnet >> + >> +OVN_CLEANUP([hv1],[hv2]) >> +AT_CLEANUP >> +]) > The changes I'm suggesting are minor, here's the full diff (some more style > issues > are addressed in it): > > https://github.com/dceara/ovn/commit/a2307c9 > > If it looks good to you too I can squash it in and push the patch to main. > Please let me know what you think. > > Regards, > Dumitru > > --- > diff --git a/northd/en-ls-arp.c b/northd/en-ls-arp.c > index 69d76ef3ee..72622fce56 100644 > --- a/northd/en-ls-arp.c > +++ b/northd/en-ls-arp.c > @@ -100,13 +100,13 @@ nat_record_data_create(struct ls_arp_record > *ls_arp_record, > > if (is_centralized_nat_record(nat_entry)) { > hmapx_add(&ls_arp_record->nat_records, > - (struct lrnat_rec *) lrnat_rec); > + (struct lrnat_rec *) lrnat_rec); > } > } > } > } > > -static struct ls_arp_record* > +static struct ls_arp_record * > ls_arp_record_lookup_by_od_(const struct ls_arp_table *table, > const struct ovn_datapath *od) > { > @@ -202,8 +202,8 @@ en_ls_arp_run(struct engine_node *node, void *data_) > const struct ovn_datapath *od; > HMAP_FOR_EACH (od, key_node, &input_data.ls_datapaths->datapaths) { > /* Filtering ARP entries at logical switch works > - * when there are physical ports on the switch. */ > - if (hmapx_is_empty(&od->ph_ports)) { > + * when there are physical ports on the switch. */ > + if (hmapx_is_empty(&od->phys_ports)) { > continue; > } > > @@ -242,7 +242,12 @@ ls_arp_northd_handler(struct engine_node *node, void > *data_) > if (!ls_arp_record) { > /* Filtering ARP entries at logical switch works > * when there are physical ports on the switch. */ > - if (hmapx_is_empty(&od->ph_ports)) { > + if (hmapx_is_empty(&od->phys_ports)) { > + /* NOTE: If the switch used to have physical ports but those > + * were removed the lr_nat node has recomputed and triggers > + * the ls_arp_lr_nat_handler() which cannot incrementally > + * process changes. This implicitly triggers correct > + * handling of the removal.*/ > continue; > } > ls_arp_record = ls_arp_record_create(&data->table, > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 748e097697..b9ac33dc3a 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -414,9 +414,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_lflow, &en_port_group, engine_noop_handler); > engine_add_input(&en_lflow, &en_lr_stateful, lflow_lr_stateful_handler); > engine_add_input(&en_lflow, &en_ls_stateful, lflow_ls_stateful_handler); > - > engine_add_input(&en_lflow, &en_ls_arp, lflow_ls_arp_handler); > - > engine_add_input(&en_lflow, &en_multicast_igmp, > lflow_multicast_igmp_handler); > engine_add_input(&en_lflow, &en_sb_acl_id, NULL); > diff --git a/northd/northd.c b/northd/northd.c > index 6ad120dde6..ec219a0c71 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -549,7 +549,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct > uuid *key, > hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key)); > od->lr_group = NULL; > hmap_init(&od->ports); > - hmapx_init(&od->ph_ports); > + hmapx_init(&od->phys_ports); > sset_init(&od->router_ips); > od->ls_peers = VECTOR_EMPTY_INITIALIZER(struct ovn_datapath *); > od->router_ports = VECTOR_EMPTY_INITIALIZER(struct ovn_port *); > @@ -586,7 +586,7 @@ ovn_datapath_destroy(struct ovn_datapath *od) > vector_destroy(&od->l3dgw_ports); > destroy_mcast_info_for_datapath(od); > destroy_ports_for_datapath(od); > - hmapx_destroy(&od->ph_ports); > + hmapx_destroy(&od->phys_ports); > sset_destroy(&od->router_ips); > free(od); > } > @@ -1613,7 +1613,7 @@ join_logical_ports_lsp(struct hmap *ports, > } > > if (lsp_is_localnet(nbsp) || lsp_is_l2gw(nbsp)) { > - hmapx_add(&od->ph_ports, op); > + hmapx_add(&od->phys_ports, op); > } > > parse_lsp_addrs(op); > @@ -9708,18 +9708,14 @@ build_lswitch_arp_chassis_resident(const struct > ovn_datapath *od, > struct lflow_table *lflows, > const struct ls_arp_record *ar) > { > - struct sset inports; > - struct sset resident_ports; > - struct sset distributed_nat_ports; > - > - sset_init(&inports); > - sset_init(&resident_ports); > - sset_init(&distributed_nat_ports); > - > + struct sset distributed_nat_ports = > + SSET_INITIALIZER(&distributed_nat_ports); > + struct sset resident_ports = SSET_INITIALIZER(&resident_ports); > + struct sset inports = SSET_INITIALIZER(&inports); > struct ds match = DS_EMPTY_INITIALIZER; > > struct hmapx_node *node; > - HMAPX_FOR_EACH (node, &od->ph_ports) { > + HMAPX_FOR_EACH (node, &od->phys_ports) { > struct ovn_port *op = node->data; > sset_add(&inports, op->json_key); > } > @@ -9745,35 +9741,33 @@ build_lswitch_arp_chassis_resident(const struct > ovn_datapath *od, > } > } > > - > if (!sset_is_empty(&inports) && !sset_is_empty(&resident_ports)) { > - const char *item; > + const char *port_name; > > - SSET_FOR_EACH (item, &inports) { > + SSET_FOR_EACH (port_name, &inports) { > ds_clear(&match); > - ds_put_format(&match, > - "(arp.op == 1 || arp.op == 2) && inport == %s", > - item); > + ds_put_format(&match, "(arp.op == 1 || arp.op == 2) " > + "&& inport == %s", > + port_name); > ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 75, > ds_cstr(&match), REGBIT_EXT_ARP " = 1; next;", > ar->lflow_ref); > } > > - SSET_FOR_EACH (item, &resident_ports) { > + SSET_FOR_EACH (port_name, &resident_ports) { > ds_clear(&match); > - ds_put_format(&match, > - REGBIT_EXT_ARP" == 1 && is_chassis_resident(%s)", > - item); > + ds_put_format(&match, REGBIT_EXT_ARP" == 1 " > + "&& is_chassis_resident(%s)", > + port_name); > ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 75, > ds_cstr(&match), "next;", ar->lflow_ref); > } > > - SSET_FOR_EACH (item, &distributed_nat_ports) { > + SSET_FOR_EACH (port_name, &distributed_nat_ports) { > ds_clear(&match); > - ds_put_format(&match, > - REGBIT_EXT_ARP > - " == 1 && is_chassis_resident(\"%s\")", > - item); > + ds_put_format(&match, REGBIT_EXT_ARP " == 1 " > + "&& is_chassis_resident(\"%s\")", > + port_name); > ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 75, > ds_cstr(&match), "next;", ar->lflow_ref); > } > @@ -9782,10 +9776,10 @@ build_lswitch_arp_chassis_resident(const struct > ovn_datapath *od, > REGBIT_EXT_ARP" == 1", "drop;", ar->lflow_ref); > } > > - ds_destroy(&match); > - sset_destroy(&inports); > - sset_destroy(&resident_ports); > sset_destroy(&distributed_nat_ports); > + sset_destroy(&resident_ports); > + sset_destroy(&inports); > + ds_destroy(&match); > } > > static bool > @@ -19802,8 +19796,7 @@ lflow_handle_ls_arp_changes(struct ovsdb_idl_txn > *ovnsb_txn, > > bool handled = lflow_ref_sync_lflows( > ls_arp_record->lflow_ref, lflows, ovnsb_txn, > - lflow_input->ls_datapaths, > - lflow_input->lr_datapaths, > + lflow_input->ls_datapaths, lflow_input->lr_datapaths, > lflow_input->ovn_internal_version_changed, > lflow_input->sbrec_logical_flow_table, > lflow_input->sbrec_logical_dp_group_table); > diff --git a/northd/northd.h b/northd/northd.h > index b5f1fac389..2869ea97e4 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -474,7 +474,7 @@ struct ovn_datapath { > struct lrouter_group *lr_group; > > /* Set of localnet or l2gw ports. */ > - struct hmapx ph_ports; > + struct hmapx phys_ports; > > /* Map of ovn_port objects belonging to this datapath. > * This map doesn't include derived ports. */ > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 0018fb0efa..380c9ff802 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -18816,14 +18816,14 @@ check ovn-nbctl set Logical_Switch_Port up_link \ > check ovn-nbctl lsp-set-addresses down_vif1 'f0:00:00:00:00:01 > 192.168.1.101' > check ovn-nbctl lsp-set-addresses down_vif2 'f0:00:00:00:00:02 > 192.168.1.102' > check ovn-nbctl lrp-set-gateway-chassis down_link hv1 > - > +check ovn-nbctl --wait=sb sync > AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), > action=(drop;) > ]) > > # Check localnet port addings trigger ls-arp flow > -check ovn-nbctl lsp-set-type down_ext localnet > +check ovn-nbctl --wait=sb lsp-set-type down_ext localnet > AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), > action=(drop;) > @@ -18831,9 +18831,10 @@ AT_CHECK([ovn-sbctl lflow-list ls1 | grep > ls_in_apply_port_sec | ovn_strip_lflow > table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && > is_chassis_resident("cr-down_link")), action=(next;) > ]) > > -# Check nat adding to dgr attached to logical switch trigger ls-arp flow > +# Check nat adding to dgr attached to logical switch trigger ls-arp flow. > check ovn-nbctl lr-nat-add lr1 dnat_and_snat 192.168.0.4 10.0.0.4 > check ovn-nbctl lr-nat-add lr1 dnat_and_snat 192.168.0.3 10.0.0.3 down_vif1 > f0:00:00:00:00:03 > +check ovn-nbctl --wait=sb sync > AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), > action=(drop;) > @@ -18842,7 +18843,7 @@ AT_CHECK([ovn-sbctl lflow-list ls1 | grep > ls_in_apply_port_sec | ovn_strip_lflow > table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && > is_chassis_resident("down_vif1")), action=(next;) > ]) > > -check ovn-nbctl lr-nat-del lr1 dnat_and_snat 192.168.0.3 > +check ovn-nbctl --wait=sb lr-nat-del lr1 dnat_and_snat 192.168.0.3 > AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), > action=(drop;) > @@ -18850,9 +18851,24 @@ AT_CHECK([ovn-sbctl lflow-list ls1 | grep > ls_in_apply_port_sec | ovn_strip_lflow > table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && > is_chassis_resident("cr-down_link")), action=(next;) > ]) > > -# Check changinf logical port type > -check ovn-nbctl lsp-set-type down_ext l2gateway > +# Check changing logical port type to l2gateway. > +check ovn-nbctl --wait=sb lsp-set-type down_ext l2gateway > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | > ovn_strip_lflows], [0], [dnl > + table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > + table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), > action=(drop;) > + table=??(ls_in_apply_port_sec), priority=70 , match=(reg0[[22]] == 1), > action=(drop;) > + table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && > is_chassis_resident("cr-down_link")), action=(next;) > +]) > > +# Check changing logical port type to vif. > +check ovn-nbctl --wait=sb lsp-set-type down_ext '' > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | > ovn_strip_lflows], [0], [dnl > + table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > + table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), > action=(drop;) > +]) > + > +# Check changing logical port type back to localnet. > +check ovn-nbctl --wait=sb lsp-set-type down_ext localnet > AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), > action=(drop;) > @@ -18860,11 +18876,12 @@ AT_CHECK([ovn-sbctl lflow-list ls1 | grep > ls_in_apply_port_sec | ovn_strip_lflow > table=??(ls_in_apply_port_sec), priority=75 , match=(reg0[[22]] == 1 && > is_chassis_resident("cr-down_link")), action=(next;) > ]) > > -check ovn-nbctl lsp-del down_ext > +# Check changing removing logical port. > +check ovn-nbctl --wait=sb lsp-del down_ext > AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_apply_port_sec | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > table=??(ls_in_apply_port_sec), priority=50 , match=(reg0[[15]] == 1), > action=(drop;) > ]) > > AT_CLEANUP > -]) > \ No newline at end of file > +]) > diff --git a/tests/ovn.at b/tests/ovn.at > index 113d64229f..6ea0d1de50 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -43896,13 +43896,13 @@ ovn_start > # set lr_down: gateway port (chassis redirect) bound to hv1 > # LS1: down_vif1 - vif port bound to hv1 > # down_vif2 - vif port bound to hv2 > -# down_ext - outher port will be iteratred as localnet, l2gateway > +# down_ext - outer (port will be iterated as localnet, l2gateway) > # > -# Test: throw ARP annonce request from vitrual ports (down_vif1, down_vif2) > +# Test: send GARP request from virtual ports (down_vif1, down_vif2) > # ensure mac_binding is always updated. > # (Fixing the issue: mac_binding is only updated for packets came from > # down_link's resident chassis) > -# throw ARP annonce request from from localnet. > +# send GARP request from from localnet. > # ensure mac_binding is updated only if localnet bound to same hv as > l3dgw > > check ovn-nbctl lr-add lr1 > @@ -43978,6 +43978,7 @@ as hv2 ovs-appctl netdev-dummy/receive vif_ext $garp > check ovn-nbctl lrp-set-gateway-chassis down_link hv2 > > wait_row_count Port_Binding 1 logical_port=cr-down_link 'chassis!=[[]]' > +check ovn-nbctl --wait=hv sync > > sha=02:00:00:00:00:ee > tha=00:00:00:00:00:00 > > -- regards, Alexandra. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
