reaper: Add a periodic reaper funcion by mrunalp · Pull Request #767 · openshift/library-go

mrunalp · 2020-04-06T21:41:44Z

This commit adds a periodic reaper function that is intended
to be used when the go process uses the os/exec package to
launch child processes. The period argument could be adjusted
according to how long the os/exec calling code waits to
gather exit status from child processes.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

mrunalp · 2020-04-06T21:44:43Z

@smarterclayton ptal. If we can confirm that this works well with router, I can optimize the code further for parsing /proc. Also, we can consolidate both types of reaping with ReaperOptions and a single function or keep it separate like the current approach.

smarterclayton · 2020-04-06T21:52:35Z

/hold

Can you test vendor this into a router PR and look at the log output? If this fixes it, we should get none of the "can't wait for process anymore"

mrunalp · 2020-04-06T22:04:28Z

@smarterclayton sure, I can do that.

mrunalp · 2020-04-07T00:06:09Z

Opened openshift/router#111

mrunalp · 2020-04-23T02:10:06Z

Updated.

mrunalp · 2020-04-23T02:10:42Z

I have a couple of logs remaining that I will take out after one more round of testing on the router PR.

mrunalp · 2020-05-04T18:33:05Z

Updated the PR. This is ready.

smarterclayton · 2020-05-04T18:50:55Z

pkg/proc/proc_linux.go

