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

Jump to bottom

Do not ask API server for a Pod during deletion unnecessarily #1304

Merged
merged 2 commits into from
Feb 1, 2023
Merged

Do not ask API server for a Pod during deletion unnecessarily #1304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
685fa8b
Do not ask API server for a `Pod` during deletion unnecessarily
jglick Jan 31, 2023
cd43a3c
`jdk11` branch may fail spuriously, so `retry`
jglick Jan 31, 2023
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
14 changes: 8 additions & 6 deletions Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()]) {
Copy link
Member

@Vlatombe Vlatombe Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

[offtopic] handleNonKubernetes: true feels weird, shouldn't the condition be renamed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well this is to handle the scenario on ci.jenkins.io where we are requesting a label which currently is handled by the kubernetes plugin but admins have left open the possibility of switching to some other cloud if performance merits it.

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'
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}

Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Always.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -19,7 +20,7 @@ public Always() {
}

@Override
public boolean shouldDeletePod(KubernetesCloud cloud, Pod pod) {
public boolean shouldDeletePod(KubernetesCloud cloud, Supplier<Pod> pod) {
return false;
}

Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Default.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -21,7 +22,7 @@ public Default() {
}

@Override
public boolean shouldDeletePod(KubernetesCloud cloud, Pod pod) {
public boolean shouldDeletePod(KubernetesCloud cloud, Supplier<Pod> pod) {
PodRetention parent = cloud.getPodRetention();
if (!(parent instanceof Default)) {
return parent.shouldDeletePod(cloud, pod);
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/Never.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -19,7 +20,7 @@ public Never() {
}

@Override
public boolean shouldDeletePod(KubernetesCloud cloud, Pod pod) {
public boolean shouldDeletePod(KubernetesCloud cloud, Supplier<Pod> pod) {
return true;
}

Expand Down
13 changes: 12 additions & 1 deletion src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/OnFailure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Pod> 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;
}
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetention.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import hudson.ExtensionPoint;
import hudson.model.AbstractDescribableImpl;
import io.fabric8.kubernetes.api.model.Pod;
import java.util.function.Supplier;

/**
* <code>PodRetention</code> instances determine if the Kubernetes pod running a Jenkins agent
Expand Down Expand Up @@ -41,7 +42,7 @@ public static PodRetention getPodTemplateDefault() {
*
* @return <code>true</code> if the agent pod should be deleted.
*/
public abstract boolean shouldDeletePod(KubernetesCloud cloud, Pod pod);
public abstract boolean shouldDeletePod(KubernetesCloud cloud, Supplier<Pod> pod);

@Override
public String toString() {
Expand Down
22 changes: 12 additions & 10 deletions src/test/java/org/csanchez/jenkins/plugins/kubernetes/pod/retention/PodRetentionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Pod> podS = () -> pod;

@Before
public void setUp() {
Expand All @@ -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) {
Expand Down