[ovs-dev] [PATCH ovn] ic: remove port_binding on ts deletion
Vladislav Odintsov
odivlad at gmail.com
Tue Sep 7 11:55:22 UTC 2021
I’ve submitted version 2 of this patch:
https://patchwork.ozlabs.org/project/ovn/patch/20210907115052.7913-1-odivlad@gmail.com/
Regards,
Vladislav Odintsov
> On 7 Sep 2021, at 11:42, Vladislav Odintsov <odivlad at gmail.com> wrote:
>
> Hi Han,
>
> thanks for the review.
>
> Regards,
> Vladislav Odintsov
>
>> On 7 Sep 2021, at 08:37, Han Zhou <hzhou at ovn.org> wrote:
>>
>> On Tue, Aug 24, 2021 at 11:45 AM Vladislav Odintsov <odivlad at gmail.com <mailto:odivlad at 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 <odivlad at gmail.com <mailto:odivlad at 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.
>
> 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
>>> dev at openvswitch.org <mailto:dev at openvswitch.org>
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org <mailto:dev at openvswitch.org>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list