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

Guru Shetty guru at ovn.org
Thu Jul 7 22:38:41 UTC 2016


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.


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



More information about the dev mailing list