On 2/6/25 9:11 AM, Frode Nordahl wrote:
> On Wed, Feb 5, 2025 at 4:11 PM Felix Huettner via dev
> <[email protected]> wrote:
>>
>> On Wed, Feb 05, 2025 at 02:31:07PM +0100, Dumitru Ceara wrote:
>>> On 2/4/25 2:59 PM, Felix Huettner via dev wrote:
>>>> Sometimes we want to use individual host routes instead of the connected
>>>> routes of LRPs.
>>>> This allows the network fabric to know which addresses are actually in
>>>> use and e.g. drop traffic to addresses that are not used anyway.
>>>>
>>>> Signed-off-by: Felix Huettner <[email protected]>
>>>> ---
>>>
>>> Hi Felix,
>>>
>>> I have 3 minor comments below. If those are the only changes in v7,
>>> feel free to add my ack:
>>
>> Hi Dumitru,
>>
>> thanks for the review.
>> I will fix the changes below in v7.
>>
>> There will be other changes on this patch based on comments by Lorenzo.
>>
>> Thanks a lot,
>> Felix
>>
>>>
>>> Acked-by: Dumitru Ceara <[email protected]>
>>>
>>>> v5->v6:
>>>> * addressed review comments
>>>> v4->v5: skipped
>>>> v3->v4:
>>>> * fix a memory leak
>>>> v2->v3:
>>>> * A lot of minor review comments.
>>>>
>>>> NEWS | 3 +
>>>> northd/en-advertised-route-sync.c | 227 ++++++++++++++++++++++++++++--
>>>> northd/en-advertised-route-sync.h | 11 ++
>>>> northd/inc-proc-northd.c | 4 +
>>>> northd/northd.c | 35 +++--
>>>> northd/northd.h | 22 +++
>>>> ovn-nb.xml | 27 ++++
>>>> tests/ovn-northd.at | 113 ++++++++++++++-
>>>> 8 files changed, 407 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/NEWS b/NEWS
>>>> index b0ca1992e..3547f659f 100644
>>>> --- a/NEWS
>>>> +++ b/NEWS
>>>> @@ -50,6 +50,9 @@ Post v24.09.0
>>>> fabric. Routes entered into the "Learned_Route" table in the
>>>> southbound
>>>> database will be learned by the respective LR. They are included
>>>> in the
>>>> route table with a lower priority than static routes.
>>>> + * Add the option "dynamic-routing-connected-as-host-routes" to LRPs.
>>>> If
>>>> + set to true then connected routes are announced as individual host
>>>> + routes.
>
> Patch 2 replaced dynamic-routing-connected option with parsing of
> dynamic-routing as separated string.
>
> Should perhaps this option also be treated this way? As in
> options:dynamic-routing=connected-as-host as an alternative to
> specifying options:dynamic-routing=connected.
>
Do you mean supporting
"options:dynamic-routing=connected-as-host;static" OR
"options:dynamic-routing=connected;static" OR any other valid "protocol"
combinations?
If so, then I agree, it's probably better.
>>>>
>>>> OVN v24.09.0 - 13 Sep 2024
>>>> --------------------------
>>>> diff --git a/northd/en-advertised-route-sync.c
>>>> b/northd/en-advertised-route-sync.c
>>>> index d4360763f..a0087c71a 100644
>>>> --- a/northd/en-advertised-route-sync.c
>>>> +++ b/northd/en-advertised-route-sync.c
>>>> @@ -20,6 +20,7 @@
>>>> #include "northd.h"
>>>>
>>>> #include "en-advertised-route-sync.h"
>>>> +#include "en-lr-stateful.h"
>>>> #include "lib/stopwatch-names.h"
>>>> #include "openvswitch/hmap.h"
>>>> #include "ovn-util.h"
>>>> @@ -28,34 +29,131 @@ static void
>>>> advertised_route_table_sync(
>>>> struct ovsdb_idl_txn *ovnsb_txn,
>>>> const struct sbrec_advertised_route_table
>>>> *sbrec_advertised_route_table,
>>>> - const struct hmap *parsed_routes);
>>>> + const struct lr_stateful_table *lr_stateful_table,
>>>> + const struct hmap *parsed_routes,
>>>> + struct advertised_route_sync_data *data);
>>>> +
>>>> +bool
>>>> +advertised_route_sync_lr_stateful_change_handler(struct engine_node *node,
>>>> + void *data_)
>>>> +{
>>>> + /* We only actually use lr_stateful data if we expose individual host
>>>> + * routes. In this case we for now just recompute.
>>>> + * */
>>>> + struct ed_type_lr_stateful *lr_stateful_data =
>>>> + engine_get_input_data("lr_stateful", node);
>>>> + struct advertised_route_sync_data *data = data_;
>>>> +
>>>> + struct hmapx_node *hmapx_node;
>>>> + const struct lr_stateful_record *lr_stateful_rec;
>>>> + HMAPX_FOR_EACH (hmapx_node, &lr_stateful_data->trk_data.crupdated) {
>>>> + lr_stateful_rec = hmapx_node->data;
>>>> + if (uuidset_contains(&data->nb_lr,
>>>> + &lr_stateful_rec->nbr_uuid)) {
>>>> + return false;
>>>> + }
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +bool
>>>> +advertised_route_sync_northd_change_handler(struct engine_node *node,
>>>> + void *data_)
>>>> +{
>>>> + struct advertised_route_sync_data *data = data_;
>>>> + struct northd_data *northd_data = engine_get_input_data("northd",
>>>> node);
>>>> + if (!northd_has_tracked_data(&northd_data->trk_data)) {
>>>> + return false;
>>>> + }
>>>> +
>>>> + /* We indirectly use northd_data->ls_ports if we announce host routes.
>>>> + * For now we just recompute on any change to lsps that are relevant
>>>> to us.
>>>> + */
>>>> + struct hmapx_node *hmapx_node;
>>>> + const struct ovn_port *op;
>>>> + HMAPX_FOR_EACH (hmapx_node, &northd_data->trk_data.trk_lsps.created) {
>>>> + op = hmapx_node->data;
>>>> + if (uuidset_contains(&data->nb_ls,
>>>> + &op->od->nbs->header_.uuid)) {
>>>> + return false;
>>>> + }
>>>> + }
>>>> + HMAPX_FOR_EACH (hmapx_node, &northd_data->trk_data.trk_lsps.updated) {
>>>> + op = hmapx_node->data;
>>>> + if (uuidset_contains(&data->nb_ls,
>>>> + &op->od->nbs->header_.uuid)) {
>>>> + return false;
>>>> + }
>>>> + }
>>>> + HMAPX_FOR_EACH (hmapx_node, &northd_data->trk_data.trk_lsps.deleted) {
>>>> + op = hmapx_node->data;
>>>> + if (uuidset_contains(&data->nb_ls,
>>>> + &op->od->nbs->header_.uuid)) {
>>>> + return false;
>>>> + }
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +static void
>>>> +routes_sync_init(struct advertised_route_sync_data *data)
>>>> +{
>>>> + uuidset_init(&data->nb_lr);
>>>> + uuidset_init(&data->nb_ls);
>>>> +}
>>>> +
>>>> +static void
>>>> +routes_sync_clear(struct advertised_route_sync_data *data)
>>>> +{
>>>> + uuidset_clear(&data->nb_lr);
>>>> + uuidset_clear(&data->nb_ls);
>>>> +}
>>>> +
>>>> +static void
>>>> +routes_sync_destroy(struct advertised_route_sync_data *data)
>>>> +{
>>>> + uuidset_destroy(&data->nb_lr);
>>>> + uuidset_destroy(&data->nb_ls);
>>>> +}
>>>>
>>>> void
>>>> *en_advertised_route_sync_init(struct engine_node *node OVS_UNUSED,
>>>> struct engine_arg *arg OVS_UNUSED)
>>>> {
>>>> - return NULL;
>>>> + struct advertised_route_sync_data *data = xzalloc(sizeof *data);
>>>> + routes_sync_init(data);
>>>> + return data;
>>>> }
>>>>
>>>> void
>>>> en_advertised_route_sync_cleanup(void *data OVS_UNUSED)
>>>> {
>>>> + routes_sync_destroy(data);
>>>> }
>>>>
>>>> void
>>>> en_advertised_route_sync_run(struct engine_node *node, void *data
>>>> OVS_UNUSED)
>>>> {
>>>> + routes_sync_clear(data);
>>>> +
>>>> + struct advertised_route_sync_data *routes_sync_data = data;
>>>> struct routes_data *routes_data
>>>> = engine_get_input_data("routes", node);
>>>> const struct engine_context *eng_ctx = engine_get_context();
>>>> const struct sbrec_advertised_route_table
>>>> *sbrec_advertised_route_table =
>>>> EN_OVSDB_GET(engine_get_input("SB_advertised_route", node));
>>>> + struct ed_type_lr_stateful *lr_stateful_data =
>>>> + engine_get_input_data("lr_stateful", node);
>>>>
>>>> stopwatch_start(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME,
>>>> time_msec());
>>>>
>>>> advertised_route_table_sync(eng_ctx->ovnsb_idl_txn,
>>>> sbrec_advertised_route_table,
>>>> - &routes_data->parsed_routes);
>>>> + &lr_stateful_data->table,
>>>> + &routes_data->parsed_routes,
>>>> + routes_sync_data);
>>>>
>>>> stopwatch_stop(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME, time_msec());
>>>> engine_set_node_state(node, EN_UPDATED);
>>>> @@ -68,19 +166,24 @@ struct ar_entry {
>>>>
>>>> const struct sbrec_port_binding *logical_port;
>>>> char *ip_prefix;
>>>> + const struct sbrec_port_binding *tracked_port;
>>>> };
>>>>
>>>> /* Add a new entries to the to-be-advertised routes.
>>>> * Takes ownership of ip_prefix. */
>>>> static struct ar_entry *
>>>> ar_add_entry(struct hmap *routes, const struct sbrec_datapath_binding
>>>> *sb_db,
>>>> - const struct sbrec_port_binding *logical_port, char
>>>> *ip_prefix)
>>>> + const struct sbrec_port_binding *logical_port, char
>>>> *ip_prefix,
>>>> + const struct sbrec_port_binding *tracked_port)
>>>
>>> Indentation is off here, these two lines should start 2 columns to the left.
>>>
>>>> {
>>>> struct ar_entry *route_e = xzalloc(sizeof *route_e);
>>>>
>>>> route_e->sb_db = sb_db;
>>>> route_e->logical_port = logical_port;
>>>> route_e->ip_prefix = ip_prefix;
>>>> + if (tracked_port) {
>>>> + route_e->tracked_port = tracked_port;
>>>> + }
>>>
>>> Nit: the if is not needed, this can be:
>>>
>>> route_e->tracked_port = tracked_port;
>>>
>>>> uint32_t hash = uuid_hash(&sb_db->header_.uuid);
>>>> hash = hash_string(logical_port->logical_port, hash);
>>>> hash = hash_string(ip_prefix, hash);
>>>> @@ -91,7 +194,8 @@ ar_add_entry(struct hmap *routes, const struct
>>>> sbrec_datapath_binding *sb_db,
>>>>
>>>> static struct ar_entry *
>>>> ar_find(struct hmap *route_map, const struct sbrec_datapath_binding
>>>> *sb_db,
>>>> - const struct sbrec_port_binding *logical_port, const char
>>>> *ip_prefix)
>>>> + const struct sbrec_port_binding *logical_port, const char
>>>> *ip_prefix,
>>>> + const struct sbrec_port_binding *tracked_port)
>>>> {
>>>> struct ar_entry *route_e;
>>>> uint32_t hash;
>>>> @@ -114,6 +218,10 @@ ar_find(struct hmap *route_map, const struct
>>>> sbrec_datapath_binding *sb_db,
>>>> continue;
>>>> }
>>>>
>>>> + if (tracked_port != route_e->tracked_port) {
>>>> + continue;
>>>> + }
>>>> +
>>>> return route_e;
>>>> }
>>>>
>>>> @@ -127,13 +235,94 @@ ar_entry_free(struct ar_entry *route_e)
>>>> free(route_e);
>>>> }
>>>>
>>>> +static void
>>>> +publish_lport_addresses(struct hmap *sync_routes,
>>>> + const struct sbrec_datapath_binding *sb_db,
>>>> + const struct ovn_port *logical_port,
>>>> + struct lport_addresses *addresses,
>>>> + const struct ovn_port *tracking_port)
>>>> +{
>>>> + for (size_t i = 0; i < addresses->n_ipv4_addrs; i++) {
>>>> + const struct ipv4_netaddr *addr = &addresses->ipv4_addrs[i];
>>>> + char *addr_s = xasprintf("%s/32", addr->addr_s);
>>>> + ar_add_entry(sync_routes, sb_db, logical_port->sb,
>>>> + addr_s, tracking_port->sb);
>>>> + }
>>>> + for (size_t i = 0; i < addresses->n_ipv6_addrs; i++) {
>>>> + if (in6_is_lla(&addresses->ipv6_addrs[i].network)) {
>>>> + continue;
>>>> + }
>>>> + const struct ipv6_netaddr *addr = &addresses->ipv6_addrs[i];
>>>> + char *addr_s = xasprintf("%s/128", addr->addr_s);
>>>> + ar_add_entry(sync_routes, sb_db, logical_port->sb,
>>>> + addr_s, tracking_port->sb);
>>>> + }
>>>> +}
>>>> +
>>>> +/* Collect all IP addresses connected to the out_port of a route.
>>>> + * This traverses all LSPs on the LS connected to the out_port. */
>>>> +static void
>>>> +publish_host_routes(struct hmap *sync_routes,
>>>> + const struct lr_stateful_table *lr_stateful_table,
>>>> + const struct parsed_route *route,
>>>> + struct advertised_route_sync_data *data)
>>>> +{
>>>> + struct ovn_port *port;
>>>> + struct ovn_datapath *lsp_od = route->out_port->peer->od;
>>>> +
>>>> + if (!lsp_od->nbs) {
>>>> + return;
>>>> + }
>>>> +
>>>> + /* We need to track the LS we are publishing routes from, so that we
>>>> can
>>>> + * recompute when any port on there changes. */
>>>> + uuidset_insert(&data->nb_ls, &lsp_od->nbs->header_.uuid);
>>>> + HMAP_FOR_EACH (port, dp_node, &lsp_od->ports) {
>>>> + if (port->peer) {
>>>> + /* This is a LSP connected to an LRP */
>>>> + struct lport_addresses *addresses = &port->peer->lrp_networks;
>>>> + publish_lport_addresses(sync_routes, route->od->sb,
>>>> + route->out_port,
>>>> + addresses, port->peer);
>>>> +
>>>> + const struct lr_stateful_record *lr_stateful_rec;
>>>> + lr_stateful_rec = lr_stateful_table_find_by_index(
>>>> + lr_stateful_table, port->peer->od->index);
>>>> + /* We also need to track this LR as we need to recompute when
>>>> + * any of its IPs change. */
>>>> + uuidset_insert(&data->nb_lr,
>>>> + &lr_stateful_rec->nbr_uuid);
>>>> + struct ovn_port_routable_addresses addrs = get_op_addresses(
>>>> + port->peer, lr_stateful_rec, false);
>>>> + for (size_t i = 0; i < addrs.n_addrs; i++) {
>>>> + publish_lport_addresses(sync_routes, route->od->sb,
>>>> + route->out_port,
>>>> + &addrs.laddrs[i],
>>>> + port->peer);
>>>> + }
>>>> + destroy_routable_addresses(&addrs);
>>>> + } else {
>>>> + /* This is just a plain LSP */
>>>> + for (size_t i = 0; i < port->n_lsp_addrs; i++) {
>>>> + publish_lport_addresses(sync_routes, route->od->sb,
>>>> + route->out_port,
>>>> + &port->lsp_addrs[i],
>>>> + port);
>>>> + }
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> static void
>>>> advertised_route_table_sync(
>>>> struct ovsdb_idl_txn *ovnsb_txn,
>>>> const struct sbrec_advertised_route_table
>>>> *sbrec_advertised_route_table,
>>>> - const struct hmap *parsed_routes)
>>>> + const struct lr_stateful_table *lr_stateful_table,
>>>> + const struct hmap *parsed_routes,
>>>> + struct advertised_route_sync_data *data)
>>>> {
>>>> struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes);
>>>> + struct uuidset host_route_lrps =
>>>> UUIDSET_INITIALIZER(&host_route_lrps);
>>>> const struct parsed_route *route;
>>>>
>>>> struct ar_entry *route_e;
>>>> @@ -148,9 +337,21 @@ advertised_route_table_sync(
>>>> if (!route->od->dynamic_routing) {
>>>> continue;
>>>> }
>>>> - if (route->source == ROUTE_SOURCE_CONNECTED &&
>>>> - !route->out_port->dynamic_routing_connected) {
>>>> - continue;
>>>> + if (route->source == ROUTE_SOURCE_CONNECTED) {
>>>> + if (!route->out_port->dynamic_routing_connected) {
>>>> + continue;
>>>> + }
>>>> + /* If we advertise host routes, we only need to do so once per
>>>> + * LRP. */
>>>> + const struct uuid *lrp_uuid =
>>>> + &route->out_port->nbrp->header_.uuid;
>>>> + if (route->out_port->dynamic_routing_connected_as_host_routes
>>>> &&
>>>> + !uuidset_contains(&host_route_lrps, lrp_uuid)) {
>>>> + uuidset_insert(&host_route_lrps, lrp_uuid);
>>>> + publish_host_routes(&sync_routes, lr_stateful_table,
>>>> + route, data);
>>>> + continue;
>>>> + }
>>>> }
>>>> if (route->source == ROUTE_SOURCE_STATIC &&
>>>> !route->out_port->dynamic_routing_static) {
>>>> @@ -160,14 +361,15 @@ advertised_route_table_sync(
>>>> 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);
>>>> + route->out_port->sb, ip_prefix, NULL);
>>>> }
>>>> + uuidset_destroy(&host_route_lrps);
>>>>
>>>> SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH_SAFE (sb_route,
>>>>
>>>> sbrec_advertised_route_table) {
>>>> route_e = ar_find(&sync_routes, sb_route->datapath,
>>>> - sb_route->logical_port,
>>>> - sb_route->ip_prefix);
>>>> + sb_route->logical_port, sb_route->ip_prefix,
>>>> + sb_route->tracked_port);
>>>> if (route_e) {
>>>> hmap_remove(&sync_routes, &route_e->hmap_node);
>>>> ar_entry_free(route_e);
>>>> @@ -182,6 +384,7 @@ advertised_route_table_sync(
>>>> sbrec_advertised_route_set_datapath(sr, route_e->sb_db);
>>>> sbrec_advertised_route_set_logical_port(sr,
>>>> route_e->logical_port);
>>>> sbrec_advertised_route_set_ip_prefix(sr, route_e->ip_prefix);
>>>> + sbrec_advertised_route_set_tracked_port(sr,
>>>> route_e->tracked_port);
>>>> ar_entry_free(route_e);
>>>> }
>>>>
>>>> diff --git a/northd/en-advertised-route-sync.h
>>>> b/northd/en-advertised-route-sync.h
>>>> index 30e7cae1f..1f24fd329 100644
>>>> --- a/northd/en-advertised-route-sync.h
>>>> +++ b/northd/en-advertised-route-sync.h
>>>> @@ -17,10 +17,21 @@
>>>> #define EN_ADVERTISED_ROUTE_SYNC_H 1
>>>>
>>>> #include "lib/inc-proc-eng.h"
>>>> +#include "lib/uuidset.h"
>>>>
>>>> struct advertised_route_sync_data {
>>>> + /* Contains the uuids of all NB Logical Routers where we used a
>>>> + * lr_stateful_record during computation. */
>>>> + struct uuidset nb_lr;
>>>> + /* Contains the uuids of all NB Logical Switches where we rely on port
>>>> + * changes for host routes. */
>>>> + struct uuidset nb_ls;
>>>> };
>>>>
>>>> +bool advertised_route_sync_lr_stateful_change_handler(struct engine_node
>>>> *,
>>>> + void *data);
>>>> +bool advertised_route_sync_northd_change_handler(struct engine_node *,
>>>> + void *data);
>>>> void *en_advertised_route_sync_init(struct engine_node *, struct
>>>> engine_arg *);
>>>> void en_advertised_route_sync_cleanup(void *data);
>>>> void en_advertised_route_sync_run(struct engine_node *, void *data);
>>>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
>>>> index fa7ca6f2d..86b7b999e 100644
>>>> --- a/northd/inc-proc-northd.c
>>>> +++ b/northd/inc-proc-northd.c
>>>> @@ -284,6 +284,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>>>> engine_add_input(&en_advertised_route_sync, &en_routes, NULL);
>>>> engine_add_input(&en_advertised_route_sync, &en_sb_advertised_route,
>>>> NULL);
>>>> + engine_add_input(&en_advertised_route_sync, &en_lr_stateful,
>>>> + advertised_route_sync_lr_stateful_change_handler);
>>>> + engine_add_input(&en_advertised_route_sync, &en_northd,
>>>> + advertised_route_sync_northd_change_handler);
>>>>
>>>> engine_add_input(&en_learned_route_sync, &en_routes, NULL);
>>>> engine_add_input(&en_learned_route_sync, &en_sb_learned_route, NULL);
>>>> diff --git a/northd/northd.c b/northd/northd.c
>>>> index 5cbf6c800..3f14cc75c 100644
>>>> --- a/northd/northd.c
>>>> +++ b/northd/northd.c
>>>> @@ -1122,19 +1122,6 @@ build_datapaths(struct ovsdb_idl_txn *ovnsb_txn,
>>>> ods_build_array_index(lr_datapaths);
>>>> }
>>>>
>>>> -/* Structure representing logical router port
>>>> - * routable addresses. This includes DNAT and Load Balancer
>>>> - * addresses. This structure will only be filled in if the
>>>> - * router port is a gateway router port. Otherwise, all pointers
>>>> - * will be NULL and n_addrs will be 0.
>>>> - */
>>>> -struct ovn_port_routable_addresses {
>>>> - /* The parsed routable addresses */
>>>> - struct lport_addresses *laddrs;
>>>> - /* Number of items in the laddrs array */
>>>> - size_t n_addrs;
>>>> -};
>>>> -
>>>> static bool lsp_can_be_inc_processed(const struct
>>>> nbrec_logical_switch_port *);
>>>>
>>>> /* This function returns true if 'op' is a gateway router port.
>>>> @@ -1169,7 +1156,7 @@ is_cr_port(const struct ovn_port *op)
>>>> return op->primary_port;
>>>> }
>>>>
>>>> -static void
>>>> +void
>>>> destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
>>>> {
>>>> for (size_t i = 0; i < ra->n_addrs; i++) {
>>>> @@ -1182,12 +1169,14 @@ static char **get_nat_addresses(const struct
>>>> ovn_port *op, size_t *n,
>>>> bool routable_only, bool include_lb_ips,
>>>> const struct lr_stateful_record *);
>>>>
>>>> -static struct ovn_port_routable_addresses
>>>> -get_op_routable_addresses(struct ovn_port *op,
>>>> - const struct lr_stateful_record
>>>> *lr_stateful_rec)
>>>> +struct ovn_port_routable_addresses
>>>> +get_op_addresses(struct ovn_port *op,
>>>> + const struct lr_stateful_record *lr_stateful_rec,
>>>> + bool routable_only)
>>>> {
>>>> size_t n;
>>>> - char **nats = get_nat_addresses(op, &n, true, true, lr_stateful_rec);
>>>> + char **nats = get_nat_addresses(op, &n, routable_only, true,
>>>> + lr_stateful_rec);
>>>>
>>>> if (!nats) {
>>>> return (struct ovn_port_routable_addresses) {
>>>> @@ -1220,6 +1209,13 @@ get_op_routable_addresses(struct ovn_port *op,
>>>> };
>>>> }
>>>>
>>>> +static struct ovn_port_routable_addresses
>>>> +get_op_routable_addresses(struct ovn_port *op,
>>>> + const struct lr_stateful_record
>>>> *lr_stateful_rec)
>>>> +{
>>>> + return get_op_addresses(op, lr_stateful_rec, true);
>>>> +}
>>>> +
>>>>
>>>> static void
>>>> ovn_port_set_nb(struct ovn_port *op,
>>>> @@ -2267,6 +2263,9 @@ join_logical_ports_lrp(struct hmap *ports,
>>>>
>>>> op->prefix_delegation = smap_get_bool(&op->nbrp->options,
>>>> "prefix_delegation", false);
>>>> + op->dynamic_routing_connected_as_host_routes = smap_get_bool(
>>>> + &op->nbrp->options,
>>>> + "dynamic-routing-connected-as-host-routes", false);
>>>> parse_dynamic_routing_redistribute(&op->nbrp->options,
>>>> &op->dynamic_routing_connected,
>>>> od->dynamic_routing_connected,
>>>> diff --git a/northd/northd.h b/northd/northd.h
>>>> index 75a3df617..15bd01c05 100644
>>>> --- a/northd/northd.h
>>>> +++ b/northd/northd.h
>>>> @@ -25,6 +25,7 @@
>>>> #include "openvswitch/hmap.h"
>>>> #include "simap.h"
>>>> #include "ovs-thread.h"
>>>> +#include "en-lr-stateful.h"
>>>>
>>>> struct northd_input {
>>>> /* Northbound table references */
>>>> @@ -625,6 +626,8 @@ struct ovn_port {
>>>> * If the option is unset it will be initialized based on the nbr
>>>> * option. */
>>>> bool dynamic_routing_connected;
>>>> + /* nbrp option "dynamic-routing-connected-as-host-routes" is "true".
>>>> */
>>>> + bool dynamic_routing_connected_as_host_routes;
>>>> /* nbrp option "dynamic-routing-redistribute" contains "static".
>>>> * If the option is unset it will be initialized based on the nbr
>>>> * option. */
>>>> @@ -935,4 +938,23 @@ void build_igmp_lflows(struct hmap *igmp_groups,
>>>> const struct hmap *ls_datapaths,
>>>> struct lflow_table *lflows,
>>>> struct lflow_ref *lflow_ref);
>>>
>>> We could use an empty line here.
>>>
>>>> +/* Structure representing logical router port routable addresses. This
>>>> + * includes DNAT and Load Balancer addresses. This structure will only
>>>> + * be filled in if the router port is a gateway router port. Otherwise,
>>>> + * all pointers will be NULL and n_addrs will be 0.
>>>> + */
>>>> +struct ovn_port_routable_addresses {
>>>> + /* The parsed routable addresses */
>>>> + struct lport_addresses *laddrs;
>>>> + /* Number of items in the laddrs array */
>>>> + size_t n_addrs;
>>>> +};
>>>> +
>>>> +struct ovn_port_routable_addresses get_op_addresses(
>>>> + struct ovn_port *op,
>>>> + const struct lr_stateful_record *lr_stateful_rec,
>>>> + bool routable_only);
>>>> +
>>>> +void destroy_routable_addresses(struct ovn_port_routable_addresses *ra);
>>>> +
>>>> #endif /* NORTHD_H */
>>>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>>>> index bf785e4d3..d0bdb5058 100644
>>>> --- a/ovn-nb.xml
>>>> +++ b/ovn-nb.xml
>>>> @@ -3752,6 +3752,33 @@ or
>>>> key="dynamic-routing-redistribute" table="Logical_Router"/> will
>>>> be
>>>> used.
>>>> </column>
>>>> + <column name="options"
>>>> key="dynamic-routing-connected-as-host-routes"
>>>> + type='{"type": "boolean"}'>
>>>> + Only relevant if <ref column="options" key="dynamic-routing"
>>>> + table="Logical_Router"/> on the respective Logical_Router is set
>>>> + to <code>true</code> and also
>>>> + <ref column="options" key="dynamic-routing-connected"/> is
>>>> enabled on
>
> This is not in sync with the actual implementation now that
> dynamic-routing accepts a separated string with these options.
>
> --
> Frode Nordahl
>
>>>> + the LR or LRP.
>>>> +
>>>> + If set to true the prefix connected to the LRP is not advertised
>>>> as a
>>>> + whole. Rather each individual IP address that is actually in use
>>>> inside
>>>> + this prefix is announced as a host route. Default is false.
>>>> +
>>>> + This can be used to:
>>>> + <ul>
>>>> + <li>
>>>> + allow the fabric outside of OVN to drop traffic towards IP
>>>> + addresses that are not actually used. This traffic would
>>>> otherwise
>>>> + hit this LR and then be dropped.
>>>> + </li>
>>>> +
>>>> + <li>
>>>> + If this LR has multiple LRPs connected to the fabric on
>>>> different
>>>> + chassis: allows the fabric outside of OVN to steer packets to
>>>> the
>>>> + chassis which already hosts this backing ip address.
>>>> + </li>
>>>> + </ul>
>>>> + </column>
>>>> </group>
>>>>
>>>> <group title="Attachment">
>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>>> index 52a458e35..18d367fa5 100644
>>>> --- a/tests/ovn-northd.at
>>>> +++ b/tests/ovn-northd.at
>>>> @@ -14845,6 +14845,66 @@ AT_CHECK([grep -w "lr_in_ip_routing" lr0flows |
>>>> ovn_strip_lflows], [0], [dnl
>>>> AT_CLEANUP
>>>> ])
>>>>
>>>> +OVN_FOR_EACH_NORTHD_NO_HV([
>>>> +AT_SETUP([dynamic-routing - host routes])
>>>> +AT_KEYWORDS([dynamic-routing])
>>>> +ovn_start
>>>> +
>>>> +# We start with announcing routes on a lr with 2 lrps.
>>>> +# LRP lr0-sw0 is connected to ls sw0.
>>>> +check ovn-nbctl lr-add lr0
>>>> +check ovn-nbctl set Logical_Router lr0 option:dynamic-routing=true \
>>>> +
>>>> option:dynamic-routing-redistribute="connected;static"
>>>> +check ovn-nbctl 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 lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 10.0.1.1/24
>>>> +sw1=$(fetch_column port_binding _uuid logical_port=lr0-sw1)
>>>> +check ovn-nbctl ls-add sw0
>>>> +check ovn-nbctl lsp-add sw0 sw0-lr0
>>>> +check ovn-nbctl --wait=sb set Logical_Switch_Port sw0-lr0 type=router
>>>> options:router-port=lr0-sw0
>>>> +check_row_count Advertised_Route 2 tracked_port='[[]]'
>>>> +datapath=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
>>>> +
>>>> +# Configuring the LRP lr0-sw0 to send host routes.
>>>> +# As sw0 is quite empty we will only see the addresses of lr0-sw0.
>>>> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0
>>>> options:dynamic-routing-connected-as-host-routes=true
>>>> +check_row_count Advertised_Route 2
>>>> +check_column 10.0.0.1/32 Advertised_Route ip_prefix datapath=$datapath
>>>> logical_port=$sw0
>>>> +check_column $sw0 Advertised_Route tracked_port datapath=$datapath
>>>> logical_port=$sw0
>>>> +
>>>> +# Adding a VIF to the LS sw0 will advertise it as well.
>>>> +check ovn-nbctl lsp-add sw0 sw0-vif0
>>>> +check ovn-nbctl --wait=sb lsp-set-addresses sw0-vif0 "00:aa:bb:cc:dd:ee
>>>> 10.0.0.2"
>>>> +vif0=$(fetch_column port_binding _uuid logical_port=sw0-vif0)
>>>> +check_row_count Advertised_Route 3
>>>> +check_row_count Advertised_Route 2 tracked_port!='[[]]'
>>>> +check_column $vif0 Advertised_Route tracked_port datapath=$datapath
>>>> logical_port=$sw0 ip_prefix=10.0.0.2/32
>>>> +
>>>> +# Adding a LR lr1 to the LS sw0 will advertise the LRP of the new router.
>>>> +check ovn-nbctl lr-add lr1
>>>> +check ovn-nbctl lrp-add lr1 lr1-sw0 00:00:00:01:ff:01 10.0.0.10/24
>>>> +check ovn-nbctl lsp-add sw0 sw0-lr1
>>>> +lr1=$(fetch_column port_binding _uuid logical_port=lr1-sw0)
>>>> +check ovn-nbctl --wait=sb set Logical_Switch_Port sw0-lr1 type=router
>>>> options:router-port=lr1-sw0
>>>> +check_row_count Advertised_Route 4
>>>> +check_row_count Advertised_Route 3 tracked_port!='[[]]'
>>>> +check_column $lr1 Advertised_Route tracked_port datapath=$datapath
>>>> logical_port=$sw0 ip_prefix=10.0.0.10/32
>>>> +
>>>> +# Adding a NAT rule to lr1 will advertise it as well.
>>>> +check ovn-nbctl --wait=sb lr-nat-add lr1 dnat_and_snat 10.0.0.100
>>>> 192.168.0.1
>>>> +check_row_count Advertised_Route 5
>>>> +check_row_count Advertised_Route 4 tracked_port!='[[]]'
>>>> +check_column $lr1 Advertised_Route tracked_port datapath=$datapath
>>>> logical_port=$sw0 ip_prefix=10.0.0.100/32
>>>> +
>>>> +# Adding a static route to lr1 will be advertised just normally.
>>>> +check ovn-nbctl --wait=sb lr-route-add lr0 172.16.0.0/24 10.0.0.200
>>>> +check_row_count Advertised_Route 6
>>>> +check_row_count Advertised_Route 4 tracked_port!='[[]]'
>>>> +check_row_count Advertised_Route 1 datapath=$datapath logical_port=$sw0
>>>> ip_prefix=172.16.0.0/24
>>>> +
>>>> +AT_CLEANUP
>>>> +])
>>>> +
>>>> OVN_FOR_EACH_NORTHD_NO_HV([
>>>> AT_SETUP([dynamic-routing incremental processing])
>>>> AT_KEYWORDS([dynamic-routing])
>>>> @@ -14868,13 +14928,16 @@ check_engine_stats lflow 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-add lr0 lr0-sw0 00:00:00:00:ff:01
>>>> 10.0.0.1/24
>>>> +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
>>>> +check ovn-nbctl ls-add sw0
>>>> +check ovn-nbctl lsp-add sw0 sw0-lr0
>>>> +check ovn-nbctl --wait=sb set Logical_Switch_Port sw0-lr0 type=router
>>>> options:router-port=lr0-sw0
>>>> sw0=$(fetch_column port_binding _uuid logical_port=lr0-sw0)
>>>> check_engine_stats northd recompute compute
>>>> -check_engine_stats routes recompute nocompute
>>>> -check_engine_stats advertised_route_sync recompute nocompute
>>>> -check_engine_stats learned_route_sync recompute nocompute
>>>> -check_engine_stats lflow recompute nocompute
>>>> +check_engine_stats routes recompute compute
>>>> +check_engine_stats advertised_route_sync recompute compute
>>>> +check_engine_stats learned_route_sync recompute compute
>>>> +check_engine_stats lflow recompute compute
>>>> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>>
>>>> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>> @@ -14980,6 +15043,46 @@ check_engine_stats learned_route_sync recompute
>>>> nocompute
>>>> check_engine_stats lflow 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
>>>> options:dynamic-routing-connected-as-host-routes=true
>>>> +check_engine_stats northd recompute nocompute
>>>> +check_engine_stats routes recompute nocompute
>>>> +check_engine_stats advertised_route_sync recompute nocompute
>>>> +check_engine_stats learned_route_sync recompute nocompute
>>>> +check_engine_stats lflow recompute nocompute
>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>> +
>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>> +check ovn-nbctl lsp-add sw0 sw0-vif0
>>>> +check ovn-nbctl --wait=sb lsp-set-addresses sw0-vif0 "00:aa:bb:cc:dd:ee
>>>> 10.0.0.2"
>>>> +check_engine_stats northd norecompute compute
>>>> +check_engine_stats routes norecompute compute
>>>> +check_engine_stats advertised_route_sync recompute nocompute
>>>> +check_engine_stats learned_route_sync norecompute compute
>>>> +check_engine_stats lflow norecompute compute
>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>> +
>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>> +check ovn-nbctl lr-add lr1
>>>> +check ovn-nbctl lrp-add lr1 lr1-sw0 00:00:00:01:ff:01 10.0.0.10/24
>>>> +check ovn-nbctl lsp-add sw0 sw0-lr1
>>>> +check ovn-nbctl --wait=sb set Logical_Switch_Port sw0-lr1 type=router
>>>> options:router-port=lr1-sw0
>>>> +check_engine_stats northd recompute compute
>>>> +check_engine_stats routes recompute compute
>>>> +check_engine_stats advertised_route_sync recompute nocompute
>>>> +check_engine_stats learned_route_sync recompute compute
>>>> +check_engine_stats lflow recompute compute
>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>> +
>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>> +check ovn-nbctl --wait=sb lr-nat-add lr1 dnat_and_snat 10.0.0.100
>>>> 192.168.0.1
>>>> +check_engine_stats northd norecompute compute
>>>> +check_engine_stats routes norecompute compute
>>>> +check_engine_stats advertised_route_sync recompute nocompute
>>>> +check_engine_stats learned_route_sync norecompute compute
>>>> +check_engine_stats lflow norecompute compute
>>>> +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
>>>
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
