I’ve submitted version 2 of this patch: https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/
Regards, Vladislav Odintsov > On 7 Sep 2021, at 11:42, Vladislav Odintsov <[email protected]> wrote: > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
