[JENKINS-52847] Avoid using -p to support basic implementations of ps #77

dwnusbaum · 2018-08-01T19:24:33Z

Some implementations of ps don't support the -p option, but we can work around that by passing a more complicated regex to grep.

I manually tested the change on OS X and Alpine Linux to make sure it works on both platforms.

dwnusbaum · 2018-08-02T15:57:48Z

src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java

@@ -140,7 +140,7 @@ public String getScript() {
        FilePath resultFile = c.getResultFile(ws);
        FilePath controlDir = c.controlDir(ws);
        if (capturingOutput) {
-            cmd = String.format("pid=$$; { while ps -o pid -p $pid | grep -q $pid && [ -d '%s' -a \\! -f '%s' ]; do touch '%s'; sleep 3; done } & jsc=%s; %s=$jsc '%s' > '%s' 2> '%s'; echo $? > '%s.tmp'; mv '%s.tmp' '%s'; wait",
+            cmd = String.format("pid=$$; { while ps -o pid | grep -q \"^\\s*$pid$\" && [ -d '%s' -a \\! -f '%s' ]; do touch '%s'; sleep 3; done } & jsc=%s; %s=$jsc '%s' > '%s' 2> '%s'; echo $? > '%s.tmp'; mv '%s.tmp' '%s'; wait",


To get this to work in csh/tcsh we'd need to use \"^\\s*$pid\"\'$\' (becomes "^\s*$pid"'$' after removing Java's escaping) because in those shells things like echo "$" throw an error.

Do we care about csh/tcsh?

I don't think so - @jglick might have other thoughts.

But is grep guaranteed to be supported?

This is making me twitch a bit. Which docker images did we test with?

No idea about whether it's guaranteed to be supported on all platforms, but it is at least included in BusyBox. I'll look around for a definitive answer.

Which docker images did we test with?

jenkinsci/slave with both alpine (3.19-1-alpine, before the procops fix) and Debian (3.20-1) tags.

is grep guaranteed to be supported

Someone could always have a custom Arch distro that specifically excludes grep, but I think it is pretty unlikely. The best info I could find about where grep is included by default is https://unix.stackexchange.com/questions/37064/which-are-the-standard-commands-available-in-every-linux-based-distribution. Based on that, and the fact that our use is compatible with BusyBox as well, I think it is pretty safe to count on grep being present, unless there are some new popular container OS's that leave it out or something.

Do we care about csh/tcsh?

No, the wrapper script is always run using sh.

jglick

POSIX ps is supposed to support -p so if there are some widely used containers with ps implementations that do not, I am not sure where that leaves us—we can only guess what might work.

I am a little nervous about dropping -p because then we start to rely on

By default, ps shall select all processes with the same effective user ID as the current user and the same controlling terminal as the invoker.

I suppose both the EUID and the controlling terminal should be identical between the main wrapper script process and the ps process. -p selects the process explicitly without this restriction.

jglick · 2018-08-06T18:13:32Z

src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java

@@ -140,7 +140,7 @@ public String getScript() {
        FilePath resultFile = c.getResultFile(ws);
        FilePath controlDir = c.controlDir(ws);
        if (capturingOutput) {
-            cmd = String.format("pid=$$; { while ps -o pid -p $pid | grep -q $pid && [ -d '%s' -a \\! -f '%s' ]; do touch '%s'; sleep 3; done } & jsc=%s; %s=$jsc '%s' > '%s' 2> '%s'; echo $? > '%s.tmp'; mv '%s.tmp' '%s'; wait",
+            cmd = String.format("pid=$$; { while ps -o pid | grep -q \"^\\s*$pid$\" && [ -d '%s' -a \\! -f '%s' ]; do touch '%s'; sleep 3; done } & jsc=%s; %s=$jsc '%s' > '%s' 2> '%s'; echo $? > '%s.tmp'; mv '%s.tmp' '%s'; wait",


Do we care about csh/tcsh?

No, the wrapper script is always run using sh.

jglick · 2018-08-06T18:30:55Z

Oh my:

We allow (and ignore) most of the above. FIXME.

See also this for example.

Extend BourneShellScriptTest to use a Docker fixture based on jenkinsci/slave:3.19-1-alpine to reproduce the problem before the fix please.

In the longer term, the current implementation using shell scripting should probably be discarded in favor of a Golang wrapper that could also do a setsid. See discussion in JENKINS-25503. Of course this means shipping binaries for each supported platform, or shipping only an x86_64 binary and falling back to the current implementation on exotic platforms.

svanoort · 2018-08-06T19:18:27Z

@jglick I have really mixed feelings about shipping Go artifacts to solve a process-tracking issue. Seems like it adds a lot of complexity without much reason -- why not simply use the Java 8 Process tooling such as 'isAlive'?

dwnusbaum · 2018-08-07T18:54:35Z

src/test/java/org/jenkinsci/plugins/durabletask/BourneShellScriptTest.java

@@ -220,7 +232,7 @@ private void runOnDocker(DumbSlave s) throws Exception {
        do {
            Thread.sleep(1000);
            baos = new ByteArrayOutputStream();
-            assertEquals(0, dockerLauncher.launch().cmds("ps", "-e", "-o", "pid,stat,command").stdout(new TeeOutputStream(baos, System.out)).join());
+            assertEquals(0, dockerLauncher.launch().cmds("ps", "-e", "-o", "pid,stat,comm").stdout(new TeeOutputStream(baos, System.out)).join());


comm appears to be the standardized name: http://pubs.opengroup.org/onlinepubs/009695399/utilities/ps.html, and BusyBox doesn't support command.

jglick · 2018-08-07T19:07:31Z

why not simply use the Java 8 Process tooling

Off topic here; as mentioned in JIRA, we probably cannot use Java code for that.

jglick · 2018-08-07T19:14:22Z

BTW things like e015167 do not actually need to be committed, much less pushed. Suffices to locally

git checkout master -- src/main

develop your test until it fails in the expected way, then

git checkout HEAD -- src/main

and verify that it passes, then commit the new test. We trust you! Alternately, to be explicit in history:

git checkout -b tmp master
# edit src/test/ until you get the expected failure
# add @Ignore
git commit -a -m 'Reproduced failure'
git checkout support-basic-ps
git merge --no-commit tmp
# remove @Ignore
git commit -a -m 'Yup it is fixed'
git branch -d tmp

or the git rebase -i equivalent if you like that sort of thing.

jglick · 2018-08-07T19:15:56Z

Fails I guess the way you intended:

java.lang.AssertionError: expected:<0> but was:<-1>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:645)
	at org.junit.Assert.assertEquals(Assert.java:631)
	at org.jenkinsci.plugins.durabletask.BourneShellScriptTest.runOnDocker(BourneShellScriptTest.java:233)
	at org.jenkinsci.plugins.durabletask.BourneShellScriptTest.runOnAlpineDocker(BourneShellScriptTest.java:214)
…
[workspace] Running shell script
ps: unrecognized option: p
BusyBox v1.27.2 (2017-12-12 10:41:50 GMT) multi-call binary.

Usage: ps [-o COL1,COL2=HEADER]

Show list of processes

	-o COL1,COL2=HEADER	Select columns for display
wrapper script does not seem to be touching the log file in /home/test/workspace@tmp/durable-e62978d8
…

So, time to restore fix?

dwnusbaum · 2018-08-07T19:17:55Z

and verify that it passes, then commit the new test. We trust you!

@jglick Ack, in the past I have been asked to show the failing test on the public CI history, but if everyone here is ok with it then it's certainly easier to just confirm locally. In any case I will squash-merge into master when ready here to get rid of the intermediate commits. The time between the commits was due to me running the tests locally to make sure everything passed.

jglick · 2018-08-07T19:21:42Z

src/test/java/org/jenkinsci/plugins/durabletask/BourneShellScriptTest.java

@@ -203,12 +205,22 @@ public void smokeTest() throws Exception {
        runOnDocker(new DumbSlave("docker", "/home/test", new SSHLauncher(container.ipBound(22), container.port(22), "test", "test", "", "")));
    }

+    @Test public void runOnAlpineDocker() throws Exception {
+        AlpineFixture container = dockerAlpine.get();
+        runOnDocker(new DumbSlave("docker", "/home/test", new SSHLauncher(container.ipBound(22), container.port(22), "test", "test", "", "")), 45);


Any particular reason for the longer sleep?

If it's lower than 30 seconds or so, the exit status is 0 and everything works fine. 45 seconds seemed to be long enough to consistently hit this code which triggers the failure.

I am not sure exactly what the minimum time to hit that case is, maybe 2 * HEARTBEAT_CHECK_INTERVAL + HEARTBEAT_MINIMUM_DELTA? I'll try to understand that code better to see if we can decrease the timeout.

Probably HEARTBEAT_CHECK_INTERVAL * 2 or something. OK.

jglick · 2018-08-07T19:22:46Z

src/test/resources/org/jenkinsci/plugins/durabletask/AlpineFixture/Dockerfile

@@ -0,0 +1,9 @@
+FROM jenkinsci/slave:3.19-1-alpine


IIUC this is the version before jenkinsci/docker-agent#28.

jglick · 2018-08-07T19:25:28Z

in the past I have been asked to show the failing test on the public CI history

At least for me it is fine if the PR author adds, say, a line comment to the line of the test where an error is thrown in the expected way with the main patch reverted, and shows the error output.

The CI history is actually less useful since the branch project will normally be deleted once the PR is merged, and with it goes any record of the test output. (To be solved at some point by external test storage!)

Avoid using -p to support basic implementations of ps

24dcdd5

dwnusbaum added the work-in-progress label Aug 1, 2018

dwnusbaum mentioned this pull request Aug 1, 2018

[JENKINS-50892] Ensure the toucher process exits if the main wrapper process is killed #75

Merged

dwnusbaum changed the title ~~Avoid using -p to support basic implementations of ps~~ [JENKINS-52847] Avoid using -p to support basic implementations of ps Aug 2, 2018

dwnusbaum removed the work-in-progress label Aug 2, 2018

Match whitespace preceding the pid

6dc4780

dwnusbaum commented Aug 2, 2018

View reviewed changes

dwnusbaum requested review from svanoort, jglick and abayer August 2, 2018 16:41

abayer approved these changes Aug 2, 2018

View reviewed changes

jglick approved these changes Aug 6, 2018

View reviewed changes

dwnusbaum added the work-in-progress label Aug 6, 2018

dwnusbaum added 2 commits August 7, 2018 14:11

Revert JENKINS-52847 temporarily

e015167

Reproduce JENKINS-52847 in a test

b6d9644

dwnusbaum commented Aug 7, 2018

View reviewed changes

Reapply JENKINS-52847 fix

75b0e92

jglick self-requested a review August 7, 2018 19:16

dwnusbaum requested a review from abayer August 7, 2018 19:18

dwnusbaum removed the work-in-progress label Aug 7, 2018

jglick approved these changes Aug 7, 2018

View reviewed changes

svanoort approved these changes Aug 7, 2018

View reviewed changes

dwnusbaum merged commit 5ded96f into jenkinsci:master Aug 7, 2018

dwnusbaum deleted the support-basic-ps branch August 7, 2018 19:44

jglick mentioned this pull request Aug 8, 2018

[JENKINS-52881] Avoid use of ps #79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-52847] Avoid using -p to support basic implementations of ps #77

[JENKINS-52847] Avoid using -p to support basic implementations of ps #77

dwnusbaum commented Aug 1, 2018 •

edited

Loading

dwnusbaum Aug 2, 2018 •

edited

Loading

svanoort Aug 2, 2018

svanoort Aug 2, 2018

dwnusbaum Aug 2, 2018

dwnusbaum Aug 2, 2018 •

edited

Loading

dwnusbaum Aug 2, 2018 •

edited

Loading

jglick Aug 6, 2018

jglick left a comment

jglick Aug 6, 2018

jglick commented Aug 6, 2018

svanoort commented Aug 6, 2018

dwnusbaum Aug 7, 2018

jglick commented Aug 7, 2018

jglick commented Aug 7, 2018

jglick commented Aug 7, 2018

dwnusbaum commented Aug 7, 2018

jglick Aug 7, 2018

dwnusbaum Aug 7, 2018 •

edited

Loading

jglick Aug 7, 2018

jglick Aug 7, 2018

jglick commented Aug 7, 2018

[JENKINS-52847] Avoid using -p to support basic implementations of ps #77

[JENKINS-52847] Avoid using -p to support basic implementations of ps #77

Conversation

dwnusbaum commented Aug 1, 2018 • edited Loading

dwnusbaum Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

dwnusbaum Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Aug 6, 2018

svanoort commented Aug 6, 2018

Choose a reason for hiding this comment

jglick commented Aug 7, 2018

jglick commented Aug 7, 2018

jglick commented Aug 7, 2018

dwnusbaum commented Aug 7, 2018

Choose a reason for hiding this comment

dwnusbaum Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Aug 7, 2018

dwnusbaum commented Aug 1, 2018 •

edited

Loading

dwnusbaum Aug 2, 2018 •

edited

Loading

dwnusbaum Aug 2, 2018 •

edited

Loading

dwnusbaum Aug 2, 2018 •

edited

Loading

dwnusbaum Aug 7, 2018 •

edited

Loading