[ovs-dev] [PATCH v2]: ofproto: Add relaxed group_mod command ADD_OR_MOD

Ben Pfaff blp at ovn.org
Tue Jun 28 21:19:54 UTC 2016


In this case it means that the patch literally fails to apply to the
current tree.  Ryan requested a rebase:
        http://openvswitch.org/pipermail/dev/2016-June/073435.html

On Tue, Jun 28, 2016 at 02:18:30PM +0000, Jan Scheurich wrote:
> 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
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list