Hi Han, thanks for the review.
Regards, Vladislav Odintsov > On 7 Sep 2021, at 08:37, Han Zhou <[email protected]> wrote: > > On Tue, Aug 24, 2021 at 11:45 AM Vladislav Odintsov <[email protected] > <mailto:[email protected]>> > wrote: >> >> When IC port_binding exists and transit switch is deleted, >> the orphan port_binding if left in the IC_SB_DB. >> >> This patch fixes such situation and adds test for this case. >> >> Signed-off-by: Vladislav Odintsov <[email protected] >> <mailto:[email protected]>> >> > > Thanks for fixing the bug! Please see my comments below. > > --- >> ic/ovn-ic.c | 35 +++++++++++++++++++++++++++++++-- >> tests/ovn-ic.at | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 85 insertions(+), 2 deletions(-) >> >> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c >> index 75e798cd1..e80cd34ca 100644 >> --- a/ic/ovn-ic.c >> +++ b/ic/ovn-ic.c >> @@ -66,6 +66,7 @@ struct ic_context { >> struct ovsdb_idl_index *nbrec_port_by_name; >> 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_az; >> struct ovsdb_idl_index *icsbrec_port_binding_by_ts; >> struct ovsdb_idl_index *icsbrec_route_by_ts; >> }; >> @@ -174,7 +175,7 @@ allocate_ts_dp_key(struct hmap *dp_tnlids) >> } >> >> static void >> -ts_run(struct ic_context *ctx) >> +ts_run(struct ic_context *ctx, const struct icsbrec_availability_zone > *az) >> { >> const struct icnbrec_transit_switch *ts; >> >> @@ -239,8 +240,25 @@ ts_run(struct ic_context *ctx) >> * SB, to avoid uncommitted ISB datapath tunnel key to be synced > back to >> * AZ. */ >> if (ctx->ovnisb_txn) { >> + struct shash isb_pbs = SHASH_INITIALIZER(&isb_pbs); >> + const struct icsbrec_port_binding *isb_pb; >> + const struct icsbrec_port_binding *isb_pb_key = >> + icsbrec_port_binding_index_init_row( >> + ctx->icsbrec_port_binding_by_az); >> + >> + icsbrec_port_binding_index_set_availability_zone(isb_pb_key, az); > > It seems this index is not used. Looks like it is supposed to be used by > the below loop. Oops. That’s my bad. Yes, it’d be used in below loop. Thanks for pointing this. > > However, I think it is better to fix this by performing the port_binding > clean up in port_binding_run(), because that's where the port_binding table > is supposed to be updated, and no need for the extra index. > The current logic in port_binding_run() didn't consider the situation when > the TS is already deleted, so the big loop for NB TS table wouldn't delete > those port_bindings. To fix it, we could create a shash (all_local_pbs) > that collects all the port_bindings in IC_SB that belong to this AZ at the > beginning of that function, and in the loop when iterating each port of the > existing TSes, remove it from the all_local_pbs: > ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (isb_pb, isb_pb_key, > > ctx->icsbrec_port_binding_by_ts) { > if (isb_pb->availability_zone == az) { > shash_add(&local_pbs, isb_pb->logical_port, isb_pb); > + // TODO: remove isb_pb from the all_local_pbs. > > Finally, at the end of the function, we can remove all the port_bindings > left in the all_local_pbs - the ones created by this AZ but without TS. > What do you think? Ack. In v2 I’ll address this approach. > >> + >> + ICSBREC_PORT_BINDING_FOR_EACH (isb_pb, ctx->ovnisb_idl) { >> + shash_add(&isb_pbs, isb_pb->transit_switch, isb_pb); >> + } >> + >> /* Create ISB Datapath_Binding */ >> ICNBREC_TRANSIT_SWITCH_FOR_EACH (ts, ctx->ovninb_idl) { >> + while (shash_find_and_delete(&isb_pbs, ts->name)) { >> + /* There may be multiple Port_Bindings */ >> + continue; >> + } >> + >> isb_dp = shash_find_and_delete(&isb_dps, ts->name); >> if (!isb_dp) { >> /* Allocate tunnel key */ >> @@ -260,6 +278,13 @@ ts_run(struct ic_context *ctx) >> SHASH_FOR_EACH (node, &isb_dps) { >> icsbrec_datapath_binding_delete(node->data); >> } >> + >> + SHASH_FOR_EACH (node, &isb_pbs) { >> + icsbrec_port_binding_delete(node->data); >> + } >> + >> + icsbrec_port_binding_index_destroy_row(isb_pb_key); >> + shash_destroy(&isb_pbs); >> } >> ovn_destroy_tnlids(&dp_tnlids); >> shash_destroy(&isb_dps); >> @@ -1493,7 +1518,7 @@ ovn_db_run(struct ic_context *ctx) >> return; >> } >> >> - ts_run(ctx); >> + ts_run(ctx, az); >> gateway_run(ctx, az); >> port_binding_run(ctx, az); >> route_run(ctx, az); >> @@ -1684,6 +1709,11 @@ main(int argc, char *argv[]) >> struct ovsdb_idl_index *sbrec_chassis_by_name >> = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, >> &sbrec_chassis_col_name); >> + >> + struct ovsdb_idl_index *icsbrec_port_binding_by_az >> + = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, >> + > &icsbrec_port_binding_col_availability_zone); >> + >> struct ovsdb_idl_index *icsbrec_port_binding_by_ts >> = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, >> > &icsbrec_port_binding_col_transit_switch); >> @@ -1731,6 +1761,7 @@ main(int argc, char *argv[]) >> .nbrec_port_by_name = nbrec_port_by_name, >> .sbrec_port_binding_by_name = sbrec_port_binding_by_name, >> .sbrec_chassis_by_name = sbrec_chassis_by_name, >> + .icsbrec_port_binding_by_az = icsbrec_port_binding_by_az, >> .icsbrec_port_binding_by_ts = icsbrec_port_binding_by_ts, >> .icsbrec_route_by_ts = icsbrec_route_by_ts, >> }; >> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at >> index ee78f4794..b6a8edb68 100644 >> --- a/tests/ovn-ic.at >> +++ b/tests/ovn-ic.at >> @@ -64,6 +64,58 @@ OVN_CLEANUP_IC([az1]) >> AT_CLEANUP >> ]) >> >> + >> +AT_SETUP([ovn-ic -- port bindings]) > > The test case name may be more specific, e.g.: port binding deletion when > TS is deleted. > The wrapper OVN_FOR_EACH_NORTHD should be used (like other test cases). >> + >> +ovn_init_ic_db >> +net_add n1 >> + >> +# 1 GW per AZ >> +for i in 1 2; do >> + az=az$i >> + ovn_start $az >> + sim_add gw-$az >> + as gw-$az >> + check ovs-vsctl add-br br-phys >> + ovn_az_attach $az n1 br-phys 192.168.1.$i >> + check ovs-vsctl set open . external-ids:ovn-is-interconn=true >> +done >> + >> +ovn_as az1 >> + >> +# create transit switch and connect to LR >> +check ovn-ic-nbctl ts-add ts1 >> +check ovn-nbctl lr-add lr1 >> +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24 >> +check ovn-nbctl lrp-set-gateway-chassis lrp1 gw-az1 >> + >> +check ovn-nbctl lsp-add ts1 lsp1 -- \ >> + lsp-set-addresses lsp1 router -- \ >> + lsp-set-type lsp1 router -- \ >> + lsp-set-options lsp1 router-port=lrp1 >> + >> +OVS_WAIT_UNTIL([ovn-sbctl list datapath_binding | grep interconn-ts | > grep ts1]) > > Better to use: wait_row_count Datapath_Binding 1 > external_ids:interconn-ts=ts1 > >> + >> +# check port binding appeared >> +AT_CHECK([ovn-ic-sbctl show | grep -A2 lsp1], [0], [dnl >> + port lsp1 >> + transit switch: ts1 >> + address: [["00:00:00:00:00:01 10.0.0.1/24"]] >> +]) >> + >> +# remove transit switch and check if port_binding is deleted >> +check ovn-ic-nbctl ts-del ts1 >> +OVS_WAIT_UNTIL([test -z "$(ovn-ic-sbctl show | grep lsp1)"]) > > wait_row_count ic-sb:Port_Binding 0 logical_port=lsp1 > > Thanks, > Han > >> + >> +for i in 1 2; do >> + az=az$i >> + OVN_CLEANUP_SBOX(gw-$az) >> + OVN_CLEANUP_AZ([$az]) >> +done >> +OVN_CLEANUP_IC >> +AT_CLEANUP >> + >> + >> OVN_FOR_EACH_NORTHD([ >> AT_SETUP([ovn-ic -- gateway sync]) >> >> -- >> 2.30.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] <mailto:[email protected]> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > _______________________________________________ > dev mailing list > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
