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 <[email protected]> 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 <[email protected]> --- 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 : 30000OVN_SBCTL_TEST_STOPAT_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. */
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
