[ovs-dev,v1] ovn-ctl and ovndb-servers set connection table

Message ID 1528080652-29276-1-git-send-email-aginwala@ebay.com
State Changes Requested
Headers show
Series
  • [ovs-dev,v1] ovn-ctl and ovndb-servers set connection table
Related show

Commit Message

aginwala June 4, 2018, 2:50 a.m.
only for master node with remote option when using load balancer to manage
ovndb clusters via pacemaker.

This is will allow setting inactivity probe on the masters.
For pacemaker to manage ovndb resources via LB, we skipped creating connection
table and hence the inactivity probe was getting set to 5000 by default.
In order to over-ride it we need this table. However, we need to skip slaves
listening on local sb and nb connections table so that LB feature is
intact and only master is listening on 0.0.0.0

e.g --remote=db:OVN_Southbound,SB_Global,connections and
    --remote=db:OVN_Northbound,NB_Global,connections

will only be set on master node when using load balancer for sb and nb dbs
respectively.

Signed-off-by: aginwala <aginwala@ebay.com>
---
 ovn/utilities/ovn-ctl           |  6 +++++-
 ovn/utilities/ovndb-servers.ocf | 31 ++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 14 deletions(-)

Comments

Han Zhou June 18, 2018, 7:08 p.m. | #1
Hi Ali, thanks for the fix. I reviewed and have some minor improvements
suggested.

On Sun, Jun 3, 2018 at 7:50 PM, aginwala <amginwal@gmail.com> wrote:
>
> only for master node with remote option when using load balancer to manage
> ovndb clusters via pacemaker.
>
> This is will allow setting inactivity probe on the masters.
> For pacemaker to manage ovndb resources via LB, we skipped creating
connection
> table and hence the inactivity probe was getting set to 5000 by default.
> In order to over-ride it we need this table. However, we need to skip
slaves
> listening on local sb and nb connections table so that LB feature is
> intact and only master is listening on 0.0.0.0
>
> e.g --remote=db:OVN_Southbound,SB_Global,connections and
>     --remote=db:OVN_Northbound,NB_Global,connections
>
> will only be set on master node when using load balancer for sb and nb dbs
> respectively.
>

I feel it might make the purpose more clear if we split this into 2 patches.

1). Allow ovn-ctl to start OVSDBs without using remote connections from DB
tables.

2). Set connections in DB tables in pacemaker ocf file, so that we can
adjust inactivity_probe for master node, while still not listening on TCP
on slave node with the help of change 1).

> Signed-off-by: aginwala <aginwala@ebay.com>
> ---
>  ovn/utilities/ovn-ctl           |  6 +++++-
>  ovn/utilities/ovndb-servers.ocf | 31 ++++++++++++++++++-------------
>  2 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> index 4b7eef5..2df8580 100755
> --- a/ovn/utilities/ovn-ctl
> +++ b/ovn/utilities/ovn-ctl
> @@ -176,7 +176,9 @@ $cluster_remote_port
>          set exec "$@"
>      fi
>
> -    set "$@" --remote=db:$schema_name,$table_name,connections
> +    if test X"$OVN_USE_REMOTE_CONN" != Xno; then
> +        set "$@" --remote=db:$schema_name,$table_name,connections
> +    fi
>      set "$@" --private-key=db:$schema_name,SSL,private_key
>      set "$@" --certificate=db:$schema_name,SSL,certificate
>      set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> @@ -463,6 +465,7 @@ set_defaults () {
>
>      OVN_NORTHD_NB_DB="unix:$DB_NB_SOCK"
>      OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
> +    OVN_USE_REMOTE_CONN="yes"
>  }
>
>  set_option () {
> @@ -577,6 +580,7 @@ File location options:
>    transport (default: $DB_SB_CLUSTER_REMOTE_PROTO)
>    --ovn-northd-nb-db=NB DB address(es) (default: $OVN_NORTHD_NB_DB)
>    --ovn-northd-sb-db=SB DB address(es) (default: $OVN_NORTHD_SB_DB)
> +  --ovn-use-remote-conn=yes|no Listen on target connection table
(default: $OVN_USE_REMOTE_CONN)

The name of the new option is confusing, since it doesn't control whether
to use remote connection, but just controls the whether to use the
connections configured in DB.

Secondly, it is better to follow the pattern that all OVSDB options has NB
and SB separately.

So, suggest to change to something like: --db-nb-use-remote-in-db and
--db-sb-use-remote-in-db.

>
>  Default directories with "configure" option and environment variable
override:
>    logs: /usr/local/var/log/openvswitch (--with-logdir, OVS_LOGDIR)
> diff --git a/ovn/utilities/ovndb-servers.ocf
b/ovn/utilities/ovndb-servers.ocf
> index 9391b89..25b4811 100755
> --- a/ovn/utilities/ovndb-servers.ocf
> +++ b/ovn/utilities/ovndb-servers.ocf
> @@ -172,25 +172,27 @@ ovsdb_server_notify() {
>              ${OVN_CTL} --ovn-manage-ovsdb=no start_northd
>          fi
>
> -        # Not needed while listening on 0.0.0.0 as we do not want to
allow
> -        # local binds. However, it is needed if vip ip is binded to
nodes.
> -        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xyes ]; then
> -            conn=`ovn-nbctl get NB_global . connections`
> -            if [ "$conn" == "[]" ]
> -            then
> -                ovn-nbctl -- --id=@conn_uuid create Connection \
> +        # In order to over-ride inactivity_probe for LB use case, we
need to
> +        # create connection entry to listen on 0.0.0.0 for master node.
> +        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xno ]; then
> +           MASTER_IP="0.0.0.0"

