WIP: Do not merge: Test periodic reaper with the router by mrunalp · Pull Request #111 · openshift/router

mrunalp · 2020-04-06T23:42:03Z

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

ironcladlou · 2020-04-07T11:49:53Z

vendor/github.com/openshift/library-go/pkg/proc/reaper.go


+// parseProcForZombies parses the current procfs mounted at /proc
+// to find proccess in the zombie state.
+func parseProcForZombies() ([]int, error) {


Does the direct use of procfs couple this to Linux?

We don't support containers on non linux right now - router wouldn't work on windows

ironcladlou · 2020-04-07T11:53:14Z

Interesting... can you explain the theory of operation? My assumption based on prior R&D is that switching from signal handling to a polling approach shortens (but does not eliminate) the window of time the reaper can race with explicit wait calls.

ironcladlou · 2020-04-07T11:54:07Z

vendor/github.com/openshift/library-go/pkg/proc/reaper.go

+// from a pid 1 process.
+// The zombie processes are reaped at the beginning of next cycle, so os.Exec calls
+// have an oppurtunity to reap their children within `period` seconds.
+func StartPeriodicReaper(period int64) {


Instead of forcing the caller to a certain time unit, how about instead receiving a time.Duration?

frobware · 2020-04-07T12:48:53Z

All of this seems like band aid to me. In calls where we lose/race when calling Wait() we're likely to log an error. Customers than report that, we say: "not really an error" which is perhaps "OK" for the case we have now. It just seems like this will mask problems.

Why don't we have some form of real init where we won't race?

See also: #78

ironcladlou · 2020-04-07T12:58:30Z

@frobware agree, not sure if we need to re-litigate it here

smarterclayton · 2020-04-13T23:35:30Z

Mrunals change is fundamentally different. The old reaper was completely wrong - you don't wait for all children, you wait for the unmanaged defunct ones. And you only need to reap defunct processes. So this logic correct now - only zombie defunct non parented children get reaped. There's no race because a defunct process stays defunct

smarterclayton · 2020-04-13T23:36:47Z

Bringing in a new init system to manage defunct processes vs 20 lines of go that will be used in every image that performs the required function. There is no real init responsibility except reaping defunct processes.

frobware · 2020-04-15T13:08:39Z

Mrunals change is fundamentally different. The old reaper was completely wrong - you don't wait for all children, you wait for the unmanaged defunct ones.

Ugh. I clearly didn't spend enough time looking at this because it's clearly waiting on zombie pids only.

I did some validation of the change here https://github.com/frobware/haproxy-hacks/blob/master/reaper/main.go#L21, this based on our previous experiments.

In the following output we see the router's reload script being invoked every second:

2020/04/15 13:01:18 Launching periodic reaper
2020/04/15 13:01:18 Sleeping for 6 seconds
cmd.CombinedOutput() err=exit status 1, out=OLD_PIDS: 
 - Checking http://localhost:80 ...
 - Exceeded max wait time (0) in health check - 0 retry attempt(s).
   30 pts/0    S+     0:00 grep defunct
#defunct 1

cmd.CombinedOutput() err=exit status 1, out=OLD_PIDS: 17
 - Checking http://localhost:80 ...
 - Exceeded max wait time (0) in health check - 0 retry attempt(s).
   52 pts/0    S+     0:00 grep defunct
#defunct 1

cmd.CombinedOutput() err=exit status 1, out=OLD_PIDS: 39 17
 - Checking http://localhost:80 ...
 - Exceeded max wait time (0) in health check - 0 retry attempt(s).
   17 ?        Zs     0:00 [haproxy] <defunct>
   73 pts/0    S+     0:00 grep defunct
#defunct 2

cmd.CombinedOutput() err=exit status 1, out=OLD_PIDS: 60 39 17
 - Checking http://localhost:80 ...
 - Exceeded max wait time (0) in health check - 0 retry attempt(s).
   17 ?        Zs     0:00 [haproxy] <defunct>
   39 ?        Zs     0:00 [haproxy] <defunct>
   94 pts/0    S+     0:00 grep defunct
#defunct 3

And later we see the reaper picking up some zombies:

cmd.CombinedOutput() err=exit status 1, out=OLD_PIDS: 1699 1678 1657 1636 1615 1594 1573 1552 1531 1510 1489 1468 1447
 - Checking http://localhost:80 ...
 - Exceeded max wait time (0) in health check - 0 retry attempt(s).
 1447 ?        Zs     0:00 [haproxy] <defunct>
 1468 ?        Zs     0:00 [haproxy] <defunct>
 1489 ?        Zs     0:00 [haproxy] <defunct>
 1510 ?        Zs     0:00 [haproxy] <defunct>
 1531 ?        Zs     0:00 [haproxy] <defunct>
 1552 ?        Zs     0:00 [haproxy] <defunct>
 1573 ?        Zs     0:00 [haproxy] <defunct>
 1594 ?        Zs     0:00 [haproxy] <defunct>
 1615 ?        Zs     0:00 [haproxy] <defunct>
 1636 ?        Zs     0:00 [haproxy] <defunct>
 1657 ?        Zs     0:00 [haproxy] <defunct>
 1678 ?        Zs     0:00 [haproxy] <defunct>
 1733 pts/0    S+     0:00 grep defunct
#defunct 13

2020/04/15 13:02:42 Zombie reaped: 1447
2020/04/15 13:02:42 Zombie reaped: 1468
2020/04/15 13:02:42 Zombie reaped: 1489
2020/04/15 13:02:42 Zombie reaped: 1510
2020/04/15 13:02:42 Zombie reaped: 1531
2020/04/15 13:02:42 Zombie reaped: 1552
2020/04/15 13:02:42 Sleeping for 6 seconds

And based on timing, we see the number of defunct drop:

cmd.CombinedOutput() err=exit status 1, out=OLD_PIDS: 1741 1720 1699 1678 1657 1636 1615 1594 1573
 - Checking http://localhost:80 ...
 - Exceeded max wait time (0) in health check - 0 retry attempt(s).
 1573 ?        Zs     0:00 [haproxy] <defunct>
 1594 ?        Zs     0:00 [haproxy] <defunct>
 1615 ?        Zs     0:00 [haproxy] <defunct>
 1636 ?        Zs     0:00 [haproxy] <defunct>
 1657 ?        Zs     0:00 [haproxy] <defunct>
 1678 ?        Zs     0:00 [haproxy] <defunct>
 1699 ?        Zs     0:00 [haproxy] <defunct>
 1720 ?        Zs     0:00 [haproxy] <defunct>
 1775 pts/0    S+     0:00 grep defunct
#defunct 9

2020/04/15 13:02:48 Zombie reaped: 1573
2020/04/15 13:05:36 Zombie reaped: 1594
2020/04/15 13:05:36 Zombie reaped: 1615
2020/04/15 13:05:36 Zombie reaped: 1636
2020/04/15 13:05:36 Zombie reaped: 1657
2020/04/15 13:05:36 Zombie reaped: 1678
2020/04/15 13:05:36 Sleeping for 6 seconds
cmd.CombinedOutput() err=exit status 1, out=OLD_PIDS: 1762 1741 1720 1699
 - Checking http://localhost:80 ...
 - Exceeded max wait time (0) in health check - 0 retry attempt(s).
 1699 ?        Zs     0:00 [haproxy] <defunct>
 1720 ?        Zs     0:00 [haproxy] <defunct>
 1741 ?        Zs     0:00 [haproxy] <defunct>
 1796 pts/0    S+     0:00 grep defunct
#defunct 4

frobware · 2020-04-15T13:09:51Z

/lgtm

frobware · 2020-04-15T13:13:32Z

vendor/github.com/openshift/library-go/pkg/proc/reaper.go

+			var zs []int
+			var err error
+			for {
+				for _, z := range zs {


Curious as to why the don't pull the call to:

zs, err = parseProcForZombies()

before this loop?

smarterclayton · 2020-04-22T00:25:25Z

vendor/github.com/openshift/library-go/pkg/proc/reaper.go

+
+// StartPeriodicReaper starts a goroutine to reap processes periodically if called
+// from a pid 1 process.
+// The zombie processes are reaped at the beginning of next cycle, so os.Exec calls


This comment isn't necessary, since os.Exec doesn't care about zombies (it will never itself have a zombie because it never quit so the child isn't reparented)

smarterclayton · 2020-04-22T00:32:45Z

Also testing in openshift/ci-search#73

smarterclayton · 2020-04-22T00:33:01Z

vendor/github.com/openshift/library-go/pkg/proc/reaper.go

+// from a pid 1 process.
+// The zombie processes are reaped at the beginning of next cycle, so os.Exec calls
+// have an oppurtunity to reap their children within `period` seconds.
+func StartPeriodicReaper(period int64) {


Take a time.duration yeah

smarterclayton · 2020-04-22T00:33:10Z

vendor/github.com/openshift/library-go/pkg/proc/reaper.go

+// have an oppurtunity to reap their children within `period` seconds.
+func StartPeriodicReaper(period int64) {
+	if os.Getpid() == 1 {
+		klog.V(4).Infof("Launching periodic reaper")


Once you've tested strip out all logging.

smarterclayton · 2020-04-22T00:34:01Z

vendor/github.com/openshift/library-go/pkg/proc/reaper.go

@@ -3,13 +3,87 @@
 package proc


Rename this file proc_linux.go and then add a proc.go with // +build !linux that exposes StartReaper

smarterclayton · 2020-04-22T00:34:15Z

pkg/cmd/infra/router/template.go

 	}

-	proc.StartReaper()
+	proc.StartPeriodicReaper(6)


Prefer StartReaper since the function hasn't changed really.

mrunalp · 2020-04-22T03:53:08Z

I will update the PR with the comments here at openshift/library-go#767 tomorrow. Thanks!

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

openshift-ci-robot · 2020-04-23T02:21:06Z

New changes are detected. LGTM label has been removed.

openshift-ci-robot · 2020-04-23T02:21:20Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [frobware]

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

openshift-ci-robot · 2020-07-31T18:11:50Z

@mrunalp: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/verify	`d0207d8`	link	`/test verify`
ci/prow/e2e	`d0207d8`	link	`/test e2e`
ci/prow/e2e-metal-ipi	`d0207d8`	link	`/test e2e-metal-ipi`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sgreene570 · 2020-09-30T13:34:07Z

Closing in favor of #190
/close

openshift-ci-robot · 2020-09-30T13:34:24Z

@sgreene570: Closed this PR.

Details

In response to this:

Closing in favor of #190
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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 Miciah and knobunc April 6, 2020 23:42

mrunalp mentioned this pull request Apr 7, 2020

reaper: Add a periodic reaper funcion openshift/library-go#767

Merged

ironcladlou reviewed Apr 7, 2020

View reviewed changes

openshift-ci-robot assigned frobware Apr 15, 2020

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

frobware reviewed Apr 15, 2020

View reviewed changes

frobware mentioned this pull request Apr 21, 2020

[WIP] template/router: explicitly handle "no child processes" error #78

Closed

smarterclayton reviewed Apr 22, 2020

View reviewed changes

Do not merge: Test periodic reaper with the router

d0207d8

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

mrunalp force-pushed the test_periodic_router branch from 4b02e46 to d0207d8 Compare April 23, 2020 02:21

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2020

sgreene570 mentioned this pull request Sep 21, 2020

Bug 1859134: Switch to periodic process reaper #190

Merged

openshift-ci-robot closed this Sep 30, 2020

sgreene570 mentioned this pull request Oct 6, 2020

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

Merged

Conversation

mrunalp commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Apr 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frobware commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ironcladlou commented Apr 7, 2020

Uh oh!

smarterclayton commented Apr 13, 2020

Uh oh!

smarterclayton commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frobware commented Apr 15, 2020

Uh oh!

frobware commented Apr 15, 2020

Uh oh!

frobware Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Apr 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 23, 2020

Uh oh!

openshift-ci-robot commented Apr 23, 2020

Uh oh!

openshift-ci-robot commented Jul 31, 2020

Uh oh!

sgreene570 commented Sep 30, 2020

Uh oh!

openshift-ci-robot commented Sep 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

frobware commented Apr 7, 2020 •

edited

Loading

smarterclayton commented Apr 13, 2020 •

edited

Loading

frobware Apr 15, 2020 •

edited

Loading

mrunalp commented Apr 22, 2020 •

edited

Loading