[ovs-dev] [patch_v1] ovn: Remove unreferenced patched datapaths

[Darrell Ball]

On Thu, Jul 7, 2016 at 3:38 PM, Guru Shetty <guru at ovn.org> wrote:

>
>
> On 6 July 2016 at 18:37, Darrell Ball <dlu998 at gmail.com> wrote:
>
>> Patched datapaths that are no longer referenced should be removed from
>> the patched_datapaths map; otherwise incorrect state references for a
>> patched datapath may be used and also datapaths that are absent will be
>> interpreted as present.
>>
>> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
>>
>
> So if I understand this correctly, currently we add things to
> patched_datapath but never remove when say a router/switch is deleted. I
> suppose, one could trigger deference problem if all we do is create a
> gateway router, connect it to some other switch or router and then delete
> gateway router. ovn-controller would then crash in update_ct_zones() as it
> will try to access port_binding record stored inside the stale
> patched_datapath.
>

hmm:

update_ct_zones() is obviously missing a NULL pointer check for

logical ports, which is another issue.

Perhaps, it would be good to test and add testcases before
associated features are added rather than after the crash as we
see several times recently.

I noticed this datastructure issue in relation to some feature that
I want to add.

I can fix it while I'm here.





>
>
>> ---
>>  ovn/controller/ovn-controller.h |  3 ++-
>>  ovn/controller/patch.c          | 23 ++++++++++++++++++++---
>>  2 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/ovn/controller/ovn-controller.h
>> b/ovn/controller/ovn-controller.h
>> index 6a021a0..8816940 100644
>> --- a/ovn/controller/ovn-controller.h
>> +++ b/ovn/controller/ovn-controller.h
>> @@ -51,8 +51,9 @@ struct local_datapath *get_local_datapath(const struct
>> hmap *,
>>   * with at least one logical patch port binding. */
>>  struct patched_datapath {
>>      struct hmap_node hmap_node;
>> -    bool local; /* 'True' if the datapath is for gateway router. */
>>      const struct sbrec_port_binding *port_binding;
>> +    bool local; /* 'True' if the datapath is for gateway router. */
>> +    bool stale;
>>  };
>>
>>  struct patched_datapath *get_patched_datapath(const struct hmap *,
>> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
>> index 589529e..a701db2 100644
>> --- a/ovn/controller/patch.c
>> +++ b/ovn/controller/patch.c
>> @@ -252,12 +252,14 @@ static void
>>  add_patched_datapath(struct hmap *patched_datapaths,
>>                       const struct sbrec_port_binding *binding_rec, bool
>> local)
>>  {
>> -    if (get_patched_datapath(patched_datapaths,
>> -                             binding_rec->datapath->tunnel_key)) {
>> +    struct patched_datapath * pd;
>> +    if (pd = get_patched_datapath(patched_datapaths,
>> +                                  binding_rec->datapath->tunnel_key)) {
>> +        pd->stale = false;
>>          return;
>>      }
>>
>> -    struct patched_datapath *pd = xzalloc(sizeof *pd);
>> +    pd = xzalloc(sizeof *pd);
>>      pd->local = local;
>>      pd->port_binding = binding_rec;
>>      hmap_insert(patched_datapaths, &pd->hmap_node,
>> @@ -300,6 +302,13 @@ add_logical_patch_ports(struct controller_ctx *ctx,
>>      }
>>
>>      const struct sbrec_port_binding *binding;
>> +
>> +    /* Mark all patched datapaths as stale for later cleanup check */
>> +    struct patched_datapath *pd;
>> +    HMAP_FOR_EACH (pd, hmap_node, patched_datapaths) {
>> +        pd->stale = true;
>> +    }
>> +
>>      SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
>>          bool local_port = false;
>>          if (!strcmp(binding->type, "gateway")) {
>> @@ -332,6 +341,14 @@ add_logical_patch_ports(struct controller_ctx *ctx,
>>              }
>>          }
>>      }
>> +    /* Clean up stale patched datapaths. */
>> +    struct patched_datapath *pd_cur_node, *pd_next_node;
>> +    HMAP_FOR_EACH_SAFE (pd_cur_node, pd_next_node, hmap_node,
>> patched_datapaths) {
>> +        if (pd_cur_node->stale == true) {
>> +            hmap_remove(patched_datapaths, &pd_cur_node->hmap_node);
>> +            free(pd_cur_node);
>> +        }
>> +    }
>>  }
>>
>>  void
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>