On Thu, Feb 06, 2025 at 09:41:14AM +0100, Dumitru Ceara wrote:
> 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.
Hi Frode, Hi Dumitru,
sounds good. I'll change it.
>
> >>>>
> >>>> 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.
With the above change i'll fix that as well.
I will also update the formatting, since that looked quite ugly.
Thanks a lot,
Felix
> >
> > --
> > 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
