diff mbox series

[ovs-dev,v2] ic: remove orphan port_binding after ts deletion

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

Checks

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

Commit Message

Vladislav Odintsov Sept. 7, 2021, 11:50 a.m. UTC
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(-)

Comments

Han Zhou Sept. 7, 2021, 6:02 p.m. UTC | #1
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])
Vladislav Odintsov Sept. 8, 2021, 1:55 a.m. UTC | #2
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 mbox series

Patch

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])