[ovs-dev] [PATCH ovn] ic: learn routes to LR only from corresponding transit switch

[Vladislav Odintsov]

Hi Han,

thanks for the review!

All requested changes I’ve addressed in a new version of this patch: https://patchwork.ozlabs.org/project/ovn/patch/20210819153012.82531-1-odivlad@gmail.com/ 
Let me know if now patch is okay or needs more improvements.

Regards,
Vladislav Odintsov

> On 19 Aug 2021, at 10:05, Han Zhou <hzhou at ovn.org> wrote:
> 
> On Mon, Aug 9, 2021 at 2:42 PM Vladislav Odintsov <odivlad at gmail.com <mailto:odivlad at gmail.com>> wrote:
>> 
>> This commit fixes an error where in case of LRs were connected
>> between different AZs with ovn-ic, their routes were leaking
>> from one LR attached to its transit switch to another LR
>> attached to another transit switch.
>> 
>> Testcase, which reproduces described problem is added as well.
>> 
>> Signed-off-by: Vladislav Odintsov <odivlad at gmail.com <mailto:odivlad at gmail.com>>
> 
> Thanks for the fix! Please see minor comments.
> 
>> ---
>> This bugfix should have no conflicts to all branches,
>> so it's reasonable to apply it down to supported branches.
>> ---
>> ic/ovn-ic.c     |  4 +++-
>> tests/ovn-ic.at | 34 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 37 insertions(+), 1 deletion(-)
>> 
>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>> index d69583956..8916ba542 100644
>> --- a/ic/ovn-ic.c
>> +++ b/ic/ovn-ic.c
>> @@ -1157,7 +1157,9 @@ sync_learned_route(struct ic_context *ctx,
>>     ovs_assert(ctx->ovnnb_txn);
>>     const struct icsbrec_route *isb_route;
>>     ICSBREC_ROUTE_FOR_EACH (isb_route, ctx->ovnisb_idl) {
>> -        if (isb_route->availability_zone == az) {
>> +        if (isb_route->availability_zone == az ||
>> +            strcmp(isb_route->transit_switch,
>> +                   ic_lr->isb_pb->transit_switch)) {
> 
> This looks good. In addition, we may use index to traverse the routes that
> have the desired transit_switch, in case the number of routes and transit
> switches are big, but I am ok to leave it for now.
> 
>>             continue;
>>         }
>>         struct in6_addr prefix, nexthop;
>> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
>> index 2a4fba031..eb63d9450 100644
>> --- a/tests/ovn-ic.at
>> +++ b/tests/ovn-ic.at
>> @@ -315,6 +315,41 @@ OVS_WAIT_WHILE([ovn_as az2 ovn-nbctl lr-route-list
> lr2 | grep learned | grep 192
>> # AZ1 shouldn't learn 10.11 any more.
>> OVS_WAIT_WHILE([ovn_as az1 ovn-nbctl lr-route-list lr1 | grep learned |
> grep 10.11])
>> 
>> +# Test routes don't leak from lr1 (ts1) to lr2 (ts2)
> 
> This comment is confusing. lr1 and lr2 are both connected to ts1, while
> lr21 and lr22 are connected to ts2. I'd suggest updating the existing test
> to use lr11 and lr12 to connect to ts1, to make the test easier to read.
> 
>> +ovn-ic-nbctl ts-add ts2
>> +for i in 1 2; do
>> +    ovn_as az$i
>> +
>> +    # Ensure route learning at AZ level
>> +    ovn-nbctl set nb_global . options:ic-route-learn=true
>> +    # Ensure route advertising at AZ level
>> +    ovn-nbctl set nb_global . options:ic-route-adv=true
>> +    # Drop blacklist
>> +    ovn-nbctl remove nb_global . options ic-route-blacklist
>> +
>> +    # Create LRP and connect to TS
>> +    ovn-nbctl lr-add lr2$i
>> +    ovn-nbctl lrp-add lr2$i lrp-lr2$i-ts2 aa:aa:aa:aa:aa:0$i
> 169.254.100.$i/24
>> +    ovn-nbctl lsp-add ts2 lsp-ts2-lr2$i \
>> +            -- lsp-set-addresses lsp-ts2-lr2$i router \
>> +            -- lsp-set-type lsp-ts2-lr2$i router \
>> +            -- lsp-set-options lsp-ts2-lr2$i router-port=lrp-lr2$i-ts2
>> +done
>> +
>> +# Create directly-connected route
>> +ovn_as az2 ovn-nbctl lrp-add lr2 lrp-lr2-ls2 aa:aa:aa:aa:bb:01 "
> 192.168.0.1/24"
>> +
>> +# Test learned routes from lr2 didn't leak to lr22
>> +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr1 |
>> +             grep learned | awk '{print $1, $2}' | sort], [0], [dnl
>> +10.11.2.0/24 169.254.100.2
>> +192.168.0.0/24 169.254.100.2
>> +])
> 
> The comment says testing lr22 but why it is checking lr1?
> 
> Thanks,
> Han
> 
>> +
>> +# Test there are no learned routes in lr21
>> +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr21 | grep learned |
>> +             awk '{print $1, $2}'], [0], [])
>> +
>> OVN_CLEANUP_IC([az1], [az2])
>> 
>> AT_CLEANUP
>> --
>> 2.30.0
>> 
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org <mailto:dev at openvswitch.org>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org <mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>