Prevent unneeded exec operations #239
Conversation
|
working well in production now for 3 days... |
| public ContainerExecDecorator(KubernetesClient client, String podName, String containerName, String namespace, EnvironmentExpander environmentExpander) { | ||
| private final FilePath ws; | ||
|
|
||
| public ContainerExecDecorator(KubernetesClient client, String podName, String containerName, String namespace, EnvironmentExpander environmentExpander, FilePath ws) { |
There was a problem hiding this comment.
can we keep binary backwards compatibility ?
| */ | ||
| public ContainerExecProc(ExecWatch watch, AtomicBoolean alive, CountDownLatch finished, | ||
| Callable<Integer> exitCode) { | ||
| public ContainerExecProc(int pid, ExecWatch watch, AtomicBoolean alive, CountDownLatch finished, |
There was a problem hiding this comment.
We can! Do we need to though? Do we expect any external code calling these?
There was a problem hiding this comment.
I'd rather keep it unless annotated with Restricted just in case
There was a problem hiding this comment.
old method needs to be added and deprecated
|
Any progress on this @iocanel ? |
|
@carlossg Can you take a look at this please? |
|
Concerns have been addressed, fixed the conflicts and rebased again! |
|
@carlossg: Can you take an other look please? |
|
Still one backwards compatibility change needed and findbugs fixes |
Restrict external uses of ContainerExecProc
|
|
||
| public ContainerExecProc(int pid, ExecWatch watch, AtomicBoolean alive, CountDownLatch finished, | ||
| Callable<Integer> exitCode) { | ||
| this.pid = pid; |
There was a problem hiding this comment.
@iocanel I don't see where the pid is used
There was a problem hiding this comment.
Right! It seems that this wasn't used after all!
pid is not needed after all
|
|
||
| private synchronized int readPidFromPidFile(String... commands) throws IOException, InterruptedException { | ||
| int pid = -1; | ||
| FilePath pidFile = ws.child(readPidFile(commands)); |
There was a problem hiding this comment.
@iocanel readPidFile can return null and cause NPE
11:43:41 java.lang.NullPointerException
11:43:41 at hudson.FilePath.isAbsolute(FilePath.java:281)
11:43:41 at hudson.FilePath.resolvePathIfRelative(FilePath.java:266)
11:43:41 at hudson.FilePath.<init>(FilePath.java:262)
11:43:41 at hudson.FilePath.child(FilePath.java:1273)
11:43:41 at org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecDecorator.readPidFromPidFile(ContainerExecDecorator.java:467)
11:43:41 at org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecDecorator.access$1000(ContainerExecDecorator.java:69)
11:43:41 at org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecDecorator$1.doLaunch(ContainerExecDecorator.java:335)
11:43:41 at org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecDecorator$1.launch(ContainerExecDecorator.java:147)
So the idea is to try avoiding exec operations invoked via ProcessLiveness, by reusing the ContainerExecProc object or by creating a fake one.
For instance:
ps -o -pid <pid>can receive a process that uses the actual ContainerExecProc to determine if its still running.ps -o -pid 9999can receive a fake that always returns an error response code.This way we avoid uneeded exec to the pod.