| Message ID | 20171207220356.13639-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofproto: Keep inserting buckets into a group from changing group type. | expand |
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.)
Thank you for testing. I applied this to master and backported it as far as OVS 2.6. On Mon, Dec 11, 2017 at 11:04:37AM +0530, shivani dommeti wrote: > Thank you Ben for suggesting the fix. > OVS is working as expected after adding the changes. > > > Regards, > Shivani. > > On Fri, Dec 8, 2017 at 3:33 AM, Ben Pfaff <blp@ovn.org> 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> > > --- > > AUTHORS.rst | 1 + > > ofproto/ofproto.c | 17 +++++++++++++++++ > > tests/ofproto.at | 25 +++++++++++++++++++++++++ > > 3 files changed, 43 insertions(+) > > > > 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 > > ]) > > -- > > 2.10.2 > > > >
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 ])
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(+)