Re: [ovs-dev] [PATCH ovn] ic: remove port_binding on ts deletion

[Vladislav Odintsov]

I’ve submitted version 2 of this patch:
https://patchwork.ozlabs.org/project/ovn/patch/20210907115052.7913-1-odiv...@gmail.com/

Regards,
Vladislav Odintsov

> On 7 Sep 2021, at 11:42, Vladislav Odintsov  wrote:
> 
> Hi Han,
> 
> thanks for the review.
> 
> Regards,
> Vladislav Odintsov
> 
>> On 7 Sep 2021, at 08:37, Han Zhou  wrote:
>> 
>> On Tue, Aug 24, 2021 at 11:45 AM Vladislav Odintsov > >
>> 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 >> >
>>> 
>> 
>> 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(_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(_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(_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(_pbs, ts->name)) {
>>> +/* There may be multiple Port_Bindings */
>>> +continue;
>>> +}
>>> +
>>>isb_dp = shash_find_and_delete(_dps, ts->name);
>>>if (!isb_dp) {
>>>/* Allocate tunnel key */
>>> @@ -260,6 +278,13 @@ ts_run(struct ic_context *ctx)
>>>SHASH_FOR_EACH (node, _dps) {
>>>icsbrec_datapath_binding_delete(node->data);
>>>}
>>> +
>>> +SHASH_FOR_EACH (node, _pbs) {
>>> +icsbrec_port_binding_delete(node->data);
>>> +}
>>> +
>>> +icsbrec_port_binding_index_destroy_row(isb_pb_key);
>>> +shash_destroy(_pbs);
>>>}
>>>ovn_destroy_tnlids(_tnlids);
>>>shash_destroy(_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 

Re: [ovs-dev] [PATCH ovn] ic: remove port_binding on ts deletion

[Vladislav Odintsov]

Hi Han,

thanks for the review.

Regards,
Vladislav Odintsov

> On 7 Sep 2021, at 08:37, Han Zhou  wrote:
> 
> On Tue, Aug 24, 2021 at 11:45 AM Vladislav Odintsov  >
> 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 > >
>> 
> 
> 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(_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(_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(_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(_pbs, ts->name)) {
>> +/* There may be multiple Port_Bindings */
>> +continue;
>> +}
>> +
>> isb_dp = shash_find_and_delete(_dps, ts->name);
>> if (!isb_dp) {
>> /* Allocate tunnel key */
>> @@ -260,6 +278,13 @@ ts_run(struct ic_context *ctx)
>> SHASH_FOR_EACH (node, _dps) {
>> icsbrec_datapath_binding_delete(node->data);
>> }
>> +
>> +SHASH_FOR_EACH (node, _pbs) {
>> +icsbrec_port_binding_delete(node->data);
>> +}
>> +
>> +icsbrec_port_binding_index_destroy_row(isb_pb_key);
>> +shash_destroy(_pbs);
>> }
>> ovn_destroy_tnlids(_tnlids);
>> shash_destroy(_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,
>>   _chassis_col_name);
>> +
>> +struct ovsdb_idl_index *icsbrec_port_binding_by_az
>> += ovsdb_idl_index_create1(ovnisb_idl_loop.idl,
>> +

Re: [ovs-dev] [PATCH ovn] ic: remove port_binding on ts deletion

[Han Zhou]

On Tue, Aug 24, 2021 at 11:45 AM Vladislav Odintsov 
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 
>

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(_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(_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(_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(_pbs, ts->name)) {
> +/* There may be multiple Port_Bindings */
> +continue;
> +}
> +
>  isb_dp = shash_find_and_delete(_dps, ts->name);
>  if (!isb_dp) {
>  /* Allocate tunnel key */
> @@ -260,6 +278,13 @@ ts_run(struct ic_context *ctx)
>  SHASH_FOR_EACH (node, _dps) {
>  icsbrec_datapath_binding_delete(node->data);
>  }
> +
> +SHASH_FOR_EACH (node, _pbs) {
> +icsbrec_port_binding_delete(node->data);
> +}
> +
> +icsbrec_port_binding_index_destroy_row(isb_pb_key);
> +shash_destroy(_pbs);
>  }
>  ovn_destroy_tnlids(_tnlids);
>  shash_destroy(_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,
>_chassis_col_name);
> +
> +struct ovsdb_idl_index *icsbrec_port_binding_by_az
> += ovsdb_idl_index_create1(ovnisb_idl_loop.idl,
> +
 _port_binding_col_availability_zone);
> +
>  struct ovsdb_idl_index *icsbrec_port_binding_by_ts
>  = ovsdb_idl_index_create1(ovnisb_idl_loop.idl,
>
 _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,
>  

[ovs-dev] [PATCH ovn] ic: remove port_binding on ts deletion

[Vladislav Odintsov]

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 
---
 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(_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);
+
+ICSBREC_PORT_BINDING_FOR_EACH (isb_pb, ctx->ovnisb_idl) {
+shash_add(_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(_pbs, ts->name)) {
+/* There may be multiple Port_Bindings */
+continue;
+}
+
 isb_dp = shash_find_and_delete(_dps, ts->name);
 if (!isb_dp) {
 /* Allocate tunnel key */
@@ -260,6 +278,13 @@ ts_run(struct ic_context *ctx)
 SHASH_FOR_EACH (node, _dps) {
 icsbrec_datapath_binding_delete(node->data);
 }
+
+SHASH_FOR_EACH (node, _pbs) {
+icsbrec_port_binding_delete(node->data);
+}
+
+icsbrec_port_binding_index_destroy_row(isb_pb_key);
+shash_destroy(_pbs);
 }
 ovn_destroy_tnlids(_tnlids);
 shash_destroy(_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,
   _chassis_col_name);
+
+struct ovsdb_idl_index *icsbrec_port_binding_by_az
+= ovsdb_idl_index_create1(ovnisb_idl_loop.idl,
+  _port_binding_col_availability_zone);
+
 struct ovsdb_idl_index *icsbrec_port_binding_by_ts
 = ovsdb_idl_index_create1(ovnisb_idl_loop.idl,
   _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])
+
+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
+