[ovs-dev] ovn-ctl: Fix the wrong pidfile argument passed to ovsdb-servers

Message ID 20181009071711.28911-1-nusiddiq@redhat.com
State Accepted
Headers show
Series
  • [ovs-dev] ovn-ctl: Fix the wrong pidfile argument passed to ovsdb-servers
Related show

Commit Message

Numan Siddique Oct. 9, 2018, 7:17 a.m.
From: Numan Siddique <nusiddiq@redhat.com>

When OVN db servers are started usinb ovn-ctl, if the pid files
(/var/run/openvswitch/ovnnb_db.pid for example) are already
present, then ovn-ctl passes "--pidfile=123" if the pid file has
'123' stored in it. Later on when OVN pacemaker RA script calls
status_ovnnb/status_ovnsb() functions, these returns "not running".

The shell function 'pidfile_is_running()' stores the contents of
the pid file as  "pid=`cat "$pidfile"`". If the caller also
uses the same variable "pid" to store the file name, it gets
overriden.

This patch fixes this issue by renaming the local variable "pid"
in the "start_ovsdb__()" shell function to "db_file_name".

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 ovn/utilities/ovn-ctl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Daniel Alvarez Sanchez Oct. 9, 2018, 7:23 a.m. | #1
Thanks Numan!
On Tue, Oct 9, 2018 at 9:17 AM <nusiddiq@redhat.com> wrote:

