On Fri, May 6, 2022 at 4:43 PM Adrian Moreno <[email protected]> wrote:
>
> On 2/28/22 12:40, Wan Junjie wrote:
> > put dump-meters' result in one line so add-meters can handle.
> > save and restore meters when restart ovs.
> > bundle functions are not implemented in this patch.
> >
> > Signed-off-by: Wan Junjie <[email protected]>
> > ---
> > v2:
> > fix failed testcases, as dump-meters format changes
> > ---
> > include/openvswitch/ofp-meter.h | 6 +++
> > lib/ofp-meter.c | 94 ++++++++++++++++++++++++++++++++-
> > tests/dpif-netdev.at | 6 +--
> > tests/ofp-print.at | 13 ++---
> > tests/ofproto-dpif.at | 3 +-
> > tests/ofproto.at | 3 +-
> > utilities/ovs-ofctl.c | 86 +++++++++++++++++++++++-------
> > utilities/ovs-save | 8 +++
> > 8 files changed, 182 insertions(+), 37 deletions(-)
> >
> > diff --git a/include/openvswitch/ofp-meter.h
> > b/include/openvswitch/ofp-meter.h
> > index 6776eae87..34ecbfdb2 100644
> > --- a/include/openvswitch/ofp-meter.h
> > +++ b/include/openvswitch/ofp-meter.h
> > @@ -79,6 +79,12 @@ char *parse_ofp_meter_mod_str(struct ofputil_meter_mod
> > *, const char *string,
> > OVS_WARN_UNUSED_RESULT;
> > void ofputil_format_meter_mod(struct ds *, const struct ofputil_meter_mod
> > *);
> >
> > +char *parse_ofp_meter_mod_file(const char *file_name,
> > + int command,
> > + struct ofputil_meter_mod **mms, size_t *n_mms,
> > + enum ofputil_protocol *usable_protocols)
> > + OVS_WARN_UNUSED_RESULT;
> > +
> > struct ofputil_meter_stats {
> > uint32_t meter_id;
> > uint32_t flow_count;
> > diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
> > index 9ea40a0bf..5fb780ba3 100644
> > --- a/lib/ofp-meter.c
> > +++ b/lib/ofp-meter.c
> > @@ -15,6 +15,7 @@
> > */
> >
> > #include <config.h>
> > +#include <errno.h>
> > #include "openvswitch/ofp-meter.h"
> > #include "byte-order.h"
> > #include "nx-match.h"
> > @@ -57,7 +58,7 @@ void
> > ofputil_format_meter_band(struct ds *s, enum ofp13_meter_flags flags,
> > const struct ofputil_meter_band *mb)
> > {
> > - ds_put_cstr(s, "\ntype=");
> > + ds_put_cstr(s, " type=");
> > switch (mb->type) {
> > case OFPMBT13_DROP:
> > ds_put_cstr(s, "drop");
> > @@ -578,6 +579,24 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod
> > *mm, char *string,
> >
> > /* Meters require at least OF 1.3. */
> > *usable_protocols = OFPUTIL_P_OF13_UP;
> > + if (command == -2) {
> > + size_t len;
> > +
> > + string += strspn(string, " \t\r\n"); /* Skip white space. */
> > + len = strcspn(string, ", \t\r\n"); /* Get length of the first
> > token. */
> > +
> > + if (!strncmp(string, "add", len)) {
> > + command = OFPMC13_ADD;
> > + } else if (!strncmp(string, "delete", len)) {
> > + command = OFPMC13_DELETE;
> > + } else if (!strncmp(string, "modify", len)) {
> > + command = OFPMC13_MODIFY;
> > + } else {
> > + len = 0;
> > + command = OFPMC13_ADD;
> > + }
> > + string += len;
> > + }
> >
> > switch (command) {
> > case -1:
> > @@ -606,6 +625,11 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod
> > *mm, char *string,
> > mm->meter.n_bands = 0;
> > mm->meter.bands = NULL;
> >
> > + if (command == OFPMC13_DELETE && string[0] == '\0') {
> > + mm->meter.meter_id = OFPM13_ALL;
> > + return NULL;
> > + }
> > +
> > if (fields & F_BANDS) {
> > band_str = strstr(string, "band");
> > if (!band_str) {
> > @@ -807,3 +831,71 @@ ofputil_format_meter_mod(struct ds *s, const struct
> > ofputil_meter_mod *mm)
> >
> > ofputil_format_meter_config(s, &mm->meter);
> > }
> > +
> > +/* If 'command' is given as -2, each line may start with a command name
> > ("add",
> > + * "modify", "delete"). A missing command name is treated as "add".
> > + */
> > +char * OVS_WARN_UNUSED_RESULT
> > +parse_ofp_meter_mod_file(const char *file_name,
> > + int command,
> > + struct ofputil_meter_mod **mms, size_t *n_mms,
> > + enum ofputil_protocol *usable_protocols)
> > +{
> > + size_t allocated_mms;
> > + int line_number;
> > + FILE *stream;
> > + struct ds s;
> > +
> > + *mms = NULL;
> > + *n_mms = 0;
> > +
> > + stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r");
> > + if (stream == NULL) {
> > + return xasprintf("%s: open failed (%s)",
> > + file_name, ovs_strerror(errno));
> > + }
> > +
> > + allocated_mms = *n_mms;
> > + ds_init(&s);
> > + line_number = 0;
> > + *usable_protocols = OFPUTIL_P_ANY;
> > + while (!ds_get_preprocessed_line(&s, stream, &line_number)) {
> > + enum ofputil_protocol usable;
> > + char *error;
> > +
> > + if (*n_mms >= allocated_mms) {
> > + *mms = x2nrealloc(*mms, &allocated_mms, sizeof **mms);
> > + }
> > + error = parse_ofp_meter_mod_str(&(*mms)[ *n_mms], ds_cstr(&s),
> > command,
> > + &usable);
> > + if (error) {
> > + size_t i;
> > +
> > + for (i = 0; i < *n_mms; i++) {
> > + if (mms[i]->meter.bands) {
> > + free(mms[i]->meter.bands);
> > + }
> > + }
> > + free(*mms);
> > + *mms = NULL;
> > + *n_mms = 0;
> > +
> > + ds_destroy(&s);
> > + if (stream != stdin) {
> > + fclose(stream);
> > + }
> > +
> > + char *ret = xasprintf("%s:%d: %s", file_name, line_number,
> > error);
> > + free(error);
> > + return ret;
> > + }
> > + *usable_protocols &= usable;
> > + *n_mms += 1;
> > + }
> > +
> > + ds_destroy(&s);
> > + if (stream != stdin) {
> > + fclose(stream);
> > + }
> > + return NULL;
> > +}
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index a79ebdb61..98c474d20 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -291,11 +291,9 @@ ovs-appctl time/stop
> >
> > AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
> > OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > -meter=1 pktps burst stats bands=
> > -type=drop rate=1 burst_size=1
> > +meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
> >
> > -meter=2 kbps burst stats bands=
> > -type=drop rate=1 burst_size=2
> > +meter=2 kbps burst stats bands= type=drop rate=1 burst_size=2
> > ])
> >
> > ovs-appctl time/warp 5000
> > diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> > index 2c7e163bd..36ebccefc 100644
> > --- a/tests/ofp-print.at
> > +++ b/tests/ofp-print.at
> > @@ -2371,8 +2371,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
> > 04 1d 00 20 00 00 00 02 00 00 00 0d 00 00 00 05 \
> > 00 01 00 10 00 00 04 00 00 00 00 80 00 00 00 00 \
> > "], [0], [dnl
> > -OFPT_METER_MOD (OF1.3) (xid=0x2): ADD meter=5 kbps burst stats bands=
> > -type=drop rate=1024 burst_size=128
> > +OFPT_METER_MOD (OF1.3) (xid=0x2): ADD meter=5 kbps burst stats bands=
> > type=drop rate=1024 burst_size=128
> > ])
> > AT_CLEANUP
> >
> > @@ -2455,12 +2454,9 @@ AT_CHECK([ovs-ofctl ofp-print "\
> > 00 01 00 10 00 02 00 00 00 00 00 00 00 00 00 00 \
> > "], [0], [dnl
> > OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > -meter=1 kbps burst bands=
> > -type=drop rate=65536 burst_size=1280
> > -type=dscp_remark rate=1048576 burst_size=61440 prec_level=0
> > +meter=1 kbps burst bands= type=drop rate=65536 burst_size=1280
> > type=dscp_remark rate=1048576 burst_size=61440 prec_level=0
> >
> > -meter=2 kbps stats bands=
> > -type=drop rate=131072
> > +meter=2 kbps stats bands= type=drop rate=131072
> > ])
> > AT_CLEANUP
> >
> > @@ -3071,8 +3067,7 @@ dnl inner msg copied and pasted from the valid
> > OFPT_METER_MOD OF1.3 test:
> > 04 1d 00 20 00 00 00 02 00 00 00 0d 00 00 00 05 \
> > 00 01 00 10 00 00 04 00 00 00 00 80 00 00 00 00 \
> > "], [0], [dnl
> > -ONFT_REQUESTFORWARD (OF1.3) (xid=0x2): reason=meter_mod ADD meter=5 kbps
> > burst stats bands=
> > -type=drop rate=1024 burst_size=128
> > +ONFT_REQUESTFORWARD (OF1.3) (xid=0x2): reason=meter_mod ADD meter=5 kbps
> > burst stats bands= type=drop rate=1024 burst_size=128
> > ])
> > AT_CLEANUP
> >
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 912f3a1da..52cb49774 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -9834,8 +9834,7 @@ ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1
> > packet=50540000000a50540000000
> > # Check that vswitchd hasn't crashed by dumping the meter added above
> > AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip], [0],
> > [dnl
> > OFPST_METER_CONFIG reply (OF1.3):
> > -meter=1 pktps bands=
> > -type=drop rate=1
> > +meter=1 pktps bands= type=drop rate=1
> > ])
> >
> > OVS_VSWITCHD_STOP
> > diff --git a/tests/ofproto.at b/tests/ofproto.at
> > index 156d3e058..a6a61d24c 100644
> > --- a/tests/ofproto.at
> > +++ b/tests/ofproto.at
> > @@ -6412,8 +6412,7 @@ OFPST_GROUP_DESC reply (OF1.1) (xid=0x2):
> > ])
> > AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
> > OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > -meter=1 pktps burst stats bands=
> > -type=drop rate=1 burst_size=1
> > +meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
> > ])
> > }
> >
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index ede7f1e61..152836276 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -474,6 +474,7 @@ usage(void)
> > " dump-group-stats SWITCH [GROUP] print group statistics\n"
> > " queue-get-config SWITCH [PORT] print queue config for
> > PORT\n"
> > " add-meter SWITCH METER add meter described by METER\n"
> > + " add-meters SWITCH FILE add meters from FILE\n"
> > " mod-meter SWITCH METER modify specific METER\n"
> > " del-meters SWITCH [METER] delete meters matching METER\n"
> > " dump-meters SWITCH [METER] print METER configuration\n"
> > @@ -4012,32 +4013,70 @@ ofctl_diff_flows(struct ovs_cmdl_context *ctx)
> > }
> >
> > static void
> > -ofctl_meter_mod__(const char *bridge, const char *str, int command)
> > +ofctl_meter_mod__(const char *remote, struct ofputil_meter_mod *mms,
> > + size_t n_mms, enum ofputil_protocol usable_protocols)
> > {
> > - struct ofputil_meter_mod mm;
> > + struct ofputil_meter_mod *mm;
> > struct vconn *vconn;
> > enum ofputil_protocol protocol;
> > - enum ofputil_protocol usable_protocols;
> > enum ofp_version version;
> > + struct ofpbuf *request;
> > + size_t i;
> >
> > memset(&mm, 0, sizeof mm);
> > - if (str) {
> > + protocol = open_vconn_for_flow_mod(remote, &vconn, usable_protocols);
> > + version = ofputil_protocol_to_ofp_version(protocol);
> > +
> > + for (i = 0; i < n_mms; i++) {
> > + mm = &mms[i];
> > + request = ofputil_encode_meter_mod(version, mm);
> > + transact_noreply(vconn, request);
> > + free(mm->meter.bands);
> > + }
> > +
> > + vconn_close(vconn);
> > +}
> > +
> > +static void
> > +ofctl_meter_mod_file(int argc OVS_UNUSED, char *argv[], int command)
> > +{
> > + enum ofputil_protocol usable_protocols;
> > + struct ofputil_meter_mod *mms = NULL;
> > + size_t n_mms = 0;
> > + char *error;
> > +
> > + if (command == OFPMC13_ADD) {
> > + /* Allow the file to specify a mix of commands. If none specified
> > at
> > + * the beginning of any given line, then the default is
> > OFPMC13_ADD, so
> > + * this is backwards compatible. */
> > + command = -2;
> > +
> > + }
> > + error = parse_ofp_meter_mod_file(argv[2], command,
> > + &mms, &n_mms, &usable_protocols);
> > + if (error) {
> > + ovs_fatal(0, "%s", error);
> > + }
> > + ofctl_meter_mod__(argv[1], mms, n_mms, usable_protocols);
> > + free(mms);
> > +}
> > +
> > +static void
> > +ofctl_meter_mod(int argc, char *argv[], uint16_t command)
> > +{
> > + if (argc > 2 && !strcmp(argv[2], "-")) {
> > + ofctl_meter_mod_file(argc, argv, command);
> > + } else {
> > + enum ofputil_protocol usable_protocols;
> > + struct ofputil_meter_mod mm;
> > char *error;
> > - error = parse_ofp_meter_mod_str(&mm, str, command,
> > &usable_protocols);
> > + error = parse_ofp_meter_mod_str(&mm, argc > 2 ? argv[2] : "",
> > command,
> > + &usable_protocols);
> > if (error) {
> > ovs_fatal(0, "%s", error);
> > }
> > - } else {
> > - usable_protocols = OFPUTIL_P_OF13_UP;
> > - mm.command = command;
> > - mm.meter.meter_id = OFPM13_ALL;
> > + ofctl_meter_mod__(argv[1], &mm, 1, usable_protocols);
> > }
> > -
> > - protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols);
> > - version = ofputil_protocol_to_ofp_version(protocol);
> > - transact_noreply(vconn, ofputil_encode_meter_mod(version, &mm));
> > - free(mm.meter.bands);
> > - vconn_close(vconn);
> > }
> >
> > static void
> > @@ -4074,19 +4113,26 @@ ofctl_meter_request__(const char *bridge, const
> > char *str,
> > static void
> > ofctl_add_meter(struct ovs_cmdl_context *ctx)
> > {
> > - ofctl_meter_mod__(ctx->argv[1], ctx->argv[2], OFPMC13_ADD);
> > + ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_ADD);
> > }
> >
> > +static void
> > +ofctl_add_meters(struct ovs_cmdl_context *ctx)
> > +{
> > + ofctl_meter_mod_file(ctx->argc, ctx->argv, OFPMC13_ADD);
> > +}
> > +
> > +
> > static void
> > ofctl_mod_meter(struct ovs_cmdl_context *ctx)
> > {
> > - ofctl_meter_mod__(ctx->argv[1], ctx->argv[2], OFPMC13_MODIFY);
> > + ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_MODIFY);
> > }
> >
> > static void
> > ofctl_del_meters(struct ovs_cmdl_context *ctx)
> > {
> > - ofctl_meter_mod__(ctx->argv[1], ctx->argc > 2 ? ctx->argv[2] : NULL,
> > OFPMC13_DELETE);
> > + ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_DELETE);
> > }
> >
> > static void
> > @@ -5000,9 +5046,11 @@ static const struct ovs_cmdl_command all_commands[]
> > = {
> > 2, 2, ofctl_diff_flows, OVS_RW },
> > { "add-meter", "switch meter",
> > 2, 2, ofctl_add_meter, OVS_RW },
> > + { "add-meters", "switch file",
> > + 2, 2, ofctl_add_meters, OVS_RW },
> > { "mod-meter", "switch meter",
> > 2, 2, ofctl_mod_meter, OVS_RW },
> > - { "del-meter", "switch meter",
> > + { "del-meter", "switch [meter]",
> > 1, 2, ofctl_del_meters, OVS_RW },
> > { "del-meters", "switch",
> > 1, 2, ofctl_del_meters, OVS_RW },
> > diff --git a/utilities/ovs-save b/utilities/ovs-save
> > index fb2025b76..e3551b9f9 100755
> > --- a/utilities/ovs-save
> > +++ b/utilities/ovs-save
> > @@ -139,6 +139,9 @@ save_flows () {
> > echo "ovs-ofctl -O $ofp_version add-groups ${bridge} \
> > \"$workdir/$bridge.groups.dump\" ${bundle}"
> >
> > + echo "ovs-ofctl -O $ofp_version add-meters ${bridge} \
> > + \"$workdir/$bridge.meters.dump\""
> > +
> > echo "ovs-ofctl -O $ofp_version replace-flows ${bridge} \
> > \"$workdir/$bridge.flows.dump\" ${bundle}"
> >
> > @@ -147,6 +150,11 @@ save_flows () {
> > -e '/^NXST_GROUP_DESC/d' > \
> > "$workdir/$bridge.groups.dump"
> >
> > + ovs-ofctl -O $ofp_version dump-meters "$bridge" | \
> > + sed -e '/^OFPST_METER_CONFIG/d' \
> > + -e '/^NXST_METER_CONFIG/d' > \
> > + "$workdir/$bridge.meters.dump"
> > +
> > ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats
> > "$bridge" | \
> > sed -e '/NXST_FLOW/d' \
> > -e '/OFPST_FLOW/d' \
> >
>
>
>
> Hi Wan,
>
> Thank you for working on this.
>
> I see that your implementation basically mimics the one in ofp-flow.c and,
> alghough I think it's good to have a more homogeneus user experience I don't
> fully understand why we'd need that much complexity.
>
> Why do we need to be able to specify "modify" and "delete" actions within the
> "add-meter" and "add-meters" commands? AFAIU, those options can be helpful for
> unit tests in which we want to replay a potentially complex sequence of
> controller flow commands. I don't understand why we need that in the meter
> table, it being so simple and I don't see any use for it in this patch either.
>
Hi Adrian,
Thank you for your kind comments.
You are right, these functions mimic operations from flows. We will
need 'modify' and 'delete' for testing bundle operations which I plan
to submit a new patch. With bundle support, replay flows with meter
actions would be possible and useful in the controller.
> On the other hand, although I agree the multi-line meter representation is not
> ideal, I'm not 100% sure we should simply modify it as other users might be
> parsing that output. The output of other ofctl commands has been considered a
> pseudo-API at times. Maybe some kind of "--single-line" knob can do the work.
> I'd like Ilya's thoughts on this
>
Make sense, will do in the next version.
> Finally, from this patch I miss:
> - Documentation: There is no modification of the man page which should now
> need
> to include all the new command options. In fact, the current documentation is
> quite confusing because it says:
>
> > del-meters switch [meter]
> >
> > Delete entries from switch's meter table. To delete only a specific
> > meter, specify its number as meter. Otherwise, if meter is omitted, or if
> > it is specified as all, then all meters are deleted.
>
> ...which is not true: [meter] cannot be just the meter id, it has to be
> "meter={meter_id}".
>
> - Unit tests for all new ways of modifying the meter table.
>
Thanks. I will update the doc and add test cases.
Wan Junjie
>
> Thanks
>
> --
> Adrián Moreno
>
> --
> Adrián Moreno
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
