refactor bpm: generate uid for each process; remove identifier lock; remove blockingBuffer

[Hexilee]

What problem does this PR solve?

Currently, the bpm locate managed processes by both PID and create time, because the OS may reuse PID, we must check the create time to avoid locating the wrong process.

However, the two-step locating is messy and the create time may be imprecise (we have fixed a relevant bug). In this PR, we generate random UID for each process to avoid id-reusing.

For example, to locate and kill a process, current code is complex:

	p, err := os.FindProcess(int(pid))
	if err != nil {
		log.Error(err, "unreachable path. `os.FindProcess` will never return an error on unix")
		return err
	}

	procState, err := process.NewProcess(int32(pid))
	if err != nil {
		// return successfully as the process has exited
		return nil
	}
	ct, err := procState.CreateTime()
	if err != nil {
		log.Error(err, "fail to read create time")
		// return successfully as the process has exited
		return nil
	}

	// There is a bug in calculating CreateTime in the new version of
	// gopsutils. This is a temporary solution before the upstream fixes it.
	if startTime-ct > 1000 || ct-startTime > 1000 {
		log.Info("process has already been killed", "startTime", ct, "expectedStartTime", startTime)
		// return successfully as the process has exited
		return nil
	}

	ppid, err := procState.Ppid()
	if err != nil {
		log.Error(err, "fail to read parent id")
		// return successfully as the process has exited
		return nil
	}
	if ppid != int32(os.Getpid()) {
		log.Info("process has already been killed", "ppid", ppid)
		// return successfully as the process has exited
		return nil
	}

	err = p.Signal(syscall.SIGTERM)

In this PR, we do that in the following code:

	proc, loaded := m.getProc(uid)
	if !loaded {
		return errors.Errorf("failed to find process with uid %s", uid)
	}

	if err := proc.cmd.Process.Signal(syscall.SIGTERM); err != nil {
		return errors.Wrap(err, "send SIGTERM to process")
	}

Moreover, the identifier lock caused some dead-lock issues, like #2660 and #2661, so we remove the lock, in this PR, applying requests will fail fast by conflicts of identifiers instead of running into dead-locked.

What's changed and how it works?

To keep compatible with v2.0 and v2.1, current we need a mapping between the pid pair(pid and create time) and the UID.

In v2.2, we should store UID in the status of CRDs (like podiochaos, podhttpchaos and stresschaos), then deprecate the instance(PID) and startTime(create time). In future versions like v3, we can remove the instance(PID) and startTime(createTime) from CRDs.

Others

• replace identifier lock with sync.Map
• replace blocking buffer with os.Pipe

Related changes

• Need to update chaos-mesh/website
• Need to update Dashboard UI
• Need to cheery-pick to release branches
  • release-2.1
  • release-2.0

Checklist

Tests

• Unit test
• E2E test
• No code
• Manual test (add steps below)

Side effects

• Breaking backward compatibility

Release note

Please add a release note.

You can safely ignore this section if you don't think this PR needs a release note.

DCO

If you find the DCO check fails, please run commands like below (Depends on the actual situations. For example, if the failed commit isn't the most recent) to fix it:

git commit --amend --signoff
git push --force

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• Andrewmatilde
• STRRL

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[Hexilee]

/run-e2e-tests

[Hexilee]

/run-e2e-tests

[codecov]

Codecov Report

Merging #2918 (8258385) into master (07508be) will increase coverage by 0.37%.
The diff coverage is 29.68%.

@@            Coverage Diff             @@
##           master    #2918      +/-   ##
==========================================
+ Coverage   36.88%   37.25%   +0.37%     
==========================================
  Files         111      110       -1     
  Lines        9487     9365     -122     
==========================================
- Hits         3499     3489      -10     
+ Misses       5666     5564     -102     
+ Partials      322      312      -10     

Impacted Files	Coverage Δ
pkg/bpm/metrics.go	0.00% <0.00%> (ø)
pkg/chaosdaemon/httpchaos_server.go	0.00% <0.00%> (ø)
pkg/chaosdaemon/iochaos_server.go	0.00% <0.00%> (ø)
pkg/chaosdaemon/stress_server_linux.go	0.00% <0.00%> (ø)
pkg/bpm/bpm.go	53.92% <60.50%> (+19.87%)	⬆️
pkg/bpm/build_linux.go	34.09% <66.66%> (ø)
pkg/chaosdaemon/server.go	38.09% <100.00%> (+0.59%)	⬆️
.../workflow/controllers/workflow_entry_reconciler.go	48.14% <0.00%> (-4.77%)	⬇️
pkg/workflow/controllers/utils.go	85.71% <0.00%> (-1.59%)	⬇️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07508be...8258385. Read the comment docs.

[Hexilee]

/run-e2e-tests

[Hexilee]

/run-e2e-tests

[Hexilee]

/run-e2e-tests

[Andrewmatilde]

LGTM

[Hexilee]

/run-e2e-tests

[STRRL]

LGTM! ❤️

My only concern is about the process lifecycle managed in BackgroundProcessManager: from starting a process to waiting for a process dead, the logic is splited into several methods. IMO, it would be much helpful if we could split the lifecycle of the process out from the uid things.

[STRRL]

If you do not want to do more things in this PR in the future, I think we could merge it. @Hexilee

[Hexilee]

/merge

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: 1138ef8

[Hexilee]

LGTM! ❤️

My only concern is about the process lifecycle managed in BackgroundProcessManager: from starting a process to waiting for a process dead, the logic is splited into several methods. IMO, it would be much helpful if we could split the lifecycle of the process out from the uid things.

Do you mean we shouldn't recycle processes by the uid?

[STRRL]

LGTM! heart
My only concern is about the process lifecycle managed in BackgroundProcessManager: from starting a process to waiting for a process dead, the logic is splited into several methods. IMO, it would be much helpful if we could split the lifecycle of the process out from the uid things.

Do you mean we shouldn't recycle processes by the uid?

We should recycle the uid, else we would cause a resource leak.

But I find that the path from cmd.Wait() to proc.Stopped() is pretty long. I wonder if there is a way to make it easier. 🤣