diff mbox

[ovs-dev,v7] ovn-nbctl: Add LB commands.

Message ID CAM_3v9KHA0JeXHuhUYnLrV7ZuvbRK9aWcY97_mjGAxzL+H1cfg@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Gurucharan Shetty Sept. 19, 2016, 8:22 p.m. UTC 
On 19 September 2016 at 09:38, nickcooper-zhangtonghao <
nickcooper-zhangtonghao@opencloud.tech> wrote:

> This patch provides the command line to create a load balancer.
> You can create a load balancer independently and add it to multiple
> switches or routers. A single load balancer can have multiple vips.
> Add a name column for the load balancer. With --add-duplicate,
> the command really creates a new load balancer with a duplicate name.
> This name has no special meaning or purpose other than to provide
> convenience for human interaction with the ovn-nb database.
> This patch also provides the unit tests and the documentation.
>
> Signed-off-by: nickcooper-zhangtonghao <nickcooper-zhangtonghao@
> opencloud.tech>
>

You should run your ./configure with ---enable-Werror option and you will
not miss any warnings as they are treated as errors.

For the case wherein the LB VIP is just a IP address, you should not
automatically give it a protocol.  The code does not look at protocol when
there are no L4 ports (as explained in ovn-nb documentation).  There are a
few lines in ovn-nbctl that exceed 78 characters. You can add the following
incremental to fix them (along with the compilation error). The incremental
does not fix the problem of automatic protocol allocation when no port is
given.



   lr-lb-del ROUTER [LB]     remove load-balancers from ROUTER\n\
@@ -1381,7 +1383,8 @@ nbctl_lb_add(struct ctl_context *ctx)
         lb_proto = ctx->argv[4];
         is_update_proto = true;
         if (strcmp(lb_proto, "tcp") && strcmp(lb_proto, "udp")) {
-            ctl_fatal("%s: protocol must be one of \"tcp\", \"udp\".",
lb_proto);
+            ctl_fatal("%s: protocol must be one of \"tcp\", \"udp\".",
+                      lb_proto);
         }
     }

