pkg/system: move process-functions to pkg/process, improve pkg/pidfile and reduce overlap#44304
pkg/system: move process-functions to pkg/process, improve pkg/pidfile and reduce overlap#44304thaJeztah merged 11 commits intomoby:masterfrom
Conversation
thaJeztah
added
status/2-code-review
kind/refactor
thaJeztah
force-pushed
the
sys_move_process
branch
from
f3fc9d4 to
809f3b5
Compare
| if pid < 1 { | ||
| return | ||
| } | ||
| _ = unix.Kill(pid, unix.SIGKILL) |
There was a problem hiding this comment.
Since this is a new package without any back-compat concerns, thoughts on returning the error to the caller?
There was a problem hiding this comment.
I guess I did so to keep the old signature (to make it a drop-in replacement), but I can change it (and just make a thin wrapper for the alias) 👍
There was a problem hiding this comment.
Hmm.. actually getting back to this; wondering how useful the error would be; SIGKILL cannot be ignored, so the process "must" be terminated, so in which case would we get a useful error?
- no permissions (not running as
root?) - process doesn't (no longer) exist; the intent of this function is to terminate the process, so this should be fine as well 🤔
Was there a specific error-case you had in mind?
There was a problem hiding this comment.
EPERM would absolutely be a useful error to return, because this function is inside a pkg-tree package. Since the policy is that pkg-tree packages are meant to be reusable outside of the daemon, functions in these packages shouldn't really be assuming they will be running as root. I think it would make sense to swallow ESRCH (no such process) because, as you point out, the intent of the call is to make the PID not exist so task failed successfully.
I think it would also be useful to not swallow any "impossible" errors such as ENOSYS or EINVAL. It's not outside the realm of possibility for some sysadmin to apply a seccomp filter or apparmor profile to the daemon which blocks the kill syscall, for example. Though the calling process would be running in a very insane environment if SIGKILL is an invalid signal or the syscall is returning nonsensical errors; perhaps the only sane thing to do in an insane world is to panic()?
thaJeztah
force-pushed
the
sys_move_process
branch
3 times, most recently
from
1df8639 to
2c0c270
Compare
|
@corhere updated; PTAL 👍 |
| } | ||
|
|
||
| // Kill force-stops a process. | ||
| func Kill(pid int) { |
There was a problem hiding this comment.
oh! forgot the windows one; give me 5 minutes
thaJeztah
force-pushed
the
sys_move_process
branch
from
2c0c270 to
34aa74c
Compare
| // Since we can not kill a zombie pid, add zombie check here | ||
| isZombie, err := system.IsProcessZombie(pid) | ||
| isZombie, err := process.Zombie(pid) | ||
| // TODO(thaJeztah) should we ignore os.IsNotExist() here? ("/proc/<pid>/stat" will be gone if the process exited) |
There was a problem hiding this comment.
Oh! Forgot this TODO; that one can be removed now
Signed-off-by: Sebastiaan van Stijn <[email protected]>
If the file doesn't exist, the process isn't running, so we should be able to ignore that. Also remove an intermediate variable. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…stead bytes.SplitN() is more performant, and skips having to do the conversion. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Using the implementation from pkg/pidfile for windows, as that implementation looks to be handling more cases to check if a process is still alive (or to be considered alive). Signed-off-by: Sebastiaan van Stijn <[email protected]>
unix.Kill() does not produce an error for PID 0, -1. As a result, checking process.Alive() would return "true" for both 0 and -1 on macOS (and previously on Linux as well). Let's shortcut these values to consider them "not alive", to prevent someone trying to kill them. A basic test was added to check the behavior. Given that the intent of these functions is to handle single processes, this patch also prevents 0 and negative values to be used. From KILL(2): https://man7.org/linux/man-pages/man2/kill.2.html If pid is positive, then signal sig is sent to the process with the ID specified by pid. If pid equals 0, then sig is sent to every process in the process group of the calling process. If pid equals -1, then sig is sent to every process for which the calling process has permission to send signals, except for process 1 (init), but see below. If pid is less than -1, then sig is sent to every process in the process group whose ID is -pid. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This allows it to be used for processes other than the daemon itself. Signed-off-by: Sebastiaan van Stijn <[email protected]>
While this was convenient for our use, it's somewhat unexpected for a function that writes a file to also create all parent directories; even more because this function may be executed as root. This patch makes the package more "safe" to use as a generic package by removing this functionality, and leaving it up to the caller to create parent directories, if needed. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This allows consumers to read back the pidFile and to check if the PID is still running. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
…file Also updated a variable name that collided with a package const. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
sys_move_process
branch
from
34aa74c to
cea8e9b
Compare
|
Let me bring this one in 👍 |
3 participants
See individual commits for details 😅