| 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 | expand |
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 >
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.
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 . >
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.
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"