[ovs-dev] [PATCH v3 ovn 1/2] ovn-northd: Clear SB records depending on stale datapaths.

Numan Siddique nusiddiq at redhat.com
Fri May 1 11:37:30 UTC 2020


On Fri, May 1, 2020 at 12:02 AM Dumitru Ceara <dceara at redhat.com> wrote:

> When purging stale SB Datapath_Binding records ovn-northd doesn't
> properly clean records from other tables that might refer the
> datapaths being deleted.
>
> One way to reproduce the issue is:
> $ ovn-nbctl lr-add lr
> $ ovn-nbctl lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24
> $ ovn-nbctl --wait=sb sync
> $ dp=$(ovn-sbctl --bare --columns _uuid list datapath .)
> $ ovn-sbctl create mac_binding logical_port="p" ip="1.1.1.2" datapath="$dp"
> $ ovn-nbctl lrp-del p -- lr-del lr -- \
>     lr-add lr -- lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24
>
> Reported-by: Dan Williams <dcbw at redhat.com>
> Reported-at: https://bugzilla.redhat.com/1828637
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>

Acked-by: Numan Siddique <numans at ovn.org>

Thanks
Numan


> ---
>  northd/ovn-northd.c |   45 ++++++++++++++++++++++++++++++++-------------
>  tests/ovn-northd.at |   24 ++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 13 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index e31794c..de59452 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -638,6 +638,12 @@ ovn_datapath_find(struct hmap *datapaths, const
> struct uuid *uuid)
>      return NULL;
>  }
>
> +static bool
> +ovn_datapath_is_stale(const struct ovn_datapath *od)
> +{
> +    return !od->nbr && !od->nbs;
> +}
> +
>  static struct ovn_datapath *
>  ovn_datapath_from_sbrec(struct hmap *datapaths,
>                          const struct sbrec_datapath_binding *sb)
> @@ -3075,11 +3081,16 @@ ovn_port_update_sbrec(struct northd_context *ctx,
>  /* Remove mac_binding entries that refer to logical_ports which are
>   * deleted. */
>  static void
> -cleanup_mac_bindings(struct northd_context *ctx, struct hmap *ports)
> +cleanup_mac_bindings(struct northd_context *ctx, struct hmap *datapaths,
> +                     struct hmap *ports)
>  {
>      const struct sbrec_mac_binding *b, *n;
>      SBREC_MAC_BINDING_FOR_EACH_SAFE (b, n, ctx->ovnsb_idl) {
> -        if (!ovn_port_find(ports, b->logical_port)) {
> +        const struct ovn_datapath *od =
> +            ovn_datapath_from_sbrec(datapaths, b->datapath);
> +
> +        if (!od || ovn_datapath_is_stale(od) ||
> +                !ovn_port_find(ports, b->logical_port)) {
>              sbrec_mac_binding_delete(b);
>          }
>      }
> @@ -3447,6 +3458,9 @@ build_ports(struct northd_context *ctx,
>      join_logical_ports(ctx, datapaths, ports, &chassis_qdisc_queues,
>                         &tag_alloc_table, &sb_only, &nb_only, &both);
>
> +    /* Purge stale Mac_Bindings if ports are deleted. */
> +    bool remove_mac_bindings = !ovs_list_is_empty(&sb_only);
> +
>      struct ovn_port *op, *next;
>      /* For logical ports that are in both databases, index the in-use
>       * tunnel_keys. */
> @@ -3461,6 +3475,12 @@ build_ports(struct northd_context *ctx,
>       * For logical ports that are in NB database, do any tag allocation
>       * needed. */
>      LIST_FOR_EACH_SAFE (op, next, list, &both) {
> +        /* When reusing stale Port_Bindings, make sure that stale
> +         * Mac_Bindings are purged.
> +         */
> +        if (op->od->sb != op->sb->datapath) {
> +            remove_mac_bindings = true;
> +        }
>          if (op->nbsp) {
>              tag_alloc_create_new_tag(&tag_alloc_table, op->nbsp);
>          }
> @@ -3496,19 +3516,15 @@ build_ports(struct northd_context *ctx,
>          sbrec_port_binding_set_tunnel_key(op->sb, tunnel_key);
>      }
>
> -    bool remove_mac_bindings = false;
> -    if (!ovs_list_is_empty(&sb_only)) {
> -        remove_mac_bindings = true;
> -    }
> -
>      /* Delete southbound records without northbound matches. */
>      LIST_FOR_EACH_SAFE(op, next, list, &sb_only) {
>          ovs_list_remove(&op->list);
>          sbrec_port_binding_delete(op->sb);
>          ovn_port_destroy(ports, op);
>      }
> +
>      if (remove_mac_bindings) {
> -        cleanup_mac_bindings(ctx, ports);
> +        cleanup_mac_bindings(ctx, datapaths, ports);
>      }
>
>      tag_alloc_destroy(&tag_alloc_table);
> @@ -10293,7 +10309,8 @@ build_lflows(struct northd_context *ctx, struct
> hmap *datapaths,
>      SBREC_LOGICAL_FLOW_FOR_EACH_SAFE (sbflow, next_sbflow,
> ctx->ovnsb_idl) {
>          struct ovn_datapath *od
>              = ovn_datapath_from_sbrec(datapaths,
> sbflow->logical_datapath);
> -        if (!od) {
> +
> +        if (!od || ovn_datapath_is_stale(od)) {
>              sbrec_logical_flow_delete(sbflow);
>              continue;
>          }
> @@ -10353,7 +10370,8 @@ build_lflows(struct northd_context *ctx, struct
> hmap *datapaths,
>      SBREC_MULTICAST_GROUP_FOR_EACH_SAFE (sbmc, next_sbmc, ctx->ovnsb_idl)
> {
>          struct ovn_datapath *od = ovn_datapath_from_sbrec(datapaths,
>                                                            sbmc->datapath);
> -        if (!od) {
> +
> +        if (!od || ovn_datapath_is_stale(od)) {
>              sbrec_multicast_group_delete(sbmc);
>              continue;
>          }
> @@ -10835,8 +10853,8 @@ build_ip_mcast(struct northd_context *ctx, struct
> hmap *datapaths)
>      const struct sbrec_ip_multicast *sb, *sb_next;
>
>      SBREC_IP_MULTICAST_FOR_EACH_SAFE (sb, sb_next, ctx->ovnsb_idl) {
> -        if (!sb->datapath ||
> -                !ovn_datapath_from_sbrec(datapaths, sb->datapath)) {
> +        od = ovn_datapath_from_sbrec(datapaths, sb->datapath);
> +        if (!od || ovn_datapath_is_stale(od)) {
>              sbrec_ip_multicast_delete(sb);
>          }
>      }
> @@ -10905,7 +10923,8 @@ build_mcast_groups(struct northd_context *ctx,
>          /* If the datapath value is stale, purge the group. */
>          struct ovn_datapath *od =
>              ovn_datapath_from_sbrec(datapaths, sb_igmp->datapath);
> -        if (!od) {
> +
> +        if (!od || ovn_datapath_is_stale(od)) {
>              sbrec_igmp_group_delete(sb_igmp);
>              continue;
>          }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 8cc3f70..a4469c7 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1390,3 +1390,27 @@ AT_CHECK([test 0 = $(ovn-sbctl dump-flows lr0 |
> grep lr_in_unsnat | \
>  grep "ip4 && ip4.dst == 192.168.2.6 && tcp && tcp.dst == 8080" -c) ])
>
>  AT_CLEANUP
> +
> +AT_SETUP([ovn -- check reconcile stale Datapath_Binding])
> +ovn_start
> +
> +ovn-nbctl lr-add lr
> +ovn-nbctl lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24
> +
> +AT_CHECK([ovn-nbctl --wait=sb sync], [0])
> +
> +# Create a MAC_Binding referring the router datapath.
> +dp=$(ovn-sbctl --bare --columns _uuid list datapath .)
> +ovn-sbctl create mac_binding logical_port="p" ip="1.1.1.2" datapath="$dp"
> +
> +ovn-nbctl lrp-del p -- lr-del lr -- \
> +    lr-add lr -- lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24
> +AT_CHECK([ovn-nbctl --wait=sb sync], [0])
> +
> +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Datapath_Binding | wc
> -l)])
> +
> +nb_uuid=$(ovn-sbctl get Datapath_Binding . external_ids:logical-router)
> +lr_uuid=$(ovn-nbctl --columns _uuid list Logical_Router .)
> +AT_CHECK[test ${nb_uuid} = ${lr_uuid}]
> +
> +AT_CLEANUP
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list