On 11/13/24 12:04 PM, Lorenzo Bianconi wrote:
> Introduce ECMP_Nexthop table in the SB db in order to track active
> ecmp-symmetric-reply connections and flush stale ones. ECMP_Nexthop
> table contains ip and mac address for each active nexthop.
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
Hi Lorenzo,
Thanks for the patch!
> northd/en-northd.c | 29 +++++++++++
> northd/en-northd.h | 4 ++
> northd/inc-proc-northd.c | 9 +++-
> northd/northd.c | 104 ++++++++++++++++++++++++++++++++++++++-
> northd/northd.h | 6 +++
> ovn-sb.ovsschema | 17 ++++++-
> ovn-sb.xml | 31 ++++++++++++
> tests/ovn-northd.at | 33 ++++++++++---
> 8 files changed, 222 insertions(+), 11 deletions(-)
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 24ed31517..165af44a0 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -404,6 +404,23 @@ en_bfd_sync_run(struct engine_node *node, void *data)
> engine_set_node_state(node, EN_UPDATED);
> }
>
> +void
> +en_ecmp_nexthop_run(struct engine_node *node, void *data OVS_UNUSED)
We started splitting out independent incremental processing engine nodes
into their own files. See northd/en-*.[ch]. I think it makes sense to
move the en_ecmp_nexthop*() functions and all other related static
functions to a new northd/en-ecmp-nexthop.[ch] module.
> +{
> + const struct engine_context *eng_ctx = engine_get_context();
> + struct northd_data *northd_data = engine_get_input_data("northd", node);
> + struct static_routes_data *static_routes_data =
> + engine_get_input_data("static_routes", node);
> + const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table =
> + EN_OVSDB_GET(engine_get_input("SB_ecmp_nexthop", node));
> +
> + build_ecmp_nexthop_table(eng_ctx->ovnsb_idl_txn,
> + &northd_data->lr_ports,
> + &static_routes_data->parsed_routes,
> + sbrec_ecmp_nexthop_table);
> + engine_set_node_state(node, EN_UPDATED);
> +}
> +
> void
> *en_northd_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> @@ -454,6 +471,13 @@ void
> return data;
> }
>
> +void *
> +en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED,
> + struct engine_arg *arg OVS_UNUSED)
> +{
> + return NULL;
> +}
> +
> void
> en_northd_cleanup(void *data)
> {
> @@ -526,3 +550,8 @@ en_bfd_sync_cleanup(void *data)
> {
> bfd_sync_destroy(data);
> }
> +
> +void
> +en_ecmp_nexthop_cleanup(void *data OVS_UNUSED)
> +{
> +}
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 631a7c17a..2666cc67e 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -42,5 +42,9 @@ bool bfd_sync_northd_change_handler(struct engine_node
> *node,
> void *data OVS_UNUSED);
> void en_bfd_sync_run(struct engine_node *node, void *data);
> void en_bfd_sync_cleanup(void *data OVS_UNUSED);
> +void en_ecmp_nexthop_run(struct engine_node *node, void *data);
> +void *en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED,
> + struct engine_arg *arg OVS_UNUSED);
> +void en_ecmp_nexthop_cleanup(void *data);
>
> #endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 8c834facb..8e16fde80 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -103,7 +103,8 @@ static unixctl_cb_func chassis_features_list;
> SB_NODE(fdb, "fdb") \
> SB_NODE(static_mac_binding, "static_mac_binding") \
> SB_NODE(chassis_template_var, "chassis_template_var") \
> - SB_NODE(logical_dp_group, "logical_dp_group")
> + SB_NODE(logical_dp_group, "logical_dp_group") \
> + SB_NODE(ecmp_nexthop, "ecmp_nexthop")
>
> enum sb_engine_node {
> #define SB_NODE(NAME, NAME_STR) SB_##NAME,
> @@ -162,6 +163,7 @@ static ENGINE_NODE(route_policies, "route_policies");
> static ENGINE_NODE(static_routes, "static_routes");
> static ENGINE_NODE(bfd, "bfd");
> static ENGINE_NODE(bfd_sync, "bfd_sync");
> +static ENGINE_NODE(ecmp_nexthop, "ecmp_nexthop");
>
> void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> struct ovsdb_idl_loop *sb)
> @@ -264,6 +266,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_bfd_sync, &en_route_policies, NULL);
> engine_add_input(&en_bfd_sync, &en_northd,
> bfd_sync_northd_change_handler);
>
> + engine_add_input(&en_ecmp_nexthop, &en_sb_ecmp_nexthop, NULL);
To avoid unnecessary processing, we also need to omit notifications for
SB.ECMP_Nexthop records in ovn-northd.c.
Here's an example for meters:
https://github.com/ovn-org/ovn/blob/ebe5d70122ce0f74067858f5cb19276c852a81da/northd/ovn-northd.c#L929-L931
> + engine_add_input(&en_ecmp_nexthop, &en_northd, NULL);
> + engine_add_input(&en_ecmp_nexthop, &en_static_routes, 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);
> @@ -334,6 +340,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_sync_from_sb, &en_sb_port_binding, NULL);
> engine_add_input(&en_sync_from_sb, &en_sb_ha_chassis_group, NULL);
>
> + engine_add_input(&en_northd_output, &en_ecmp_nexthop, NULL);
> engine_add_input(&en_northd_output, &en_sync_from_sb, NULL);
> engine_add_input(&en_northd_output, &en_sync_to_sb,
> northd_output_sync_to_sb_handler);
> diff --git a/northd/northd.c b/northd/northd.c
> index 64b2e3859..d54fbf14e 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10720,6 +10720,106 @@ build_bfd_map(const struct nbrec_bfd_table
> *nbrec_bfd_table,
> }
> }
>
> +struct ecmp_nexthop_data {
> + struct hmap_node hmap_node;
> + const struct sbrec_ecmp_nexthop *sb_ecmp_nh;
> + bool stale;
We don't need this 'stale' field, please see the comment related to
build_ecmp_nexthop_table() below.
> +};
> +
> +static struct ecmp_nexthop_data *
> +ecmp_nexthop_alloc_entry(const struct sbrec_ecmp_nexthop *sb_ecmp_nh,
> + struct hmap *map)
The name is misleading. We actually insert the entry. I'd call this
ecmp_nexthop_insert_entry().
> +{
> + struct ecmp_nexthop_data *e = xmalloc(sizeof *e);
> + e->sb_ecmp_nh = sb_ecmp_nh;
> +
> + const char *sb_port = sb_ecmp_nh->port->logical_port;
> + const char *sb_nexthop = sb_ecmp_nh->nexthop;
> +
> + uint32_t hash = hash_string(sb_nexthop, 0);
> + hash = hash_add(hash, hash_string(sb_port, 0));
Port names can be long. What about hashing the port binding key instead?
We could rewrite this as:
uint32_t hash = hash_string(sb_ecmp_nh->nexthop, 0);
hash = hash_int(sb_ecmp_nh->port->tunnel_key, hash);
hmap_insert(map, &e->hmap_node, hash);
> + hmap_insert(map, &e->hmap_node, hash);
> +
> + return e;
> +}
> +
> +static struct ecmp_nexthop_data *
> +ecmp_nexthop_find_entry(const char *nexthop, const char *port,
> + struct hmap *map)
> +{
> + uint32_t hash = hash_string(nexthop, 0);
> + hash = hash_add(hash, hash_string(port, 0));
> +
If we change the hash function to use the port binding key, this should
also change to something like:
static struct ecmp_nexthop_data *
ecmp_nexthop_find_entry(const char *nexthop,
const struct sbrec_port_binding *pb,
struct hmap *map)
{
uint32_t hash = hash_string(nexthop, 0);
hash = hash_int(pb->tunnel_key, hash);
> + struct ecmp_nexthop_data *e;
> + HMAP_FOR_EACH_WITH_HASH (e, hmap_node, hash, map) {
> + const char *sb_port = e->sb_ecmp_nh->port->logical_port;
> + const char *sb_nexthop = e->sb_ecmp_nh->nexthop;
> + if (!strcmp(sb_nexthop, nexthop) && !strcmp(sb_port, port)) {
> + return e;
> + }
> + }
> + return NULL;
> +}
> +
> +void
> +build_ecmp_nexthop_table(
> + struct ovsdb_idl_txn *ovnsb_txn,
> + const struct hmap *lr_ports, const struct hmap *routes,
> + const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table)
> +{
> + if (!ovnsb_txn) {
> + return;
> + }
> +
> + struct hmap sb_nexthops_map = HMAP_INITIALIZER(&sb_nexthops_map);
> +
> + const struct sbrec_ecmp_nexthop *sb_ecmp_nexthop;
> + SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH (sb_ecmp_nexthop,
> + sbrec_ecmp_nexthop_table) {
> + struct ecmp_nexthop_data *e = ecmp_nexthop_alloc_entry(
> + sb_ecmp_nexthop, &sb_nexthops_map);
> + e->stale = true;
> + }
> +
> + struct parsed_route *pr;
> + HMAP_FOR_EACH (pr, key_node, routes) {
> + if (!pr->ecmp_symmetric_reply) {
> + continue;
> + }
> +
> + if (!pr->out_port) {
> + continue;
> + }
> +
> + struct ovn_port *out_port = ovn_port_find(lr_ports,
> pr->out_port->key);
Why do we lookup again here? pr->out_port is the ovn_port already.
> + if (!out_port || !out_port->sb) {
> + continue;
> + }
> +
> + const struct nbrec_logical_router_static_route *r = pr->route;
> + const char *pb_name = out_port->sb->logical_port;
> +
> + struct ecmp_nexthop_data *e = ecmp_nexthop_find_entry(
> + r->nexthop, pb_name, &sb_nexthops_map);
> + if (!e) {
> + sb_ecmp_nexthop = sbrec_ecmp_nexthop_insert(ovnsb_txn);
> + sbrec_ecmp_nexthop_set_nexthop(sb_ecmp_nexthop, r->nexthop);
> + sbrec_ecmp_nexthop_set_port(sb_ecmp_nexthop, out_port->sb);
> + e = ecmp_nexthop_alloc_entry(sb_ecmp_nexthop, &sb_nexthops_map);
Instead of adding the entry to sb_nexthops_map here we can just remove
'e' from sb_nexthops_map and free it if found (else branch of this if) ...
> + }
> + e->stale = false;
> + }
> +
> + struct ecmp_nexthop_data *e;
> + HMAP_FOR_EACH_POP (e, hmap_node, &sb_nexthops_map) {
> + if (e->stale) {
> + sbrec_ecmp_nexthop_delete(e->sb_ecmp_nh);
... then here all remaining items in sb_nexthops_map are stale.
> + }
> + free(e);
> + }
> + hmap_destroy(&sb_nexthops_map);
> +}
> +
> /* Returns a string of the IP address of the router port 'op' that
> * overlaps with 'ip_s". If one is not found, returns NULL.
> *
> @@ -11160,10 +11260,11 @@ parsed_routes_add(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> }
>
> /* Verify that ip_prefix and nexthop are on the same network. */
> + struct ovn_port *out_port = NULL;
> if (!is_discard_route &&
> !find_static_route_outport(od, lr_ports, route,
> IN6_IS_ADDR_V4MAPPED(&prefix),
> - NULL, NULL)) {
> + NULL, &out_port)) {
> return;
> }
>
> @@ -11206,6 +11307,7 @@ parsed_routes_add(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> new_pr->hash = route_hash(new_pr);
> new_pr->route = route;
> new_pr->nbr = od->nbr;
> + new_pr->out_port = out_port;
> new_pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
> "ecmp_symmetric_reply",
> false);
> diff --git a/northd/northd.h b/northd/northd.h
> index c1442ff40..3bd2e29e3 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -703,6 +703,7 @@ struct parsed_route {
> uint32_t route_table_id;
> uint32_t hash;
> const struct nbrec_logical_router_static_route *route;
> + struct ovn_port *out_port;
> bool ecmp_symmetric_reply;
> bool is_discard_route;
> const struct nbrec_logical_router *nbr;
> @@ -746,6 +747,11 @@ void bfd_destroy(struct bfd_data *);
> void bfd_sync_init(struct bfd_sync_data *);
> void bfd_sync_destroy(struct bfd_sync_data *);
>
> +void build_ecmp_nexthop_table(
> + struct ovsdb_idl_txn *ovnsb_txn,
> + const struct hmap *lr_ports, const struct hmap *routes,
> + const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table);
> +
> struct lflow_table;
> struct lr_stateful_tracked_data;
> struct ls_stateful_tracked_data;
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index 73abf2c8d..864cb0ed6 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> - "version": "20.37.0",
> - "cksum": "1950136776 31493",
> + "version": "20.38.0",
> + "cksum": "466210938 32119",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -610,6 +610,19 @@
> "refTable": "Datapath_Binding"}}}},
> "indexes": [["logical_port", "ip"]],
> "isRoot": true},
> + "ECMP_Nexthop": {
> + "columns": {
> + "nexthop": {"type": "string"},
> + "port": {"type": {"key": {"type": "uuid",
> + "refTable": "Port_Binding",
> + "refType": "strong"},
> + "min": 0, "max": 1}},
I thought there's never a case when we don't have a port for the
next-hop. I think this shouldn't be optional.
> + "mac": {"type": "string"},
> + "external_ids": {
> + "type": {"key": "string", "value": "string",
> + "min": 0, "max": "unlimited"}}},
> + "indexes": [["nexthop", "port"]],
> + "isRoot": true},
> "Chassis_Template_Var": {
> "columns": {
> "chassis": {"type": "string"},
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index ea4adc1c3..ea1d484a7 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -5217,4 +5217,35 @@ tcp.flags = RST;
> The set of variable values for a given chassis.
> </column>
> </table>
> +
> + <table name="ECMP_Nexthop">
> + <p>
> + Each record in this table represents an active ECMP route committed by
This seems wrong, it's not a route, it's a next hop. And the next hop
can be shared by multiple routes.
Also, we only create ECMP_Nexthop records for routes that have
--ecmp-symmetric-reply set. It's probably good to mention that here in
case users expect other ECMP nexthops to be created in the SB.
> + <code>ovn-northd</code> to <code>ovs</code> connection-tracking table.
Nit: maybe rephrase this to "to the <code>ovs</code> connection tracker".
> + <code>ECMP_Nexthop</code> table is used by <code>ovn-controller</code>
Nit: The <code>ECMP_Nexthop</code> table.
> + to track active ct entries and to flush stale ones.
> + </p>
> + <column name="nexthop">
> + <p>
> + Nexthop IP address for this ECMP route. Nexthop IP address should
> + be the IP address of a connected router port or the IP address of
> + an external device used as nexthop for the given destination.
> + </p>
> + </column>
> + <column name="port">
> + <p>
> + The reference to <ref table="Port_Binding"/> table for the port used
> + to connect to the configured next-hop.
> + </p>
> + </column>
> + <column name="mac">
> + <p>
> + Nexthop mac address.
> + </p>
> + </column>
> +
> + <column name="external_ids">
> + See <em>External IDs</em> at the beginning of this document.
> + </column>
> + </table>
> </database>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 8477e4250..1e01c2614 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3886,7 +3886,7 @@ wait_row_count bfd 1 logical_port=r0-sw3 detect_mult=5
> dst_ip=192.168.3.2 \
> check_engine_stats northd norecompute nocompute
> check_engine_stats bfd recompute nocompute
> check_engine_stats lflow recompute nocompute
> -check_engine_stats northd_output norecompute compute
> +check_engine_stats northd_output recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>
> @@ -3901,7 +3901,7 @@ wait_row_count bfd 1 logical_port=r0-sw1 min_rx=1000
> min_tx=1000 detect_mult=100
> check_engine_stats northd norecompute nocompute
> check_engine_stats bfd recompute nocompute
> check_engine_stats lflow recompute nocompute
> -check_engine_stats northd_output norecompute compute
> +check_engine_stats northd_output recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>
> @@ -3913,7 +3913,7 @@ check_engine_stats northd recompute nocompute
> check_engine_stats bfd recompute nocompute
> check_engine_stats static_routes recompute nocompute
> check_engine_stats lflow recompute nocompute
> -check_engine_stats northd_output norecompute compute
> +check_engine_stats northd_output recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>
> @@ -3929,7 +3929,7 @@ check_engine_stats northd recompute nocompute
> check_engine_stats bfd recompute nocompute
> check_engine_stats static_routes recompute nocompute
> check_engine_stats lflow recompute nocompute
> -check_engine_stats northd_output norecompute compute
> +check_engine_stats northd_output recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>
> @@ -3941,7 +3941,7 @@ check_engine_stats northd recompute nocompute
> check_engine_stats bfd recompute nocompute
> check_engine_stats route_policies recompute nocompute
> check_engine_stats lflow recompute nocompute
> -check_engine_stats northd_output norecompute compute
> +check_engine_stats northd_output recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>
> @@ -3976,7 +3976,7 @@ check_engine_stats northd recompute nocompute
> check_engine_stats bfd recompute nocompute
> check_engine_stats static_routes recompute nocompute
> check_engine_stats lflow recompute nocompute
> -check_engine_stats northd_output norecompute compute
> +check_engine_stats northd_output recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>
> @@ -3991,7 +3991,7 @@ check_engine_stats northd recompute nocompute
> check_engine_stats bfd recompute nocompute
> check_engine_stats route_policies recompute nocompute
> check_engine_stats lflow recompute nocompute
> -check_engine_stats northd_output norecompute compute
> +check_engine_stats northd_output recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>
> @@ -6816,6 +6816,10 @@ check ovn-nbctl lsp-set-options public-lr0
> router-port=lr0-public
>
> # ECMP flows will be added even if there is only one next-hop.
> check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1
> 192.168.0.10
> +check_row_count ECMP_Nexthop 1
As far as I can tell we only do positive checks, i.e., check that next
hop records are created in the SB for routes with --ecmp-symmetric-reply
set.
We could also add a test to ensure that no ECMP_Nexthop is created for
routes that have --ecmp set but no --ecmp-symmetric-reply.
> +uuid=$(fetch_column Port_Binding _uuid logical_port=lr0-public)
> +check_column 192.168.0.10 ECMP_Nexthop nexthop
> +check_column "$uuid" ECMP_Nexthop port
>
> ovn-sbctl dump-flows lr0 > lr0flows
>
> @@ -6835,6 +6839,13 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows |
> ovn_strip_lflows], [0], [dn
> ])
>
> check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1
> 192.168.0.20
> +check_row_count ECMP_Nexthop 2
> +AT_CHECK([ovn-sbctl --columns nexthop --bare find ECMP_Nexthop
> nexthop=192.168.0.10], [0], [dnl
> +192.168.0.10
> +])
> +AT_CHECK([ovn-sbctl --columns nexthop --bare find ECMP_Nexthop
> nexthop=192.168.0.20], [0], [dnl
> +192.168.0.20
> +])
>
> ovn-sbctl dump-flows lr0 > lr0flows
> AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> @@ -6864,6 +6875,13 @@ AT_CHECK([grep -e "lr_in_arp_resolve.*ecmp" lr0flows |
> ovn_strip_lflows], [0], [
>
> # add ecmp route with wrong nexthop
> check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1
> 192.168.1.20
> +check_row_count ECMP_Nexthop 2
> +AT_CHECK([ovn-sbctl --columns nexthop --bare find ECMP_Nexthop
> nexthop=192.168.0.10], [0], [dnl
> +192.168.0.10
> +])
> +AT_CHECK([ovn-sbctl --columns nexthop --bare find ECMP_Nexthop
> nexthop=192.168.0.20], [0], [dnl
> +192.168.0.20
> +])
>
> ovn-sbctl dump-flows lr0 > lr0flows
> AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> @@ -6883,6 +6901,7 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows |
> sed 's/192\.168\.0\..0/192.
>
> check ovn-nbctl lr-route-del lr0
> wait_row_count nb:Logical_Router_Static_Route 0
> +check_row_count ECMP_Nexthop 0
>
> check ovn-nbctl --wait=sb lr-route-add lr0 1.0.0.0/24 192.168.0.10
> ovn-sbctl dump-flows lr0 > lr0flows
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
