[JENKINS-48149] Add Pod Retention policies #354

adambkaplan · 2018-06-28T16:40:06Z

New configuration to control how the kubernetes-plugin deletes
slave build pods.

Support for cloud and pod template configurations in Jenkins UI
Groovy support for pipeline builds

OpenShift Bug 1515940

adambkaplan

Implementation details in comments
@carlossg ptal

adambkaplan · 2018-06-28T16:43:01Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java

+            } catch (NoSuchMethodException ex) {
+                return null;
+            }
+            e.setNoReconnect(true);


SlaveDisconnector tells the slave pod's JNLP agent to not attempt any further reconnects. Without this, the slave JNLP agent will fail after the Jenkins build completes, leading to a final Pod phase of Failed.

Again a code comment seems warranted here

adambkaplan · 2018-06-28T16:43:56Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java

+        if (template != null && template.getPodRetention() != PodRetention.DEFAULT) {
+            retentionPolicy = template.getPodRetention();
+        }
+        return retentionPolicy;


Pod retention policy can be overridden by the pod template. Defaults to the cloud-level setting.

adambkaplan · 2018-06-28T16:45:42Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java


+        boolean deletePod = shouldDeletePod(client, cloud);


Check if we should delete the pod before disconnecting the JNLP agent. At this point a healthy build will have a running pod with a JNLP agent running.

A code comment here seems warranted

adambkaplan · 2018-06-28T16:46:30Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java

+            deleteSlavePod(listener, client);
+        } else {
+            LOGGER.log(Level.WARNING, "Slave pod {0} was not deleted due to retention policy {1}.",
+                    new Object[] { name, getPodRetention(cloud) });


Log a warning if the slave pod was not deleted, as this may mean a running pod is still active and consuming resources.

adambkaplan · 2018-06-28T16:47:44Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java

+        PodStatus podStatus = getSlavePodStatus(client);
+        boolean slaveHasErrors  = false;
+        if (podStatus != null) {
+            slaveHasErrors = podStatus.getPhase().toLowerCase().matches("(failed|unknown)");


If a pod has the phase Failed or Unknown, it is in an error state.

adambkaplan · 2018-06-28T16:51:07Z

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

+    @DataBoundSetter
+    public void setPodRetention(PodRetention podRetention) {
+        this.podRetention = podRetention;
+    }


For Groovy input we do not care if the PodRetention value is null.

adambkaplan · 2018-06-28T16:52:10Z

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

-                    LOGGER.log(Level.WARNING, "Failed to delete pod for agent {0}/{1}: not found",
-                            new String[] { client.getNamespace(), podTemplate.getName() });
-                    return;
-                }


Do not delete the pod with the finished callback - this is taken care of by KubernetesSlave._terminate()

adambkaplan · 2018-06-28T16:54:09Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlaveTest.java

+    }
+
+    @Test
+    public void testShouldDeletePod() {


Unit test logic for deleting pods

adambkaplan · 2018-06-28T16:54:26Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlaveTest.java

@@ -56,4 +75,162 @@ private void assertRegex(String name, String regex) {
        assertNotNull(name);
        assertTrue(String.format("Name does not match regex [%s]: '%s'", regex, name), name.matches(regex));
    }
+
+    @Test
+    public void testGetPodRetention() {


Unit test pod retention for the slave

adambkaplan · 2018-06-28T16:54:45Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java

@@ -371,4 +371,14 @@ public void runWithActiveDeadlineSeconds() throws Exception {
        r.assertLogNotContains("Hello from container!", b);
    }

+    @Test
+    public void runInPodWithRetention() throws Exception {


Integration test using the pod retention setting

adambkaplan · 2018-06-28T17:00:17Z

@gabemontero may also want to take a look

gabemontero

Just a couple of minor items/questions in line.

Also, since I disconnected once you got off the ground with this, just so I'm clear, you backed away from a new RetentionStrategy implementation because of:
a) the JNLP reconnect thing ?
b) the once vs. cloud discrepancies made things cumbersome ?
c) it was just easier to code up ?
d) some combination of the above ?
e) something else ?

I'm curious as much as anything ... what you have integrated in just fine, but am curious of @carlossg or others on the jenkinsci side think this logic needs to be encapsulated there

Nice work :-) !

gabemontero · 2018-06-28T17:10:52Z

README.md

@@ -130,7 +130,8 @@ Either way it provides access to the following fields:
 * **annotations** Annotations to apply to the pod.
 * **inheritFrom** List of one or more pod templates to inherit from *(more details below)*.
 * **slaveConnectTimeout** Timeout in seconds for an agent to be online.
-* **activeDeadlineSeconds** Pod is deleted after this deadline is passed.
+* **podRetention** Controls the behavior of keeping slave pods. Can be 'NEVER', 'ON_FAILURE', or 'ALWAYS' - if empty will default to the plugin's "Pod Retention" setting.


might be clearer to say ... if empty will default to deleting the pod after activeDeadlineSeconds has passed.

