On Thu, Jul 7, 2016 at 4:09 PM, Darrell Ball <dlu...@gmail.com> wrote:
>
>
> On Thu, Jul 7, 2016 at 3:38 PM, Guru Shetty <g...@ovn.org> wrote:
>
>>
>>
>> On 6 July 2016 at 18:37, Darrell Ball <dlu...@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 <dlu...@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.
>
Guru and I discussed offline
I will him look into the specific NAT support aspects separately from
the general patched datapaths issues.
>
>
>
>
>>
>>
>>> ---
>>> 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@openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev
>>>
>>
>>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
