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

Han Zhou hzhou at ovn.org
Thu Aug 19 07:05:37 UTC 2021


On Mon, Aug 9, 2021 at 2:42 PM Vladislav Odintsov <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>

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
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list