that's true if the cloud's setting isn't changed (which, granted, will cover most cases)

yep ... and mentioning the "inherit from the cloud setting" should be mentioned somehow in this doc as well I would think ... so combine the two pieces here

gabemontero · 2018-06-28T17:14:56Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java

-        }
+        PodList templateSlaveList = client.pods().inNamespace(templateNamespace).withLabels(labelsMap).list();
+        List<Pod> allTemplateSlavePods = templateSlaveList.getItems().stream()
+                .filter(x -> x.getStatus().getPhase().toLowerCase().matches("(running|pending)"))


the name allTemplateSlavePod implies to me that you would NOT filter based on phase .... can you reconcile either with removing the filter, changing the var name, comment explaining what is up, etc.

gabemontero · 2018-06-28T17:18:27Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java


+        boolean deletePod = shouldDeletePod(client, cloud);


A code comment here seems warranted

gabemontero · 2018-06-28T17:20:09Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java

+            } catch (NoSuchMethodException ex) {
+                return null;
+            }
+            e.setNoReconnect(true);


Again a code comment seems warranted here

adambkaplan · 2018-06-28T18:08:10Z

@gabemontero at the end of the day I felt the delete pod logic belonged within the KubernetesSlave class, and should run as a part of the node termination process. IMO a caller of node.terminate() expects the slave to shut down and clean up its resources. Using a separate RetentionStrategy would require the delete pod logic to be called externally of node.terminate()

gabemontero · 2018-06-28T18:23:48Z

On Thu, Jun 28, 2018 at 2:08 PM, Adam Kaplan ***@***.***> wrote: @gabemontero <https://github.com/gabemontero> at the end of the day I felt the delete pod logic belonged within the KubernetesSlave class, and should run as a part of the node termination process. IMO a caller of node.terminate() expects the slave to shut down and clean up its resources.

Yeah sounds fair. Admittedly as this has evolved and I've iterated over these classes, the separation of concerns between `RetentionStrategy` and `hudson.slaves.AbstractCloudSlave` had me wondering somewhat. Most likely I was being overly pessimistic in anticipating possible contention. "sgtm"

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#354 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADbadEegLuirzwHTS9TRaeeP2A_KayIsks5uBRuPgaJpZM4U7t49> .

gabemontero

/lgtm

Vlatombe

The implementation looks good to me.

I would prefer using Describable/Descriptor though for PodRetention in order to make the system open for future extensions and keep the logic self-contained.

Vlatombe · 2018-07-02T13:00:27Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java

@@ -87,6 +87,8 @@
    /** Default timeout for idle workers that don't correctly indicate exit. */
    private static final int DEFAULT_RETENTION_TIMEOUT_MINUTES = 5;

+    static final PodRetention DEFAULT_POD_RETENTION_POLICY = PodRetention.NEVER;


@VisibleForTesting

Vlatombe · 2018-07-02T13:08:50Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodRetention.java

