On Tue, Aug 24, 2021 at 11:45 AM Vladislav Odintsov <odiv...@gmail.com>
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 <odiv...@gmail.com>
>
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.
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?
> +
> + 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
