[ovs-dev] [PATCH ovn] ovn-sbctl: Fix lflow-list (etc.) in daemon mode and upon races.

Numan Siddique numans at ovn.org
Thu Jun 3 23:38:01 UTC 2021


On Thu, Jun 3, 2021 at 3:48 PM Ben Pfaff <blp at ovn.org> wrote:
>
> Utilities like ovn-sbctl sometimes need to retry their transactions
> because of races.  For this reason, instead of sending user output
> directly to stdout, they buffer it until the transaction succeeds.
> Some of the ovn-sbctl commands didn't do this properly, so they would
> output multiple times upon a race.  Another way to see the problem
> was to use daemon mode, in which the output written directly with
> printf() would not appear at all, since the daemon's stdout is not
> connected to ovn-sbctl's stdout.
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1965780
> Reported-by: Alexey Roytman <aroytman at redhat.com>

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

Numan

> ---
>  utilities/ovn-sbctl.c | 151 ++++++++++++++++++++++--------------------
>  1 file changed, 79 insertions(+), 72 deletions(-)
>
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index 53f10cdd897a..4146384e74fb 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -618,7 +618,8 @@ sbctl_open_vconn(struct shash *options)
>  }
>
>  static void
> -sbctl_dump_openflow(struct vconn *vconn, const struct uuid *uuid, bool stats)
> +sbctl_dump_openflow(struct vconn *vconn, const struct uuid *uuid, bool stats,
> +                    struct ds *s)
>  {
>      struct ofputil_flow_stats_request fsr = {
>          .cookie = htonll(uuid->parts[0]),
> @@ -639,28 +640,26 @@ sbctl_dump_openflow(struct vconn *vconn, const struct uuid *uuid, bool stats)
>      }
>
>      if (n_fses) {
> -        struct ds s = DS_EMPTY_INITIALIZER;
>          for (size_t i = 0; i < n_fses; i++) {
>              const struct ofputil_flow_stats *fs = &fses[i];
>
> -            ds_clear(&s);
> +            ds_put_cstr(s, "    ");
>              if (stats) {
> -                ofputil_flow_stats_format(&s, fs, NULL, NULL, true);
> +                ofputil_flow_stats_format(s, fs, NULL, NULL, true);
>              } else {
> -                ds_put_format(&s, "%stable=%s%"PRIu8" ",
> +                ds_put_format(s, "%stable=%s%"PRIu8" ",
>                                colors.special, colors.end, fs->table_id);
> -                match_format(&fs->match, NULL, &s, OFP_DEFAULT_PRIORITY);
> -                if (ds_last(&s) != ' ') {
> -                    ds_put_char(&s, ' ');
> +                match_format(&fs->match, NULL, s, OFP_DEFAULT_PRIORITY);
> +                if (ds_last(s) != ' ') {
> +                    ds_put_char(s, ' ');
>                  }
>
> -                ds_put_format(&s, "%sactions=%s", colors.actions, colors.end);
> -                struct ofpact_format_params fp = { .s = &s };
> +                ds_put_format(s, "%sactions=%s", colors.actions, colors.end);
> +                struct ofpact_format_params fp = { .s = s };
>                  ofpacts_format(fs->ofpacts, fs->ofpacts_len, &fp);
>              }
> -            printf("    %s\n", ds_cstr(&s));
> +            ds_put_char(s, '\n');
>          }
> -        ds_destroy(&s);
>      }
>
>      for (size_t i = 0; i < n_fses; i++) {
> @@ -670,37 +669,37 @@ sbctl_dump_openflow(struct vconn *vconn, const struct uuid *uuid, bool stats)
>  }
>
>  static void
> -print_datapath_name(const struct sbrec_datapath_binding *dp)
> +print_datapath_name(const struct sbrec_datapath_binding *dp, struct ds *s)
>  {
>      const struct smap *ids = &dp->external_ids;
>      const char *name = smap_get(ids, "name");
>      const char *name2 = smap_get(ids, "name2");
>      if (name && name2) {
> -        printf("\"%s\" aka \"%s\"", name, name2);
> +        ds_put_format(s, "\"%s\" aka \"%s\"", name, name2);
>      } else if (name || name2) {
> -        printf("\"%s\"", name ? name : name2);
> +        ds_put_format(s, "\"%s\"", name ? name : name2);
>      }
>  }
>
>  static void
>  print_vflow_datapath_name(const struct sbrec_datapath_binding *dp,
> -                          bool do_print)
> +                          bool do_print, struct ds *s)
>  {
>      if (!do_print) {
>          return;
>      }
> -    printf("datapath=");
> -    print_datapath_name(dp);
> -    printf(", ");
> +    ds_put_cstr(s, "datapath=");
> +    print_datapath_name(dp, s);
> +    ds_put_cstr(s, ", ");
>  }
>
>  static void
> -print_uuid_part(const struct uuid *uuid, bool do_print)
> +print_uuid_part(const struct uuid *uuid, bool do_print, struct ds *s)
>  {
>      if (!do_print) {
>          return;
>      }
> -    printf("uuid=0x%08"PRIx32", ", uuid->parts[0]);
> +    ds_put_format(s, "uuid=0x%08"PRIx32", ", uuid->parts[0]);
>  }
>
>  static void
> @@ -717,16 +716,17 @@ cmd_lflow_list_port_bindings(struct ctl_context *ctx, struct vconn *vconn,
>          }
>
>          if (!pb_prev) {
> -            printf("\nPort Bindings:\n");
> +            ds_put_cstr(&ctx->output, "\nPort Bindings:\n");
>          }
>
> -        printf("  ");
> -        print_uuid_part(&pb->header_.uuid, print_uuid);
> -        print_vflow_datapath_name(pb->datapath, !datapath);
> -        printf("logical_port=%s, tunnel_key=%-5"PRId64"\n",
> -               pb->logical_port, pb->tunnel_key);
> +        ds_put_cstr(&ctx->output, "  ");
> +        print_uuid_part(&pb->header_.uuid, print_uuid, &ctx->output);
> +        print_vflow_datapath_name(pb->datapath, !datapath, &ctx->output);
> +        ds_put_format(&ctx->output,
> +                      "logical_port=%s, tunnel_key=%-5"PRId64"\n",
> +                      pb->logical_port, pb->tunnel_key);
>          if (vconn) {
> -            sbctl_dump_openflow(vconn, &pb->header_.uuid, stats);
> +            sbctl_dump_openflow(vconn, &pb->header_.uuid, stats, &ctx->output);
>          }
>
>          pb_prev = pb;
> @@ -746,17 +746,17 @@ cmd_lflow_list_mac_bindings(struct ctl_context *ctx, struct vconn *vconn,
>          }
>
>          if (!mb_prev) {
> -            printf("\nMAC Bindings:\n");
> +            ds_put_cstr(&ctx->output, "\nMAC Bindings:\n");
>          }
>
> -        printf("  ");
> -        print_uuid_part(&mb->header_.uuid, print_uuid);
> -        print_vflow_datapath_name(mb->datapath, !datapath);
> +        ds_put_cstr(&ctx->output, "  ");
> +        print_uuid_part(&mb->header_.uuid, print_uuid, &ctx->output);
> +        print_vflow_datapath_name(mb->datapath, !datapath, &ctx->output);
>
> -        printf("logical_port=%s, ip=%s, mac=%s\n",
> +        ds_put_format(&ctx->output, "logical_port=%s, ip=%s, mac=%s\n",
>                 mb->logical_port, mb->ip, mb->mac);
>          if (vconn) {
> -            sbctl_dump_openflow(vconn, &mb->header_.uuid, stats);
> +            sbctl_dump_openflow(vconn, &mb->header_.uuid, stats, &ctx->output);
>          }
>
>          mb_prev = mb;
> @@ -776,25 +776,25 @@ cmd_lflow_list_mc_groups(struct ctl_context *ctx, struct vconn *vconn,
>          }
>
>          if (!mc_prev) {
> -            printf("\nMC Groups:\n");
> +            ds_put_cstr(&ctx->output, "\nMC Groups:\n");
>          }
>
> -        printf("  ");
> -        print_uuid_part(&mc->header_.uuid, print_uuid);
> -        print_vflow_datapath_name(mc->datapath, !datapath);
> +        ds_put_cstr(&ctx->output, "  ");
> +        print_uuid_part(&mc->header_.uuid, print_uuid, &ctx->output);
> +        print_vflow_datapath_name(mc->datapath, !datapath, &ctx->output);
>
> -        printf("name=%s, tunnel_key=%-5"PRId64", ports=(",
> -               mc->name, mc->tunnel_key);
> +        ds_put_format(&ctx->output, "name=%s, tunnel_key=%-5"PRId64", ports=(",
> +                      mc->name, mc->tunnel_key);
>          for (size_t i = 0; i < mc->n_ports; i++) {
> -            printf("%s", mc->ports[i]->logical_port);
> +            ds_put_cstr(&ctx->output, mc->ports[i]->logical_port);
>              if (i != mc->n_ports - 1) {
> -                printf(", ");
> +                ds_put_cstr(&ctx->output, ", ");
>              }
>          }
> -        printf(")\n");
> +        ds_put_cstr(&ctx->output, ")\n");
>
>          if (vconn) {
> -            sbctl_dump_openflow(vconn, &mc->header_.uuid, stats);
> +            sbctl_dump_openflow(vconn, &mc->header_.uuid, stats, &ctx->output);
>          }
>
>          mc_prev = mc;
> @@ -809,15 +809,16 @@ cmd_lflow_list_chassis(struct ctl_context *ctx, struct vconn *vconn,
>      const struct sbrec_chassis *chassis_prev = NULL;
>      SBREC_CHASSIS_FOR_EACH (chassis, ctx->idl) {
>          if (!chassis_prev) {
> -            printf("\nChassis:\n");
> +            ds_put_cstr(&ctx->output, "\nChassis:\n");
>          }
>
> -        printf("  ");
> -        print_uuid_part(&chassis->header_.uuid, print_uuid);
> +        ds_put_cstr(&ctx->output, "  ");
> +        print_uuid_part(&chassis->header_.uuid, print_uuid, &ctx->output);
>
> -        printf("name=%s\n", chassis->name);
> +        ds_put_format(&ctx->output, "name=%s\n", chassis->name);
>          if (vconn) {
> -            sbctl_dump_openflow(vconn, &chassis->header_.uuid, stats);
> +            sbctl_dump_openflow(vconn, &chassis->header_.uuid, stats,
> +                                &ctx->output);
>          }
>
>          chassis_prev = chassis;
> @@ -847,27 +848,30 @@ cmd_lflow_list_load_balancers(struct ctl_context *ctx, struct vconn *vconn,
>          }
>
>          if (!lb_prev) {
> -            printf("\nLoad Balancers:\n");
> +            ds_put_cstr(&ctx->output, "\nLoad Balancers:\n");
>          }
>
> -        printf("  ");
> -        print_uuid_part(&lb->header_.uuid, print_uuid);
> -        printf("name=\"%s\", protocol=\"%s\", ", lb->name, lb->protocol);
> +        ds_put_cstr(&ctx->output, "  ");
> +        print_uuid_part(&lb->header_.uuid, print_uuid, &ctx->output);
> +        ds_put_format(&ctx->output, "name=\"%s\", protocol=\"%s\", ",
> +                      lb->name, lb->protocol);
>          if (!dp_found) {
>              for (size_t i = 0; i < lb->n_datapaths; i++) {
> -                print_vflow_datapath_name(lb->datapaths[i], true);
> +                print_vflow_datapath_name(lb->datapaths[i], true,
> +                                          &ctx->output);
>              }
>          }
>
> -        printf("\n  vips:\n");
> +        ds_put_cstr(&ctx->output, "\n  vips:\n");
>          struct smap_node *node;
>          SMAP_FOR_EACH (node, &lb->vips) {
> -            printf("    %s = %s\n", node->key, node->value);
> +            ds_put_format(&ctx->output, "    %s = %s\n",
> +                          node->key, node->value);
>          }
> -        printf("\n");
> +        ds_put_cstr(&ctx->output, "\n");
>
>          if (vconn) {
> -            sbctl_dump_openflow(vconn, &lb->header_.uuid, stats);
> +            sbctl_dump_openflow(vconn, &lb->header_.uuid, stats, &ctx->output);
>          }
>
>          lb_prev = lb;
> @@ -997,24 +1001,27 @@ cmd_lflow_list(struct ctl_context *ctx)
>          if (!prev
>              || prev->dp != curr->dp
>              || strcmp(prev->lflow->pipeline, curr->lflow->pipeline)) {
> -            printf("Datapath: ");
> -            print_datapath_name(curr->dp);
> -            printf(" ("UUID_FMT")  Pipeline: %s\n",
> -                   UUID_ARGS(&curr->dp->header_.uuid),
> -                   curr->lflow->pipeline);
> +            ds_put_cstr(&ctx->output, "Datapath: ");
> +            print_datapath_name(curr->dp, &ctx->output);
> +            ds_put_format(&ctx->output, " ("UUID_FMT")  Pipeline: %s\n",
> +                          UUID_ARGS(&curr->dp->header_.uuid),
> +                          curr->lflow->pipeline);
>          }
>
>          /* Print the flow. */
> -        printf("  ");
> -        print_uuid_part(&curr->lflow->header_.uuid, print_uuid);
> -        printf("table=%-2"PRId64"(%-19s), priority=%-5"PRId64
> -               ", match=(%s), action=(%s)\n",
> -               curr->lflow->table_id,
> -               smap_get_def(&curr->lflow->external_ids, "stage-name", ""),
> -               curr->lflow->priority, curr->lflow->match,
> -               curr->lflow->actions);
> +        ds_put_cstr(&ctx->output, "  ");
> +        print_uuid_part(&curr->lflow->header_.uuid, print_uuid, &ctx->output);
> +        ds_put_format(&ctx->output,
> +                      "table=%-2"PRId64"(%-19s), priority=%-5"PRId64
> +                      ", match=(%s), action=(%s)\n",
> +                      curr->lflow->table_id,
> +                      smap_get_def(&curr->lflow->external_ids,
> +                                   "stage-name", ""),
> +                      curr->lflow->priority, curr->lflow->match,
> +                      curr->lflow->actions);
>          if (vconn) {
> -            sbctl_dump_openflow(vconn, &curr->lflow->header_.uuid, stats);
> +            sbctl_dump_openflow(vconn, &curr->lflow->header_.uuid, stats,
> +                                &ctx->output);
>          }
>          prev = curr;
>      }
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list