On 2/26/25 4:47 PM, martin.kal...@canonical.com wrote:
> On Wed, 2025-02-26 at 15:44 +0100, Ilya Maximets wrote:
>> A peer of a switch can be another switch, so the port type has to be
>> checked. The missing check doesn't seem to lead to crashes, but it
>> leads to addresses of switch-switch ports not being advertised.
>
> Oh, interesting. Is the switch-switch connection introduced as part of
> your spine-leaf topology Ilya?
>
> I wonder if this affects also `build_{lb,nat}_connected_parsed_routes`
> [0][1]. We employ similar logic in these functions, but instead of
>
> HMAP_FOR_EACH (port, dp_node, &peer_od->ports)
>
> we iterate only router ports in peer_od
>
> for (size_t i = 0; i < peer_od->n_router_ports; i++)
>
> Do you think we need to update our logic in there too? E.g
>
> LR0 --- LS0 --- LS1 --- LR1
>
> If we redistribute NAT/LB on LR0, do we need to traverse all the way to
> the LS1 and search for connected routers in there too, to find LR1?
>
On a similar note, I was wondering initially if we should do a full
traversal and advertise everything that's indirectly connected but I was
never convinced about it.
However, for switch-switch connections that might be good to do
automatically indeed.
> [0]
> https://github.com/ovn-org/ovn/blob/dde3bdca2ddd82d0aaab6fad8638b4b321e82a43/northd/northd.c#L11478
> [1]
> https://github.com/ovn-org/ovn/blob/dde3bdca2ddd82d0aaab6fad8638b4b321e82a43/northd/northd.c#L11384
>
>
> Thanks,
> Martin.
>
Regards,
Dumitru
>> Fix that by checking the port type instead of assuming.
>>
>> The issue is a result of two features being developed at the same
>> time
>> and the code not being re-checked before merging the routing patches.
>>
>> Fixes: 93f541f342f9 ("northd: Allow announcing individual host
>> routes.")
>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>> ---
>> northd/en-advertised-route-sync.c | 2 +-
>> tests/ovn-northd.at | 15 +++++++++++++++
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/northd/en-advertised-route-sync.c b/northd/en-
>> advertised-route-sync.c
>> index 794a87cb3..f28073a80 100644
>> --- a/northd/en-advertised-route-sync.c
>> +++ b/northd/en-advertised-route-sync.c
>> @@ -386,7 +386,7 @@ publish_host_routes(struct hmap *sync_routes,
>>
>> struct ovn_port *port;
>> HMAP_FOR_EACH (port, dp_node, &peer_od->ports) {
>> - if (port->peer) {
>> + if (port->peer && port->peer->nbrp) {
>> /* This is a LSP connected to an LRP */
>> publish_host_routes_lrp(sync_routes, lr_stateful_table,
>> route,
>> data, port->peer);
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index bee6ed7b1..714da6826 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -15507,6 +15507,21 @@ check_row_count Advertised_Route 6
>> tracked_port!='[[]]'
>> check_row_count Advertised_Route 1 datapath=$datapath
>> logical_port=$lr0lr2 \
>> ip_prefix=10.10.0.20 tracked_port=$lr2lr0
>>
>> +# Add a directly connected switch and make sure that northd is not
>> confused.
>> +# The IP on a switch-switch port should be advertised.
>> +check ovn-nbctl ls-add sw1
>> +check ovn-nbctl lsp-add sw0 sw0-sw1
>> +check ovn-nbctl lsp-add sw1 sw1-sw0
>> +check ovn-nbctl lsp-set-type sw0-sw1 switch peer=sw1-sw0 -- \
>> + lsp-set-type sw1-sw0 switch peer=sw0-sw1
>> +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_column $sw0sw1 Advertised_Route tracked_port \
>> + datapath=$datapath logical_port=$sw0 ip_prefix=10.0.0.42
>> +
>> AT_CLEANUP
>> ])
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
