[ovs-dev] [PATCH v2]: ofproto: Add relaxed group_mod command ADD_OR_MOD
Jan Scheurich
jan.scheurich at ericsson.com
Tue Jun 28 14:18:30 UTC 2016
I have noticed in patchwork that this patch has been set to "Not Applicable"
http://patchwork.ozlabs.org/patch/630624/
What does that mean? Is there anything I did wrong? How should I fix this?
I checked today and the patch still applies on master except for the NEWS file.
I can submit a new version that fixes this.
Thanks, Jan
> -----Original Message-----
> From: Jan Scheurich [mailto:jan.scheurich at web.de]
> Sent: Monday, 06 June, 2016 00:31
> To: dev at openvswitch.org
> Cc: jan.scheurich at web.de
> Subject: [PATCH v2]: ofproto: Add relaxed group_mod command
> ADD_OR_MOD
>
> This patch adds support for a new Group Mod command
> OFPGC_ADD_OR_MOD to OVS for all OpenFlow versions that support groups
> (OF11 and higher). The new ADD_OR_MOD creates a group that does not yet
> exist (like ADD) and modifies an existing group (like MODIFY).
>
> Rational: In OpenFlow 1.x the Group Mod commands OFPGC_ADD and
> OFPGC_MODIFY have strict semantics: ADD fails if the group exists, while
> MODIFY fails if the group does not exist. This requires a controller to exactly
> know the state of the switch when programming a group in order not run the
> risk of getting an OFP Error message in response. This is hard to achieve and
> maintain at all times in view of possible switch and controller restarts or other
> connection losses between switch and controller.
>
> Due to the un-acknowledged nature of the Group Mod message
> programming groups safely and efficiently at the same time is virtually
> impossible as the controller has to either query the existence of the group
> prior to each Group Mod message or to insert a Barrier Request/Reply after
> every group to be sure that no Error can be received at a later stage and
> require a complicated roll-back of any dependent actions taken between the
> failed Group Mod and the Error.
>
> In the ovs-ofctl command line the ADD_OR_MOD command is made
> available through the new option --may-create in the mod-group command:
>
> $ ovs-ofctl -Oopenflow13 del-groups br-int group_id=100
>
> $ ovs-ofctl -Oopenflow13 mod-group br-int
> group_id=100,type=indirect,bucket=actions=2
> OFPT_ERROR (OF1.3) (xid=0x2): OFPGMFC_UNKNOWN_GROUP
> OFPT_GROUP_MOD (OF1.3) (xid=0x2):
> MOD group_id=100,type=indirect,bucket=actions=output:2
>
> $ ovs-ofctl -Oopenflow13 --may-create mod-group br-int
> group_id=100,type=indirect,bucket=actions=2
>
> $ ovs-ofctl -Oopenflow13 dump-groups br-int OFPST_GROUP_DESC reply
> (OF1.3) (xid=0x2):
> group_id=100,type=indirect,bucket=actions=output:2
>
> $ ovs-ofctl -Oopenflow13 --may-create mod-group br-int
> group_id=100,type=indirect,bucket=actions=3
>
> $ ovs-ofctl -Oopenflow13 dump-groups br-int OFPST_GROUP_DESC reply
> (OF1.3) (xid=0x2):
> group_id=100,type=indirect,bucket=actions=output:3
>
>
> Patch v2:
> - Replaced new ovs-ofctl write-group command with --may-create option for
> mod-group
> - Updated ovs-ofctl --help message
> - Added a test for the new command option
> - Updated documentation
>
> Signed-off-by: Jan Scheurich <jan.scheurich at web.de>
>
> To aid review this patch is available at:
> tree: https://github.com/JScheurich/openvswitch
> branch: dev/group_add_modify
> tag: group_add_modify_v2
>
> ---
>
> NEWS | 3 +++
> include/openflow/openflow-1.1.h | 1 +
> include/openflow/openflow-1.5.h | 1 +
> lib/ofp-parse.c | 4 ++++
> lib/ofp-print.c | 4 ++++
> lib/ofp-util.c | 7 ++++++-
> ofproto/ofproto.c | 25 +++++++++++++++++++++++++
> tests/ofproto.at | 26 ++++++++++++++++++++++++++
> utilities/ovs-ofctl.8.in | 12 +++++++++---
> utilities/ovs-ofctl.c | 14 +++++++++++++-
> 10 files changed, 92 insertions(+), 5 deletions(-)
>
>
> diff --git a/NEWS b/NEWS
> index ba201cf..74bda47 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -15,10 +15,13 @@ Post-v2.5.0
> now implemented. Only flow mod and port mod messages are
> supported
> in bundles.
> * New OpenFlow extension NXM_NX_MPLS_TTL to provide access to
> MPLS TTL.
> + * New command OFPGC_ADD_OR_MOD for OFPT_GROUP_MOD
> message that adds a
> + new group or modifies an existing groups
> - ovs-ofctl:
> * queue-get-config command now allows a queue ID to be specified.
> * '--bundle' option can now be used with OpenFlow 1.3.
> * New option "--color" to produce colorized output for some commands.
> + * '--may-create' option to use OFPGC_ADD_OR_MOD in mod-group
> command.
> - DPDK:
> * New option "n_rxq" for PMD interfaces.
> Old 'other_config:n-dpdk-rxqs' is no longer supported.
> diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-
> 1.1.h index 805f222..de28475 100644
> --- a/include/openflow/openflow-1.1.h
> +++ b/include/openflow/openflow-1.1.h
> @@ -172,6 +172,7 @@ enum ofp11_group_mod_command {
> OFPGC11_ADD, /* New group. */
> OFPGC11_MODIFY, /* Modify all matching groups. */
> OFPGC11_DELETE, /* Delete all matching groups. */
> + OFPGC11_ADD_OR_MOD = 0x8000, /* Create new or modify existing
> + group. */
> };
>
> /* OpenFlow 1.1 specific capabilities supported by the datapath (struct diff --
> git a/include/openflow/openflow-1.5.h b/include/openflow/openflow-1.5.h
> index 0c478d1..3649e6c 100644
> --- a/include/openflow/openflow-1.5.h
> +++ b/include/openflow/openflow-1.5.h
> @@ -59,6 +59,7 @@ enum ofp15_group_mod_command {
> /* OFPGCXX_YYY = 4, */ /* Reserved for future use. */
> OFPGC15_REMOVE_BUCKET = 5,/* Remove all action buckets or any
> specific
> action bucket from matching group */
> + OFPGC15_ADD_OR_MOD = 0x8000, /* Create new or modify existing
> + group. */
> };
>
> /* Group bucket property types. */
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 71c5cdf..4af6d9b 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -1380,6 +1380,10 @@ parse_ofp_group_mod_str__(struct
> ofputil_group_mod *gm, uint16_t
> fields = F_GROUP_TYPE | F_BUCKETS;
> break;
>
> + case OFPGC11_ADD_OR_MOD:
> + fields = F_GROUP_TYPE | F_BUCKETS;
> + break;
> +
> case OFPGC15_INSERT_BUCKET:
> fields = F_BUCKETS | F_COMMAND_BUCKET_ID;
> *usable_protocols &= OFPUTIL_P_OF15_UP; diff --git a/lib/ofp-print.c
> b/lib/ofp-print.c index b21d76f..ce2eecb 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -2673,6 +2673,10 @@ ofp_print_group_mod__(struct ds *s, enum
> ofp_version ofp_version,
> ds_put_cstr(s, "MOD");
> break;
>
> + case OFPGC11_ADD_OR_MOD:
> + ds_put_cstr(s, "ADD_OR_MOD");
> + break;
> +
> case OFPGC11_DELETE:
> ds_put_cstr(s, "DEL");
> break;
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 2c6fb1f..83685ca 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -8787,6 +8787,7 @@ parse_group_prop_ntr_selection_method(struct
> ofpbuf *payload,
> switch (group_cmd) {
> case OFPGC15_ADD:
> case OFPGC15_MODIFY:
> + case OFPGC15_ADD_OR_MOD:
> break;
> case OFPGC15_DELETE:
> case OFPGC15_INSERT_BUCKET:
> @@ -9115,6 +9116,7 @@ bad_group_cmd(enum
> ofp15_group_mod_command cmd)
> switch (cmd) {
> case OFPGC15_ADD:
> case OFPGC15_MODIFY:
> + case OFPGC15_ADD_OR_MOD:
> case OFPGC15_DELETE:
> version = "1.1";
> opt_version = "11";
> @@ -9136,6 +9138,7 @@ bad_group_cmd(enum
> ofp15_group_mod_command cmd)
> break;
>
> case OFPGC15_MODIFY:
> + case OFPGC15_ADD_OR_MOD:
> cmd_str = "mod-group";
> break;
>
> @@ -9175,7 +9178,7 @@ ofputil_encode_group_mod(enum ofp_version
> ofp_version,
> case OFP12_VERSION:
> case OFP13_VERSION:
> case OFP14_VERSION:
> - if (gm->command > OFPGC11_DELETE) {
> + if (gm->command > OFPGC11_DELETE && gm->command !=
> + OFPGC11_ADD_OR_MOD) {
> bad_group_cmd(gm->command);
> }
> return ofputil_encode_ofp11_group_mod(ofp_version, gm); @@ -
> 9246,6 +9249,7 @@ ofputil_pull_ofp15_group_mod(struct ofpbuf *msg,
> enum ofp_version
>
> case OFPGC11_ADD:
> case OFPGC11_MODIFY:
> + case OFPGC11_ADD_OR_MOD:
> case OFPGC11_DELETE:
> default:
> if (gm->command_bucket_id == OFPG15_BUCKET_ALL) { @@ -9324,6
> +9328,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh,
> switch (gm->command) {
> case OFPGC11_ADD:
> case OFPGC11_MODIFY:
> + case OFPGC11_ADD_OR_MOD:
> case OFPGC11_DELETE:
> case OFPGC15_INSERT_BUCKET:
> break;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 835a397..6321756
> 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6634,6 +6634,27 @@ out:
> return error;
> }
>
> +/* Implements the OFPGC11_ADD_OR_MOD command which creates the
> group
> +when it does not
> + * exist yet and modifies it otherwise */ static enum ofperr
> +add_or_modify_group(struct ofproto *ofproto, const struct
> +ofputil_group_mod *gm) {
> + struct ofgroup *ofgroup;
> + enum ofperr error;
> + bool exists;
> +
> + ovs_rwlock_rdlock(&ofproto->groups_rwlock);
> + exists = ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup);
> + ovs_rwlock_unlock(&ofproto->groups_rwlock);
> +
> + if (!exists) {
> + error = add_group(ofproto, gm);
> + } else {
> + error = modify_group(ofproto, gm);
> + }
> + return error;
> +}
> +
> static void
> delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup)
> OVS_RELEASES(ofproto->groups_rwlock)
> @@ -6723,6 +6744,10 @@ handle_group_mod(struct ofconn *ofconn, const
> struct ofp_header *
> error = modify_group(ofproto, &gm);
> break;
>
> + case OFPGC11_ADD_OR_MOD:
> + error = add_or_modify_group(ofproto, &gm);
> + break;
> +
> case OFPGC11_DELETE:
> delete_group(ofproto, gm.group_id);
> error = 0;
> diff --git a/tests/ofproto.at b/tests/ofproto.at index f8c0d07..57ca50a 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -393,6 +393,32 @@ AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-
> group br0 'group_id=12 OVS_VSWITCHD_STOP AT_CLEANUP
>
> +AT_SETUP([ofproto - group_mod with mod and add_or_mod command])
> +OVS_VSWITCHD_START dnl Check that mod-group for non-existing group
> +fails without --may-create AT_DATA([stderr], [dnl OFPT_ERROR (OF1.3)
> +(xid=0x2): OFPGMFC_UNKNOWN_GROUP OFPT_GROUP_MOD (OF1.3)
> (xid=0x2):
> + MOD group_id=1234,type=indirect,bucket=actions=output:2
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn mod-group br0
> +'group_id=1234,type=indirect,buc dnl Check that mod-group for
> +non-existing group succeeds with --may-create AT_CHECK([ovs-ofctl -O
> +OpenFlow13 -vwarn --may-create mod-group br0 'group_id=1234,type
> +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-groups br0], [0],
> +[stdout]) AT_CHECK([strip_xids < stdout], [0], [dnl OFPST_GROUP_DESC
> reply (OF1.3):
> + group_id=1234,type=indirect,bucket=actions=output:2
> +])
> +dnl Check that mod-group for existing group succeeds with --may-create
> +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn --may-create mod-group br0
> +'group_id=1234,type AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn
> +dump-groups br0], [0], [stdout]) AT_CHECK([strip_xids < stdout], [0],
> +[dnl OFPST_GROUP_DESC reply (OF1.3):
> + group_id=1234,type=indirect,bucket=actions=output:3
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> dnl This is really bare-bones.
> dnl It at least checks request and reply serialization and deserialization.
> dnl Actions definition listed in both supported formats (w/ actions=) diff --git
> a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in old mode 100644 new mode
> 100755 index e2e26f7..843480c
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -411,10 +411,10 @@ zero or more groups in the same syntax, one per
> line.
> .IQ "\fBadd\-groups \fIswitch file\fR"
> Add each group entry to \fIswitch\fR's tables.
> .
> -.IP "\fBmod\-group \fIswitch group\fR"
> -.IQ "\fBmod\-group \fIswitch \fB\- < \fIfile\fR"
> +.IP "[\fB\-\-may\-create\fR] \fBmod\-group \fIswitch group\fR"
> +.IQ "[\fB\-\-may\-create\fR] \fBmod\-group \fIswitch \fB\- < \fIfile\fR"
> Modify the action buckets in entries from \fIswitch\fR's tables for -each
> group entry.
> +each group entry. Optionally create non-existing group entries.
> .
> .IP "\fBdel\-groups \fIswitch\fR"
> .IQ "\fBdel\-groups \fIswitch \fR[\fIgroup\fR]"
> @@ -2911,8 +2911,14 @@ Bundles require OpenFlow 1.4 or higher. An
> explicit \fB-O OpenFlow14\fR option is not needed, but you may need to
> enable OpenFlow 1.4 support for OVS by setting the OVSDB \fIprotocols\fR
> column in the \fIbridge\fR table.
> +.
> .RE
> .
> +.IP "\fB\-\-may\-create\fR"
> +A mod-group command creates a group if it doesn't exist yet. This uses
> +an Open vSwitch extension to OpenFlow and only works with Open vSwitch
> +2.6 and later.
> +.
> .so lib/ofp-version.man
> .
> .IP "\fB\-F \fIformat\fR[\fB,\fIformat\fR...]"
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index a8116d9..5b90a82
> 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -84,6 +84,10 @@ static bool enable_color;
> */
> static bool strict;
>
> +/* --may-create: A mod-group command creates a group that does not yet
> exist.
> + */
> +static bool may_create = false;
> +
> /* --readd: If true, on replace-flows, re-add even flows that have not
> changed
> * (to reset flow counters). */
> static bool readd;
> @@ -175,6 +179,7 @@ parse_options(int argc, char *argv[])
> OPT_UNIXCTL,
> OPT_BUNDLE,
> OPT_COLOR,
> + OPT_MAY_CREATE,
> DAEMON_OPTION_ENUMS,
> OFP_VERSION_OPTION_ENUMS,
> VLOG_OPTION_ENUMS
> @@ -194,6 +199,7 @@ parse_options(int argc, char *argv[])
> {"option", no_argument, NULL, 'o'},
> {"bundle", no_argument, NULL, OPT_BUNDLE},
> {"color", optional_argument, NULL, OPT_COLOR},
> + {"may-create", no_argument, NULL, OPT_MAY_CREATE},
> DAEMON_LONG_OPTIONS,
> OFP_VERSION_LONG_OPTIONS,
> VLOG_LONG_OPTIONS,
> @@ -319,6 +325,10 @@ parse_options(int argc, char *argv[])
> }
> break;
>
> + case OPT_MAY_CREATE:
> + may_create = true;
> + break;
> +
> DAEMON_OPTION_HANDLERS
> OFP_VERSION_OPTION_HANDLERS
> VLOG_OPTION_HANDLERS
> @@ -440,6 +450,7 @@ usage(void)
> vlog_usage();
> printf("\nOther options:\n"
> " --strict use strict match for flow commands\n"
> + " --may-create mod-group creates a non-existing group\n"
> " --readd replace flows that haven't changed\n"
> " -F, --flow-format=FORMAT force particular flow format\n"
> " -P, --packet-in-format=FRMT force particular packet in format\n"
> @@ -2520,7 +2531,8 @@ ofctl_add_groups(struct ovs_cmdl_context *ctx)
> static void ofctl_mod_group(struct ovs_cmdl_context *ctx) {
> - ofctl_group_mod(ctx->argc, ctx->argv, OFPGC11_MODIFY);
> + ofctl_group_mod(ctx->argc, ctx->argv,
> + may_create ? OFPGC11_ADD_OR_MOD : OFPGC11_MODIFY);
> }
>
> static void
More information about the dev
mailing list