+			for {
+				zs, err = parseProcForZombies()
+				if err != nil {
+					klog.V(4).Infof(err.Error())


Give this an error prefix so we know it was from reaping

smarterclayton · 2020-05-04T18:52:11Z

pkg/proc/proc_linux.go

+
+// StartReaper starts a goroutine to reap processes periodically if called
+// from a pid 1 process.
+func StartReaper(period time.Duration) {


If period is less than or equal to zero set it to five seconds and add a comment above. Also describe why you might change the default period (if you create tens of thousands of defunct processes every five seconds).

mrunalp · 2020-06-03T16:46:28Z

@smarterclayton could you take another look?

smarterclayton · 2020-06-03T18:09:19Z

pkg/proc/proc_linux.go

+
+// StartReaper starts a goroutine to reap processes periodically if called
+// from a pid 1 process.
+// If period is less than 5 seconds, then it is set to 5 seconds.


I would have it be only when 0, not when less than five. If you have a very fast subprocess creating app, you might need to set it faster.

smarterclayton · 2020-06-03T18:09:38Z

pkg/proc/proc_linux.go

+func StartReaper(period time.Duration) {
+	if os.Getpid() == 1 {
+		const minReaperPeriodSeconds = 5
+		if period.Seconds() < minReaperPeriodSeconds {


if period == 0 { period = 5 * time.Second }`

sounds good! Default to 5.

pkg/proc/proc_linux.go

This commit modifies the reaper behavior to periodically scan the procfs for zombies and then reap them in the next cycle. This should work better for container processes that spawn child processes through os.Exec Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

smarterclayton · 2020-06-08T18:21:55Z

/lgtm

smarterclayton · 2020-06-08T18:22:08Z

/hold cancel

openshift-ci-robot · 2020-06-08T18:22:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, smarterclayton

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

reaper: Add a periodic reaper funcion

Switch the router process reaper to use the new periodic reaper brought in by openshift/library-go#767

Bump openshift/library-go dep to include the new periodic reaper function merged in openshift/library-go#767

Switch the router process reaper to use the new periodic reaper brought in by openshift/library-go#767

Bump openshift/library-go dep to include the new periodic reaper function merged in openshift/library-go#767

Switch the router process reaper to use the new periodic reaper brought in by openshift/library-go#767

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020

openshift-ci-robot requested review from smarterclayton and sttts April 6, 2020 21:42

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2020

mrunalp force-pushed the add_periodic_reaper branch from de978fb to 8682157 Compare April 6, 2020 23:34

mrunalp mentioned this pull request Apr 22, 2020

WIP: Do not merge: Test periodic reaper with the router openshift/router#111

Closed

mrunalp force-pushed the add_periodic_reaper branch from 8682157 to e10c451 Compare April 23, 2020 02:09

mrunalp force-pushed the add_periodic_reaper branch from e10c451 to dda569a Compare May 4, 2020 18:31

mrunalp changed the title ~~WIP: reaper: Add a periodic reaper funcion~~ reaper: Add a periodic reaper funcion May 4, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2020

smarterclayton suggested changes May 4, 2020

View reviewed changes

mrunalp force-pushed the add_periodic_reaper branch 2 times, most recently from eb5a6cb to 9989b9e Compare May 4, 2020 23:19

smarterclayton reviewed Jun 3, 2020

View reviewed changes

pkg/proc/proc_linux.go Outdated Show resolved Hide resolved

smarterclayton reviewed Jun 3, 2020

View reviewed changes

pkg/proc/proc_linux.go Outdated Show resolved Hide resolved

mrunalp force-pushed the add_periodic_reaper branch from 9989b9e to fe841e6 Compare June 3, 2020 22:10

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2020

openshift-ci-robot assigned smarterclayton Jun 8, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2020

smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2020

openshift-merge-robot merged commit c10453a into openshift:master Jun 8, 2020

bertinatto pushed a commit to bertinatto/library-go that referenced this pull request Jul 2, 2020

Merge pull request openshift#767 from mrunalp/add_periodic_reaper

83b9d03

reaper: Add a periodic reaper funcion

sgreene570 added a commit to sgreene570/router that referenced this pull request Sep 21, 2020

pkg/cmd: Switch to periodic reaper

ef8fb3c

Switch the router process reaper to use the new periodic reaper brought in by openshift/library-go#767

sgreene570 mentioned this pull request Sep 21, 2020

Bug 1859134: Switch to periodic process reaper openshift/router#190

Merged

sgreene570 added a commit to sgreene570/router that referenced this pull request Sep 30, 2020

pkg/cmd: Switch to periodic reaper

0529d96

Switch the router process reaper to use the new periodic reaper brought in by openshift/library-go#767

sgreene570 added a commit to sgreene570/router that referenced this pull request Oct 6, 2020

go.mod: bump openshift/library-go

9c2d4f9

Bump openshift/library-go dep to include the new periodic reaper function merged in openshift/library-go#767

sgreene570 added a commit to sgreene570/router that referenced this pull request Oct 6, 2020

vendor: bump openshift/library-go

a29f726

Bump openshift/library-go dep to include the new periodic reaper function merged in openshift/library-go#767

sgreene570 added a commit to sgreene570/router that referenced this pull request Oct 6, 2020

pkg/cmd: Switch to periodic reaper

25faaa1

Switch the router process reaper to use the new periodic reaper brought in by openshift/library-go#767

sgreene570 mentioned this pull request Oct 6, 2020

[release-4.5] Bug 1885688: Back port Switch to periodic process reaper openshift/router#195

Merged

sgreene570 added a commit to sgreene570/router that referenced this pull request Oct 6, 2020

go.mod: bump openshift/library-go

76dbbb2

Bump openshift/library-go dep to include the new periodic reaper function merged in openshift/library-go#767

sgreene570 added a commit to sgreene570/router that referenced this pull request Oct 6, 2020

vendor: bump openshift/library-go

75a8ad2

Bump openshift/library-go dep to include the new periodic reaper function merged in openshift/library-go#767

sgreene570 added a commit to sgreene570/router that referenced this pull request Oct 6, 2020

pkg/cmd: Switch to periodic reaper

156c529

Switch the router process reaper to use the new periodic reaper brought in by openshift/library-go#767

Conversation

mrunalp commented Apr 6, 2020

Uh oh!

mrunalp commented Apr 6, 2020

Uh oh!

smarterclayton commented Apr 6, 2020

Uh oh!

mrunalp commented Apr 6, 2020

Uh oh!

mrunalp commented Apr 7, 2020

Uh oh!

mrunalp commented Apr 23, 2020

Uh oh!

mrunalp commented Apr 23, 2020

Uh oh!

mrunalp commented May 4, 2020

Uh oh!

smarterclayton May 4, 2020

Choose a reason for hiding this comment

Uh oh!

smarterclayton May 4, 2020

Choose a reason for hiding this comment

Uh oh!

mrunalp May 4, 2020

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Jun 3, 2020

Uh oh!

smarterclayton Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

smarterclayton Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

mrunalp Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smarterclayton commented Jun 8, 2020

Uh oh!

smarterclayton commented Jun 8, 2020

Uh oh!

openshift-ci-robot commented Jun 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants