Jenkins 47225: Fix ProcStarter #236

MattLud · 2017-10-16T18:28:10Z

Addressed to allow fixes for #232

…calling processes

marvinthepa · 2017-10-16T19:38:39Z

Looks good at first glance.

Could you please try to reproduce the problems that @scoheb had with this, ideally in a test?

I think I reproduced it using withEnv(["a.b=c"]). Checking with sh 'env' on the jnlp slave, the "invalid" environment variable a.b was not exported either. So I guess we can safely ignore those invalid environment variables in the container as well.

MattLud · 2017-10-16T20:34:09Z

Sure thing - I'll include it in this PR as well. Thanks for all the help in getting this work into a good spot!

…

On Mon, Oct 16, 2017 at 2:38 PM, Martin Sander ***@***.***> wrote: Looks good at first glance. Could you please try to reproduce the problems <#218 (comment)> that @scoheb <https://github.com/scoheb> had with this, ideally in a test? I think I reproduced it using withEnv(["a.b=c"]). Checking with sh 'env' on the jnlp slave, the "invalid" environment variable a.b was not exported either. So I guess we can safely ignore those invalid environment variables in the container as well. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#236 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUwfdjiWAibCoQhbXIt7C61Hrjkzv5pks5ss7DDgaJpZM4P7DAC> .

MattLud · 2017-10-17T18:27:50Z

Looks like the error is getting swallowed up in the k8s client. Looks like the best approach will be to just detect "invalid" environment variables and skip trying to export them.

I'm guessing implement something like this - https://stackoverflow.com/a/2821183

scoheb · 2017-10-17T18:36:18Z

I would vote for fixing them rather than skipping them...that way at least users can see what has happened by doing an 'env'...or we log warnings if we skip them.

How about this: https://paste.fedoraproject.org/paste/6vTS7Gv4RD9qBY~VkA8EMg

MattLud · 2017-10-17T18:46:39Z

Fixing it is fine by me, though the current behavior on a regular executor is to skip that environment variable.

scoheb · 2017-10-17T18:49:18Z

@MattLud

In that case, let's go with what the current behavior is!

marvinthepa · 2017-10-19T08:10:32Z

Related issues:

https://issues.jenkins-ci.org/browse/JENKINS-47225
https://issues.jenkins-ci.org/browse/JENKINS-42582

MattLud · 2017-10-24T20:59:36Z

Fixed that broken build - Let me know if there's anything else this PR needs!

marvinthepa · 2017-10-25T15:14:04Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecorator.java

-                            ).getBytes(StandardCharsets.UTF_8)
-                    );
-                }
+                    //Check that key is bash compliant.


I personally would go "uncle bob style" and extract a method with a nice name instead of commenting:

if (isLegalShellVariableName(entry.getKey()) { ...

Comments are very prone to code rot.

marvinthepa · 2017-10-30T13:59:21Z

@MattLud:

There seems to be something wrong with this (which I unfortunately only recognized AFTER I merged this).
Seems like the exit code is not picked up correctly:

Executing shell script inside container [ssh-client] of pod [jenkins-slave-r7hwz-mlgt4]
Executing command: "ssh-agent" 
printf "EXITCODE %3d" $?; exit
SSH_AUTH_SOCK=/tmp/ssh-s0bZApqhjHuY/agent.15; export SSH_AUTH_SOCK;
SSH_AGENT_PID=16; export SSH_AGENT_PID;
echo Agent pid 16;
EXITCODE   0SSH_AUTH_SOCK=/tmp/ssh-s0bZApqhjHuY/agent.15
SSH_AGENT_PID=16
Executing shell script inside container [ssh-client] of pod [jenkins-slave-r7hwz-mlgt4]
Executing command: "ssh-add" "/home/jenkins/workspace/empty@tmp/private_key_5661184397489639826.key" 
printf "EXITCODE %3d" $?; exit
EXITCODE   0EXITCODE   0Identity added: /home/jenkins/workspace/empty@tmp/private_key_5661184397489639826.key (/home/jenkins/workspace/empty@tmp/private_key_5661184397489639826.key)
Identity added: /home/jenkins/workspace/empty@tmp/private_key_5661184397489639826.key (/home/jenkins/workspace/empty@tmp/private_key_5661184397489639826.key)
[Pipeline] // sshagent
[Pipeline] }
[Pipeline] // container
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // node
[Pipeline] }
[Pipeline] // podTemplate
[Pipeline] End of Pipeline
ERROR: Failed to run ssh-add
Finished: FAILURE

