| Message ID | 20210819153012.82531-1-odivlad@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] ic: learn routes to LR only from corresponding transit switch | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Thu, Aug 19, 2021 at 8:30 AM Vladislav Odintsov <odivlad@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@gmail.com> > --- > v1 -> v2: > - Address Han's review comments: > - Make comments and test more clear to understand. > - Use ovsdb_idl_index to optimize transit switch lookup. > --- > ic/ovn-ic.c | 17 ++++++++++++++- > tests/ovn-ic.at | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 71 insertions(+), 1 deletion(-) > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index d69583956..75e798cd1 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -67,6 +67,7 @@ struct ic_context { > struct ovsdb_idl_index *sbrec_chassis_by_name; > struct ovsdb_idl_index *sbrec_port_binding_by_name; > struct ovsdb_idl_index *icsbrec_port_binding_by_ts; > + struct ovsdb_idl_index *icsbrec_route_by_ts; > }; > > struct ic_state { > @@ -1156,7 +1157,14 @@ 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) { > + const struct icsbrec_route *isb_route_key = > + icsbrec_route_index_init_row(ctx->icsbrec_route_by_ts); > + > + icsbrec_route_index_set_transit_switch(isb_route_key, > + ic_lr->isb_pb->transit_switch); > + > + ICSBREC_ROUTE_FOR_EACH_EQUAL (isb_route, isb_route_key, > + ctx->icsbrec_route_by_ts) { > if (isb_route->availability_zone == az) { > continue; > } > @@ -1208,6 +1216,8 @@ sync_learned_route(struct ic_context *ctx, > ic_lr->lr, nb_route); > } > } > + icsbrec_route_index_destroy_row(isb_route_key); > + > /* Delete extra learned routes. */ > struct ic_route_info *route_learned, *next; > HMAP_FOR_EACH_SAFE (route_learned, next, node, &ic_lr->routes_learned) { > @@ -1678,6 +1688,10 @@ main(int argc, char *argv[]) > = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, > &icsbrec_port_binding_col_transit_switch); > > + struct ovsdb_idl_index *icsbrec_route_by_ts > + = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, > + &icsbrec_route_col_transit_switch); > + > /* Main loop. */ > exiting = false; > state.had_lock = false; > @@ -1718,6 +1732,7 @@ main(int argc, char *argv[]) > .sbrec_port_binding_by_name = sbrec_port_binding_by_name, > .sbrec_chassis_by_name = sbrec_chassis_by_name, > .icsbrec_port_binding_by_ts = icsbrec_port_binding_by_ts, > + .icsbrec_route_by_ts = icsbrec_route_by_ts, > }; > > if (!state.had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) { > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > index 2a4fba031..ee78f4794 100644 > --- a/tests/ovn-ic.at > +++ b/tests/ovn-ic.at > @@ -315,6 +315,61 @@ 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]) > > +# cleanup > +ovn-ic-nbctl --if-exists ts-del ts1 > +ovn_as az1 ovn-nbctl lr-del lr1 > +ovn_as az2 ovn-nbctl lr-del lr2 > + > +# Create new transit switches and LRs. Test topology is next: > +# logical router (lr11) - transit switch (ts11) - logical router (lr12) > +# logical router (lr21) - transit switch (ts22) - logical router (lr22) > +# > +# lr12 has static route 10.0.0.0/24 and directly connected network 192.168.0.0/24 > +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 > + > + for j in 1 2; do > + ts=ts$j$j > + ovn-ic-nbctl --may-exist ts-add $ts > + > + # Create LRP and connect to TS > + lr=lr$j$i > + echo lr: $lr, ts: $ts > + ovn-nbctl lr-add $lr > + ovn-nbctl lrp-add $lr lrp-$lr-$ts aa:aa:aa:aa:aa:0$j 169.254.100.$i/24 > + ovn-nbctl lsp-add $ts lsp-$ts-$lr \ > + -- lsp-set-addresses lsp-$ts-$lr router \ > + -- lsp-set-type lsp-$ts-$lr router \ > + -- lsp-set-options lsp-$ts-$lr router-port=lrp-$lr-$ts > + done > +done > + > +# Create directly-connected routes > +ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12-ls2 aa:aa:aa:aa:bb:01 " 192.168.0.1/24" > +ovn_as az2 ovn-nbctl lr-route-add lr12 10.10.10.0/24 192.168.0.10 > + > +echo az1 > +ovn_as az1 ovn-nbctl show > +echo az2 > +ovn_as az2 ovn-nbctl show > + > +# Test routes from lr12 were learned to lr11 > +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | > + grep learned | awk '{print $1, $2}' | sort], [0], [dnl > +10.10.10.0/24 169.254.100.2 > +192.168.0.0/24 169.254.100.2 > +]) > + > +# Test routes from lr12 didn't leak as learned to lr21 > +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr21], [0], []) > + > OVN_CLEANUP_IC([az1], [az2]) > > AT_CLEANUP > -- > 2.30.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks Vladislav. I applied to all branches.
Thanks! Regards, Vladislav Odintsov > On 19 Aug 2021, at 20:06, Han Zhou <zhouhan@gmail.com> wrote: > > On Thu, Aug 19, 2021 at 8:30 AM Vladislav Odintsov <odivlad@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@gmail.com> >> --- >> v1 -> v2: >> - Address Han's review comments: >> - Make comments and test more clear to understand. >> - Use ovsdb_idl_index to optimize transit switch lookup. >> --- >> ic/ovn-ic.c | 17 ++++++++++++++- >> tests/ovn-ic.at | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 71 insertions(+), 1 deletion(-) >> >> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c >> index d69583956..75e798cd1 100644 >> --- a/ic/ovn-ic.c >> +++ b/ic/ovn-ic.c >> @@ -67,6 +67,7 @@ struct ic_context { >> struct ovsdb_idl_index *sbrec_chassis_by_name; >> struct ovsdb_idl_index *sbrec_port_binding_by_name; >> struct ovsdb_idl_index *icsbrec_port_binding_by_ts; >> + struct ovsdb_idl_index *icsbrec_route_by_ts; >> }; >> >> struct ic_state { >> @@ -1156,7 +1157,14 @@ 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) { >> + const struct icsbrec_route *isb_route_key = >> + icsbrec_route_index_init_row(ctx->icsbrec_route_by_ts); >> + >> + icsbrec_route_index_set_transit_switch(isb_route_key, >> + > ic_lr->isb_pb->transit_switch); >> + >> + ICSBREC_ROUTE_FOR_EACH_EQUAL (isb_route, isb_route_key, >> + ctx->icsbrec_route_by_ts) { >> if (isb_route->availability_zone == az) { >> continue; >> } >> @@ -1208,6 +1216,8 @@ sync_learned_route(struct ic_context *ctx, >> ic_lr->lr, nb_route); >> } >> } >> + icsbrec_route_index_destroy_row(isb_route_key); >> + >> /* Delete extra learned routes. */ >> struct ic_route_info *route_learned, *next; >> HMAP_FOR_EACH_SAFE (route_learned, next, node, > &ic_lr->routes_learned) { >> @@ -1678,6 +1688,10 @@ main(int argc, char *argv[]) >> = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, >> > &icsbrec_port_binding_col_transit_switch); >> >> + struct ovsdb_idl_index *icsbrec_route_by_ts >> + = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, >> + &icsbrec_route_col_transit_switch); >> + >> /* Main loop. */ >> exiting = false; >> state.had_lock = false; >> @@ -1718,6 +1732,7 @@ main(int argc, char *argv[]) >> .sbrec_port_binding_by_name = sbrec_port_binding_by_name, >> .sbrec_chassis_by_name = sbrec_chassis_by_name, >> .icsbrec_port_binding_by_ts = icsbrec_port_binding_by_ts, >> + .icsbrec_route_by_ts = icsbrec_route_by_ts, >> }; >> >> if (!state.had_lock && > ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) { >> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at >> index 2a4fba031..ee78f4794 100644 >> --- a/tests/ovn-ic.at >> +++ b/tests/ovn-ic.at >> @@ -315,6 +315,61 @@ 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]) >> >> +# cleanup >> +ovn-ic-nbctl --if-exists ts-del ts1 >> +ovn_as az1 ovn-nbctl lr-del lr1 >> +ovn_as az2 ovn-nbctl lr-del lr2 >> + >> +# Create new transit switches and LRs. Test topology is next: >> +# logical router (lr11) - transit switch (ts11) - logical router (lr12) >> +# logical router (lr21) - transit switch (ts22) - logical router (lr22) >> +# >> +# lr12 has static route 10.0.0.0/24 and directly connected network > 192.168.0.0/24 >> +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 >> + >> + for j in 1 2; do >> + ts=ts$j$j >> + ovn-ic-nbctl --may-exist ts-add $ts >> + >> + # Create LRP and connect to TS >> + lr=lr$j$i >> + echo lr: $lr, ts: $ts >> + ovn-nbctl lr-add $lr >> + ovn-nbctl lrp-add $lr lrp-$lr-$ts aa:aa:aa:aa:aa:0$j > 169.254.100.$i/24 >> + ovn-nbctl lsp-add $ts lsp-$ts-$lr \ >> + -- lsp-set-addresses lsp-$ts-$lr router \ >> + -- lsp-set-type lsp-$ts-$lr router \ >> + -- lsp-set-options lsp-$ts-$lr router-port=lrp-$lr-$ts >> + done >> +done >> + >> +# Create directly-connected routes >> +ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12-ls2 aa:aa:aa:aa:bb:01 " > 192.168.0.1/24" >> +ovn_as az2 ovn-nbctl lr-route-add lr12 10.10.10.0/24 192.168.0.10 >> + >> +echo az1 >> +ovn_as az1 ovn-nbctl show >> +echo az2 >> +ovn_as az2 ovn-nbctl show >> + >> +# Test routes from lr12 were learned to lr11 >> +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | >> + grep learned | awk '{print $1, $2}' | sort], [0], [dnl >> +10.10.10.0/24 169.254.100.2 >> +192.168.0.0/24 169.254.100.2 >> +]) >> + >> +# Test routes from lr12 didn't leak as learned to lr21 >> +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr21], [0], []) >> + >> OVN_CLEANUP_IC([az1], [az2]) >> >> AT_CLEANUP >> -- >> 2.30.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks Vladislav. I applied to all branches. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index d69583956..75e798cd1 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -67,6 +67,7 @@ struct ic_context { struct ovsdb_idl_index *sbrec_chassis_by_name; struct ovsdb_idl_index *sbrec_port_binding_by_name; struct ovsdb_idl_index *icsbrec_port_binding_by_ts; + struct ovsdb_idl_index *icsbrec_route_by_ts; }; struct ic_state { @@ -1156,7 +1157,14 @@ 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) { + const struct icsbrec_route *isb_route_key = + icsbrec_route_index_init_row(ctx->icsbrec_route_by_ts); + + icsbrec_route_index_set_transit_switch(isb_route_key, + ic_lr->isb_pb->transit_switch); + + ICSBREC_ROUTE_FOR_EACH_EQUAL (isb_route, isb_route_key, + ctx->icsbrec_route_by_ts) { if (isb_route->availability_zone == az) { continue; } @@ -1208,6 +1216,8 @@ sync_learned_route(struct ic_context *ctx, ic_lr->lr, nb_route); } } + icsbrec_route_index_destroy_row(isb_route_key); + /* Delete extra learned routes. */ struct ic_route_info *route_learned, *next; HMAP_FOR_EACH_SAFE (route_learned, next, node, &ic_lr->routes_learned) { @@ -1678,6 +1688,10 @@ main(int argc, char *argv[]) = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, &icsbrec_port_binding_col_transit_switch); + struct ovsdb_idl_index *icsbrec_route_by_ts + = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, + &icsbrec_route_col_transit_switch); + /* Main loop. */ exiting = false; state.had_lock = false; @@ -1718,6 +1732,7 @@ main(int argc, char *argv[]) .sbrec_port_binding_by_name = sbrec_port_binding_by_name, .sbrec_chassis_by_name = sbrec_chassis_by_name, .icsbrec_port_binding_by_ts = icsbrec_port_binding_by_ts, + .icsbrec_route_by_ts = icsbrec_route_by_ts, }; if (!state.had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) { diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at index 2a4fba031..ee78f4794 100644 --- a/tests/ovn-ic.at +++ b/tests/ovn-ic.at @@ -315,6 +315,61 @@ 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]) +# cleanup +ovn-ic-nbctl --if-exists ts-del ts1 +ovn_as az1 ovn-nbctl lr-del lr1 +ovn_as az2 ovn-nbctl lr-del lr2 + +# Create new transit switches and LRs. Test topology is next: +# logical router (lr11) - transit switch (ts11) - logical router (lr12) +# logical router (lr21) - transit switch (ts22) - logical router (lr22) +# +# lr12 has static route 10.0.0.0/24 and directly connected network 192.168.0.0/24 +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 + + for j in 1 2; do + ts=ts$j$j + ovn-ic-nbctl --may-exist ts-add $ts + + # Create LRP and connect to TS + lr=lr$j$i + echo lr: $lr, ts: $ts + ovn-nbctl lr-add $lr + ovn-nbctl lrp-add $lr lrp-$lr-$ts aa:aa:aa:aa:aa:0$j 169.254.100.$i/24 + ovn-nbctl lsp-add $ts lsp-$ts-$lr \ + -- lsp-set-addresses lsp-$ts-$lr router \ + -- lsp-set-type lsp-$ts-$lr router \ + -- lsp-set-options lsp-$ts-$lr router-port=lrp-$lr-$ts + done +done + +# Create directly-connected routes +ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12-ls2 aa:aa:aa:aa:bb:01 "192.168.0.1/24" +ovn_as az2 ovn-nbctl lr-route-add lr12 10.10.10.0/24 192.168.0.10 + +echo az1 +ovn_as az1 ovn-nbctl show +echo az2 +ovn_as az2 ovn-nbctl show + +# Test routes from lr12 were learned to lr11 +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | + grep learned | awk '{print $1, $2}' | sort], [0], [dnl +10.10.10.0/24 169.254.100.2 +192.168.0.0/24 169.254.100.2 +]) + +# Test routes from lr12 didn't leak as learned to lr21 +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr21], [0], []) + OVN_CLEANUP_IC([az1], [az2]) AT_CLEANUP
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@gmail.com> --- v1 -> v2: - Address Han's review comments: - Make comments and test more clear to understand. - Use ovsdb_idl_index to optimize transit switch lookup. --- ic/ovn-ic.c | 17 ++++++++++++++- tests/ovn-ic.at | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-)