@@ -0,0 +1,15 @@
+package org.csanchez.jenkins.plugins.kubernetes;
+
+public enum PodRetention {


More Jenkins-y to use a Describable/Descriptor pattern here instead. Move implementation of shouldDeletePod in the Describable class so that you can implement additional behaviours through extensions.

Vlatombe · 2018-07-02T13:16:39Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java

+            }
+            // Ensure the slave supports setNoReconnect
+            try {
+                Engine.class.getMethod("setNoReconnect", boolean.class);


The method has been there since 2009. I don't think this check is worth it. jenkinsci/remoting@e43e787

adambkaplan · 2018-07-09T20:32:29Z

@Vlatombe Is there more recent documentation on selecting Describable/Descriptor instances from the web UI? I've found these:

I could also use guidance on supporting "static" instance types via the groovy DSL

adambkaplan · 2018-07-16T16:59:23Z

@Vlatombe refactored as Describable/Descriptor - ptal

adambkaplan · 2018-07-26T14:44:33Z

@Vlatombe have you had a chance to look at my updates?

Vlatombe · 2018-07-26T14:46:03Z

I'm sorry, I got swamped in other work. I'll take a look now

Vlatombe

Looks good. Some code could be pulled up in PodRetention

Vlatombe · 2018-07-26T14:47:19Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java

    @DataBoundConstructor
    public PodTemplate() {
    }

-    public PodTemplate(PodTemplate from) {
+	public PodTemplate(PodTemplate from) {


broken indentation

Vlatombe · 2018-07-26T14:49:05Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Always.java

+
+    @Override
+    public String toString() {
+        return "Always";


Maybe getDescriptor().getDisplayName() instead ? Could be defined in the parent class

Vlatombe · 2018-07-26T14:49:16Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Default.java

+
+    private static final long serialVersionUID = -5209499689925746138L;
+
+	@DataBoundConstructor


Indentation

adambkaplan · 2018-07-26T15:53:35Z

Updated & squashed. I could pull up the hashCode override as well, but I feel that would break standard Java conventions.

Bad indents came from my IDE using tabs ☹️

adambkaplan · 2018-07-27T12:03:18Z

@carlossg ok to merge?

New configuration to control how the kuberenetes-plugin deletes slave build pods. * Support for cloud and pod template configurations in Jenkins UI * Groovy support for pipeline builds * Describable implementation for future extensions OpenShift Bug 1515940

adambkaplan · 2018-07-31T14:23:09Z

@carlossg @Vlatombe rebased to address merge conflicts. ptal.

vickychijwani · 2018-08-01T18:06:20Z

Does this PR also address the following point from JENKINS-48149?

It'd be really nice if this log [Reason: OOMKilled] could appear in the build log as well as/instead of the Jenkins master log.

Probably not, but just want to confirm as we have a large-ish setup (~1000 builds/day) and run into OOMKilled often, which usually manifests as a ChannelClosedException.

carlossg · 2018-08-01T18:12:00Z

no, getting it into the logs is a bit harder

adambkaplan · 2018-08-01T21:32:56Z

no, getting it into the logs is a bit harder

more like "almost impossible" as kubernetes OOM leads to SIGKILL being sent to the offending container process. This OOM should not be confused with a JVM OutOfMemoryError - this is an ongoing challenge for Java applications running on Kubernetes.

If the kubelet kills the container because it exceeds its resource quota, OOMKilled will show up in the Pod data as the reason for the pod/container's current status.

dioss-Machiel · 2018-08-03T07:38:47Z

After installing the latest version on Jenkins with this change all my pods were marked as
Pod Retention: Always which was a bit unexpected.

carlossg · 2018-08-03T09:33:04Z

see #362

jglick · 2021-08-17T18:09:15Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Default.java

+    }
+
+    @Extension
+    @Symbol("default")


Not a valid symbol as per https://github.com/jenkinsci/structs-plugin/blob/a7765136112ec72dc2d686d3a958fd010e47e07a/annotation/src/main/java/org/jenkinsci/Symbol.java#L32 https://docs.groovy-lang.org/latest/html/documentation/#_keywords though it does not much matter since as the default value you would not explicitly select it in a script anyway.

adambkaplan commented Jun 28, 2018

View reviewed changes

adambkaplan mentioned this pull request Jun 28, 2018

Jenkins Slave Pod Retention Proposal #347

Closed

gabemontero suggested changes Jun 28, 2018

View reviewed changes

carlossg requested a review from Vlatombe June 28, 2018 18:12

gabemontero approved these changes Jun 28, 2018

View reviewed changes

adambkaplan mentioned this pull request Jun 29, 2018

[3.11] Pod Retention for Jenkins Pipeline Builds openshift/openshift-docs#10536

Merged

Vlatombe reviewed Jul 2, 2018

View reviewed changes

adambkaplan force-pushed the JENKINS-48149 branch 2 times, most recently from 80a5b1b to 70becc2 Compare July 16, 2018 15:02

Vlatombe reviewed Jul 26, 2018

View reviewed changes

adambkaplan force-pushed the JENKINS-48149 branch from 3c5adb1 to 6b50978 Compare July 26, 2018 15:50

Vlatombe approved these changes Jul 26, 2018

View reviewed changes

adambkaplan force-pushed the JENKINS-48149 branch from 6b50978 to 0cee31c Compare July 31, 2018 14:22

Vlatombe approved these changes Jul 31, 2018

View reviewed changes

carlossg changed the title ~~[JENKINS-48149] Slave Pod Retention~~ [JENKINS-48149] Add Pod Retention policies Jul 31, 2018

carlossg merged commit 65ae56a into jenkinsci:master Jul 31, 2018

adambkaplan deleted the JENKINS-48149 branch August 1, 2018 21:33

jglick reviewed Aug 17, 2021

View reviewed changes

		@@ -0,0 +1,15 @@
		package org.csanchez.jenkins.plugins.kubernetes;

		public enum PodRetention {


		private static final long serialVersionUID = -5209499689925746138L;

		@DataBoundConstructor

[JENKINS-48149] Add Pod Retention policies #354

[JENKINS-48149] Add Pod Retention policies #354

Uh oh!

Conversation

adambkaplan commented Jun 28, 2018 • edited by Vlatombe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Jun 28, 2018

Uh oh!

gabemontero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabemontero commented Jun 28, 2018 via email

Uh oh!

gabemontero left a comment

Choose a reason for hiding this comment

Uh oh!

Vlatombe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Jul 9, 2018

Uh oh!

adambkaplan commented Jul 16, 2018

Uh oh!

adambkaplan commented Jul 26, 2018

Uh oh!

Vlatombe commented Jul 26, 2018

Uh oh!

Vlatombe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Jun 28, 2018 •

edited by Vlatombe

Loading

adambkaplan commented Jun 28, 2018 •

edited

Loading

Vlatombe left a comment •

edited

Loading

adambkaplan commented Aug 1, 2018 •

edited

Loading