+ Felix (explicitly) On 2/11/25 11:22 AM, Dumitru Ceara wrote: > On 2/11/25 9:35 AM, Felix Huettner via dev wrote: >> Previously all routes of a logical router where announced. However in >> some cases it makes more sense to only announce static or connected >> routes. Therefor we add options to LR and LRP to define which routes to >> advertise. >> >> Acked-by: Lorenzo Bianconi <[email protected]> >> Signed-off-by: Felix Huettner <[email protected]> >> --- > > Hi Felix, > >> v7->v8: >> * rework header file based on suggestions by Dumitru >> v6->v7: >> * addressed review comments >> v5->v6: >> * addressed review comments >> * changed option to "dynamic-routing-redistribute" >> v4->v5: skipped >> v2->v3: >> * A lot of minor review comments. >> * Reworked NEWs entry to make default more clear >> >> NEWS | 8 ++- >> northd/en-advertised-route-sync.c | 11 ++++ >> northd/northd.c | 41 +++++++++++++++ >> northd/northd.h | 33 ++++++++++++ >> ovn-nb.xml | 51 ++++++++++++++++++ >> tests/ovn-northd.at | 87 ++++++++++++++++++++++++++++++- >> 6 files changed, 227 insertions(+), 4 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index 8c1c5c863..94358417a 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -55,9 +55,13 @@ Post v24.09.0 >> - Added vxlan_mode parameter in IC_NB_GLOBAL option column to enable or >> disable VXLAN encapsulation type in OVN-interconnect mode. >> - Dynamic Routing: >> - * Add the option "dynamic-routing" to Logical Routers. If set to true >> all >> - static and connected routes attached to the router are shared to the >> + * Add the option "dynamic-routing" to Logical Routers. If set to true >> + static and connected routes matching the below filter are shared to >> the >> southbound "Advertised_Route" table for sharing outside of OVN. >> + The routes can further be configured by setting >> + `dynamic-routing-redistribute` on the LR or LRP. The LRP settings >> + overwrite the LR settings for all routes using this interface to >> + forward traffic on. >> >> OVN v24.09.0 - 13 Sep 2024 >> -------------------------- >> diff --git a/northd/en-advertised-route-sync.c >> b/northd/en-advertised-route-sync.c >> index 3c5065154..7da76491d 100644 >> --- a/northd/en-advertised-route-sync.c >> +++ b/northd/en-advertised-route-sync.c >> @@ -149,6 +149,17 @@ advertised_route_table_sync( >> continue; >> } >> >> + enum dynamic_routing_redistribute_mode drr = >> + route->out_port->dynamic_routing_redistribute; >> + if (route->source == ROUTE_SOURCE_CONNECTED && >> + !drr_mode_CONNECTED_is_set(drr)) { >> + continue; >> + } >> + if (route->source == ROUTE_SOURCE_STATIC && >> + !drr_mode_STATIC_is_set(drr)) { >> + continue; >> + } >> + >> char *ip_prefix = normalize_v46_prefix(&route->prefix, route->plen); >> route_e = ar_add_entry(&sync_routes, route->od->sb, >> route->out_port->sb, ip_prefix); >> diff --git a/northd/northd.c b/northd/northd.c >> index 261a02e17..087332d05 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -812,6 +812,43 @@ ovn_datapath_update_external_ids(struct ovn_datapath >> *od) >> smap_destroy(&ids); >> } >> >> +static enum dynamic_routing_redistribute_mode >> +parse_dynamic_routing_redistribute( >> + const struct smap *options, >> + enum dynamic_routing_redistribute_mode default_dynamic_mode) >> +{ >> + char *save_ptr = NULL; >> + enum dynamic_routing_redistribute_mode out = DRRM_NONE; >> + >> + const char *dynamic_routing_redistribute = smap_get( >> + options, "dynamic-routing-redistribute"); >> + if (!dynamic_routing_redistribute) { >> + return default_dynamic_mode; >> + } >> + >> + char *tokstr = xstrdup(dynamic_routing_redistribute); >> + >> + for (char *token = strtok_r(tokstr, ";", &save_ptr); >> + token != NULL; >> + token = strtok_r(NULL, ";", &save_ptr)) { >> + > > I didn't go through all the patches in this version yet but I'm assuming > it's probably ready to be pushed. I would however like to make a change > when applying it. Would you mind if I change the separator to "," > instead of ";"? Like that it would be more in sync with the > dynamic-routing-port-mapping option format you're adding in patch 12/16. > That one doesn't support ";" as separator. > > Please let me know what you think. > > I'll review the rest of the series in the meantime. > > Thanks, > Dumitru > >> + if (!strcmp(token, "connected")) { >> + out |= DRRM_CONNECTED; >> + continue; >> + } >> + if (!strcmp(token, "static")) { >> + out |= DRRM_STATIC; >> + continue; >> + } >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >> + VLOG_WARN_RL(&rl, "unkown dynamic-routing-redistribute option '%s'", >> + token); >> + } >> + >> + free(tokstr); >> + return out; >> +} >> + >> static void >> join_datapaths(const struct nbrec_logical_switch_table *nbrec_ls_table, >> const struct nbrec_logical_router_table *nbrec_lr_table, >> @@ -912,6 +949,8 @@ join_datapaths(const struct nbrec_logical_switch_table >> *nbrec_ls_table, >> } >> od->dynamic_routing = smap_get_bool(&od->nbr->options, >> "dynamic-routing", false); >> + od->dynamic_routing_redistribute = >> parse_dynamic_routing_redistribute( >> + &od->nbr->options, DRRM_NONE); >> ovs_list_push_back(lr_list, &od->lr_list); >> } >> } >> @@ -2264,6 +2303,8 @@ join_logical_ports_lrp(struct hmap *ports, >> >> op->prefix_delegation = smap_get_bool(&op->nbrp->options, >> "prefix_delegation", false); >> + op->dynamic_routing_redistribute = parse_dynamic_routing_redistribute( >> + &op->nbrp->options, od->dynamic_routing_redistribute); >> >> for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) { >> sset_add(&op->od->router_ips, >> diff --git a/northd/northd.h b/northd/northd.h >> index 9aa0934fa..d63b8013a 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -307,6 +307,32 @@ struct mcast_port_info { >> * (e.g., IGMP join/leave). */ >> }; >> >> +#define DRR_MODES \ >> + DRR_MODE(CONNECTED, 0) \ >> + DRR_MODE(STATIC, 1) >> + >> +enum dynamic_routing_redistribute_mode_bits { >> +#define DRR_MODE(PROTOCOL, BIT) DRRM_##PROTOCOL##_BIT = BIT, >> + DRR_MODES >> +#undef DRR_MODE >> +}; >> + >> +enum dynamic_routing_redistribute_mode { >> + DRRM_NONE = 0, >> +#define DRR_MODE(PROTOCOL, BIT) DRRM_##PROTOCOL = (1 << >> DRRM_##PROTOCOL##_BIT), >> + DRR_MODES >> +#undef DRR_MODE >> +}; >> + >> +#define DRR_MODE(PROTOCOL, BIT) \ >> + static inline bool drr_mode_##PROTOCOL##_is_set( \ >> + enum dynamic_routing_redistribute_mode value) \ >> + { \ >> + return !!(value & DRRM_##PROTOCOL); \ >> + } >> +DRR_MODES >> +#undef DRR_MODE >> + >> /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or >> * sb->external_ids:logical-switch. */ >> struct ovn_datapath { >> @@ -373,6 +399,8 @@ struct ovn_datapath { >> bool redirect_bridged; >> /* nbr has the option "dynamic-routing" set to true. */ >> bool dynamic_routing; >> + /* The modes contained in the nbr option >> "dynamic-routing-redistribute". */ >> + enum dynamic_routing_redistribute_mode dynamic_routing_redistribute; >> >> struct ovn_port **localnet_ports; >> size_t n_localnet_ports; >> @@ -628,6 +656,11 @@ struct ovn_port { >> struct lport_addresses lrp_networks; >> bool prefix_delegation; /* True if IPv6 prefix delegation enabled. */ >> >> + /* The modes contained in the nbrp option >> "dynamic-routing-redistribute". >> + * If the option is unset it will be initialized based on the nbr >> + * option. */ >> + enum dynamic_routing_redistribute_mode dynamic_routing_redistribute; >> + >> /* Logical port multicast data. */ >> struct mcast_port_info mcast_info; >> >> diff --git a/ovn-nb.xml b/ovn-nb.xml >> index 2a178ab06..ae5e483d1 100644 >> --- a/ovn-nb.xml >> +++ b/ovn-nb.xml >> @@ -3041,6 +3041,36 @@ or >> applied to this Logical Router >> </li> >> </ul> >> + >> + Users will need to use the following settings to opt into individual >> + route types that should be advertised. See: >> + <ul> >> + <li><ref column="options" key="dynamic-routing-redistribute" >> + table="Logical_Router"/></li> >> + <li><ref column="options" key="dynamic-routing-redistribute" >> + table="Logical_Router_Port"/></li> >> + </ul> >> + </column> >> + >> + <column name="options" key="dynamic-routing-redistribute" >> + type='{"type": "string"}'> >> + Only relevant if <ref column="options" key="dynamic-routing" >> + table="Logical_Router"/> is set to <code>true</code>. >> + >> + This is a list of elements separated by <code>;</code>. >> + >> + If <code>connected</code> is in the list then northd will >> synchronize >> + all "connected" routes to the southbound <ref table="Route" >> + db="OVN_SB"/> table. "Connected" here means routes implicitly >> created >> + by networks associated with the LRPs. >> + >> + If <code>static</code> is in the list then northd will synchronize >> all >> + <ref table="Logical_Router_Static_Route"/> to the southbound >> + <ref table="Route" db="OVN_SB"/> table. >> + >> + This value can be overwritten on a per LRP basis using >> + <ref column="options" key="dynamic-routing-redistribute" >> + table="Logical_Router_Port"/>. >> </column> >> </group> >> >> @@ -3808,7 +3838,28 @@ or >> status of <code>Transit Router</code> see >> <ref table="Logical_Router"/> table for more details. >> </p> >> + </column> >> >> + <column name="options" key="dynamic-routing-redistribute" >> + type='{"type": "string"}'> >> + Only relevant if <ref column="options" key="dynamic-routing" >> + table="Logical_Router"/> on the respective Logical_Router is set >> + to <code>true</code>. >> + >> + This is a list of elements separated by <code>;</code>. >> + >> + If <code>connected</code> is in the list then northd will >> synchronize >> + all "connected" routes to the southbound <ref table="Route" >> + db="OVN_SB"/> table. "Connected" here means routes implicitly >> created >> + by networks associated with the LRPs. >> + >> + If <code>static</code> is in the list then northd will synchronize >> all >> + <ref table="Logical_Router_Static_Route"/> to the southbound >> + <ref table="Route" db="OVN_SB"/> table. >> + >> + If not set the value from <ref column="options" >> + key="dynamic-routing-redistribute" table="Logical_Router"/> will be >> + used. >> </column> >> </group> >> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 850c6aeb7..6cf08c8ea 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -14925,7 +14925,8 @@ ovn_start >> >> # Adding a router - no route advertised. >> check ovn-nbctl lr-add lr0 >> -check ovn-nbctl --wait=sb set Logical_Router lr0 option:dynamic-routing=true >> +check ovn-nbctl --wait=sb set Logical_Router lr0 >> option:dynamic-routing=true \ >> + >> option:dynamic-routing-redistribute="connected;static" >> check_row_count Advertised_Route 0 >> datapath=$(fetch_column datapath_binding _uuid external_ids:name=lr0) >> >> @@ -14978,6 +14979,59 @@ check_row_count Advertised_Route 0 >> AT_CLEANUP >> ]) >> >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> +AT_SETUP([dynamic-routing - sync to sb filtering]) >> +AT_KEYWORDS([dynamic-routing]) >> +ovn_start >> + >> +# We start with announcing everything on a lr with 2 lrps and 2 static >> routes. >> +check ovn-nbctl lr-add lr0 >> +check ovn-nbctl --wait=sb set Logical_Router lr0 >> option:dynamic-routing=true \ >> + >> option:dynamic-routing-redistribute="connected;static" >> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 >> +sw0=$(fetch_column port_binding _uuid logical_port=lr0-sw0) >> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 >> 2001:db8::1/64 >> +sw1=$(fetch_column port_binding _uuid logical_port=lr0-sw1) >> +check ovn-nbctl --wait=sb lr-route-add lr0 192.168.0.0/24 10.0.0.10 >> +check ovn-nbctl --wait=sb lr-route-add lr0 2001:db8:1::/64 2001:db8::10 >> +check_row_count Advertised_Route 4 >> +datapath=$(fetch_column datapath_binding _uuid external_ids:name=lr0) >> + >> +# Disabling connected routes just keeps the static ones. >> +check ovn-nbctl --wait=sb set Logical_Router lr0 >> option:dynamic-routing-redistribute="static" >> +check_row_count Advertised_Route 2 >> +check_column 192.168.0.0/24 Advertised_Route ip_prefix datapath=$datapath >> logical_port=$sw0 >> +check_column 2001:db8:1::/64 Advertised_Route ip_prefix datapath=$datapath >> logical_port=$sw1 >> + >> +# Enabling it on lr0-sw0 will just bring this one route back. >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 >> option:dynamic-routing-redistribute="connected;static" >> +check_row_count Advertised_Route 3 >> +check_row_count Advertised_Route 2 logical_port=$sw0 >> +check_row_count Advertised_Route 1 logical_port=$sw0 ip_prefix=10.0.0.0/24 >> +check_row_count Advertised_Route 1 logical_port=$sw0 >> ip_prefix=192.168.0.0/24 >> + >> +# Disabling static routes just keeps the one explicit connected route. >> +check ovn-nbctl --wait=sb remove Logical_Router lr0 option >> dynamic-routing-redistribute >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 >> option:dynamic-routing-redistribute="connected" >> +check_row_count Advertised_Route 1 >> +check_column 10.0.0.0/24 Advertised_Route ip_prefix datapath=$datapath >> logical_port=$sw0 >> + >> +# Enabling static routes on the LR, but disabeling them on lr0-sw0 also >> works. >> +check ovn-nbctl --wait=sb set Logical_Router lr0 >> option:dynamic-routing-redistribute="static" >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 >> option:dynamic-routing-redistribute="connected" >> +check_row_count Advertised_Route 2 >> +check_column 10.0.0.0/24 Advertised_Route ip_prefix datapath=$datapath >> logical_port=$sw0 >> +check_column 2001:db8:1::/64 Advertised_Route ip_prefix datapath=$datapath >> logical_port=$sw1 >> + >> +# Setting an empty dynamic-routing-redistribute will block advertisements >> from >> +# this interface. >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 >> option:dynamic-routing-redistribute='""' >> +check_row_count Advertised_Route 1 >> +check_column 2001:db8:1::/64 Advertised_Route ip_prefix datapath=$datapath >> logical_port=$sw1 >> + >> +AT_CLEANUP >> +]) >> + >> OVN_FOR_EACH_NORTHD_NO_HV([ >> AT_SETUP([dynamic-routing incremental processing]) >> AT_KEYWORDS([dynamic-routing]) >> @@ -14989,7 +15043,8 @@ ovn_start >> check ovn-nbctl --wait=sb sync >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> check ovn-nbctl lr-add lr0 >> -check ovn-nbctl --wait=sb set Logical_Router lr0 option:dynamic-routing=true >> +check ovn-nbctl --wait=sb set Logical_Router lr0 >> option:dynamic-routing=true \ >> + >> option:dynamic-routing-redistribute="connected;static" >> >> check_engine_stats northd recompute nocompute >> check_engine_stats routes recompute nocompute >> @@ -15038,6 +15093,34 @@ check_engine_stats routes recompute nocompute >> check_engine_stats advertised_route_sync recompute nocompute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb set Logical_Router lr0 >> option:dynamic-routing-redistribute="static" >> +check_engine_stats northd recompute nocompute >> +check_engine_stats routes recompute nocompute >> +check_engine_stats advertised_route_sync recompute nocompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb remove Logical_Router lr0 option >> dynamic-routing-redistribute >> +check_engine_stats northd recompute nocompute >> +check_engine_stats routes recompute nocompute >> +check_engine_stats advertised_route_sync recompute nocompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw1 >> option:dynamic-routing-redistribute="connected" >> +check_engine_stats northd recompute nocompute >> +check_engine_stats routes recompute nocompute >> +check_engine_stats advertised_route_sync recompute nocompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 >> option:dynamic-routing-redistribute="connected" >> +check_engine_stats northd recompute nocompute >> +check_engine_stats routes recompute nocompute >> +check_engine_stats advertised_route_sync recompute nocompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> check ovn-nbctl --wait=sb lrp-del lr0-sw0 >> check_engine_stats northd recompute compute
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
