Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Dec 13, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b6f650f
Rewrite working directory if the container uses a different working d…
narayanan Sep 19, 2019
543f904
Rewrite working directory if the container uses a different working d…
narayanan Sep 19, 2019
dff9610
Merge branch 'Fix-Working-Dir-Exec' of github.com:narayanan/kubernete…
narayanan Sep 27, 2019
ce5fd04
Added unit tests
narayanan Oct 3, 2019
ce46f95
Rewrite working directory if the container uses a different working d…
narayanan Sep 19, 2019
5871cea
Rewrite working directory if the container uses a different working d…
narayanan Sep 19, 2019
e916f11
Added unit tests
narayanan Oct 3, 2019
263c171
Merge branch 'Fix-Working-Dir-Exec' of github.com:narayanan/kubernete…
narayanan Oct 3, 2019
c3fa703
Fix imports
narayanan Oct 3, 2019
b47b7be
Fix imports
narayanan Oct 3, 2019
5d524e7
Modified pwd update logic
narayanan Oct 4, 2019
0378d7a
Fixed spotbug warning
narayanan Oct 5, 2019
a2d894c
Merge branch 'master' into Fix-Working-Dir-Exec
Vlatombe Oct 9, 2019
d4fe42b
Merge branch 'master' into Fix-Working-Dir-Exec
narayanan Nov 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,8 @@ public Proc launch(ProcStarter starter) throws IOException {
// find container working dir
KubernetesSlave slave = (KubernetesSlave) node;
FilePath containerWorkingDirFilePath = starter.pwd();
String containerWorkingDirFilePathStr = containerWorkingDirFilePath != null
? containerWorkingDirFilePath.getRemote() : ContainerTemplate.DEFAULT_WORKING_DIR;
String containerWorkingDirStr = ContainerTemplate.DEFAULT_WORKING_DIR;
if (slave != null && slave.getPod().isPresent() && containerName != null) {
Optional<Container> container = slave.getPod().get().getSpec().getContainers().stream()
Expand All @@ -264,12 +266,17 @@ public Proc launch(ProcStarter starter) throws IOException {
if (container.isPresent() && container.get().getWorkingDir() != null) {
containerWorkingDir = Optional.of(container.get().getWorkingDir());
}
if (containerWorkingDir.isPresent() && ! containerWorkingDirFilePath.getRemote().equals(containerWorkingDir.toString())) {
// Container has a custom workingDir, set pwd to the container workspace root
if (containerWorkingDir.isPresent()) {
containerWorkingDirStr = containerWorkingDir.get();
containerWorkingDirFilePath = new FilePath(containerWorkingDirFilePath.getChannel(), containerWorkingDirStr);
}

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@narayanan narayanan Nov 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Member

@jtnord jtnord Jan 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

containerWorkingDirFilePath = new FilePath(containerWorkingDirFilePath.getChannel(), containerWorkingDirFilePathStr);
LOGGER.log(Level.FINEST, "Modified the pwd to match {0} containers workspace directory : {1}",
new String[]{containerName, containerWorkingDirStr});
new String[]{containerName, containerWorkingDirFilePathStr});
}
}

Expand Down Expand Up @@ -328,7 +335,7 @@ public Proc launch(ProcStarter starter) throws IOException {
}
}
return doLaunch(starter.quiet(), envVars, starter.stdout(), containerWorkingDirFilePath, starter.masks(),
getCommands(starter, containerWorkingDirStr));
getCommands(starter, containerWorkingDirFilePathStr));
}

private Proc doLaunch(boolean quiet, String[] cmdEnvs, OutputStream outputForCaller, FilePath pwd,
Expand Down Expand Up @@ -600,9 +607,11 @@ static String[] getCommands(Launcher.ProcStarter starter, String containerWorkin
// BourneShellScript.launchWithCookie escapes $ as $$, we convert it to \$
for (String cmd : starter.cmds()) {
String fixedCommand = cmd.replaceAll("\\$\\$", "\\\\\\$");
if (!ContainerTemplate.DEFAULT_WORKING_DIR.equals(containerWorkingDirStr)) {
String oldRemoteDir = starter.pwd() != null ? starter.pwd().getRemote() : null;
if (oldRemoteDir != null && ! oldRemoteDir.isEmpty() &&
!oldRemoteDir.equals(containerWorkingDirStr) && fixedCommand.contains(oldRemoteDir)) {
// Container has a custom workingDir, update the dir in commands
fixedCommand = fixedCommand.replaceAll(ContainerTemplate.DEFAULT_WORKING_DIR, containerWorkingDirStr);
fixedCommand = fixedCommand.replaceAll(oldRemoteDir, containerWorkingDirStr);
}
allCommands.add(fixedCommand);
}
Expand Down
1 change: 1 addition & 0 deletions ...est/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecoratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ public void testCommandExecutionWithNohup() throws Exception {
public void commandsEscaping() {
ProcStarter procStarter = new DummyLauncher(null).launch();
procStarter = procStarter.cmds("$$$$", "$$?");

String[] commands = ContainerExecDecorator.getCommands(procStarter, null);
assertArrayEquals(new String[] { "\\$\\$", "\\$?" }, commands);
}
Expand Down
8 changes: 6 additions & 2 deletions ...java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesDeclarativeAgentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,12 @@ public void declarativeCustomWorkspace() throws Exception {
public void declarativeCustomWorkingDir() throws Exception {
assertNotNull(createJobThenScheduleRun());
r.assertBuildStatusSuccess(r.waitForCompletion(b));
r.assertLogContains("Apache Maven 3.3.9", b);
r.assertLogContains("Workspace dir is", b);
r.assertLogContains("[jnlp] current dir is /home/jenkins/agent/workspace/declarative Custom Working Dir/foo", b);
r.assertLogContains("[jnlp] WORKSPACE=/home/jenkins/agent/workspace/declarative Custom Working Dir", b);
r.assertLogContains("[maven] current dir is /home/jenkins/wsp1/workspace/declarative Custom Working Dir/foo", b);
r.assertLogContains("[maven] WORKSPACE=/home/jenkins/wsp1/workspace/declarative Custom Working Dir", b);
r.assertLogContains("[default:maven] current dir is /home/jenkins/wsp1/workspace/declarative Custom Working Dir/foo", b);
r.assertLogContains("[default:maven] WORKSPACE=/home/jenkins/wsp1/workspace/declarative Custom Working Dir", b);
}

@Issue("JENKINS-57548")
Expand Down
16 changes: 13 additions & 3 deletions ...urces/org/csanchez/jenkins/plugins/kubernetes/pipeline/declarativeCustomWorkingDir.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,19 @@ spec:
stages {
stage('Run maven') {
steps {
sh 'mvn -version'
sh "echo Workspace dir is ${pwd()}"
sh "echo \$WORKSPACE"
dir('foo') {
container('jnlp') {
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'
}
sh 'echo [default:maven] current dir is $(pwd)'
sh 'echo [default:maven] WORKSPACE=$WORKSPACE'
}
}
}
}
Expand Down