Message ID | 20210907115052.7913-1-odivlad@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,v2] ic: remove orphan port_binding after ts deletion | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Tue, Sep 7, 2021 at 4:51 AM Vladislav Odintsov <odivlad@gmail.com> wrote: > > When IC port_binding exists and transit switch is deleted, > the orphan port_binding is left in the IC_SB_DB. > > This patch fixes such situation and adds test for this case. > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > --- > v1 -> v2: > - moved port_binding cleanup from ts_run() to port_binding_run() > according to Han's suggestion Hi Vladislav, thanks for the revision. All looks good except that you may have missed my comments for the tests/ovn-ic.at. Since those are very minor points, I just modified before merging, and I hope it is fine for you. I applied to both master and branch-21.06. Please see the diff below: ------------------- ><8 ----------------------------------------------------------------------8>< ----------------------- diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at index b6a8edb68..319329841 100644 --- a/tests/ovn-ic.at +++ b/tests/ovn-ic.at @@ -64,8 +64,8 @@ OVN_CLEANUP_IC([az1]) AT_CLEANUP ]) - -AT_SETUP([ovn-ic -- port bindings]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-ic -- port bindings deletion upon TS deletion]) ovn_init_ic_db net_add n1 @@ -94,7 +94,7 @@ check ovn-nbctl lsp-add ts1 lsp1 -- \ 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]) +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 @@ -105,7 +105,7 @@ AT_CHECK([ovn-ic-sbctl show | grep -A2 lsp1], [0], [dnl # 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 for i in 1 2; do az=az$i @@ -114,7 +114,7 @@ for i in 1 2; do done OVN_CLEANUP_IC AT_CLEANUP - +]) OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn-ic -- gateway sync])
Hi Han, Yes, somehow I’ve not seen your comments for the test, sorry. I’m okay with those changes, thanks. Regards, Vladislav Odintsov > On 7 Sep 2021, at 21:02, Han Zhou <hzhou@ovn.org> wrote: > > On Tue, Sep 7, 2021 at 4:51 AM Vladislav Odintsov <odivlad@gmail.com> wrote: >> >> When IC port_binding exists and transit switch is deleted, >> the orphan port_binding is left in the IC_SB_DB. >> >> This patch fixes such situation and adds test for this case. >> >> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >> --- >> v1 -> v2: >> - moved port_binding cleanup from ts_run() to port_binding_run() >> according to Han's suggestion > > Hi Vladislav, thanks for the revision. All looks good except that you may > have missed my comments for the tests/ovn-ic.at. Since those are very minor > points, I just modified before merging, and I hope it is fine for you. I > applied to both master and branch-21.06. > > Please see the diff below: > ------------------- ><8 > ----------------------------------------------------------------------8>< > ----------------------- > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > index b6a8edb68..319329841 100644 > --- a/tests/ovn-ic.at > +++ b/tests/ovn-ic.at > @@ -64,8 +64,8 @@ OVN_CLEANUP_IC([az1]) > AT_CLEANUP > ]) > > - > -AT_SETUP([ovn-ic -- port bindings]) > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-ic -- port bindings deletion upon TS deletion]) > > ovn_init_ic_db > net_add n1 > @@ -94,7 +94,7 @@ check ovn-nbctl lsp-add ts1 lsp1 -- \ > 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]) > +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 > @@ -105,7 +105,7 @@ AT_CHECK([ovn-ic-sbctl show | grep -A2 lsp1], [0], [dnl > > # 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 > > for i in 1 2; do > az=az$i > @@ -114,7 +114,7 @@ for i in 1 2; do > done > OVN_CLEANUP_IC > AT_CLEANUP > - > +]) > > OVN_FOR_EACH_NORTHD([ > AT_SETUP([ovn-ic -- gateway sync]) > _______________________________________________ > 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 5a4a997e1..99356253d 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; struct ovsdb_idl_index *icsbrec_route_by_ts_az; @@ -742,6 +743,20 @@ port_binding_run(struct ic_context *ctx, return; } + struct shash isb_all_local_pbs = SHASH_INITIALIZER(&isb_all_local_pbs); + struct shash_node *node; + + 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); + + ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (isb_pb, isb_pb_key, + ctx->icsbrec_port_binding_by_az) { + shash_add(&isb_all_local_pbs, isb_pb->logical_port, isb_pb); + } + icsbrec_port_binding_index_destroy_row(isb_pb_key); + const struct icnbrec_transit_switch *ts; ICNBREC_TRANSIT_SWITCH_FOR_EACH (ts, ctx->ovninb_idl) { const struct nbrec_logical_switch *ls = find_ts_in_nb(ctx, ts->name); @@ -752,16 +767,16 @@ port_binding_run(struct ic_context *ctx, struct shash local_pbs = SHASH_INITIALIZER(&local_pbs); struct shash remote_pbs = SHASH_INITIALIZER(&remote_pbs); struct hmap pb_tnlids = HMAP_INITIALIZER(&pb_tnlids); - 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_ts); + isb_pb_key = icsbrec_port_binding_index_init_row( + ctx->icsbrec_port_binding_by_ts); icsbrec_port_binding_index_set_transit_switch(isb_pb_key, ts->name); 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); + shash_find_and_delete(&isb_all_local_pbs, + isb_pb->logical_port); } else { shash_add(&remote_pbs, isb_pb->logical_port, isb_pb); } @@ -804,7 +819,6 @@ port_binding_run(struct ic_context *ctx, } /* Delete extra port-binding from ISB */ - struct shash_node *node; SHASH_FOR_EACH (node, &local_pbs) { icsbrec_port_binding_delete(node->data); } @@ -818,6 +832,12 @@ port_binding_run(struct ic_context *ctx, shash_destroy(&remote_pbs); ovn_destroy_tnlids(&pb_tnlids); } + + SHASH_FOR_EACH (node, &isb_all_local_pbs) { + icsbrec_port_binding_delete(node->data); + } + + shash_destroy(&isb_all_local_pbs); } struct ic_router_info { @@ -1684,6 +1704,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); @@ -1736,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, .icsbrec_route_by_ts_az = icsbrec_route_by_ts_az, 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]) + +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]) + +# 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)"]) + +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])
When IC port_binding exists and transit switch is deleted, the orphan port_binding is left in the IC_SB_DB. This patch fixes such situation and adds test for this case. Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- v1 -> v2: - moved port_binding cleanup from ts_run() to port_binding_run() according to Han's suggestion --- ic/ovn-ic.c | 36 +++++++++++++++++++++++++++++----- tests/ovn-ic.at | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 5 deletions(-)