diff mbox

[ovs-dev,v3] : ofproto: Add relaxed group_mod command ADD_OR_MOD

Message ID 20160629002925.0000042a@web.de
State Accepted
Headers show

Commit Message

Jan Scheurich June 28, 2016, 10:29 p.m. UTC
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 v3:
- Rebased to master

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>

---

 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 mbox

Patch

diff --git a/NEWS b/NEWS
index 802e7f8..f7b202b 100644
--- a/NEWS
+++ b/NEWS
@@ -17,10 +17,13 @@  Post-v2.5.0
      * New OpenFlow extension NXM_NX_MPLS_TTL to provide access to MPLS TTL.
      * New output option, output(port=N,max_len=M), to allow truncating a
        packet to size M bytes when outputting to port N.
+     * 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.
+     * New option '--may-create' to use OFPGC_ADD_OR_MOD in mod-group command.
    - IPFIX:
      * New "sampling_port" option for "sample" action to allow sampling
        ingress and egress tunnel metadata with IPFIX.
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 5747ec6..919c95d 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2703,6 +2703,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 cc9f5e0..2b214ea 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -8896,6 +8896,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:
@@ -9224,6 +9225,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";
@@ -9245,6 +9247,7 @@  bad_group_cmd(enum ofp15_group_mod_command cmd)
         break;

     case OFPGC15_MODIFY:
+    case OFPGC15_ADD_OR_MOD:
         cmd_str = "mod-group";
         break;

@@ -9284,7 +9287,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);
@@ -9355,6 +9358,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) {
@@ -9433,6 +9437,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 cf3235a..5f84aa1 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6695,6 +6695,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)
@@ -6784,6 +6805,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/tests/ofproto.at b/tests/ofproto.at
index c89c641..bac6981 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=1236,type=indir
 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,bucket=actions=2'], [1], , [stderr])
+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=indirect,bucket=actions=2'])
+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=indirect,bucket=actions=3'])
+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 208f671..d69fe13
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -431,10 +431,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]"
@@ -2944,8 +2944,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 5e42d1c..0bf8e44 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
@@ -442,6 +452,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"
@@ -2534,7 +2545,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