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-odiv...@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 <hz...@ovn.org> wrote: > > On Mon, Aug 9, 2021 at 2:42 PM Vladislav Odintsov <odiv...@gmail.com > <mailto:odiv...@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 <odiv...@gmail.com >> <mailto:odiv...@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 >> d...@openvswitch.org <mailto:d...@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > _______________________________________________ > dev mailing list > d...@openvswitch.org <mailto:d...@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev