[ovs-dev] [PATCH v4] ovs:group: support to insert bucket with weight value for select type

Ben Pfaff blp at ovn.org
Thu Dec 27 20:07:29 UTC 2018


On Mon, Dec 17, 2018 at 12:24:04PM +0800, solomon wrote:
> After creating a group with hash select type,then  we need to insert a new bucket with weight,
> But,it fails. Commands are as following:
> 
> # ovs-ofctl  -O OpenFlow15 add-group br0 "group_id=10, type=select, selection_method=hash,fields=tcp_src, bucket=bucket_id=10,weight:99,actions=output:1, bucket=bucket_id=20,weight:199,actions=output:1 "
> 
> # ovs-ofctl -O OpenFlow15 insert-buckets br0 "group_id=10,type=select command_bucket_id=last,bucket=bucket_id=3,weight=100,actions=output:1"
> ovs-ofctl: type is not needed
> 
> # ovs-ofctl -O OpenFlow15 insert-buckets br0 "group_id=10 command_bucket_id=last,bucket=bucket_id=3,weight=100,actions=output:1"
> ovs-ofctl: Only select groups can have bucket weights.
> 
> 
> This patch can help us. However, for other types that are not select, the check of the parameters
> is not strict, but it does not affect their function, because other types do not use this weight parameter.
> 
> 
> v3--v4:
> 1. remove the redundant space in testcase。
>    ref:https://api.travis-ci.org/v3/job/467942792/log.txt
> 
> v2-->v3:
> 1. add testcase.
> 
> v1-->v2:
> 1. fix warning from 0-day robot.
> 
> 
> Signed-off-by: solomon <liwei.solomon at gmail.com>

Thanks for the revision.

I think that, if we apply this, then every OFPTGC15_INSERT_BUCKET that
ovs-ofctl sends will include a weight.  That is OK if it is talking to
the latest ovs-vswitchd, but anything older will reject it.  So, I think
that it is better if OFPTGC15_INSERT_BUCKET only includes a weight if
the user supplied one.  I think that we can do that easily, by checking
for a nonzero weight instead of the command, and if we do that then we
don't need the command anyway.  So here is what I would suggest folding
in:

diff --git a/lib/ofp-group.c b/lib/ofp-group.c
index d9eba735da58..92ff35842986 100644
--- a/lib/ofp-group.c
+++ b/lib/ofp-group.c
@@ -1177,8 +1177,7 @@ ofputil_put_ofp11_bucket(const struct ofputil_bucket *bucket,
 static void
 ofputil_put_ofp15_bucket(const struct ofputil_bucket *bucket,
                          uint32_t bucket_id, enum ofp11_group_type group_type,
-                         struct ofpbuf *openflow, enum ofp_version ofp_version,
-                         int group_command)
+                         struct ofpbuf *openflow, enum ofp_version ofp_version)
 {
     struct ofp15_bucket *ob;
     size_t start, actions_start, actions_len;
@@ -1190,9 +1189,7 @@ ofputil_put_ofp15_bucket(const struct ofputil_bucket *bucket,
     ofpacts_put_openflow_actions(bucket->ofpacts, bucket->ofpacts_len,
                                  openflow, ofp_version);
     actions_len = openflow->size - actions_start;
-
-    if (group_type == OFPGT11_SELECT
-        || group_command == OFPGC15_INSERT_BUCKET) {
+    if (group_type == OFPGT11_SELECT || bucket->weight) {
         ofpprop_put_u16(openflow, OFPGBPT15_WEIGHT, bucket->weight);
     }
     if (bucket->watch_port != OFPP_ANY) {
@@ -1269,7 +1266,7 @@ ofputil_append_ofp15_group_desc_reply(const struct ofputil_group_desc *gds,
     start_buckets = reply->size;
     LIST_FOR_EACH (bucket, list_node, buckets) {
         ofputil_put_ofp15_bucket(bucket, bucket->bucket_id,
-                                 gds->type, reply, version, -1);
+                                 gds->type, reply, version);
     }
     ogds = ofpbuf_at_assert(reply, start_ogds, sizeof *ogds);
     ogds->type = gds->type;
@@ -2069,8 +2066,7 @@ ofputil_encode_ofp15_group_mod(enum ofp_version ofp_version,
             bucket_id = bucket->bucket_id;
         }
 
-        ofputil_put_ofp15_bucket(bucket, bucket_id, gm->type, b, ofp_version,
-                                 gm->command);
+        ofputil_put_ofp15_bucket(bucket, bucket_id, gm->type, b, ofp_version);
     }
     ogm = ofpbuf_at_assert(b, start_ogm, sizeof *ogm);
     ogm->command = htons(gm->command != OFPGC11_ADD_OR_MOD || group_existed < 0
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 0af4fd731573..15a2914836d4 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -679,7 +679,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn insert-buckets br0 group_id=1234,comman
   [ovs-ofctl: insert-bucket needs OpenFlow 1.5 or later ('-O OpenFlow15')
 ])
 
-#Verify insert-buckets command to insert bucket with weight value for select group.
+# Verify insert-buckets command to insert bucket with weight value for select group.
 AT_CHECK([ovs-ofctl -O OpenFlow15 --strict del-groups br0 group_id=1234])
 AT_DATA([groups.txt], [dnl
 group_id=1234,type=select,selection_method=hash,bucket=bucket_id=1,weight:100,output:11


More information about the dev mailing list