| Message ID | 20220608132912.3209028-1-twilson@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v5,1/2] Handle re-used pids in pidfile_is_running | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Bleep bloop. Greetings Terry Wilson, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 135 characters long (recommended limit is 79)
#28 FILE: utilities/ovn-ctl:46:
test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && [ -z $cmd -o pid_comm_check "$cmd" "$pid" ]
Lines checked: 34, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
I added Ihar's Ack and my Ack to both patches and pushed them to main, branch-22.06, branch-22.03, and branch-21.12. On 6/8/22 09:29, Terry Wilson wrote: > Since pids can be re-used, it is necessary to check that the > process that is running with a pid matches the one that we expect. > > This adds the ability to optionally pass a 'binary' argument to > pidfile_is_running, and if it is passed to match the binary against > /proc/$pid/exe. > > Signed-off-by: Terry Wilson <twilson@redhat.com> > --- > utilities/ovn-ctl | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl > index d733aa42d..14d37a3d6 100755 > --- a/utilities/ovn-ctl > +++ b/utilities/ovn-ctl > @@ -42,7 +42,8 @@ ovn_ic_db_conf_file="$ovn_etcdir/ovn-ic-db-params.conf" > > pidfile_is_running () { > pidfile=$1 > - test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" > + cmd=$2 > + test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && [ -z $cmd -o pid_comm_check "$cmd" "$pid" ] > } >/dev/null 2>&1 > > stop_nb_ovsdb() { >
On Thu, Jun 9, 2022 at 2:47 PM Mark Michelson <mmichels@redhat.com> wrote: > > I added Ihar's Ack and my Ack to both patches and pushed them to main, > branch-22.06, branch-22.03, and branch-21.12. > Looks like this patch or the other one has caused regressions. "ovn-ctl start_northd" is not starting ovn-northd. -- /etc/ovn/ovnnb_db.db does not exist ... (warning). Creating empty database /etc/ovn/ovnnb_db.db [ OK ] Starting ovsdb-nb [ OK ] /etc/ovn/ovnsb_db.db does not exist ... (warning). Creating empty database /etc/ovn/ovnsb_db.db [ OK ] Starting ovsdb-sb [ OK ] OVN Northbound DB is not running ... failed! --- Please see this job for more details - https://github.com/ovn-org/ovn-fake-multinode/runs/6830884375?check_suite_focus=true Numan > On 6/8/22 09:29, Terry Wilson wrote: > > Since pids can be re-used, it is necessary to check that the > > process that is running with a pid matches the one that we expect. > > > > This adds the ability to optionally pass a 'binary' argument to > > pidfile_is_running, and if it is passed to match the binary against > > /proc/$pid/exe. > > > > Signed-off-by: Terry Wilson <twilson@redhat.com> > > --- > > utilities/ovn-ctl | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl > > index d733aa42d..14d37a3d6 100755 > > --- a/utilities/ovn-ctl > > +++ b/utilities/ovn-ctl > > @@ -42,7 +42,8 @@ ovn_ic_db_conf_file="$ovn_etcdir/ovn-ic-db-params.conf" > > > > pidfile_is_running () { > > pidfile=$1 > > - test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" > > + cmd=$2 > > + test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && [ -z $cmd -o pid_comm_check "$cmd" "$pid" ] > > } >/dev/null 2>&1 > > > > stop_nb_ovsdb() { > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Ouch. I believe this should do: https://patchwork.ozlabs.org/project/ovn/patch/20220613185922.2700748-1-ihrachys@redhat.com/ Ihar On Mon, Jun 13, 2022 at 1:47 PM Numan Siddique <numans@ovn.org> wrote: > > On Thu, Jun 9, 2022 at 2:47 PM Mark Michelson <mmichels@redhat.com> wrote: > > > > I added Ihar's Ack and my Ack to both patches and pushed them to main, > > branch-22.06, branch-22.03, and branch-21.12. > > > > Looks like this patch or the other one has caused regressions. > "ovn-ctl start_northd" is not starting ovn-northd. > > -- > /etc/ovn/ovnnb_db.db does not exist ... (warning). > Creating empty database /etc/ovn/ovnnb_db.db [ OK ] > Starting ovsdb-nb [ OK ] > /etc/ovn/ovnsb_db.db does not exist ... (warning). > Creating empty database /etc/ovn/ovnsb_db.db [ OK ] > Starting ovsdb-sb [ OK ] > OVN Northbound DB is not running ... failed! > --- > > Please see this job for more details - > https://github.com/ovn-org/ovn-fake-multinode/runs/6830884375?check_suite_focus=true > > Numan > > > > On 6/8/22 09:29, Terry Wilson wrote: > > > Since pids can be re-used, it is necessary to check that the > > > process that is running with a pid matches the one that we expect. > > > > > > This adds the ability to optionally pass a 'binary' argument to > > > pidfile_is_running, and if it is passed to match the binary against > > > /proc/$pid/exe. > > > > > > Signed-off-by: Terry Wilson <twilson@redhat.com> > > > --- > > > utilities/ovn-ctl | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl > > > index d733aa42d..14d37a3d6 100755 > > > --- a/utilities/ovn-ctl > > > +++ b/utilities/ovn-ctl > > > @@ -42,7 +42,8 @@ ovn_ic_db_conf_file="$ovn_etcdir/ovn-ic-db-params.conf" > > > > > > pidfile_is_running () { > > > pidfile=$1 > > > - test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" > > > + cmd=$2 > > > + test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && [ -z $cmd -o pid_comm_check "$cmd" "$pid" ] > > > } >/dev/null 2>&1 > > > > > > stop_nb_ovsdb() { > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl index d733aa42d..14d37a3d6 100755 --- a/utilities/ovn-ctl +++ b/utilities/ovn-ctl @@ -42,7 +42,8 @@ ovn_ic_db_conf_file="$ovn_etcdir/ovn-ic-db-params.conf" pidfile_is_running () { pidfile=$1 - test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" + cmd=$2 + test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && [ -z $cmd -o pid_comm_check "$cmd" "$pid" ] } >/dev/null 2>&1 stop_nb_ovsdb() {
Since pids can be re-used, it is necessary to check that the process that is running with a pid matches the one that we expect. This adds the ability to optionally pass a 'binary' argument to pidfile_is_running, and if it is passed to match the binary against /proc/$pid/exe. Signed-off-by: Terry Wilson <twilson@redhat.com> --- utilities/ovn-ctl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)