[ovs-dev] ofproto: Keep inserting buckets into a group from changing group type.

Message ID 20171207220356.13639-1-blp@ovn.org
State New
Headers show
Series
  • [ovs-dev] ofproto: Keep inserting buckets into a group from changing group type.
Related show

Commit Message

Ben Pfaff Dec. 7, 2017, 10:03 p.m.
The "insert buckets" and "delete buckets" operations on a group should not
change the group's type or properties, but the implementation did this by
mistake.  This fixes the problem.

Reported-by: shivani dommeti <shivani.dommeti@gmail.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045830.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 AUTHORS.rst       |  1 +
 ofproto/ofproto.c | 17 +++++++++++++++++
 tests/ofproto.at  | 25 +++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

Comments

Ben Pfaff Dec. 7, 2017, 10:08 p.m. | #1
On Thu, Dec 07, 2017 at 02:03:56PM -0800, Ben Pfaff wrote:
> The "insert buckets" and "delete buckets" operations on a group should not
> change the group's type or properties, but the implementation did this by
> mistake.  This fixes the problem.
> 
> Reported-by: shivani dommeti <shivani.dommeti@gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045830.html
> Signed-off-by: Ben Pfaff <blp@ovn.org>

This isn't well specified by OpenFlow so I filed EXT-570 in the ONF's
JIRA as a request for clarification.  (I expect no response.)

Patch

diff --git a/AUTHORS.rst b/AUTHORS.rst
index 075b2f72d47d..979cfd7bce41 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -589,6 +589,7 @@  likunyun                        kunyunli@hotmail.com
 meishengxin                     meishengxin@huawei.com
 neeraj mehta                    mehtaneeraj07@gmail.com
 rahim entezari                  rahim.entezari@gmail.com
+shivani dommeti                 shivani.dommeti@gmail.com
 weizj                           34965317@qq.com
 俊 赵                           zhaojun12@outlook.com
 冯全树(Crab)                    fqs888@126.com
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 82c2bb27d348..84eb18e901ed 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6905,6 +6905,13 @@  handle_queue_get_config_request(struct ofconn *ofconn,
     return error;
 }
 
+/* Allocates, initializes, and constructs a new group in 'ofproto', obtaining
+ * all the attributes for it from 'gm', and stores a pointer to it in
+ * '*ofgroup'.  Makes the new group visible from the flow table starting from
+ * 'version'.
+ *
+ * Returns 0 if successful, otherwise an error code.  If there is an error then
+ * '*ofgroup' is indeterminate upon return. */
 static enum ofperr
 init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm,
            ovs_version_t version, struct ofgroup **ofgroup)
@@ -7113,6 +7120,16 @@  modify_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm)
         return OFPERR_OFPGMFC_UNKNOWN_GROUP;
     }
 
+    /* Inserting or deleting a bucket should not change the group's type or
+     * properties, so change the group mod so that these aspects match the old
+     * group.  (See EXT-570.)  */
+    if (ogm->gm.command == OFPGC15_INSERT_BUCKET ||
+        ogm->gm.command == OFPGC15_REMOVE_BUCKET) {
+        ogm->gm.type = old_group->type;
+        ofputil_group_properties_destroy(&ogm->gm.props);
+        ofputil_group_properties_copy(&ogm->gm.props, &old_group->props);
+    }
+
     if (old_group->type != ogm->gm.type
         && (ofproto->n_groups[ogm->gm.type]
             >= ofproto->ogf.max_groups[ogm->gm.type])) {
diff --git a/tests/ofproto.at b/tests/ofproto.at
index c19a3d10daed..74a30cbefe3b 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -621,7 +621,32 @@  OFPST_GROUP_DESC reply (OF1.5):
  group_id=1234,type=all,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11,bucket=bucket_id:12,actions=output:12,bucket=bucket_id:13,actions=output:13,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15,bucket=bucket_id:20,actions=output:20,bucket=bucket_id:21,actions=output:21
 ])
 
+# Delete groups.
+AT_CHECK([ovs-ofctl -O OpenFlow15 del-groups br0])
+
+# Add "fast_failover" group, then insert a bucket into it and make
+# sure that the type of the group doesn't change.  (There was a bug
+# that caused this to happen.)
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-group br0 group_id=1234,type=ff])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
+AT_CHECK([strip_xids < stdout], [0], [dnl
+OFPST_GROUP_DESC reply (OF1.5):
+ group_id=1234,type=ff
+])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=first,bucket=bucket_id:20,actions=output:20,bucket=bucket_id:21,actions=output:21])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
+AT_CHECK([strip_xids < stdout], [0], [dnl
+OFPST_GROUP_DESC reply (OF1.5):
+ group_id=1234,type=ff,bucket=bucket_id:20,actions=output:20,bucket=bucket_id:21,actions=output:21
+])
+
 # Negative tests.
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=123,type=indirect,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1], [1], [],
+  [ovs-ofctl: type is not needed
+])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=123,selection_method=dp_hash,type=indirect,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1], [1], [],
+  [ovs-ofctl: selection method is not needed
+])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=0xffffff01,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1], [1], [],
   [ovs-ofctl: invalid command bucket id 4294967041
 ])