From 685fa8b5cf4db1c9b02920d80660d336d6488dfc Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 31 Jan 2023 18:14:10 -0500 Subject: [PATCH 1/2] Do not ask API server for a `Pod` during deletion unnecessarily --- .../plugins/kubernetes/KubernetesSlave.java | 4 ++-- .../kubernetes/pod/retention/Always.java | 3 ++- .../kubernetes/pod/retention/Default.java | 3 ++- .../kubernetes/pod/retention/Never.java | 3 ++- .../kubernetes/pod/retention/OnFailure.java | 13 ++++++++++- .../pod/retention/PodRetention.java | 3 ++- .../pod/retention/PodRetentionTest.java | 22 ++++++++++--------- 7 files changed, 34 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java index 0a6731b45e..90a5d12369 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java @@ -310,8 +310,7 @@ protected void _terminate(TaskListener listener) throws IOException, Interrupted // Prior to termination, determine if we should delete the slave pod based on // the slave pod's current state and the pod retention policy. // Healthy slave pods should still have a JNLP agent running at this point. - Pod pod = client.pods().inNamespace(getNamespace()).withName(name).get(); - boolean deletePod = getPodRetention(cloud).shouldDeletePod(cloud, pod); + boolean deletePod = getPodRetention(cloud).shouldDeletePod(cloud, () -> client.pods().inNamespace(getNamespace()).withName(name).get()); Computer computer = toComputer(); if (computer == null) { @@ -367,6 +366,7 @@ private void deleteSlavePod(TaskListener listener, KubernetesClient client) thro e.getMessage()); LOGGER.log(Level.WARNING, msg, e); listener.error(msg); + // TODO should perhaps retry later, in case API server is just overloaded currently return; } diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Always.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Always.java index 4d2374d030..93c7d82699 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Always.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Always.java @@ -8,6 +8,7 @@ import hudson.Extension; import io.fabric8.kubernetes.api.model.Pod; +import java.util.function.Supplier; public class Always extends PodRetention implements Serializable { @@ -19,7 +20,7 @@ public Always() { } @Override - public boolean shouldDeletePod(KubernetesCloud cloud, Pod pod) { + public boolean shouldDeletePod(KubernetesCloud cloud, Supplier pod) { return false; } diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Default.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Default.java index 56e66d4ec3..f413a8d69f 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Default.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Default.java @@ -10,6 +10,7 @@ import hudson.model.Descriptor; import hudson.model.DescriptorVisibilityFilter; import io.fabric8.kubernetes.api.model.Pod; +import java.util.function.Supplier; public class Default extends PodRetention implements Serializable { @@ -21,7 +22,7 @@ public Default() { } @Override - public boolean shouldDeletePod(KubernetesCloud cloud, Pod pod) { + public boolean shouldDeletePod(KubernetesCloud cloud, Supplier pod) { PodRetention parent = cloud.getPodRetention(); if (!(parent instanceof Default)) { return parent.shouldDeletePod(cloud, pod); diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Never.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Never.java index 24d6716a02..6db7af7fbc 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Never.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Never.java @@ -8,6 +8,7 @@ import hudson.Extension; import io.fabric8.kubernetes.api.model.Pod; +import java.util.function.Supplier; public class Never extends PodRetention implements Serializable { @@ -19,7 +20,7 @@ public Never() { } @Override - public boolean shouldDeletePod(KubernetesCloud cloud, Pod pod) { + public boolean shouldDeletePod(KubernetesCloud cloud, Supplier pod) { return true; } diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/OnFailure.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/OnFailure.java index 11491361e5..594b572f01 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/OnFailure.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/OnFailure.java @@ -8,18 +8,29 @@ import hudson.Extension; import io.fabric8.kubernetes.api.model.Pod; +import java.util.function.Supplier; +import java.util.logging.Level; +import java.util.logging.Logger; public class OnFailure extends PodRetention implements Serializable { private static final long serialVersionUID = 6424267627207206819L; + private static final Logger LOGGER = Logger.getLogger(OnFailure.class.getName()); + @DataBoundConstructor public OnFailure() { } @Override - public boolean shouldDeletePod(KubernetesCloud cloud, Pod pod) { + public boolean shouldDeletePod(KubernetesCloud cloud, Supplier podS) { + Pod pod = null; + try { + pod = podS.get(); + } catch (RuntimeException x) { + LOGGER.log(Level.WARNING, null, x); + } if (pod == null || pod.getStatus() == null) { return false; } diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetention.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetention.java index 702cfaecbf..2610cc809f 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetention.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetention.java @@ -5,6 +5,7 @@ import hudson.ExtensionPoint; import hudson.model.AbstractDescribableImpl; import io.fabric8.kubernetes.api.model.Pod; +import java.util.function.Supplier; /** * PodRetention instances determine if the Kubernetes pod running a Jenkins agent @@ -41,7 +42,7 @@ public static PodRetention getPodTemplateDefault() { * * @return true if the agent pod should be deleted. */ - public abstract boolean shouldDeletePod(KubernetesCloud cloud, Pod pod); + public abstract boolean shouldDeletePod(KubernetesCloud cloud, Supplier pod); @Override public String toString() { diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetentionTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetentionTest.java index e5a30157a3..1465b1a2eb 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetentionTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetentionTest.java @@ -9,11 +9,13 @@ import io.fabric8.kubernetes.api.model.Pod; import io.fabric8.kubernetes.api.model.PodStatus; import io.fabric8.kubernetes.api.model.PodStatusBuilder; +import java.util.function.Supplier; public class PodRetentionTest { private KubernetesCloud cloud; private Pod pod; + private Supplier podS = () -> pod; @Before public void setUp() { @@ -24,39 +26,39 @@ public void setUp() { @Test public void testAlwaysPodRetention() { PodRetention subject = new Always(); - assertFalse(subject.shouldDeletePod(cloud, pod)); + assertFalse(subject.shouldDeletePod(cloud, podS)); } @Test public void testNeverPodRetention() { PodRetention subject = new Never(); - assertTrue(subject.shouldDeletePod(cloud, pod)); + assertTrue(subject.shouldDeletePod(cloud, podS)); } @Test public void testDefaultPodRetention() { PodRetention subject = new Default(); cloud.setPodRetention(new Always()); - assertFalse(subject.shouldDeletePod(cloud, pod)); + assertFalse(subject.shouldDeletePod(cloud, podS)); cloud.setPodRetention(new Never()); - assertTrue(subject.shouldDeletePod(cloud, pod)); + assertTrue(subject.shouldDeletePod(cloud, podS)); cloud.setPodRetention(new Default()); - assertTrue(subject.shouldDeletePod(cloud, pod)); + assertTrue(subject.shouldDeletePod(cloud, podS)); cloud.setPodRetention(null); - assertTrue(subject.shouldDeletePod(cloud, pod)); + assertTrue(subject.shouldDeletePod(cloud, podS)); } @Test public void testOnFailurePodRetention() { PodRetention subject = new OnFailure(); pod.setStatus(buildStatus("Failed")); - assertFalse(subject.shouldDeletePod(cloud, pod)); + assertFalse(subject.shouldDeletePod(cloud, podS)); pod.setStatus(buildStatus("Unknown")); - assertFalse(subject.shouldDeletePod(cloud, pod)); + assertFalse(subject.shouldDeletePod(cloud, podS)); pod.setStatus(buildStatus("Running")); - assertTrue(subject.shouldDeletePod(cloud, pod)); + assertTrue(subject.shouldDeletePod(cloud, podS)); pod.setStatus(buildStatus("Pending")); - assertTrue(subject.shouldDeletePod(cloud, pod)); + assertTrue(subject.shouldDeletePod(cloud, podS)); } private PodStatus buildStatus(String phase) { From cd43a3cba09a2223b0cd4bfb46b03549d83dc3b8 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 31 Jan 2023 18:33:04 -0500 Subject: [PATCH 2/2] `jdk11` branch may fail spuriously, so `retry` --- Jenkinsfile | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Jenkinsfile b/Jenkinsfile index c28484b892..b3e15ce2fe 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -42,12 +42,14 @@ stage('Tests') { } } branches['jdk11'] = { - node('maven-11') { - timeout(60) { - checkout scm - sh 'mvn -B -ntp -Dset.changelist -Dmaven.test.failure.ignore clean install' - infra.prepareToPublishIncrementals() - junit 'target/surefire-reports/*.xml' + retry(count: 3, conditions: [kubernetesAgent(handleNonKubernetes: true), nonresumable()]) { + node('maven-11') { + timeout(60) { + checkout scm + sh 'mvn -B -ntp -Dset.changelist -Dmaven.test.failure.ignore clean install' + infra.prepareToPublishIncrementals() + junit 'target/surefire-reports/*.xml' + } } } }