MattLud · 2017-10-30T15:05:06Z

Odd. Is this with the existing test & key or another key? If it's another key, can you send a gist of it to test out?

…

On Oct 30, 2017 8:59 AM, "Martin Sander" ***@***.***> wrote: @MattLud <https://github.com/mattlud>: There seems to be something wrong with this (which I unfortunately only recognized AFTER I merged this). Seems like the exit code is not picked up correctly: Executing shell script inside container [ssh-client] of pod [jenkins-slave-r7hwz-mlgt4] Executing command: "ssh-agent" printf "EXITCODE %3d" $?; exit SSH_AUTH_SOCK=/tmp/ssh-s0bZApqhjHuY/agent.15; export SSH_AUTH_SOCK; SSH_AGENT_PID=16; export SSH_AGENT_PID; echo Agent pid 16; EXITCODE 0SSH_AUTH_SOCK=/tmp/ssh-s0bZApqhjHuY/agent.15 SSH_AGENT_PID=16 Executing shell script inside container [ssh-client] of pod [jenkins-slave-r7hwz-mlgt4] Executing command: "ssh-add" ***@***.***/private_key_5661184397489639826.key" printf "EXITCODE %3d" $?; exit EXITCODE 0EXITCODE 0Identity added: ***@***.***/private_key_5661184397489639826.key ***@***.***/private_key_5661184397489639826.key) Identity added: ***@***.***/private_key_5661184397489639826.key ***@***.***/private_key_5661184397489639826.key) [Pipeline] // sshagent [Pipeline] } [Pipeline] // container [Pipeline] } [Pipeline] // stage [Pipeline] } [Pipeline] // node [Pipeline] } [Pipeline] // podTemplate [Pipeline] End of Pipeline ERROR: Failed to run ssh-add Finished: FAILURE — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#236 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUwfWBjeUfpAxc2VUvr1rLE1C0EnCogks5sxdY7gaJpZM4P7DAC> .

MattLud · 2017-10-30T15:15:20Z

Also, what is the jnlp image/pod template being used?

marvinthepa · 2017-11-02T09:16:12Z

@MattLud: I used the pipeline and key from the test, but I was also able to reproduce using other keys.