Since MASTER_IP is the global variable that comes from the configuration,
it is better not change it directly, but instead use another temporary
variable to achieve the same purpose.

> +        fi
> +        conn=`ovn-nbctl get NB_global . connections`
> +        if [ "$conn" == "[]" ]
> +        then
> +            ovn-nbctl -- --id=@conn_uuid create Connection \
>  target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${MASTER_IP}" \
>  inactivity_probe=$INACTIVE_PROBE -- set NB_Global .
connections=@conn_uuid
> -            fi
> +        fi
>
> -            conn=`ovn-sbctl get SB_global . connections`
> -            if [ "$conn" == "[]" ]
> -            then
> -                ovn-sbctl -- --id=@conn_uuid create Connection \
> +        conn=`ovn-sbctl get SB_global . connections`
> +        if [ "$conn" == "[]" ]
> +        then
> +            ovn-sbctl -- --id=@conn_uuid create Connection \
>  target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${MASTER_IP}" \
>  inactivity_probe=$INACTIVE_PROBE -- set SB_Global .
connections=@conn_uuid
> -            fi
>          fi
> +
>      else
>          if [ "$MANAGE_NORTHD" = "yes" ]; then
>              # Stop ovn-northd service. Set --ovn-manage-ovsdb=no so that
> @@ -355,6 +357,9 @@ ovsdb_server_start() {
>          set $@ --db-nb-sync-from-proto=${NB_MASTER_PROTO}
>          set $@ --db-sb-sync-from-port=${SB_MASTER_PORT}
>          set $@ --db-sb-sync-from-proto=${SB_MASTER_PROTO}
> +        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xno ]; then
> +            set $@ --ovn-use-remote-conn="no"
> +        fi
>
>      fi
>
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Thanks,
Han
aginwala June 19, 2018, 12:34 a.m. | #2
Thanks for the review Han. Have addressed the comments and have split it
into 2 patches: https://patchwork.ozlabs.org/patch/931228/ and
https://patchwork.ozlabs.org/patch/931229/ . Please review the same and let
me know for questions further.


Regards,

On Mon, Jun 18, 2018 at 12:08 PM, Han Zhou <zhouhan@gmail.com> wrote:

> Hi Ali, thanks for the fix. I reviewed and have some minor improvements
> suggested.
>
> On Sun, Jun 3, 2018 at 7:50 PM, aginwala <amginwal@gmail.com> wrote:
> >
> > only for master node with remote option when using load balancer to
> manage
> > ovndb clusters via pacemaker.
> >
> > This is will allow setting inactivity probe on the masters.
> > For pacemaker to manage ovndb resources via LB, we skipped creating
> connection
> > table and hence the inactivity probe was getting set to 5000 by default.
> > In order to over-ride it we need this table. However, we need to skip
> slaves
> > listening on local sb and nb connections table so that LB feature is
> > intact and only master is listening on 0.0.0.0
> >
> > e.g --remote=db:OVN_Southbound,SB_Global,connections and
> >     --remote=db:OVN_Northbound,NB_Global,connections
> >
> > will only be set on master node when using load balancer for sb and nb
> dbs
> > respectively.
> >
>
> I feel it might make the purpose more clear if we split this into 2
> patches.
>
> 1). Allow ovn-ctl to start OVSDBs without using remote connections from DB
> tables.
>
> 2). Set connections in DB tables in pacemaker ocf file, so that we can
> adjust inactivity_probe for master node, while still not listening on TCP
> on slave node with the help of change 1).
>
> > Signed-off-by: aginwala <aginwala@ebay.com>
> > ---
> >  ovn/utilities/ovn-ctl           |  6 +++++-
> >  ovn/utilities/ovndb-servers.ocf | 31 ++++++++++++++++++-------------
> >  2 files changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> > index 4b7eef5..2df8580 100755
> > --- a/ovn/utilities/ovn-ctl
> > +++ b/ovn/utilities/ovn-ctl
> > @@ -176,7 +176,9 @@ $cluster_remote_port
> >          set exec "$@"
> >      fi
> >
> > -    set "$@" --remote=db:$schema_name,$table_name,connections
> > +    if test X"$OVN_USE_REMOTE_CONN" != Xno; then
> > +        set "$@" --remote=db:$schema_name,$table_name,connections
> > +    fi
> >      set "$@" --private-key=db:$schema_name,SSL,private_key
> >      set "$@" --certificate=db:$schema_name,SSL,certificate
> >      set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> > @@ -463,6 +465,7 @@ set_defaults () {
> >
> >      OVN_NORTHD_NB_DB="unix:$DB_NB_SOCK"
> >      OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
> > +    OVN_USE_REMOTE_CONN="yes"
> >  }
> >
> >  set_option () {
> > @@ -577,6 +580,7 @@ File location options:
> >    transport (default: $DB_SB_CLUSTER_REMOTE_PROTO)
> >    --ovn-northd-nb-db=NB DB address(es) (default: $OVN_NORTHD_NB_DB)
> >    --ovn-northd-sb-db=SB DB address(es) (default: $OVN_NORTHD_SB_DB)
> > +  --ovn-use-remote-conn=yes|no Listen on target connection table
> (default: $OVN_USE_REMOTE_CONN)
>
> The name of the new option is confusing, since it doesn't control whether
> to use remote connection, but just controls the whether to use the
> connections configured in DB.
>
> Secondly, it is better to follow the pattern that all OVSDB options has NB
> and SB separately.
>
> So, suggest to change to something like: --db-nb-use-remote-in-db and
> --db-sb-use-remote-in-db.
>
> >
> >  Default directories with "configure" option and environment variable
> override:
> >    logs: /usr/local/var/log/openvswitch (--with-logdir, OVS_LOGDIR)
> > diff --git a/ovn/utilities/ovndb-servers.ocf
> b/ovn/utilities/ovndb-servers.ocf
> > index 9391b89..25b4811 100755
> > --- a/ovn/utilities/ovndb-servers.ocf
> > +++ b/ovn/utilities/ovndb-servers.ocf
> > @@ -172,25 +172,27 @@ ovsdb_server_notify() {
> >              ${OVN_CTL} --ovn-manage-ovsdb=no start_northd
> >          fi
> >
> > -        # Not needed while listening on 0.0.0.0 as we do not want to
> allow
> > -        # local binds. However, it is needed if vip ip is binded to
> nodes.
> > -        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xyes ]; then
> > -            conn=`ovn-nbctl get NB_global . connections`
> > -            if [ "$conn" == "[]" ]
> > -            then
> > -                ovn-nbctl -- --id=@conn_uuid create Connection \
> > +        # In order to over-ride inactivity_probe for LB use case, we
> need to
> > +        # create connection entry to listen on 0.0.0.0 for master node.
> > +        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xno ]; then
> > +           MASTER_IP="0.0.0.0"
>
> Since MASTER_IP is the global variable that comes from the configuration,
> it is better not change it directly, but instead use another temporary
> variable to achieve the same purpose.
>
> > +        fi
> > +        conn=`ovn-nbctl get NB_global . connections`
> > +        if [ "$conn" == "[]" ]
> > +        then
> > +            ovn-nbctl -- --id=@conn_uuid create Connection \
> >  target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${MASTER_IP}" \
> >  inactivity_probe=$INACTIVE_PROBE -- set NB_Global .
> connections=@conn_uuid
> > -            fi
> > +        fi
> >
> > -            conn=`ovn-sbctl get SB_global . connections`
> > -            if [ "$conn" == "[]" ]
> > -            then
> > -                ovn-sbctl -- --id=@conn_uuid create Connection \
> > +        conn=`ovn-sbctl get SB_global . connections`
> > +        if [ "$conn" == "[]" ]
> > +        then
> > +            ovn-sbctl -- --id=@conn_uuid create Connection \
> >  target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${MASTER_IP}" \
> >  inactivity_probe=$INACTIVE_PROBE -- set SB_Global .
> connections=@conn_uuid
> > -            fi
> >          fi
> > +
> >      else
> >          if [ "$MANAGE_NORTHD" = "yes" ]; then
> >              # Stop ovn-northd service. Set --ovn-manage-ovsdb=no so that
> > @@ -355,6 +357,9 @@ ovsdb_server_start() {
> >          set $@ --db-nb-sync-from-proto=${NB_MASTER_PROTO}
> >          set $@ --db-sb-sync-from-port=${SB_MASTER_PORT}
> >          set $@ --db-sb-sync-from-proto=${SB_MASTER_PROTO}
> > +        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xno ]; then
> > +            set $@ --ovn-use-remote-conn="no"
> > +        fi
> >
> >      fi
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
> Thanks,
> Han
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch

diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
index 4b7eef5..2df8580 100755
--- a/ovn/utilities/ovn-ctl
+++ b/ovn/utilities/ovn-ctl
@@ -176,7 +176,9 @@  $cluster_remote_port
         set exec "$@"
     fi
 
-    set "$@" --remote=db:$schema_name,$table_name,connections
+    if test X"$OVN_USE_REMOTE_CONN" != Xno; then
+        set "$@" --remote=db:$schema_name,$table_name,connections
+    fi
     set "$@" --private-key=db:$schema_name,SSL,private_key
     set "$@" --certificate=db:$schema_name,SSL,certificate
     set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
@@ -463,6 +465,7 @@  set_defaults () {
 
     OVN_NORTHD_NB_DB="unix:$DB_NB_SOCK"
     OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
+    OVN_USE_REMOTE_CONN="yes"
 }
 
 set_option () {
@@ -577,6 +580,7 @@  File location options:
   transport (default: $DB_SB_CLUSTER_REMOTE_PROTO)
   --ovn-northd-nb-db=NB DB address(es) (default: $OVN_NORTHD_NB_DB)
   --ovn-northd-sb-db=SB DB address(es) (default: $OVN_NORTHD_SB_DB)
+  --ovn-use-remote-conn=yes|no Listen on target connection table (default: $OVN_USE_REMOTE_CONN)
 
 Default directories with "configure" option and environment variable override:
   logs: /usr/local/var/log/openvswitch (--with-logdir, OVS_LOGDIR)
diff --git a/ovn/utilities/ovndb-servers.ocf b/ovn/utilities/ovndb-servers.ocf
index 9391b89..25b4811 100755
--- a/ovn/utilities/ovndb-servers.ocf
+++ b/ovn/utilities/ovndb-servers.ocf
@@ -172,25 +172,27 @@  ovsdb_server_notify() {
             ${OVN_CTL} --ovn-manage-ovsdb=no start_northd
         fi
 
-        # Not needed while listening on 0.0.0.0 as we do not want to allow
-        # local binds. However, it is needed if vip ip is binded to nodes.
-        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xyes ]; then
-            conn=`ovn-nbctl get NB_global . connections`
-            if [ "$conn" == "[]" ]
-            then
-                ovn-nbctl -- --id=@conn_uuid create Connection \
+        # In order to over-ride inactivity_probe for LB use case, we need to
+        # create connection entry to listen on 0.0.0.0 for master node.
+        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xno ]; then
+           MASTER_IP="0.0.0.0"
+        fi
+        conn=`ovn-nbctl get NB_global . connections`
+        if [ "$conn" == "[]" ]
+        then
+            ovn-nbctl -- --id=@conn_uuid create Connection \
 target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${MASTER_IP}" \
 inactivity_probe=$INACTIVE_PROBE -- set NB_Global . connections=@conn_uuid
-            fi
+        fi
 
-            conn=`ovn-sbctl get SB_global . connections`
-            if [ "$conn" == "[]" ]
-            then
-                ovn-sbctl -- --id=@conn_uuid create Connection \
+        conn=`ovn-sbctl get SB_global . connections`
+        if [ "$conn" == "[]" ]
+        then
+            ovn-sbctl -- --id=@conn_uuid create Connection \
 target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${MASTER_IP}" \
 inactivity_probe=$INACTIVE_PROBE -- set SB_Global . connections=@conn_uuid
-            fi
         fi
+
     else
         if [ "$MANAGE_NORTHD" = "yes" ]; then
             # Stop ovn-northd service. Set --ovn-manage-ovsdb=no so that
@@ -355,6 +357,9 @@  ovsdb_server_start() {
         set $@ --db-nb-sync-from-proto=${NB_MASTER_PROTO}
         set $@ --db-sb-sync-from-port=${SB_MASTER_PORT}
         set $@ --db-sb-sync-from-proto=${SB_MASTER_PROTO}
+        if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xno ]; then
+            set $@ --ovn-use-remote-conn="no"
+        fi
 
     fi