[ovs-dev] [PATCH ovn v6] ovn-sbctl.c Add logical flows count numbers
Mark Michelson
mmichels at redhat.com
Fri Jun 11 20:51:26 UTC 2021
Hi Alexey,
Because of commit b6f0e51d8b52cf2381503c3c1c5c2a0d6bd7afa6, this needs a
rebase. It also needs to have its printf() calls removed in favor of
writing to the ctx->output dynamic string.
See below for some other comments, some admittedly kind of nitpicky.
On 5/29/21 12:01 PM, Alexey Roytman wrote:
> From: Alexey Roytman <roytman at il.ibm.com>
>
> For big scale deployments, when number of logical flows can be 2M+,
> sometimes users just need to know the total number of logical flows
> and numbers of logical flows per table/per datapath.
>
> New command output
> ovn-sbctl count-flows
> Datapath: "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda) pipeline: ingress
> table=0 (ls_in_port_sec_l2 ) lflows=2
> table=1 (ls_in_port_sec_ip ) lflows=1
> table=2 (ls_in_port_sec_nd ) lflows=1
> table=3 (ls_in_lookup_fdb ) lflows=1
> table=4 (ls_in_put_fdb ) lflows=1
> table=5 (ls_in_pre_acl ) lflows=2
> table=6 (ls_in_pre_lb ) lflows=3
> table=7 (ls_in_pre_stateful ) lflows=2
> table=8 (ls_in_acl_hint ) lflows=1
> table=9 (ls_in_acl ) lflows=2
> table=10(ls_in_qos_mark ) lflows=1
> table=11(ls_in_qos_meter ) lflows=1
> table=12(ls_in_lb ) lflows=1
> table=13(ls_in_stateful ) lflows=8
> table=14(ls_in_pre_hairpin ) lflows=1
> table=15(ls_in_nat_hairpin ) lflows=1
> table=16(ls_in_hairpin ) lflows=1
> table=17(ls_in_arp_rsp ) lflows=1
> table=18(ls_in_dhcp_options ) lflows=1
> table=19(ls_in_dhcp_response) lflows=1
> table=20(ls_in_dns_lookup ) lflows=1
> table=21(ls_in_dns_response ) lflows=1
> table=22(ls_in_external_port) lflows=1
> table=23(ls_in_l2_lkup ) lflows=3
> table=24(ls_in_l2_unknown ) lflows=2
> Total number of logical flows in the datapath "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda) pipeline: ingress = 41
>
> Datapath: "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda) pipeline: egress
> table=0 (ls_out_pre_lb ) lflows=3
> table=1 (ls_out_pre_acl ) lflows=2
> table=2 (ls_out_pre_stateful) lflows=2
> table=3 (ls_out_lb ) lflows=1
> table=4 (ls_out_acl_hint ) lflows=1
> table=5 (ls_out_acl ) lflows=2
> table=6 (ls_out_qos_mark ) lflows=1
> table=7 (ls_out_qos_meter ) lflows=1
> table=8 (ls_out_stateful ) lflows=3
> table=9 (ls_out_port_sec_ip ) lflows=1
> table=10(ls_out_port_sec_l2 ) lflows=1
> Total number of logical flows in the datapath "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda) pipeline: egress = 18
>
> Total number of logical flows = 59
>
> Signed-off-by: Alexey Roytman <roytman at il.ibm.com>
> ---
> v5 -> v6
> * Addressed Ben's comments about replacemen the --count flag of lflow-list/dump-flows by a a "count-flows" command.
> v3 -> v4
> * Addressed review comments from Mark
>
> NOTE: In the current ovn-sbctl (and probably ovn-nbctl) implementation, some of the methods do not work, when the
> utility is running in the daemon mode (there is no output). All printf methods should be replaced by "ds_put_*" methods.
> I can work on that in a separate path. Meantime, I added to temporary functions "print_datapath_name_new" and
> "print_datapath_prompt_new" just for finishing this task.
>
> tests/ovn-sbctl.at | 45 ++++++++++-
> utilities/ovn-sbctl.8.xml | 4 +
> utilities/ovn-sbctl.c | 153 +++++++++++++++++++++++++++++++++++---
> 3 files changed, 190 insertions(+), 12 deletions(-)
>
> diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
> index f49134381..ff727281e 100644
> --- a/tests/ovn-sbctl.at
> +++ b/tests/ovn-sbctl.at
> @@ -175,4 +175,47 @@ inactivity_probe : 30000
>
> OVN_SBCTL_TEST([ovn_sbctl_invalid_0x_flow], [invalid 0x flow], [
> check ovn-sbctl lflow-list 0x12345678
> -])
> \ No newline at end of file
If you see this, add a new line to the end of the file.
> +])
> +
> +dnl ---------------------------------------------------------------------
> +
> +OVN_SBCTL_TEST([ovn_sbctl_count_flows], [ovn-sbctl - count-flows], [
> +
> +count_entries() {
> + ovn-sbctl --column=_uuid list Logical_Flow | sed -r '/^\s*$/d' | wc -l
> +}
> +
> +count_pipeline() {
> + ovn-sbctl --column=pipeline list Logical_Flow | grep $1 | sed -r '/^\s*$/d' | wc -l
> +}
> +
> +# we start with empty Logical_Flow table
> +# validate that the table is indeed empty
> +AT_CHECK([count_entries], [0], [dnl
> +0
> +])
> +
> +AT_CHECK([ovn-sbctl count-flows], [0], [stdout], [stderr])
> +
> +AT_CHECK([ovn-sbctl count-flows], [0], [dnl
> +Total number of logical flows = 0
> +])
> +
> +# create some logical flows
> +check ovn-nbctl ls-add count-test
> +
> +OVS_WAIT_UNTIL([total_lflows=`count_entries`; test $total_lflows -ne 0])
> +
> +egress_lflows=`count_pipeline egress`
> +ingress_lflows=`count_pipeline ingress`
> +
> +AT_CHECK_UNQUOTED([ovn-sbctl count-flows | grep "flows =" | awk 'NF>1{print $NF}'], [0], [dnl
> +$total_lflows
> +])
> +AT_CHECK_UNQUOTED([ovn-sbctl count-flows | grep Total | grep egress | awk 'NF>1{print $NF}'], [0], [dnl
> +$egress_lflows
> +])
> +AT_CHECK_UNQUOTED([ovn-sbctl count-flows | grep Total | grep ingress | awk 'NF>1{print $NF}'], [0], [dnl
> +$ingress_lflows
> +])
> +])
> diff --git a/utilities/ovn-sbctl.8.xml b/utilities/ovn-sbctl.8.xml
> index 4e6b21c47..ad16f5fa3 100644
> --- a/utilities/ovn-sbctl.8.xml
> +++ b/utilities/ovn-sbctl.8.xml
> @@ -416,10 +416,14 @@
> <var>chassis</var>. The <code>--ovs</code> and <code>--stats</code>
> can also be used in conjunction with <code>--vflows</code>.
> </p>
> +
> </dd>
>
> <dt>[<code>--uuid</code>] <code>dump-flows</code> [<var>logical-datapath</var>]</dt>
> <dd>Alias for <code>lflow-list</code>.</dd>
> +
> + <dt><code>count-flows</code> [<var>logical-datapath</var>]</dt>
> + <dd>prints numbers of logical flows per table and per datapath.</dd>
> </dl>
>
> <h2>Remote Connectivity Commands</h2>
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index 53f10cdd8..ba352c05b 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -105,8 +105,9 @@ Port binding commands:\n\
> lsp-unbind PORT reset the port binding of logical port PORT\n\
> \n\
> Logical flow commands:\n\
> - lflow-list [DATAPATH] [LFLOW...] List logical flows for DATAPATH\n\
> - dump-flows [DATAPATH] [LFLOW...] Alias for lflow-list\n\
> + lflow-list [DATAPATH] [LFLOW...] list logical flows for DATAPATH\n\
> + dump-flows [DATAPATH] [LFLOW...] alias for lflow-list\n\
> + count-flows [DATAPATH] count logical flows for DATAPATH\n\
> \n\
> Connection commands:\n\
> get-connection print the connections\n\
> @@ -682,6 +683,24 @@ print_datapath_name(const struct sbrec_datapath_binding *dp)
> }
> }
>
> +/*
> + A temp function, will replace the original 'print_datapath_name'
> + when all prints will be replaced by ds_put methods.
I suspect that because of the commit referenced at the beginning of my
message, you won't need a temporary function.
> +*/
> +static void
> +print_datapath_name_new(struct ds *ds, const struct sbrec_datapath_binding *dp)
> +{
> + const struct smap *ids = &dp->external_ids;
> + const char *name = smap_get(ids, "name");
> + const char *name2 = smap_get(ids, "name2");
> + if (name && name2) {
> + ds_put_format(ds, "\"%s\" aka \"%s\"", name, name2);
> + } else if (name || name2) {
> + ds_put_format(ds, "\"%s\"", name ? name : name2);
> + }
> +}
> +
> +
> static void
> print_vflow_datapath_name(const struct sbrec_datapath_binding *dp,
> bool do_print)
> @@ -898,14 +917,120 @@ sbctl_lflow_add(struct sbctl_lflow **lflows,
> if (*n_flows == *n_capacity) {
> *lflows = x2nrealloc(*lflows, n_capacity, sizeof **lflows);
> }
> - (*lflows)[*n_flows].lflow = lflow;
> - (*lflows)[*n_flows].dp = dp;
> + (*lflows)[ *n_flows ].lflow = lflow;
> + (*lflows)[ *n_flows ].dp = dp;
The changes on the two lines above are not necessary.
> (*n_flows)++;
> }
>
> +static void
> +print_datapath_prompt(const struct sbrec_datapath_binding *dp,
> + const struct uuid *uuid,
> + char *pipeline) {
> + printf("Datapath: ");
> + print_datapath_name(dp);
> + printf(" ("UUID_FMT") pipeline: %s\n", UUID_ARGS(uuid), pipeline);
> +}
> +
> +/*
> + A temp function, will replace the original 'print_datapath_prompt'
> + when all prints will be replaced by ds_put methods.
> +*/
Similarly to print_datapath_name_new(), I think you can get rid of this
temporary function.
> +static void
> +print_datapath_prompt_new(struct ds *ds,
> + const struct sbrec_datapath_binding *dp,
> + const struct uuid *uuid,
> + char *pipeline) {
> + ds_put_cstr(ds, "Datapath: ");
> + print_datapath_name_new(ds, dp);
> + ds_put_format(ds,
> + " ("UUID_FMT") pipeline: %s\n",
> + UUID_ARGS(uuid), pipeline);
> +}
> +
> +
> +static void
> +print_datapath_sum(struct ds *ds,
> + const struct sbrec_datapath_binding *dp,
> + const struct uuid *uuid,
> + char *pipeline,
> + long lflows) {
> + ds_put_cstr(ds, "Total number of logical flows in the datapath ");
> + print_datapath_name_new(ds, dp);
> + ds_put_format(ds, " ("UUID_FMT") pipeline: %s = %ld\n\n",
> + UUID_ARGS(uuid), pipeline, lflows);
> +}
> +
> +
> +static void
> +print_lflows_count(struct ds *ds,
> + int64_t table_id,
> + const char *name,
> + long count) {
> + ds_put_format(ds, " table=%-2"PRId64"(%-19s) lflows=%ld\n", \
> + table_id, name, count);
> +}
> +
> +static void
> +print_lflow_counters(struct ds *ds, size_t n_flows, struct sbctl_lflow *lflows)
> +{
> + const struct sbctl_lflow *curr, *prev = NULL;
> + long table_lflows = 0;
> + long dp_lflows = 0;
> +
> + for (size_t i = 0; i < n_flows; i++) {
> + bool new_datapath = false;
> + curr = &lflows[i];
> + if (!prev
> + || prev->dp != curr->dp
> + || strcmp(prev->lflow->pipeline, curr->lflow->pipeline)) {
> + new_datapath = true;
> + }
> +
> + if (prev &&
> + (prev->lflow->table_id != curr->lflow->table_id || new_datapath)) {
> + print_lflows_count(ds, prev->lflow->table_id,
> + smap_get_def(&prev->lflow->external_ids, "stage-name", ""),
> + table_lflows);
> + table_lflows = 1;
> + if (new_datapath) {
> + print_datapath_sum(ds, prev->dp, &prev->dp->header_.uuid,
> + prev->lflow->pipeline, dp_lflows);
> + dp_lflows = 1;
> + } else {
> + dp_lflows++;
> + }
> + } else {
> + dp_lflows++;
> + table_lflows++;
> + }
> +
> + if (new_datapath) {
> + print_datapath_prompt_new(ds,
> + curr->dp,
> + &curr->dp->header_.uuid,
> + curr->lflow->pipeline);
> + }
> + prev = curr;
> + }
> + if (n_flows > 0) {
> + print_lflows_count(ds,
> + prev->lflow->table_id,
> + smap_get_def(&prev->lflow->external_ids,
> + "stage-name",
> + ""),
> + table_lflows);
> + print_datapath_sum(ds,
> + prev->dp, &prev->dp->header_.uuid,
> + prev->lflow->pipeline, dp_lflows);
> +
> + }
> + ds_put_format(ds, "Total number of logical flows = %ld\n", n_flows);
> +}
> +
> static void
> cmd_lflow_list(struct ctl_context *ctx)
> {
> + const char *cmd = ctx->argv[0];
> const struct sbrec_datapath_binding *datapath = NULL;
> if (ctx->argc > 1) {
> const struct ovsdb_idl_row *row;
> @@ -966,6 +1091,11 @@ cmd_lflow_list(struct ctl_context *ctx)
> qsort(lflows, n_flows, sizeof *lflows, sbctl_lflow_cmp);
> }
>
> + if (strcmp(cmd, "count-flows")==0) {
I don't think there's actually a coding guideline that states this, but
it's unusual to compare the result of strcmp like this. I think most of
us are trained to see "strcmp()" as meaning "the strings are different"
and "!strcmp()" as meaning "the strings are the same". So it tripped me
up for a second to see it written this way.
Also, please put a space before and after the "==" operator when you're
using it. I'm actually surprised checkpatch didn't flag that.
> + print_lflow_counters(&ctx->output, n_flows, lflows);
> + goto cleanup;
> + }
> +
> bool print_uuid = shash_find(&ctx->options, "--uuid") != NULL;
>
> const struct sbctl_lflow *curr, *prev = NULL;
> @@ -997,11 +1127,9 @@ 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);
> + print_datapath_prompt(curr->dp,
> + &curr->dp->header_.uuid,
> + curr->lflow->pipeline);
> }
>
> /* Print the flow. */
> @@ -1028,6 +1156,7 @@ cmd_lflow_list(struct ctl_context *ctx)
> cmd_lflow_list_load_balancers(ctx, vconn, datapath, stats, print_uuid);
> }
>
> +cleanup:
> vconn_close(vconn);
> free(lflows);
> }
> @@ -1378,8 +1507,10 @@ static const struct ctl_command_syntax sbctl_commands[] = {
> "--uuid,--ovs?,--stats,--vflows?", RO},
> {"dump-flows", 0, INT_MAX, "[DATAPATH] [LFLOW...]",
> pre_get_info, cmd_lflow_list, NULL,
> - "--uuid,--ovs?,--stats,--vflows?",
> - RO}, /* Friendly alias for lflow-list */
> + "--uuid,--ovs?,--stats,--vflows?", RO},
> + /* Friendly alias for lflow-list */
> + {"count-flows", 0, 1, "[DATAPATH]",
> + pre_get_info, cmd_lflow_list, NULL, "", RO},
>
> /* IP multicast commands. */
> {"ip-multicast-flush", 0, 1, "SWITCH",
>
More information about the dev
mailing list