On Thu, Jul 31, 2025 at 1:10 PM Felix Huettner <felix.huettner@stackit.cloud>
wrote:
> On Wed, Jul 30, 2025 at 02:57:57PM +0200, Ales Musil wrote:
> > When "connected-as-host" was configured for option
> > "dynamic-routing-redistribute" OVN would attempt to advertise also
> > NATs and LBs connected to that LRP. Prevent that by advertising only
> > the LRP networks. This also prevents commit fail loop when
> > "connected-as-host" and "nat" are used at the same time. Also fix
> > the case when there is "conflict" between LRP IP and SNAT. In
> > that case honor the LRP IP advertised route.
> >
> > Fixes: 93f541f342f9 ("northd: Allow announcing individual host routes.")
> > Reported-at: https://issues.redhat.com/browse/FDP-1564
> > Signed-off-by: Ales Musil <amu...@redhat.com>
> > ---
> > northd/en-advertised-route-sync.c | 72 ++++++++++---------------------
> > tests/ovn-northd.at | 16 +++----
> > tests/system-ovn.at | 26 +++++------
> > 3 files changed, 43 insertions(+), 71 deletions(-)
> >
> > diff --git a/northd/en-advertised-route-sync.c
> b/northd/en-advertised-route-sync.c
> > index 6c653529b..2c9ff50cc 100644
> > --- a/northd/en-advertised-route-sync.c
> > +++ b/northd/en-advertised-route-sync.c
> > @@ -127,7 +127,6 @@ static void
> > advertised_route_table_sync(
> > struct ovsdb_idl_txn *ovnsb_txn,
> > const struct sbrec_advertised_route_table
> *sbrec_advertised_route_table,
> > - const struct lr_stateful_table *lr_stateful_table,
> > const struct hmap *routes,
> > const struct hmap *dynamic_routes,
> > struct advertised_route_sync_data *data);
> > @@ -245,14 +244,11 @@ en_advertised_route_sync_run(struct engine_node
> *node, void *data OVS_UNUSED)
> > 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,
> > - &lr_stateful_data->table,
> > &routes_data->parsed_routes,
> > &dynamic_routes_data->routes,
> > routes_sync_data);
> > @@ -564,40 +560,10 @@ publish_lport_addresses(struct hmap *sync_routes,
> > }
> > }
> >
> > -/* Collect all IP addresses connected via this LRP. */
> > -static void
> > -publish_host_routes_lrp(struct hmap *sync_routes,
> > - const struct lr_stateful_table
> *lr_stateful_table,
> > - const struct ovn_datapath *advertising_od,
> > - const struct ovn_port *advertising_op,
> > - struct advertised_route_sync_data *data,
> > - const struct ovn_port *lrp)
> > -{
> > - /* This is a LSP connected to an LRP */
> > - const struct lport_addresses *addresses = &lrp->lrp_networks;
> > - publish_lport_addresses(sync_routes, advertising_od,
> > - advertising_op, addresses, lrp);
> > -
> > - const struct lr_stateful_record *lr_stateful_rec;
> > - lr_stateful_rec =
> > - lr_stateful_table_find_by_index(lr_stateful_table,
> lrp->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(lrp, lr_stateful_rec, false);
> > - for (size_t i = 0; i < addrs.n_addrs; i++) {
> > - publish_lport_addresses(sync_routes, advertising_od,
> > - advertising_op, &addrs.laddrs[i], lrp);
> > - }
> > - destroy_routable_addresses(&addrs);
> > -}
> > -
> > /* 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 ovn_datapath *advertising_od,
> > const struct ovn_port *advertising_op,
> > struct advertised_route_sync_data *data)
> > @@ -613,8 +579,9 @@ publish_host_routes(struct hmap *sync_routes,
> >
> > if (peer_od->nbr) {
> > /* This is a LRP directly connected to another LRP. */
> > - publish_host_routes_lrp(sync_routes, lr_stateful_table,
> advertising_od,
> > - advertising_op, data,
> advertising_op->peer);
> > + const struct ovn_port *lrp = advertising_op->peer;
> > + publish_lport_addresses(sync_routes, advertising_od,
> > + advertising_op, &lrp->lrp_networks,
> lrp);
> > return;
> > }
> >
> > @@ -626,9 +593,9 @@ publish_host_routes(struct hmap *sync_routes,
> > HMAP_FOR_EACH (port, dp_node, &peer_od->ports) {
> > if (port->peer && port->peer->nbrp) {
> > /* This is a LSP connected to an LRP */
> > - publish_host_routes_lrp(sync_routes, lr_stateful_table,
> > - advertising_od, advertising_op,
> > - data, port->peer);
> > + const struct ovn_port *lrp = port->peer;
> > + publish_lport_addresses(sync_routes, advertising_od,
> > + advertising_op, &lrp->lrp_networks,
> lrp);
> > } else {
> > /* This is just a plain LSP */
> > for (size_t i = 0; i < port->n_lsp_addrs; i++) {
> > @@ -688,7 +655,6 @@ should_advertise_route(const struct uuidset
> *host_route_lrps,
> > */
> > static bool
> > process_prereqs_advertise_route(
> > - const struct lr_stateful_table *lr_stateful_table,
> > struct advertised_route_sync_data *data,
> > struct uuidset *host_route_lrps,
> > struct hmap *sync_routes,
> > @@ -705,8 +671,7 @@ process_prereqs_advertise_route(
> > if (drr_mode_CONNECTED_AS_HOST_is_set(drr)) {
> > const struct uuid *lrp_uuid =
> &advertising_op->nbrp->header_.uuid;
> > uuidset_insert(host_route_lrps, lrp_uuid);
> > - publish_host_routes(sync_routes, lr_stateful_table,
> > - advertising_od, advertising_op,
> > + publish_host_routes(sync_routes, advertising_od,
> advertising_op,
> > data);
> > return true;
> > }
> > @@ -748,7 +713,6 @@ static void
> > advertised_route_table_sync(
> > struct ovsdb_idl_txn *ovnsb_txn,
> > const struct sbrec_advertised_route_table
> *sbrec_advertised_route_table,
> > - const struct lr_stateful_table *lr_stateful_table,
> > const struct hmap *routes,
> > const struct hmap *dynamic_routes,
> > struct advertised_route_sync_data *data)
> > @@ -772,9 +736,9 @@ advertised_route_table_sync(
> > continue;
> > }
> >
> > - if (process_prereqs_advertise_route(lr_stateful_table, data,
> > - &host_route_lrps,
> &sync_routes,
> > - route->od, route->out_port,
> > + if (process_prereqs_advertise_route(data, &host_route_lrps,
> > + &sync_routes, route->od,
> > + route->out_port,
> > route->tracked_port,
> > route->source)) {
> > continue;
> > @@ -794,14 +758,22 @@ advertised_route_table_sync(
> > continue;
> > }
> >
> > - if (process_prereqs_advertise_route(lr_stateful_table, data,
> > - &host_route_lrps,
> &sync_routes,
> > - route_e->od, route_e->op,
> > - route_e->tracked_port,
> > + if (process_prereqs_advertise_route(data, &host_route_lrps,
> > + &sync_routes, route_e->od,
> > + route_e->op,
> route_e->tracked_port,
> > route_e->source)) {
> > continue;
> > }
> >
> > + const struct sbrec_port_binding *tracked_pb =
> > + route_e->tracked_port ? route_e->tracked_port->sb : NULL;
> > + if (ar_entry_find(&sync_routes, route_e->od->sb,
> route_e->op->sb,
> > + route_e->ip_prefix, tracked_pb)) {
> > + /* We could already have advertised route entry for LRP IP
> that
> > + * corresponds to "snat" when "connected-as-host" is
> combined
> > + * with "nat". Skip it. */
> > + continue;
> > + }
> > ar_entry_add(&sync_routes, route_e->od, route_e->op,
> > route_e->ip_prefix, route_e->tracked_port,
> > route_e->source);
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 5ddb15587..ec7009b28 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -15545,7 +15545,7 @@ 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-redistribute="connected-as-host,static"
> > + options:dynamic-routing-redistribute="connected-as-host,static,nat"
> > check_row_count Advertised_Route 2
> > check_column 10.0.0.1 Advertised_Route ip_prefix \
> > datapath=$datapath logical_port=$sw0
> > @@ -15577,14 +15577,14 @@ check_column $lr1 Advertised_Route
> tracked_port \
> > # 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 \
> > +check_row_count Advertised_Route 3 tracked_port!='[[]]'
> > +check_column '' Advertised_Route tracked_port \
> > datapath=$datapath logical_port=$sw0 ip_prefix=10.0.0.100
> >
> > # 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 3 tracked_port!='[[]]'
> > check_row_count Advertised_Route 1 datapath=$datapath logical_port=$sw0
> \
> > ip_prefix=172.16.0.0/24
> >
> > @@ -15593,16 +15593,16 @@ check_row_count Advertised_Route 1
> datapath=$datapath logical_port=$sw0 \
> > check ovn-nbctl lr-add lr2
> > check ovn-nbctl lrp-add lr0 lr0-lr2 00:00:00:02:ff:01 10.10.0.1/24
> peer=lr2-lr0
> > check ovn-nbctl set Logical_Router_Port lr0-lr2 \
> > - options:dynamic-routing-redistribute="connected-as-host,static"
> > + options:dynamic-routing-redistribute="connected-as-host,static,nat"
> > lr0lr2=$(fetch_column port_binding _uuid logical_port=lr0-lr2)
> > check ovn-nbctl lrp-add lr2 lr2-lr0 00:00:00:02:ff:02 10.10.0.2/24
> peer=lr0-lr2
> > lr2lr0=$(fetch_column port_binding _uuid logical_port=lr2-lr0)
> > check ovn-nbctl lrp-add lr2 lr2-sw1 00:00:00:02:ff:03 10.11.0.1/24
> > check ovn-nbctl --wait=sb lr-nat-add lr2 dnat_and_snat 10.10.0.20
> 10.20.0.123
> > check_row_count Advertised_Route 8
> > -check_row_count Advertised_Route 6 tracked_port!='[[]]'
> > +check_row_count Advertised_Route 4 tracked_port!='[[]]'
> > check_row_count Advertised_Route 1 datapath=$datapath
> logical_port=$lr0lr2 \
> > - ip_prefix=10.10.0.20 tracked_port=$lr2lr0
> > + ip_prefix=10.10.0.20 tracked_port='[[]]'
> >
> > # Add a directly connected switch and make sure that northd is not
> confused.
> > # The IP on a switch-switch port should be advertised.
> > @@ -15615,7 +15615,7 @@ check ovn-nbctl --wait=sb lsp-set-addresses
> sw0-sw1 \
> > "11:aa:bb:cc:dd:ee 10.0.0.42"
> > sw0sw1=$(fetch_column port_binding _uuid logical_port=sw0-sw1)
> > check_row_count Advertised_Route 9
> > -check_row_count Advertised_Route 7 tracked_port!='[[]]'
> > +check_row_count Advertised_Route 5 tracked_port!='[[]]'
> > check_column $sw0sw1 Advertised_Route tracked_port \
> > datapath=$datapath logical_port=$sw0 ip_prefix=10.0.0.42
> >
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index e0407383a..c1dc207fd 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -16125,7 +16125,7 @@ blackhole 198.51.100.0/24 proto ovn metric
> 1000])
> > # The last 3 of them are local to the current chassis so we expect a
> better
> > # prio.
> > check ovn-nbctl --wait=hv set Logical_Router_Port internet-public \
> > - options:dynamic-routing-redistribute="connected-as-host,static"
> > + options:dynamic-routing-redistribute="connected-as-host,static,nat"
> >
> > OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 1000
> > @@ -16155,7 +16155,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 1000
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000])
>
> Hi Ales,
>
Hi Felix,
thank you for the reply.
>
> i am concerned about this and the below changes.
> In line 16134 192.0.2.10 is announced with metric 100 as pr1-public is
> on hv1.
>
> In line 16147 it has metric 1000 as pr1-public is on some non existing
> hv.
>
> However in line 16158 pr1-public is back to hv1, so the same as in the
> first case. I don't think that the routes should then have different
> metrics.
>
> From a short look it seems that the Advertised_Route entry for
> 192.0.2.10 does not have a tracked_port set. Before this patch the
> tracked_port was pointing to pr1-public.
>
The difference is that the tracked_port was provided the connected-as-host
code instead of the nat. So the tracked_port was always set even in the
distributed case which probably isn't right.
>
> If the nat entry can actually be handled distributed then i guess the
> priority of 1000 is generally fine, but then that would need to be the
> case as soon as dynamic-routing-redistribute is set and not only after
> moving pr1-public once. Or maybe i am missing something here?
>
You are right that is not correct and I completely ignored that when
changing the test. It seems that we have a bug unrelated to this change
but I'll make sure it's fixed in v2.
Other than that is this approach fine by you? Doesn't this create issues for
your use case?
> Thanks a lot,
> Felix
>
>
>
Thanks,
Ales
> >
> > @@ -16165,7 +16165,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 1000
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 1000
> > blackhole 198.51.100.0/24 proto ovn metric 1000])
> >
> > @@ -16175,7 +16175,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 1000
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000])
> >
> > @@ -16243,7 +16243,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 1000
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000
> > 233.252.0.0/24 via 192.168.10.10 dev lo onlink
> > @@ -16260,7 +16260,7 @@ OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
> > blackhole 192.0.2.1 proto ovn metric 1000
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000
> > 233.252.0.0/24 via 192.168.10.10 dev lo onlink
> > @@ -16272,7 +16272,7 @@ OVN_ROUTE_EQUAL([ovnvrf1338], [dnl
> > blackhole 192.0.2.1 proto ovn metric 1000
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000
> > 233.252.0.0/24 via 192.168.10.10 dev lo onlink
> > @@ -16485,7 +16485,7 @@ blackhole 198.51.100.0/24 proto ovn metric
> 1000])
> > # The last 3 of them are local to the current chassis so we expect a
> better
> > # prio.
> > check ovn-nbctl --wait=hv set Logical_Router_Port internet-public \
> > - options:dynamic-routing-redistribute="connected-as-host,static"
> > + options:dynamic-routing-redistribute="connected-as-host,static,nat"
> >
> > OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 100
> > @@ -16515,7 +16515,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 100
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000])
> >
> > @@ -16525,7 +16525,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 100
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 1000
> > blackhole 198.51.100.0/24 proto ovn metric 1000])
> >
> > @@ -16535,7 +16535,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 100
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000])
> >
> > @@ -16603,7 +16603,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 100
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000
> > 233.252.0.0/24 via 192.168.10.10 dev lo onlink
> > @@ -16615,7 +16615,7 @@ OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
> > blackhole 192.0.2.1 proto ovn metric 100
> > blackhole 192.0.2.2 proto ovn metric 100
> > blackhole 192.0.2.3 proto ovn metric 100
> > -blackhole 192.0.2.10 proto ovn metric 100
> > +blackhole 192.0.2.10 proto ovn metric 1000
> > blackhole 192.0.2.20 proto ovn metric 100
> > blackhole 198.51.100.0/24 proto ovn metric 1000
> > 233.252.0.0/24 via 192.168.10.10 dev lo onlink
> > --
> > 2.50.0
> >
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