My current suspicion is that the output of "ssh-agent" and the "printf "EXITCODE %3d" $?" (which is introduced by the kubernetes plugin) are interleaved (I don't know why).

I guess this will all change completely with #239

marvinthepa · 2017-11-02T09:16:44Z

#242 might be related as well

MattLud · 2017-11-09T23:17:16Z

Yep - you're right that they are interleaved - We finally bumped into the issue related to this and it seems to be race condition sensitive -

For failures, your log will show
EXITCODE 0EXITCODE 0Identity added: .....
but on successful builds it'll be

Identity added: ...
EXITCODE   0EXITCODE   0

MattLud · 2017-11-10T18:36:53Z

After further testing, including #242, I still could not find a fix.

Adding a sleep 1 to printf \"" + EXIT_COMMAND_TXT + " %3d\" $?; " + EXIT + NEWLINE; seems to have worked around the race condition for now - this was based on 940 test cycles with SSH agent that were originally failing every 2-3 tests.

balihb · 2017-11-21T14:07:53Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecorator.java

+
+                //check if the cmd is sourced from Jenkins, rather than another plugin; if so, skip cmdEnvs as we are getting other environment variables
+                for (String cmd : cmdEnvs) {
+                    if (cmd.startsWith(JENKINS_HOME)) {


why was this introduced? It is preventing environment variable injecting wrappers to inject env into sh steps.

From #232 - "One issue that cropped up was that when executing sh commands, we would get the jnlp agent's injected environment variables as well, causing obvious problems such as JAVA_HOME being overwritten. The unsatisfying answer was to check for the presence of JENKINS_HOME in the cmdenvs and skip if present."

might this help in this situation?
https://github.com/jenkinsci/kubernetes-plugin/pull/242/files#diff-8a1ab34452b01252e89ffeee0b2eda45R278

aroq · 2017-12-13T10:36:31Z

Guys, do you have any updates on the exicode issue above? I had to downgrade to the previous version of the plugin because of failed ssh-add.

balihb · 2017-12-13T11:24:34Z

I'll update my PR to te latest master today.

…

On Dec 13, 2017 11:36, "Aleksandr Tolstikov" ***@***.***> wrote: Guys, do you have any updates on the exicode issue above? I had to downgrade to the previous version of the plugin because of failed ssh-add. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#236 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACvMmN2AjVBfbI75FzOZxjWfYOF4AvYiks5s_6ixgaJpZM4P7DAC> .

MattLud and others added 3 commits October 16, 2017 12:59

JENKINS-47225: Add ProcStarter envs and listeners to be included for …

8f19fe1

…calling processes

Add test for proc starter

b0af782

add encrypted to ensure passphrase isn't leaked to logs by executor

ce034c3

Check proc env variable keys for bash compliance and skip if not

0e40d82

MattLud force-pushed the JENKINS-47225 branch from ffacd8d to 3d05e03 Compare October 20, 2017 19:52

Remove try/catch for IOException on exporting env

f77c947

MattLud force-pushed the JENKINS-47225 branch from 3d05e03 to f77c947 Compare October 24, 2017 20:15

marvinthepa reviewed Oct 25, 2017

View reviewed changes

marvinthepa approved these changes Oct 25, 2017

View reviewed changes

marvinthepa merged commit a2c450e into jenkinsci:master Oct 30, 2017

balihb reviewed Nov 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jenkins 47225: Fix ProcStarter #236

Jenkins 47225: Fix ProcStarter #236

MattLud commented Oct 16, 2017

marvinthepa commented Oct 16, 2017

MattLud commented Oct 16, 2017 via email

MattLud commented Oct 17, 2017

scoheb commented Oct 17, 2017

MattLud commented Oct 17, 2017

scoheb commented Oct 17, 2017

marvinthepa commented Oct 19, 2017

MattLud commented Oct 24, 2017

marvinthepa Oct 25, 2017

marvinthepa commented Oct 30, 2017

MattLud commented Oct 30, 2017 via email

MattLud commented Oct 30, 2017

marvinthepa commented Nov 2, 2017

marvinthepa commented Nov 2, 2017

MattLud commented Nov 9, 2017

MattLud commented Nov 10, 2017

balihb Nov 21, 2017

MattLud Nov 22, 2017

balihb Nov 22, 2017

aroq commented Dec 13, 2017

balihb commented Dec 13, 2017 via email

Jenkins 47225: Fix ProcStarter #236

Jenkins 47225: Fix ProcStarter #236

Conversation

MattLud commented Oct 16, 2017

marvinthepa commented Oct 16, 2017

MattLud commented Oct 16, 2017 via email

MattLud commented Oct 17, 2017

scoheb commented Oct 17, 2017

MattLud commented Oct 17, 2017

scoheb commented Oct 17, 2017

marvinthepa commented Oct 19, 2017

MattLud commented Oct 24, 2017

marvinthepa Oct 25, 2017

Choose a reason for hiding this comment

marvinthepa commented Oct 30, 2017

MattLud commented Oct 30, 2017 via email

MattLud commented Oct 30, 2017

marvinthepa commented Nov 2, 2017

marvinthepa commented Nov 2, 2017

MattLud commented Nov 9, 2017

MattLud commented Nov 10, 2017

balihb Nov 21, 2017

Choose a reason for hiding this comment

MattLud Nov 22, 2017

Choose a reason for hiding this comment

balihb Nov 22, 2017

Choose a reason for hiding this comment

aroq commented Dec 13, 2017

balihb commented Dec 13, 2017 via email