Fix check pid existence when running in different process namespace (container) #821

i-prudnikov · 2020-01-17T11:13:57Z

This fix is to address issue mentioned in #713, originally comes from influxdata/telegraf#6813

shirou · 2020-01-18T06:57:17Z

Indeed. It could break cgroup independency, but if a user does not want to allow a process can watch a process existance out side of a container, simply does not add environment variable. So I agree to this PR.

so basically I agree to this great PR, but could you go fmt please?

Lomanic · 2020-01-18T10:38:10Z

process/process_posix.go

@@ -5,6 +5,7 @@ package process
 import (
 	"context"
 	"fmt"
+	"github.com/shirou/gopsutil/internal/common"


Please follow our convention to separate stdlib and 3rd-party packages in import statements.

Done, gofmt was surprisingly switched off :)

Lomanic · 2020-01-18T10:44:51Z

process/process_posix.go

+		file, err := os.Open(common.HostProc(strconv.Itoa(int(pid))))
+		defer file.Close()
+		if err!=nil{return false, err} else{return true, nil}
+	}else{ //Assume that we are in the same process namespace, try to signal process


Useless else as we always return in the if.

This and below, just restructured this part taking into consideration current and all of the comments below.

Lomanic · 2020-01-18T10:56:08Z

process/process_posix.go

-	case syscall.EPERM:
-		return true, nil
+
+	if ! strings.HasPrefix(common.HostProc(),"/proc") { //Means that proc is mounted not to /proc, and we highly


This code is called on freebsd openbsd darwin where there is no procfs, could lead to downstream breakage if users blindly set the HOST_PROC variable on every OS. Probably check for common.HostProc() existence instead/also.

Lomanic · 2020-01-18T11:01:09Z

process/process_posix.go

+	if ! strings.HasPrefix(common.HostProc(),"/proc") { //Means that proc is mounted not to /proc, and we highly
+		// likely running inside container with a different process namespace (by default), so we check pid existence
+		// based on /<HOST_PROC>/proc/<PID> folder
+		file, err := os.Open(common.HostProc(strconv.Itoa(int(pid))))


Use os.Stat instead as in this commit 2583438

Lomanic · 2020-01-18T11:04:43Z

process/process_posix.go

-		return true, nil
+
+	if ! strings.HasPrefix(common.HostProc(),"/proc") { //Means that proc is mounted not to /proc, and we highly
+		// likely running inside container with a different process namespace (by default), so we check pid existence


Likewise, all your comments assume we're running on Linux, which is confusing because this code is common to all POSIX platforms.

i-prudnikov · 2020-01-20T15:49:39Z

Overall, I followed this logic:

If /<HOST_PROC>/proc or /proc exist, then PID existence is checked via existence of folder in proc filesystem
Otherwise PID existence is checked by signalling.

i-prudnikov · 2020-01-20T15:50:34Z

process/process_posix.go

-		defer file.Close()
-		if err!=nil{return false, err} else{return true, nil}
-	}else{ //Assume that we are in the same process namespace, try to signal process
+	if _, err := os.Stat(common.HostProc()); err == nil { //Means that proc filesystem exist


Currently it is 2 calls to os.Stat, can be reduced to 1 if we cache somewhere in global var the result of this line.
@Lomanic, @shirou could you please advise the right place for that?

These os.Stat target are different and second one is a fallback. so we can not merge.

Well, they are. But maybe my point is not clear. What I'm trying to say that the os.Stat(common.HostProc()) call is useless to be made every time when we check pid existence, this is something that could be checked once at startup or init. Possible use case here is when you have an array of PIDs (let's call it pidArray) and you check every entry in cycle for PID existence. In this case you have n useless os.Stat calls, where n=len(pidArray).

shirou · 2020-01-26T09:00:46Z

Thank you @Lomanic and @i-prudnikov . LGTM. If you don't have a comment, please merge this PR.

Lomanic · 2020-01-26T17:43:04Z

@i-prudnikov please squash your commits as to preserve lines history. Or @shirou please enable this option for merges. Thanks.

shirou · 2020-01-27T01:19:49Z

Oh, roger. I have enabled squash merge!

…container)

i-prudnikov · 2020-01-27T09:08:42Z

@shirou, @Lomanic commits are squashed, thank you for review.

shirou · 2020-02-08T16:25:25Z

LGTM now. Thank you for your contribution!

Lomanic · 2020-02-13T18:42:28Z

This shouldn't have been merged as now the following test is failing:

--- FAIL: Test_IsRunning (1.00s)
    process_test.go:620: IsRunning error: open /proc/11200/stat: no such file or directory

A non-running process should not return an error.

We really need a better CI (#734) to catch this kind of regressions.

i-prudnikov · 2020-02-14T13:18:50Z

@Lomanic , could you please provide the commit SHA from which you run the failed test. I'll try to investigate what goes wrong. I see that I have mismatched process_test.go file in my source branch.

i-prudnikov · 2020-02-14T14:48:00Z

@Lomanic, I've run test against master branch, the root case is the returned error from PidExistsWithContext function, that should be nil, in case process is not found. The fix is a simple as:

_, err := os.Stat(common.HostProc(strconv.Itoa(int(pid))))
if e, ok := err.(*os.PathError); ok && e.Err.(syscall.Errno) == syscall.ENOENT {
	return err == nil, nil
}
return err == nil, err

I've opened a new merge request for that fix: #840

Lomanic reviewed Jan 18, 2020

View reviewed changes

i-prudnikov force-pushed the process_check_pid_exnistense_fix branch from 4c34bd1 to cf14906 Compare January 20, 2020 15:38

i-prudnikov commented Jan 20, 2020

View reviewed changes

Fix check pid existence when running in different process namespace (…

8dec3d8

…container)

i-prudnikov force-pushed the process_check_pid_exnistense_fix branch from cf14906 to 8dec3d8 Compare January 27, 2020 08:58

i-prudnikov mentioned this pull request Feb 5, 2020

Will this be merged? #831

Closed

shirou merged commit a9d510e into shirou:master Feb 8, 2020

i-prudnikov mentioned this pull request Feb 14, 2020

Correct returned error (set to nil) in case process is not exist #840

Merged

Lomanic mentioned this pull request Mar 3, 2020

Processes() is extremely CPU intensive #842

Open

1 task

Lomanic mentioned this pull request Mar 18, 2021

Freebsd(jail): process.NewProcess doesnt work if no procfs mounted #1049

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix check pid existence when running in different process namespace (container) #821

Fix check pid existence when running in different process namespace (container) #821

i-prudnikov commented Jan 17, 2020

shirou commented Jan 18, 2020

Lomanic Jan 18, 2020

i-prudnikov Jan 20, 2020

Lomanic Jan 18, 2020

i-prudnikov Jan 20, 2020 •

edited

Loading

Lomanic Jan 18, 2020 •

edited

Loading

Lomanic Jan 18, 2020

Lomanic Jan 18, 2020

i-prudnikov commented Jan 20, 2020 •

edited

Loading

i-prudnikov Jan 20, 2020

shirou Jan 26, 2020

i-prudnikov Jan 27, 2020 •

edited

Loading

shirou commented Jan 26, 2020 •

edited

Loading

Lomanic commented Jan 26, 2020

shirou commented Jan 27, 2020

i-prudnikov commented Jan 27, 2020

shirou commented Feb 8, 2020

Lomanic commented Feb 13, 2020

i-prudnikov commented Feb 14, 2020

i-prudnikov commented Feb 14, 2020

Fix check pid existence when running in different process namespace (container) #821

Fix check pid existence when running in different process namespace (container) #821

Conversation

i-prudnikov commented Jan 17, 2020

shirou commented Jan 18, 2020

Lomanic Jan 18, 2020

Choose a reason for hiding this comment

i-prudnikov Jan 20, 2020

Choose a reason for hiding this comment

Lomanic Jan 18, 2020

Choose a reason for hiding this comment

i-prudnikov Jan 20, 2020 • edited Loading

Choose a reason for hiding this comment

Lomanic Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

Lomanic Jan 18, 2020

Choose a reason for hiding this comment

Lomanic Jan 18, 2020

Choose a reason for hiding this comment

i-prudnikov commented Jan 20, 2020 • edited Loading

i-prudnikov Jan 20, 2020

Choose a reason for hiding this comment

shirou Jan 26, 2020

Choose a reason for hiding this comment

i-prudnikov Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

shirou commented Jan 26, 2020 • edited Loading

Lomanic commented Jan 26, 2020

shirou commented Jan 27, 2020

i-prudnikov commented Jan 27, 2020

shirou commented Feb 8, 2020

Lomanic commented Feb 13, 2020

i-prudnikov commented Feb 14, 2020

i-prudnikov commented Feb 14, 2020

i-prudnikov Jan 20, 2020 •

edited

Loading

Lomanic Jan 18, 2020 •

edited

Loading

i-prudnikov commented Jan 20, 2020 •

edited

Loading

i-prudnikov Jan 27, 2020 •

edited

Loading

shirou commented Jan 26, 2020 •

edited

Loading