You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/05/08 12:51:04 UTC
[GitHub] [apisix] kwanhur opened a new pull request, #7006: feat(ops): check process running with posix.signal insteadof lsof
kwanhur opened a new pull request, #7006:
URL: https://github.com/apache/apisix/pull/7006
### Description
Partial OS no `lsof` tool, so if use `lsof` to check APISIX running or not will fail.
Now send signal none(0) to check via luaposix.
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
* remove shell command `lsof` dependency
* use signal SIGNONE(0) check process running or not
Fixes #6977
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r871329085
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
Review Comment:
You're right. Because of `-1` pid, it always gets success return from [kill()](https://elixir.bootlin.com/linux/latest/source/kernel/signal.c#L3773). Need to change it with another fake pid like `66666`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r869244966
##########
t/cli/test_ops.sh:
##########
@@ -0,0 +1,50 @@
+#!/usr/bin/env bash
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
+
+# test cli operations
+git checkout conf/config.yaml
+
+make init
+
+# apisix start - nginx.pid exists but no such process
+fakepid=9999
Review Comment:
Reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r869246004
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
+
+ local ok, err, errno = signal.kill(pid, signone)
Review Comment:
Did it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868100419
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
+
+ local ok, err, errno = signal.kill(pid, signone)
Review Comment:
The third return is [error number](https://luaposix.github.io/luaposix/modules/posix.signal.html#kill). I'm not sure `code` can show the representation.
I still think `errno` the same as POSIX style, like [man errno](https://man7.org/linux/man-pages/man3/errno.3.html).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7006:
URL: https://github.com/apache/apisix/pull/7006#issuecomment-1126917216
Please make the CI pass, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868104749
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
Review Comment:
It can, but no constants defined at module level now, there are many constant variables in `ops.lua`.
Maybe they doesn't hurt the performance as used a command-line tool.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r870918835
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
+ if ok then
+ print("APISIX is running...")
+
Review Comment:
We can save a blank line here?
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
+ if ok then
+ print("APISIX is running...")
+
+ return
+ -- no such process
Review Comment:
The comment is in the wrong place?
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
Review Comment:
The CI is failed sometimes: https://github.com/apache/apisix/runs/6399336014?check_suite_focus=true
I guess it is caused by the special meaning of pid -1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r877071785
##########
t/cli/test_main.sh:
##########
@@ -706,6 +706,26 @@ fi
./bin/apisix stop
echo "pass: ignore stale nginx.pid"
+# check operation not permitted
+sudo make run
Review Comment:
Added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r867570784
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
Review Comment:
Better to add a test case to cover the changes. You can find some references from the `t/cli/` directory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on PR #7006:
URL: https://github.com/apache/apisix/pull/7006#issuecomment-1127475160
Had merged. Plz rerun [this action](https://github.com/apache/apisix/runs/6448958745?check_suite_focus=true).
cc @spacewander @tokers @tzssangglass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on PR #7006:
URL: https://github.com/apache/apisix/pull/7006#issuecomment-1126962933
CI action run failed on `t/plugin/traffic-split.t` [6441580676](https://github.com/apache/apisix/runs/6441580676?check_suite_focus=true) and `t/plugin/api-breaker.t` [6441580676](https://github.com/apache/apisix/runs/6441580676?check_suite_focus=true) [6441580789](https://github.com/apache/apisix/runs/6441580789?check_suite_focus=true)
Plz trigger to rerun them :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7006:
URL: https://github.com/apache/apisix/pull/7006
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868069292
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
Review Comment:
Looks like it's a constant? Can it be defined at module level?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868740794
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
+
+ local ok, err, errno = signal.kill(pid, signone)
Review Comment:
I get
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r871942148
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
Review Comment:
I suggest updating the code to pass the existing test instead of changing the test to make refactor pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r876518081
##########
t/cli/test_main.sh:
##########
@@ -706,6 +706,26 @@ fi
./bin/apisix stop
echo "pass: ignore stale nginx.pid"
+# check operation not permitted
+sudo make run
Review Comment:
Can we replace this test with a test that checks `no such process`? It seems we don't have test to cover it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r871347290
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
Review Comment:
Changed fake pid, `CLI Test` all the cases passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r874400558
##########
t/cli/test_main.sh:
##########
@@ -706,6 +706,16 @@ fi
./bin/apisix stop
echo "pass: ignore stale nginx.pid"
+# check running when run repeatedly
Review Comment:
Would you add a new test to cover `err_no ~= errno.ESRCH` branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r877657574
##########
t/cli/test_main.sh:
##########
@@ -706,6 +706,30 @@ fi
./bin/apisix stop
echo "pass: ignore stale nginx.pid"
+# check no corresponding process
+make run
+oldpid=`cat logs/nginx.pid`
Review Comment:
```suggestion
oldpid=$(< logs/nginx.pid)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r869245393
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7006:
URL: https://github.com/apache/apisix/pull/7006#issuecomment-1128297553
@kwanhur Please make the CI pass, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868068514
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
+
+ local ok, err, errno = signal.kill(pid, signone)
Review Comment:
```suggestion
local no_proc = 3
local ok, err, code = signal.kill(pid, signone)
```
is better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868743635
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
+
+ local ok, err, errno = signal.kill(pid, signone)
Review Comment:
`errno` -> `err_no` is better?
It seems common within APISIX to use underscores to separate words
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r871286508
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
+ if ok then
+ print("APISIX is running...")
+
+ return
+ -- no such process
Review Comment:
Here want to show meaning of `errno.ESRCH` is `no such process`, so place upon it.
Need to remove it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r874813341
##########
t/cli/test_main.sh:
##########
@@ -706,6 +706,16 @@ fi
./bin/apisix stop
echo "pass: ignore stale nginx.pid"
+# check running when run repeatedly
Review Comment:
I want to cover it with `Operation not permitted` case of `sudo make run; make run`, previous runs as root, but it lacks some dependency, like the below
```shell
+ sudo make run
[ info ] run -> [ Start ]
/home/runner/work/apisix/apisix/bin/apisix start
lua ./apisix/cli/apisix.lua start
lua: error loading module 'ssl.core' from file '/usr/local/lib/lua/5.1/ssl.so':
/usr/local/lib/lua/5.1/ssl.so: undefined symbol: luaL_setfuncs
stack traceback:
```
Any other option to mock?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #7006:
URL: https://github.com/apache/apisix/pull/7006#issuecomment-1128357292
> Had merged. Plz rerun [this action](https://github.com/apache/apisix/runs/6448958745?check_suite_focus=true).
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r872915571
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
Review Comment:
Ok, did it return and output invalid tips when negative.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r867955959
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
Review Comment:
Added test cases to cover in `t/cli/test_ops.sh`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868736893
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
Review Comment:
We can use the constant here: https://luaposix.github.io/luaposix/modules/posix.errno.html
##########
t/cli/test_ops.sh:
##########
@@ -0,0 +1,50 @@
+#!/usr/bin/env bash
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
+
+# test cli operations
+git checkout conf/config.yaml
+
+make init
+
+# apisix start - nginx.pid exists but no such process
+fakepid=9999
Review Comment:
In fact, there is already a test to cover it: https://github.com/apache/apisix/blob/5da97515cff8da8fa0f1c8ab9ee55662bd871f13/t/cli/test_main.sh#L698
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r876517297
##########
t/cli/test_main.sh:
##########
@@ -706,6 +706,16 @@ fi
./bin/apisix stop
echo "pass: ignore stale nginx.pid"
+# check running when run repeatedly
Review Comment:
I see
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on PR #7006:
URL: https://github.com/apache/apisix/pull/7006#issuecomment-1131711033
These failed workflows take error on compilation, plz trigger to re-run them, thanks very much. @tzssangglass
```plaintext
../ngx_lua-0.10.20/src/ngx_http_lua_common.h:26:10: fatal error: ngx_meta_lua_api.h: No such file or directory
#include "ngx_meta_lua_api.h"
^~~~~~~~~~~~~~~~~~~~
compilation terminated.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868769437
##########
t/cli/test_ops.sh:
##########
@@ -0,0 +1,50 @@
+#!/usr/bin/env bash
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
+
+# test cli operations
+git checkout conf/config.yaml
+
+make init
+
+# apisix start - nginx.pid exists but no such process
+fakepid=9999
Review Comment:
Thks, I'll drop these test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r868768585
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +729,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+ local errno_noproc = 3
Review Comment:
It's a good idea. I'll take it a later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on a diff in pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7006:
URL: https://github.com/apache/apisix/pull/7006#discussion_r871343714
##########
apisix/cli/ops.lua:
##########
@@ -728,14 +730,16 @@ local function start(env, ...)
local pid = util.read_file(pid_path)
pid = tonumber(pid)
if pid then
- local lsof_cmd = "lsof -p " .. pid
- local res, err = util.execute_cmd(lsof_cmd)
- if not (res and res == "") then
- if not res then
- print(err)
- else
- print("APISIX is running...")
- end
+ local signone = 0
+
+ local ok, err, err_no = signal.kill(pid, signone)
+ if ok then
+ print("APISIX is running...")
+
Review Comment:
Removed them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kwanhur commented on pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
kwanhur commented on PR #7006:
URL: https://github.com/apache/apisix/pull/7006#issuecomment-1126615785
Any test case need to cover `no such process`? It seems no similar in `test_cli_main.sh`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7006: feat(ops): check process running with posix.signal insteadof lsof
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7006:
URL: https://github.com/apache/apisix/pull/7006#issuecomment-1127292039
Let's merge master to make CI pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org