@@ -1391,13 +1394,15 @@ nbctl_lb_add(struct ctl_context *ctx)
         if (lb) {
             if (smap_get(&lb->vips, lb_vip)) {
                 if (!may_exist) {
-                    ctl_fatal("%s: a load balancer with this vip (%s)
already exists", lb_name, lb_vip);
+                    ctl_fatal("%s: a load balancer with this vip (%s)
already "
+                              "exists", lb_name, lb_vip);
                 }
                 /* Update the vips. */
-                smap_replace(CONST_CAST(struct smap * ,&lb->vips), lb_vip,
lb_ips);
+                smap_replace(CONST_CAST(struct smap *, &lb->vips), lb_vip,
+                             lb_ips);
             } else {
                 /* Add the new vips. */
-                smap_add(CONST_CAST(struct smap * ,&lb->vips), lb_vip,
lb_ips);
+                smap_add(CONST_CAST(struct smap *, &lb->vips), lb_vip,
lb_ips);
             }

             /* Update the load balancer. */
@@ -1481,7 +1486,7 @@ nbctl_lb_list_router(struct ctl_context *ctx, struct
smap *lbs,
                             node->key,
                             node->value);
                 } else {
-                    ds_put_format(&val,"\n%49s %-8s %-20s %s",
+                    ds_put_format(&val, "\n%49s %-8s %-20s %s",
                             "",
                             lb->protocol,
                             node->key,
@@ -1490,7 +1495,7 @@ nbctl_lb_list_router(struct ctl_context *ctx, struct
smap *lbs,
             }

             ds_put_format(&key, "%-10.8s", lb->name);
-            smap_add_format(lbs, ds_cstr(&key), ds_cstr(&val));
+            smap_add(lbs, ds_cstr(&key), ds_cstr(&val));

             ds_destroy(&key);
             ds_destroy(&val);
@@ -1537,7 +1542,7 @@ nbctl_lb_list_switch(struct ctl_context *ctx, struct
smap *lbs,
             }

             ds_put_format(&key, "%-10.8s", lb->name);
-            smap_add_format(lbs, ds_cstr(&key), ds_cstr(&val));
+            smap_add(lbs, ds_cstr(&key), ds_cstr(&val));

             ds_destroy(&key);
             ds_destroy(&val);
@@ -1581,7 +1586,7 @@ nbctl_lb_list_all(struct ctl_context *ctx, struct
smap *lbs,
             }

             ds_put_format(&key, "%-10.8s", lb->name);
-            smap_add_format(lbs, ds_cstr(&key), ds_cstr(&val));
+            smap_add(lbs, ds_cstr(&key), ds_cstr(&val));

             ds_destroy(&key);
             ds_destroy(&val);
@@ -1635,8 +1640,8 @@ nbctl_lr_lb_add(struct ctl_context *ctx)
             if (may_exist) {
                 return;
             }
-            ctl_fatal(UUID_FMT " : a load balancer with this UUID already
exists",
-                    UUID_ARGS(&lb->header_.uuid));
+            ctl_fatal(UUID_FMT " : a load balancer with this UUID already "
+                      "exists", UUID_ARGS(&lb->header_.uuid));
         }
     }

@@ -1646,8 +1651,10 @@ nbctl_lr_lb_add(struct ctl_context *ctx)
         = xmalloc(sizeof *new_lbs * (lr->n_load_balancer + 1));

     memcpy(new_lbs, lr->load_balancer, sizeof *new_lbs *
lr->n_load_balancer);
-    new_lbs[lr->n_load_balancer] = CONST_CAST(struct nbrec_load_balancer
*, new_lb);
-    nbrec_logical_router_set_load_balancer(lr, new_lbs,
lr->n_load_balancer + 1);
+    new_lbs[lr->n_load_balancer] = CONST_CAST(struct nbrec_load_balancer *,
+                                              new_lb);
+    nbrec_logical_router_set_load_balancer(lr, new_lbs,
+                                           lr->n_load_balancer + 1);
     free(new_lbs);
 }

@@ -1676,7 +1683,8 @@ nbctl_lr_lb_del(struct ctl_context *ctx)
             nbrec_logical_router_verify_load_balancer(lr);

             struct nbrec_load_balancer **new_lbs
-                = xmemdup(lr->load_balancer, sizeof *new_lbs *
lr->n_load_balancer);
+                = xmemdup(lr->load_balancer,
+                    sizeof *new_lbs * lr->n_load_balancer);
             new_lbs[i] = lr->load_balancer[lr->n_load_balancer - 1];
             nbrec_logical_router_set_load_balancer(lr, new_lbs,
                                           lr->n_load_balancer - 1);
@@ -1701,7 +1709,8 @@ nbctl_lr_lb_list(struct ctl_context *ctx)
     if (ctx->argc == 2) {
         nodes = nbctl_lb_list_router(ctx, &lbs, ctx->argv[1], NULL, false);
     } else {
-        nodes = nbctl_lb_list_router(ctx, &lbs, ctx->argv[1],
ctx->argv[2], true);
+        nodes = nbctl_lb_list_router(ctx, &lbs, ctx->argv[1], ctx->argv[2],
+                                     true);
     }

     if (nodes) {
@@ -1735,8 +1744,8 @@ nbctl_ls_lb_add(struct ctl_context *ctx)
             if (may_exist) {
                 return;
             }
-            ctl_fatal(UUID_FMT " : a load balancer with this UUID already
exists",
-                    UUID_ARGS(&lb->header_.uuid));
+            ctl_fatal(UUID_FMT " : a load balancer with this UUID already "
+                     "exists", UUID_ARGS(&lb->header_.uuid));
         }
     }

@@ -1746,8 +1755,10 @@ nbctl_ls_lb_add(struct ctl_context *ctx)
         = xmalloc(sizeof *new_lbs * (ls->n_load_balancer + 1));

     memcpy(new_lbs, ls->load_balancer, sizeof *new_lbs *
ls->n_load_balancer);
-    new_lbs[ls->n_load_balancer] = CONST_CAST(struct nbrec_load_balancer
*, new_lb);
-    nbrec_logical_switch_set_load_balancer(ls, new_lbs,
ls->n_load_balancer + 1);
+    new_lbs[ls->n_load_balancer] = CONST_CAST(struct nbrec_load_balancer *,
+                                              new_lb);
+    nbrec_logical_switch_set_load_balancer(ls, new_lbs,
+                                           ls->n_load_balancer + 1);
     free(new_lbs);
 }

@@ -1776,7 +1787,8 @@ nbctl_ls_lb_del(struct ctl_context *ctx)
             nbrec_logical_switch_verify_load_balancer(ls);

             struct nbrec_load_balancer **new_lbs
-                = xmemdup(ls->load_balancer, sizeof *new_lbs *
ls->n_load_balancer);
+                = xmemdup(ls->load_balancer,
+                          sizeof *new_lbs * ls->n_load_balancer);
             new_lbs[i] = ls->load_balancer[ls->n_load_balancer - 1];
             nbrec_logical_switch_set_load_balancer(ls, new_lbs,
                                           ls->n_load_balancer - 1);
