[service/proctelemetry] Unset HOST_PROC to make sure we use and report the actual process and introduce an option to allow user to overwrite the unset logic and use the value set in the environment variable #7434

dloucasfx · 2023-03-27T16:24:53Z

Description:

While debugging the below error in k8s env

Error: failed to register process metrics: process does not exist
2023/03/23 03:44:47 main.go:115: application run finished with error: failed to register process metrics: process does not exist

I have noticed that the metric server is calling GOPSUTIL while the HOST_PROC variable is set , this causes gopsutil PidExistsWithContext to retrieve the process from the host instead from the container

func PidExistsWithContext(ctx context.Context, pid int32) (bool, error) {
	if pid <= 0 {
		return false, fmt.Errorf("invalid pid %v", pid)
	}
	proc, err := os.FindProcess(int(pid))
	if err != nil {
		return false, err
	}

	if isMount(common.HostProc()) { // if /<HOST_PROC>/proc exists and is mounted, check if /<HOST_PROC>/proc/<PID> folder exists
		_, err := os.Stat(common.HostProc(strconv.Itoa(int(pid))))
		if os.IsNotExist(err) {
			return false, nil
		}
		return err == nil, err
	}

This PR unsets and resets the host_proc variable and introduces an option to allow the use of host_proc if for whatever reason they need to

Link to tracking Issue:

Testing: < Describe what testing was performed and which tests were added.>
unit tests
Documentation: < Describe the documentation added.>

Please delete paragraphs that you did not use before submitting.

… introduce an option to allow user to overwrite the unset logic and use the value set in the environment variable Signed-off-by: Dani Louca <[email protected]>

rmfitzpatrick · 2023-03-27T16:28:02Z

service/internal/proctelemetry/process_telemetry.go

 	pm.proc, err = process.NewProcess(int32(os.Getpid()))
 	if err != nil {
 		return err
 	}
+	// restore immediately if it was previously set, don't use defer
+	if !useHostProcEnvVar && exists {
+		os.Setenv("HOST_PROC", hostproc)


Do you know if there's a chance of memoization or is there some guarantee from gopsutil that subsequent lookups w/ the restored env var will be reliably using the hostfs procfs?

Also this should be probably be deferred so that it always happens moving forward regardless of changes in error handling.

Sorry, just realized that subsequent stat updates are all though gopsutil so this would need to happen for each update cycle, which is almost certainly going to cause race conditions w/ the host metrics receiver.

yep, this is a bust! gopsutil re-use the hostproc on every call :(

func (p *Process) fillFromTIDStatWithContext(ctx context.Context, tid int32) (uint64, int32, *cpu.TimesStat, int64, uint32, int32, *PageFaultsStat, error) { pid := p.Pid var statPath string if tid == -1 { statPath = common.HostProc(strconv.Itoa(int(pid)), "stat") } else { statPath = common.HostProc(strconv.Itoa(int(pid)), "task", strconv.Itoa(int(tid)), "stat") }

rmfitzpatrick · 2023-03-27T16:41:34Z

service/internal/proctelemetry/process_telemetry.go

+	// unset HOST_PROC env variable so the process search occurs locally
+	if !useHostProcEnvVar && exists {
+		os.Unsetenv("HOST_PROC")
+	}
 	pm.proc, err = process.NewProcess(int32(os.Getpid()))


I think the issue is that this pid needs to be from the host's perspective** and not gathered from the container's syscall.

mx-psi · 2023-03-27T16:51:14Z

service/telemetry/config.go

+
+	// UseHostProcEnvVar when set to true, the metric server, through gopsutil,
+	// lookup for the otel process in the proc path defined in the HOST_PROC environment variable.
+	UseHostProcEnvVar bool `mapstructure:"useHostProcEnvVar"`


Do we actually need to expose this? If it's unclear, I would remove this from the PR to avoid extending the public API surface

mx-psi · 2023-03-27T16:55:21Z

service/internal/proctelemetry/process_telemetry.go

 	pm.proc, err = process.NewProcess(int32(os.Getpid()))
 	if err != nil {
 		return err
 	}
+	// restore immediately if it was previously set, don't use defer


If we don't use defer and process.NewProcess panics we won't set the environment variable back, so I don't think this approach is acceptable. What are the challenges with using defer?

I think a better approach for this would be for gopsutil to allow us specifying the use or not of HOST_PROC instead of us modifying the environment variables. Have we asked the gopsutil maintainer about this?

dloucasfx · 2023-03-27T17:45:00Z

Thanks @rmfitzpatrick @mx-psi for reviewing.
I based my PR on the assumption that gopsutil will only need the host_proc to locate the files once, but after @rmfitzpatrick pointed it out, I also noticed that it reuses host_proc on every call:
example when getting CPU time:

func (p *Process) fillFromTIDStatWithContext(ctx context.Context, tid int32) (uint64, int32, *cpu.TimesStat, int64, uint32, int32, *PageFaultsStat, error) {
	pid := p.Pid
	var statPath string

	if tid == -1 {
		statPath = common.HostProc(strconv.Itoa(int(pid)), "stat")
	} else {
		statPath = common.HostProc(strconv.Itoa(int(pid)), "task", strconv.Itoa(int(tid)), "stat")
	}

so looks like the fix is more involved , so we either need to:

wait for allow to pass context values to override environment variables shirou/gopsutil#1439 to be merged
or implement our own package to pull the data/or use our own fork of gopsutil that includes allow to pass context values to override environment variables shirou/gopsutil#1439

cc: @dmitryax

dloucasfx · 2023-03-27T19:09:49Z

closing PR and moving discussion to #7435

…ariable with a programmatic value (#7998) Reprising #7434 **Description:** While debugging the below error in k8s env ```` Error: failed to register process metrics: process does not exist 2023/03/23 03:44:47 main.go:115: application run finished with error: failed to register process metrics: process does not exist ```` I have noticed that the metric server is calling GOPSUTIL while the HOST_PROC variable is set , this causes gopsutil `PidExistsWithContext ` to retrieve the process from the host instead from the container ```` func PidExistsWithContext(ctx context.Context, pid int32) (bool, error) { if pid <= 0 { return false, fmt.Errorf("invalid pid %v", pid) } proc, err := os.FindProcess(int(pid)) if err != nil { return false, err } if isMount(common.HostProc()) { // if /<HOST_PROC>/proc exists and is mounted, check if /<HOST_PROC>/proc/<PID> folder exists _, err := os.Stat(common.HostProc(strconv.Itoa(int(pid)))) if os.IsNotExist(err) { return false, nil } return err == nil, err } ```` This PR unsets and resets the host_proc variable and introduces an option to allow the use of host_proc if for whatever reason they need to **Link to tracking Issue:** Fixes #7435 **Testing:** unit tests --------- Signed-off-by: Dani Louca <[email protected]> Co-authored-by: Dani Louca <[email protected]> Co-authored-by: Alex Boten <[email protected]>

dloucasfx requested review from a team and mx-psi March 27, 2023 16:24

Unset HOST_PROC to make sure we use and report the actual process and…

620b2c6

… introduce an option to allow user to overwrite the unset logic and use the value set in the environment variable Signed-off-by: Dani Louca <[email protected]>

dloucasfx force-pushed the metric-server-hostproc branch from a9c5b6f to 620b2c6 Compare March 27, 2023 16:26

rmfitzpatrick reviewed Mar 27, 2023

View reviewed changes

mx-psi requested changes Mar 27, 2023

View reviewed changes

dloucasfx mentioned this pull request Mar 27, 2023

[service/proctelemetry] when host_proc env variable is set, metric server uses and reports on the wrong process #7435

Closed

dloucasfx closed this Mar 27, 2023

atoulme mentioned this pull request Jun 28, 2023

[service/proctelemetry] Offer to override the HOST_PROC environment variable with a programmatic value #7998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[service/proctelemetry] Unset HOST_PROC to make sure we use and report the actual process and introduce an option to allow user to overwrite the unset logic and use the value set in the environment variable #7434

[service/proctelemetry] Unset HOST_PROC to make sure we use and report the actual process and introduce an option to allow user to overwrite the unset logic and use the value set in the environment variable #7434

dloucasfx commented Mar 27, 2023 •

edited

Loading

rmfitzpatrick Mar 27, 2023

rmfitzpatrick Mar 27, 2023

rmfitzpatrick Mar 27, 2023

dloucasfx Mar 27, 2023

rmfitzpatrick Mar 27, 2023 •

edited

Loading

mx-psi Mar 27, 2023

mx-psi Mar 27, 2023

dloucasfx commented Mar 27, 2023

dloucasfx commented Mar 27, 2023

[service/proctelemetry] Unset HOST_PROC to make sure we use and report the actual process and introduce an option to allow user to overwrite the unset logic and use the value set in the environment variable #7434

[service/proctelemetry] Unset HOST_PROC to make sure we use and report the actual process and introduce an option to allow user to overwrite the unset logic and use the value set in the environment variable #7434

Conversation

dloucasfx commented Mar 27, 2023 • edited Loading

rmfitzpatrick Mar 27, 2023

Choose a reason for hiding this comment

rmfitzpatrick Mar 27, 2023

Choose a reason for hiding this comment

rmfitzpatrick Mar 27, 2023

Choose a reason for hiding this comment

dloucasfx Mar 27, 2023

Choose a reason for hiding this comment

rmfitzpatrick Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

mx-psi Mar 27, 2023

Choose a reason for hiding this comment

mx-psi Mar 27, 2023

Choose a reason for hiding this comment

dloucasfx commented Mar 27, 2023

dloucasfx commented Mar 27, 2023

dloucasfx commented Mar 27, 2023 •

edited

Loading

rmfitzpatrick Mar 27, 2023 •

edited

Loading