> From: Numan Siddique <nusiddiq@redhat.com>
>
> When OVN db servers are started usinb ovn-ctl, if the pid files
> (/var/run/openvswitch/ovnnb_db.pid for example) are already
> present, then ovn-ctl passes "--pidfile=123" if the pid file has
> '123' stored in it. Later on when OVN pacemaker RA script calls
> status_ovnnb/status_ovnsb() functions, these returns "not running".
>
> The shell function 'pidfile_is_running()' stores the contents of
> the pid file as  "pid=`cat "$pidfile"`". If the caller also
> uses the same variable "pid" to store the file name, it gets
> overriden.
>
> This patch fixes this issue by renaming the local variable "pid"
> in the "start_ovsdb__()" shell function to "db_file_name".
>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> ---
>  ovn/utilities/ovn-ctl | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> index 3ff0df68e..950467c4e 100755
> --- a/ovn/utilities/ovn-ctl
> +++ b/ovn/utilities/ovn-ctl
> @@ -95,7 +95,7 @@ promote_ovnsb() {
>
>  start_ovsdb__() {
>      local DB=$1 db=$2 schema_name=$3 table_name=$4
> -    local pid
> +    local db_pid_file
>      local cluster_local_addr
>      local cluster_local_port
>      local cluster_local_proto
> @@ -116,7 +116,7 @@ start_ovsdb__() {
>      local addr
>      local active_conf_file
>      local use_remote_in_db
> -    eval pid=\$DB_${DB}_PID
> +    eval db_pid_file=\$DB_${DB}_PID
>      eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
>      eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
>      eval cluster_local_proto=\$DB_${DB}_CLUSTER_LOCAL_PROTO
> @@ -139,7 +139,7 @@ start_ovsdb__() {
>      eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
>
>      # Check and eventually start ovsdb-server for DB
> -    if pidfile_is_running $pid; then
> +    if pidfile_is_running $db_pid_file; then
>          return
>      fi
>
> @@ -169,7 +169,7 @@ $cluster_remote_port
>
>      set ovsdb-server
>      set "$@" $log --log-file=$logfile
> -    set "$@" --remote=punix:$sock --pidfile=$pid
> +    set "$@" --remote=punix:$sock --pidfile=$db_pid_file
>      set "$@" --unixctl=ovn${db}_db.ctl
>
>      [ "$OVS_USER" != "" ] && set "$@" --user "$OVS_USER"
> --
> 2.17.1
>
> Acked-by: Daniel Alvarez <dalvarez@redhat.com>

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff Oct. 11, 2018, 9:16 p.m. | #2
On Tue, Oct 09, 2018 at 12:47:11PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> When OVN db servers are started usinb ovn-ctl, if the pid files
> (/var/run/openvswitch/ovnnb_db.pid for example) are already
> present, then ovn-ctl passes "--pidfile=123" if the pid file has
> '123' stored in it. Later on when OVN pacemaker RA script calls
> status_ovnnb/status_ovnsb() functions, these returns "not running".
> 
> The shell function 'pidfile_is_running()' stores the contents of
> the pid file as  "pid=`cat "$pidfile"`". If the caller also
> uses the same variable "pid" to store the file name, it gets
> overriden.
> 
> This patch fixes this issue by renaming the local variable "pid"
> in the "start_ovsdb__()" shell function to "db_file_name".
> 
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

Thanks, applied to master.

It would probably be a good idea to more consistently use "local".
Using it for $pidfile and $pid in pidfile_is_running would have avoided
this problem.
Numan Siddique Oct. 12, 2018, 12:32 a.m. | #3
On Fri, Oct 12, 2018, 2:46 AM Ben Pfaff <blp@ovn.org> wrote:

> On Tue, Oct 09, 2018 at 12:47:11PM +0530, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > When OVN db servers are started usinb ovn-ctl, if the pid files
> > (/var/run/openvswitch/ovnnb_db.pid for example) are already
> > present, then ovn-ctl passes "--pidfile=123" if the pid file has
> > '123' stored in it. Later on when OVN pacemaker RA script calls
> > status_ovnnb/status_ovnsb() functions, these returns "not running".
> >
> > The shell function 'pidfile_is_running()' stores the contents of
> > the pid file as  "pid=`cat "$pidfile"`". If the caller also
> > uses the same variable "pid" to store the file name, it gets
> > overriden.
> >
> > This patch fixes this issue by renaming the local variable "pid"
> > in the "start_ovsdb__()" shell function to "db_file_name".
> >
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>
> Thanks, applied to master.
>
> It would probably be a good idea to more consistently use "local".
> Using it for $pidfile and $pid in pidfile_is_running would have avoided
> this problem


Thanks for the review. Agree.
Is it possible to backport to 2.10 and 2.9 ? The issue is seen both the
branches.

Thanks
Numan

.
>
Ben Pfaff Oct. 12, 2018, 12:41 a.m. | #4
On Fri, Oct 12, 2018 at 06:02:27AM +0530, Numan Siddique wrote:
> On Fri, Oct 12, 2018, 2:46 AM Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Tue, Oct 09, 2018 at 12:47:11PM +0530, nusiddiq@redhat.com wrote:
> > > From: Numan Siddique <nusiddiq@redhat.com>
> > >
> > > When OVN db servers are started usinb ovn-ctl, if the pid files
> > > (/var/run/openvswitch/ovnnb_db.pid for example) are already
> > > present, then ovn-ctl passes "--pidfile=123" if the pid file has
> > > '123' stored in it. Later on when OVN pacemaker RA script calls
> > > status_ovnnb/status_ovnsb() functions, these returns "not running".
> > >
> > > The shell function 'pidfile_is_running()' stores the contents of
> > > the pid file as  "pid=`cat "$pidfile"`". If the caller also
> > > uses the same variable "pid" to store the file name, it gets
> > > overriden.
> > >
> > > This patch fixes this issue by renaming the local variable "pid"
> > > in the "start_ovsdb__()" shell function to "db_file_name".
> > >
> > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> >
> > Thanks, applied to master.
> >
> > It would probably be a good idea to more consistently use "local".
> > Using it for $pidfile and $pid in pidfile_is_running would have avoided
> > this problem
> 
> 
> Thanks for the review. Agree.
> Is it possible to backport to 2.10 and 2.9 ? The issue is seen both the
> branches.

Sure.  Done.

Patch

diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
index 3ff0df68e..950467c4e 100755
--- a/ovn/utilities/ovn-ctl
+++ b/ovn/utilities/ovn-ctl
@@ -95,7 +95,7 @@  promote_ovnsb() {
 
 start_ovsdb__() {
     local DB=$1 db=$2 schema_name=$3 table_name=$4
-    local pid
+    local db_pid_file
     local cluster_local_addr
     local cluster_local_port
     local cluster_local_proto
@@ -116,7 +116,7 @@  start_ovsdb__() {
     local addr
     local active_conf_file
     local use_remote_in_db
-    eval pid=\$DB_${DB}_PID
+    eval db_pid_file=\$DB_${DB}_PID
     eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
     eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
     eval cluster_local_proto=\$DB_${DB}_CLUSTER_LOCAL_PROTO
@@ -139,7 +139,7 @@  start_ovsdb__() {
     eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
 
     # Check and eventually start ovsdb-server for DB
-    if pidfile_is_running $pid; then
+    if pidfile_is_running $db_pid_file; then
         return
     fi
 
@@ -169,7 +169,7 @@  $cluster_remote_port
 
     set ovsdb-server
     set "$@" $log --log-file=$logfile
-    set "$@" --remote=punix:$sock --pidfile=$pid
+    set "$@" --remote=punix:$sock --pidfile=$db_pid_file
     set "$@" --unixctl=ovn${db}_db.ctl
 
     [ "$OVS_USER" != "" ] && set "$@" --user "$OVS_USER"