@@ -1801,7 +1813,8 @@ nbctl_ls_lb_list(struct ctl_context *ctx)
     if (ctx->argc == 2) {
         nodes = nbctl_lb_list_switch(ctx, &lbs, ctx->argv[1], NULL, false);
     } else {
-        nodes = nbctl_lb_list_switch(ctx, &lbs, ctx->argv[1],
ctx->argv[2], true);
+        nodes = nbctl_lb_list_switch(ctx, &lbs, ctx->argv[1], ctx->argv[2],
+                                     true);
     }

     if (nodes) {

Comments

nickcooper-zhangtonghao Sept. 21, 2016, 1:08 p.m. UTC | #1 
> On Sep 20, 2016, at 4:22 AM, Guru Shetty <guru@ovn.org> wrote:
> 
> 
> On 19 September 2016 at 09:38, nickcooper-zhangtonghao <nickcooper-zhangtonghao@opencloud.tech <mailto:nickcooper-zhangtonghao@opencloud.tech>> wrote:
> This patch provides the command line to create a load balancer.
> You can create a load balancer independently and add it to multiple
> switches or routers. A single load balancer can have multiple vips.
> Add a name column for the load balancer. With --add-duplicate,
> the command really creates a new load balancer with a duplicate name.
> This name has no special meaning or purpose other than to provide
> convenience for human interaction with the ovn-nb database.
> This patch also provides the unit tests and the documentation.
> 
> Signed-off-by: nickcooper-zhangtonghao <nickcooper-zhangtonghao@opencloud.tech>
> 
> You should run your ./configure with ---enable-Werror option and you will not miss any warnings as they are treated as errors.
> 
> For the case wherein the LB VIP is just a IP address, you should not automatically give it a protocol.  The code does not look at protocol when there are no L4 ports (as explained in ovn-nb documentation).  There are a few lines in ovn-nbctl that exceed 78 characters. You can add the following incremental to fix them (along with the compilation error). The incremental does not fix the problem of automatic protocol allocation when no port is given.


The 'ovn-nbctl lb-add’ will check the vip and ips. The protocol will be unnecessary when there are no L4 ports.
The ips should not contain port number if the vip does not contains a port number. each ip of ips should contain a
port number if the vip contains a port number. There are 16 characters for LB name. The v8 patch updates the test case.

Unfortunately， I can’t get the compilation error, ./configure —enable-Werror
diff mbox

Patch

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index bdccc23..991ecea 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -478,9 +478,9 @@ 

       <dt><code>ls-lb-list</code> <var>switch</var> [<var>lb</var>]</dt>
       <dd>
-        Lists the LBs.  If <var>switch</var> is supplied, all the LBs from
-        the logical switch are listed.  If <var>lb</var> is also specified,
-        only the <var>lb</var> will be listed from the logical switch.
+        Lists the LBs for the given <var>switch</var>.  If <var>lb</var> is
+        also specified, only the <var>lb</var> will be listed from the
logical
+        switch.
       </dd>

       <dt>[<code>--may-exist</code>] <code>lr-lb-add</code>
<var>router</var> <var>lb</var></dt>
@@ -502,9 +502,9 @@ 

       <dt><code>lr-lb-list</code> <var>router</var> [<var>lb</var>]</dt>
       <dd>
-        Lists the LBs.  If <var>router</var> is supplied, all the LBs from
-        the logical router are listed. If <var>lb</var> is also specified,
-        then only the <var>lb</var> will be listed from the logical router.
+        Lists the LBs for the given <var>router</var>.  If <var>lb</var> is
+        also specified, then only the <var>lb</var> will be listed from the
+        logical router.
       </dd>
     </dl>

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index f47070f..2541ac5 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -386,8 +386,10 @@  Route commands:\n\
 \n\
 LB commands:\n\
   lb-add LB VIP[:PORT] IP[:PORT]... [PROTOCOL]\n\
-                            add a load-balancer or VIP to load balancer\n\
-  lb-del LB [VIP]           remove a load-balancer or VIP from load
balancer\n\
+                            create a load-balancer or add a VIP to an\n\
+                            existing load balancer\n\
+  lb-del LB [VIP]           remove a load-balancer or just the VIP from\n\
+                            the load balancer\n\
   lb-list [LB]              print load-balancers\n\
   lr-lb-add ROUTER LB       add a load-balancer to ROUTER\n\