Hi Mark, Thanks for the comments. With a delay, but I've fixed the patch. Mistakenly, I used a slightly different email subject, so please see the fixes in the [ovs-dev] [PATCH ovn v5] ovn-sbctl.c Add logical flows count numbers <https://www.mail-archive.com/[email protected]/msg55127.html> thread. Thanks Alexey.
On Mon, May 10, 2021 at 11:42 PM Mark Michelson <[email protected]> wrote: > 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 : 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. */ > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
