Message ID | 20160521004249.00003c06@web.de |
---|---|
State | Changes Requested |
Headers | show |
Could someone perhaps have a look at the latest version of the patch? Thanks a lot, Jan On Sat, 21 May 2016 00:42:49 +0200 Jan Scheurich <jan.scheurich@web.de> wrote: > *** Reposting the patch once more. First two attempts got corrupted > *** *** Sorry, still trying to find a suitable email client *** > > This patch adds support for a new Group Mod command OFPGC_ADD_OR_MOD > to OVS for all OpenFlow versions supporting groups (OF11 and higher). > The new ADD_OR_MOD creates a group that does not yet exist (like ADD) > or 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 new Group Mod command is made > available in form of new write-group(s) commands that can be used in > the same way as add-group(s) but do not lead to an error when a groups > exists: > > ovs-ofctl -Oopenflow13 del-groups br-int group_id=100 > > ovs-ofctl -Oopenflow13 write-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 write-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 > >
On Sat, May 21, 2016 at 12:42:49AM +0200, Jan Scheurich wrote: > *** Reposting the patch once more. First two attempts got corrupted *** > *** Sorry, still trying to find a suitable email client *** Hi, thanks for the patch. It seems almost ready to me. I have a few comments. In some places, this is called "add_or_modify" and in other places just "write". I suggest using a consistent name. Please add a test for the new command. In the documentation, please note that this uses an Open vSwitch extension to OpenFlow and that it only works with Open vSwitch 2.6 or later. Please add an item mentioning this new command to NEWS. Thanks, Ben.
Hi Ben, Thanks for the review. Will update. > In some places, this is called "add_or_modify" and in other places just > "write". I suggest using a consistent name. Which one would you suggest? BR, Jan
On Fri, Jun 03, 2016 at 05:07:39PM +0200, Jan Scheurich wrote: > Thanks for the review. Will update. > > > In some places, this is called "add_or_modify" and in other places just > > "write". I suggest using a consistent name. > > Which one would you suggest? I don't like the idea of using "write", because "write-actions" has a specific meaning in OpenFlow and users might falsely associate it with "write-groups". On the other hand, "add_or_modify" is a bit long and awkward. Here is what I suggest. In code, use "add_or_modify", because it is clear. In the ovs-ofctl command-line interface, do not add a new command; instead, add a new option --may-create to the mod-group command which uses add_or_modify. This has precedent in the ovs-dpctl command, which has a --may-create option on its mod-flow command. Thanks, Ben.
On Thu, 2 Jun 2016 16:03:02 -0700 Ben Pfaff <blp@ovn.org> wrote: > > In the documentation, please note that this uses an Open vSwitch > extension to OpenFlow and that it only works with Open vSwitch 2.6 or > later. > I'm a bit lost where to put this information. Is there a documentation file that gathers all Open vSwitch OF extensions and modifications? If so I haven't found it. Or do you mean the ovs-ofctl man page? /Jan
On Sat, Jun 04, 2016 at 12:15:20AM +0200, Jan Scheurich wrote: > On Thu, 2 Jun 2016 16:03:02 -0700 > Ben Pfaff <blp@ovn.org> wrote: > > In the documentation, please note that this uses an Open vSwitch > > extension to OpenFlow and that it only works with Open vSwitch 2.6 or > > later. > > I'm a bit lost where to put this information. Is there a documentation > file that gathers all Open vSwitch OF extensions and modifications? If > so I haven't found it. Or do you mean the ovs-ofctl man page? The ovs-ofctl manpage.
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 command, 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..7b79091 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"; @@ -9139,6 +9141,10 @@ bad_group_cmd(enum ofp15_group_mod_command cmd) cmd_str = "mod-group"; break; + case OFPGC15_ADD_OR_MOD: + cmd_str = "write-group"; + break; + case OFPGC15_DELETE: cmd_str = "del-group"; break; @@ -9175,7 +9181,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 +9252,7 @@ ofputil_pull_ofp15_group_mod(struct ofpbuf *msg, enum ofp_version 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 +9331,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 ae527a4..010d1e5 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -6612,6 +6612,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) @@ -6701,6 +6722,10 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) 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/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 1b25145..45a91fc --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -416,6 +416,12 @@ Add each group entry to \fIswitch\fR's tables. Modify the action buckets in entries from \fIswitch\fR's tables for each group entry. . +.IP "\fBwrite\-group \fIswitch group\fR" +.IQ "\fBwrite\-group \fIswitch \fB\- < \fIfile\fR" +.IQ "\fBwrite\-groups \fIswitch file\fR" +Add each group entry to or modify existing group entries in \fIswitch\fR's +tables. +. .IP "\fBdel\-groups \fIswitch\fR" .IQ "\fBdel\-groups \fIswitch \fR[\fIgroup\fR]" .IQ "\fBdel\-groups \fIswitch \fB\- < \fIfile\fR" diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index a8116d9..2ba6ed1 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -2524,6 +2524,18 @@ ofctl_mod_group(struct ovs_cmdl_context *ctx) } static void +ofctl_add_mod_group(struct ovs_cmdl_context *ctx) +{ + ofctl_group_mod(ctx->argc, ctx->argv, OFPGC11_ADD_OR_MOD); +} + +static void +ofctl_add_mod_groups(struct ovs_cmdl_context *ctx) +{ + ofctl_group_mod_file(ctx->argc, ctx->argv, OFPGC11_ADD_OR_MOD); +} + +static void ofctl_del_groups(struct ovs_cmdl_context *ctx) { ofctl_group_mod(ctx->argc, ctx->argv, OFPGC11_DELETE); @@ -4038,6 +4050,10 @@ static const struct ovs_cmdl_command all_commands[] = { 1, 2, ofctl_add_groups }, { "mod-group", "switch group", 1, 2, ofctl_mod_group }, + { "write-group", "switch group", + 1, 2, ofctl_add_mod_group }, + { "write-groups", "switch group", + 1, 2, ofctl_add_mod_groups }, { "del-groups", "switch [group]", 1, 2, ofctl_del_groups }, { "insert-buckets", "switch [group]",
*** Reposting the patch once more. First two attempts got corrupted *** *** Sorry, still trying to find a suitable email client *** This patch adds support for a new Group Mod command OFPGC_ADD_OR_MOD to OVS for all OpenFlow versions supporting groups (OF11 and higher). The new ADD_OR_MOD creates a group that does not yet exist (like ADD) or 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 new Group Mod command is made available in form of new write-group(s) commands that can be used in the same way as add-group(s) but do not lead to an error when a groups exists: ovs-ofctl -Oopenflow13 del-groups br-int group_id=100 ovs-ofctl -Oopenflow13 write-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 write-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 Signed-off-by: Jan Scheurich <jan.scheurich@web.de> To aid review this series is available at: tree: https://github.com/JScheurich/openvswitch branch: dev/group_add_modify tag: group_add_modify_v1 --- 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 | 10 +++++++++- ofproto/ofproto.c | 25 +++++++++++++++++++++++++ utilities/ovs-ofctl.8.in | 6 ++++++ utilities/ovs-ofctl.c | 16 ++++++++++++++++ 8 files changed, 66 insertions(+), 1 deletion(-)