-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Added new retention policy "onJobFailure" #1265
base: master
Are you sure you want to change the base?
Conversation
This new retention policy prevents the deletion of a pod in case of a failed build result by the job that was using the pod. This is useful for debugging failed jobs because you are still able to open a terminal in any pod and investigate log files or similar, without having to set the retention policy to 'Always' or using workarounds like long timeouts.
Any update on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea, but it needs more work before it can be merged.
Also consider adding an integration test under KubernetesPipelineTest
package org.csanchez.jenkins.plugins.kubernetes.pod.retention; | ||
|
||
import hudson.Extension; | ||
import hudson.model.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please set your IDE to avoid wildcard imports.
// small convenience function | ||
private void LOG(Level level, String message) { | ||
LOGGER.log(level, () -> MODULENAME + ": " + message); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded, the class name is already printed as part of the standard formatter.
@Override | ||
public boolean shouldDeletePod(KubernetesCloud cloud, Pod pod) { | ||
if (cloud == null || pod == null) { | ||
LOG(Level.INFO, "shouldDeletePod called without actual cloud and pod"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Demote all this logging to FINE.
Jenkins jenkins = Jenkins.getInstanceOrNull(); | ||
if (jenkins == null) { | ||
LOG(Level.INFO, "Couldn't get the current Jenkins reference"); | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jenkins jenkins = Jenkins.getInstanceOrNull(); | |
if (jenkins == null) { | |
LOG(Level.INFO, "Couldn't get the current Jenkins reference"); | |
return true; | |
} | |
Jenkins jenkins = Jenkins.get(); |
if (parts.length < 3) { | ||
LOG(Level.INFO, "runUrl has unknown format: " + runUrl); | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way too simplistic. If you need to look up the run, I would advise instead to add new annotations (one for the item full name, another for the build id)
Conflict with #1304 |
@@ -6,6 +6,7 @@ | |||
<ol> | |||
<li>Never - always delete the agent pod.</li> | |||
<li>On Failure - keep the agent pod if it fails during the build.</li> | |||
<li>On Job Failure - keep the agent pod if the build itself fails.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should rather be called On Build Failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in Jenkins terminology fails means FAILURE
—the build did not run to normal completion. UNSTABLE
(ran to completion but with test failures) would be considered “successful”. If you mean to use the current impl (checking for SUCCESS
) then be clear that an unstable build will also retain the pod.
This new retention policy
onJobFailure
prevents the deletion of a pod in case of a failed build result by the job that was using the pod. This is useful for debugging failed jobs because you are still able to open a terminal in any pod and investigate log files or similar, without having to set the retention policy to 'Always' or using workarounds like long timeouts.The logic is comparatively simple: Use the
runUrl
pod annotation to query the particular run from Jenkins and check its result. Sometimes the result isn't available yet, in which case we wait up to 30 seconds.To prevent having to import Jenkins classes into the test code, I'm only testing the utility methods.
This fixes JENKINS-688969