[ovs-dev] [PATCH ovn v3] ovn-sbctl: Add logical flows count numbers
Mark Michelson
mmichels at redhat.com
Mon May 10 20:42:38 UTC 2021
Hi Alexey,
In general, the C side of things looks good. I recommend that you run
./utilities/checkpatch.py against the resulting patch, though.
checkpatch.py recently had a bug fixed in it so that it correctly
catches lines that are in excess of 80 characters now. You'll find that
there are several violations in this patch.
See below for comments about the added test.
On 4/13/21 11:18 AM, 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
> 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 = 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 = 18
>
> Total number of logical flows = 59
>
> Signed-off-by: Alexey Roytman <roytman at il.ibm.com>
> ---
> tests/ovn-sbctl.at | 29 ++++++++++++++++
> utilities/ovn-sbctl.8.in | 5 +++
> utilities/ovn-sbctl.c | 72 ++++++++++++++++++++++++++++++++++++----
> 3 files changed, 99 insertions(+), 7 deletions(-)
>
> diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
> index 2712cc154..b15b15964 100644
> --- a/tests/ovn-sbctl.at
> +++ b/tests/ovn-sbctl.at
> @@ -148,3 +148,32 @@ inactivity_probe : 30000
>
> OVN_SBCTL_TEST_STOP
> AT_CLEANUP
> +
> +dnl ---------------------------------------------------------------------
> +
> +AT_SETUP([ovn-sbctl --count lflow-list/dump-flows])
> +OVN_SBCTL_TEST_START
> +
> +AT_CHECK([ovn-nbctl ls-add count-test])
You can use the "check" function here as a shortcut since you are not
checking any output:
check ovn-nbctl ls-add count-test
> +AT_CHECK([ovn-sbctl --count lflow-list | cat <<EOF | python3
> +import sys
> +
> +lflows=0
> +dp_flows=0
> +total_flows=0
> +
> +for line in sys.stdin:
> + x = line.rsplit("=", 1)
> + if "table=" in line:
> + lflows += int(x[1])
> + elif "flows in the datapath" in line:
> + dp_flows += int(x[1])
> + elif "Total number of logical flows =" in line:
> + total_flows = int(x[1])
> +if lflows == dp_flows == total_flows:
> + sys.exit(0)
> +sys.exit(1)
> +EOF], [0], [ignore], [ignore])
> +
> +OVN_SBCTL_TEST_STOP
> +AT_CLEANUP
The issue I have with this test is that it doesn't check the values
against any expected result. It ensures that the values reported are
internally consistent, but it does not ensure that the counts are correct.
In an ideal world, you'd be able to write up some logical networks with
known numbers of generated logical flows. Then you'd run the test
command and ensure that the flows are what you expect. However, being
able to accurately predict the logical flows generated from a given
network is impractical and difficult. Plus, the number is likely to
change at any given time. So I think you have to rely on something other
than a pre-computation to determine the correct number of flows.
I think you have two courses of action here:
1) Query the logical_flow table directly from the test code to determine
the number of records in it and compare to what the new command returns
2) Use ovn-sbctl lflow-list (without --count) to determine the number of
flows
(1) is likely to be more cumbersome but it also has the advantage that
it is comparing your count to the data being counted.
And finally, I suggest doing an edge case test where you ensure that
when the logical_flow table is empty, that the --count option shows 0
for everything.
> diff --git a/utilities/ovn-sbctl.8.in b/utilities/ovn-sbctl.8.in
> index 153e72e6c..6de4ab968 100644
> --- a/utilities/ovn-sbctl.8.in
> +++ b/utilities/ovn-sbctl.8.in
> @@ -207,6 +207,11 @@ conjunction with \fB\-\-vflows\fR.
> .IP "[\fB\-\-uuid\fR] \fBdump\-flows\fR [\fIlogical-datapath\fR]"
> Alias for \fBlflow\-list\fB.
> .
> +.IP "[\fB\-\-count\fR] \fBlflow\-list\fR [\fIlogical-datapath\fR]"
> +and
> +.IP "[\fB\-\-count\fR] \fBdump\-flows\fR [\fIlogical-datapath\fR]"
> +Print numbers of logical flows per table and per datapath.
> +.
> .SS "Remote Connectivity Commands"
> .
> These commands manipulate the \fBconnections\fR column in the \fBSB_Global\fR
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index e3aa7a68e..4cb032173 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -1114,6 +1114,63 @@ sbctl_lflow_add(struct sbctl_lflow **lflows,
> (*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);
> +}
> +
> +static void
> +print_lflows_count(int64_t table_id, const char *name, long count) {
> + printf(" table=%-2"PRId64"(%-19s) lflows=%ld\n", \
> + table_id, name, count);
> +}
> +
> +static void
> +print_lflow_counters(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(prev->lflow->table_id, smap_get_def(&prev->lflow->external_ids, "stage-name", ""), table_lflows);
> + table_lflows = 1;
> + if (new_datapath) {
> + printf("Total number of logical flows in the datapath = %ld\n\n", dp_lflows);
> + dp_lflows = 1;
> + } else {
> + dp_lflows++;
> + }
> + } else {
> + dp_lflows++;
> + table_lflows++;
> + }
> +
> + if (new_datapath) {
> + print_datapath_prompt(curr->dp, &curr->dp->header_.uuid, curr->lflow->pipeline);
> + }
> + prev = curr;
> + }
> + if (n_flows > 0) {
> + print_lflows_count(prev->lflow->table_id, smap_get_def(&prev->lflow->external_ids, "stage-name", ""), table_lflows);
> + printf("Total number of logical flows in the datapath = %ld\n\n", dp_lflows);
> + }
> + printf("Total number of logical flows = %ld\n", n_flows);
> +}
> +
> static void
> cmd_lflow_list(struct ctl_context *ctx)
> {
> @@ -1176,6 +1233,10 @@ cmd_lflow_list(struct ctl_context *ctx)
> qsort(lflows, n_flows, sizeof *lflows, sbctl_lflow_cmp);
> }
>
> + if (shash_find(&ctx->options, "--count") != NULL) {
> + print_lflow_counters(n_flows, lflows);
> + goto cleanup;
> + }
> bool print_uuid = shash_find(&ctx->options, "--uuid") != NULL;
>
> const struct sbctl_lflow *curr, *prev = NULL;
> @@ -1207,11 +1268,7 @@ 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. */
> @@ -1238,6 +1295,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);
> }
> @@ -1824,10 +1882,10 @@ static const struct ctl_command_syntax sbctl_commands[] = {
> /* Logical flow commands */
> {"lflow-list", 0, INT_MAX, "[DATAPATH] [LFLOW...]",
> pre_get_info, cmd_lflow_list, NULL,
> - "--uuid,--ovs?,--stats,--vflows?", RO},
> + "--uuid,--ovs?,--stats,--vflows?,--count?", RO},
> {"dump-flows", 0, INT_MAX, "[DATAPATH] [LFLOW...]",
> pre_get_info, cmd_lflow_list, NULL,
> - "--uuid,--ovs?,--stats,--vflows?",
> + "--uuid,--ovs?,--stats,--vflows?,--count?",
> RO}, /* Friendly alias for lflow-list */
>
> /* IP multicast commands. */
>
More information about the dev
mailing list