[JENKINS-58975] - Update the working directory to match container working directory #610

narayanan · 2019-10-03T04:04:59Z

Modified working directory to match the container working directory before executing commands in a container
Added unit tests

…irectory

…s-plugin into Fix-Working-Dir-Exec

…irectory

…s-plugin into Fix-Working-Dir-Exec

Vlatombe

Looks promising

Vlatombe · 2019-10-03T09:20:18Z

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

+                    if (containerWorkingDir.isPresent() && ! containerWorkingDirFilePath.getRemote().equals(containerWorkingDir.toString())) {
+                        // Container has a custom workingDir, set pwd to the container workspace root
+                        containerWorkingDirStr = containerWorkingDir.get();
+                        containerWorkingDirFilePath = new FilePath(containerWorkingDirFilePath.getChannel(), containerWorkingDirStr);


Could be misleading, if the working directory has been changed using dir('foo'){...}, this will alter it to the container root working dir.

I think I'm missing something. What do you mean by container root working dir? How is it different from pwd?

if you have something like

container('foo') { dir('mydir') { sh 'pwd' } }

Then mydir will be ignored with the current code.

Got it, I'll update the groovy as suggested in the other comment, that will cover this use case and modify the code accordingly.

What do we do if user has not set a dir(mydir)? In such case the pwd was pointing to /home/jenkins/agent which does not exist with in the container, should we change it in such a scenario?

I think it should always be interpreted relatively to the current working dir.

So if you have dir('foo'), pwd should be $WORKSPACE/foo.

Vlatombe · 2019-10-03T09:42:18Z

...esources/org/csanchez/jenkins/plugins/kubernetes/pipeline/declarativeCustomWorkingDir.groovy

+pipeline {
+    agent {
+        kubernetes {
+            defaultContainer 'maven'
+            yaml """
+metadata:
+  labels:
+    some-label: some-label-value
+    class: KubernetesDeclarativeAgentTest
+spec:
+  containers:
+  - name: jnlp
+    env:
+    - name: CONTAINER_ENV_VAR
+      value: jnlp      
+  - name: maven
+    image: maven:3.3.9-jdk-8-alpine
+    workingDir: /home/jenkins/wsp1
+    command:
+    - cat
+    tty: true
+    env:
+    - name: CONTAINER_ENV_VAR
+      value: maven
+"""
+        }
+    }
+
+    stages {
+        stage('Run maven') {
+            steps {
+                sh 'mvn -version'
+                sh "echo Workspace dir is ${pwd()}"
+                sh "echo \$WORKSPACE"
+            }
+        }
+    }
+}


I suggest the following changes to cover more ground. The java part should assert with working dirs.

diff --git a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/declarativeCustomWorkingDir.groovy b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/declarativeCustomWorkingDir.groovy index 72034c55..d4a84771 100644 --- a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/declarativeCustomWorkingDir.groovy +++ b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/declarativeCustomWorkingDir.groovy @@ -1,7 +1,6 @@ pipeline { agent { kubernetes { - defaultContainer 'maven' yaml """ metadata: labels: @@ -29,9 +28,15 @@ spec: stages { stage('Run maven') { steps { - sh 'mvn -version' - sh "echo Workspace dir is ${pwd()}" - sh "echo \$WORKSPACE" + dir('foo') { + sh 'echo [jnlp] current dir is $(pwd)' + sh 'echo [jnlp] WORKSPACE=$WORKSPACE' + container('maven') { + sh 'mvn -version' + sh 'echo [maven] current dir is $(pwd)' + sh 'echo [maven] WORKSPACE=$WORKSPACE' + } + } } } }

Thanks for the suggestion. I took the example groovy from the JENKINS bug. I'll enhance the test case.

narayanan · 2019-10-05T04:32:12Z

@Vlatombe I've updated the logic to address pwd. Please take a look.

Also, I ran maven build multiple times locally using the following command (same as in the jenkins file) against local minikube. I do not see test failure, so not sure why tests are failing in the Jenkins.

mvn -B -ntp clean install -Dmaven.test.skip=true;mvn -e -DconnectorHost=0.0.0.0 -Djenkins.host.address=192.168.99.1 test

narayanan · 2019-10-08T18:25:05Z

@Vlatombe Please review and provide your feedback when you get a chance.

Vlatombe

Another test with a custom workingDir for the jnlp would be good to cover the remaining case.

Vlatombe · 2019-10-09T08:26:01Z

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

+                    if (containerWorkingDir.isPresent() && ! containerWorkingDirFilePath.getRemote().startsWith(containerWorkingDirStr)) {
+                        // Container has a custom workingDir, updated the pwd to match container working dir
+                        containerWorkingDirFilePathStr = containerWorkingDirFilePath.getRemote().replaceFirst(
+                                ContainerTemplate.DEFAULT_WORKING_DIR, containerWorkingDirStr);


Won't work if the jnlp container uses a custom working directory.

@Vlatombe Setting a custom working directory for JNLP container runs into error much before invoking the kubernetes plugin to execute the shell script. Here is my observation

Jenkins always cosiders /home/jenkins/agent as the workspace directory for declarative pipelines. This is happening because KubernetesSlave sets remoteFs as default working dir in the following method. When this method gets invoked, YAML declaration of the pod was not yet parsed, so, there is no container so, always sets the default container working dir

public String getRemoteFs() { return getFirstContainer().map(ContainerTemplate::getWorkingDir).orElse(ContainerTemplate.DEFAULT_WORKING_DIR); }

In the build console output this can be clearly noticed. Following output is printed even when none of the containers use /home/jenkins/agent as working directory, basically this directory will not exist within any of the containers.
Running on declarative-custom-working-dir-1-0597g-xhtl4-dqklj in /home/jenkins/agent/workspace/declarative Custom Working Dir

When executing shell command, Jenkins tries to store the shell commands as a shell script under the remoteFs directory which will be /home/jenkins/agent for all declarative pipelines. But with in the JNLP container, workspace is mounted in a different directory, not in the /home/jenkins/agent. So creation of the shell script in the remote slave fails because that directory does not exist with in JNLP container. This shell script file creation happens with in Jenkins, only after creation of the Shell script control is passed to kubernetes plugin to execute the command.

So, my observation is, for declarative pipelines, JNLP container should always use /home/jenkins/agent as working directory. Declaring a different one will result in error. I think the right solution would be to set the remoteFs to the working directory of JNLP. But I think that involves a bigger refactoring as YAML has to be parsed prior to setting the remoteFs.

I think for now, we can treat JNLP containers not able to use a custom working dir as a limitation. Let me know your thoughts.

Looks good to me.

I think for now, we can treat JNLP containers not able to use a custom working dir as a limitation. Let me know your thoughts.

They used to be able to - and indeed when upgrading I am no longer able to and my pipelines using kaniko became broken - may or may not have been this change, but changing the workingDir back to the default works (ftr may or may not have been this change - but people with custom jnlp and container images expect to be able to set abitrary working directories) - especially given the field it is still exposed.

Vlatombe · 2019-10-09T08:41:49Z

@narayanan I merged master into your branch, I think it should fix the remaining tests in the CI.

narayanan · 2019-12-02T17:54:33Z

Another test with a custom workingDir for the jnlp would be good to cover the remaining case.

@Vlatombe Setting a custom working directory for JNLP container runs into error much before invoking the kubernetes plugin to execute the shell script. Here is my observation

Jenkins always cosiders /home/jenkins/agent as the workspace directory for declarative pipelines. This is happening because KubernetesSlave sets remoteFs as default working dir in the following method. When this method gets invoked, YAML declaration of the pod was not yet parsed, so, there is no container so, always sets the default container working dir

public String getRemoteFs() {
    return getFirstContainer().map(ContainerTemplate::getWorkingDir).orElse(ContainerTemplate.DEFAULT_WORKING_DIR);
}

In the build console output this can be clearly noticed. Following output is printed even when none of the containers use /home/jenkins/agent as working directory, basically this directory will not exist within any of the containers.
Running on declarative-custom-working-dir-1-0597g-xhtl4-dqklj in /home/jenkins/agent/workspace/declarative Custom Working Dir
When executing shell command, Jenkins tries to store the shell commands as a shell script under the remoteFs directory which will be /home/jenkins/agent for all declarative pipelines. But with in the JNLP container, workspace is mounted in a different directory, not in the /home/jenkins/agent. So creation of the shell script in the remote slave fails because that directory does not exist with in JNLP container. This shell script file creation happens with in Jenkins, only after creation of the Shell script control is passed to kubernetes plugin to execute the command.

So, my observation is, for declarative pipelines, JNLP container should always use /home/jenkins/agent as working directory. Declaring a different one will result in error. I think the right solution would be to set the remoteFs to the working directory of JNLP. But I think that involves a bigger refactoring as YAML has to be parsed prior to setting the remoteFs.

I think for now, we can treat JNLP containers not able to use a custom working dir as a limitation. Let me know your thoughts.

zerkms · 2019-12-19T23:13:14Z

Team, this PR triggers an exception:

00:03:31.526  java.lang.NullPointerException
00:03:31.526  	at org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecDecorator$1.launch(ContainerExecDecorator.java:267)

Details: https://issues.jenkins-ci.org/browse/JENKINS-60517

narayanan added 10 commits September 19, 2019 14:00

Rewrite working directory if the container uses a different working d…

b6f650f

…irectory

Rewrite working directory if the container uses a different working d…

543f904

…irectory

Merge branch 'Fix-Working-Dir-Exec' of github.com:narayanan/kubernete…

dff9610

…s-plugin into Fix-Working-Dir-Exec

Added unit tests

ce5fd04

Rewrite working directory if the container uses a different working d…

ce46f95

…irectory

Rewrite working directory if the container uses a different working d…

5871cea

…irectory

Added unit tests

e916f11

Merge branch 'Fix-Working-Dir-Exec' of github.com:narayanan/kubernete…

263c171

…s-plugin into Fix-Working-Dir-Exec

Fix imports

c3fa703

Fix imports

b47b7be

Vlatombe added the bug Bug Fixes label Oct 3, 2019

Vlatombe reviewed Oct 3, 2019

View reviewed changes

narayanan added 2 commits October 4, 2019 01:05

Modified pwd update logic

5d524e7

Fixed spotbug warning

0378d7a

Merge branch 'master' into Fix-Working-Dir-Exec

a2d894c

Vlatombe reviewed Oct 9, 2019

View reviewed changes

Merge branch 'master' into Fix-Working-Dir-Exec

d4fe42b

Vlatombe added ready enhancement Improvements and removed bug Bug Fixes labels Dec 13, 2019

Vlatombe merged commit cc58b4c into jenkinsci:master Dec 13, 2019

narayanan deleted the Fix-Working-Dir-Exec branch December 13, 2019 23:13

jglick mentioned this pull request Jan 21, 2022

Adds socketPath configuration to allow setting a predictable path jenkinsci/ssh-agent-plugin#62

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-58975] - Update the working directory to match container working directory #610

[JENKINS-58975] - Update the working directory to match container working directory #610

narayanan commented Oct 3, 2019

Vlatombe left a comment

Vlatombe Oct 3, 2019

narayanan Oct 3, 2019

Vlatombe Oct 3, 2019 •

edited

Loading

narayanan Oct 3, 2019

narayanan Oct 3, 2019

Vlatombe Oct 3, 2019

Vlatombe Oct 3, 2019

narayanan Oct 3, 2019

narayanan commented Oct 5, 2019

narayanan commented Oct 8, 2019

Vlatombe left a comment

Vlatombe Oct 9, 2019

narayanan Nov 29, 2019 •

edited

Loading

Vlatombe Dec 13, 2019

jtnord Jan 6, 2020 •

edited

Loading

Vlatombe commented Oct 9, 2019

narayanan commented Dec 2, 2019

zerkms commented Dec 19, 2019

[JENKINS-58975] - Update the working directory to match container working directory #610

[JENKINS-58975] - Update the working directory to match container working directory #610

Conversation

narayanan commented Oct 3, 2019

Vlatombe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vlatombe Oct 3, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narayanan commented Oct 5, 2019

narayanan commented Oct 8, 2019

Vlatombe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narayanan Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Vlatombe commented Oct 9, 2019

narayanan commented Dec 2, 2019

zerkms commented Dec 19, 2019

Vlatombe Oct 3, 2019 •

edited

Loading

narayanan Nov 29, 2019 •

edited

Loading

jtnord Jan 6, 2020 •

edited

Loading