From de6846bbe9044e6f5af56bb454f6f7227de90078 Mon Sep 17 00:00:00 2001 From: Carlos Sanchez Date: Thu, 10 Aug 2017 11:27:55 +0200 Subject: [PATCH 1/3] [JENKINS-45910] Print errors deleting pods to task log --- .../plugins/kubernetes/KubernetesSlave.java | 71 ++++++++++++------- 1 file changed, 45 insertions(+), 26 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 50ca2f21cd..6deb3b5a00 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java @@ -1,6 +1,10 @@ package org.csanchez.jenkins.plugins.kubernetes; import java.io.IOException; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.UnrecoverableKeyException; +import java.security.cert.CertificateEncodingException; import java.util.logging.Level; import java.util.logging.Logger; @@ -22,10 +26,8 @@ import hudson.slaves.JNLPLauncher; import hudson.slaves.OfflineCause; import hudson.slaves.RetentionStrategy; -import io.fabric8.kubernetes.api.model.DoneablePod; -import io.fabric8.kubernetes.api.model.Pod; import io.fabric8.kubernetes.client.KubernetesClient; -import io.fabric8.kubernetes.client.dsl.PodResource; +import io.fabric8.kubernetes.client.KubernetesClientException; import jenkins.model.Jenkins; /** @@ -131,35 +133,52 @@ protected void _terminate(TaskListener listener) throws IOException, Interrupted String msg = String.format("Cloud name is not set for agent, can't terminate: %s", name); LOGGER.log(Level.SEVERE, msg); listener.fatalError(msg); + computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline"))); + return; + } + + Cloud cloud = getCloud(); + if (cloud == null) { + String msg = String.format("Agent cloud no longer exists: %s", getCloudName()); + LOGGER.log(Level.WARNING, msg); + listener.fatalError(msg); + computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline"))); + return; + } + if (!(cloud instanceof KubernetesCloud)) { + String msg = String.format("Agent cloud is not a KubernetesCloud, something is very wrong: %s", + getCloudName()); + LOGGER.log(Level.SEVERE, msg); + listener.fatalError(msg); + computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline"))); + return; + } + KubernetesClient client; + try { + client = ((KubernetesCloud) cloud).connect(); + } catch (UnrecoverableKeyException | CertificateEncodingException | NoSuchAlgorithmException + | KeyStoreException e) { + String msg = String.format("Failed to connect to cloud %s", getCloudName()); + listener.fatalError(msg); + computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline"))); return; } try { - Cloud cloud = getCloud(); - if (cloud == null) { - String msg = String.format("Slave cloud no longer exists: %s", getCloudName()); - LOGGER.log(Level.WARNING, msg); - listener.fatalError(msg); - return; - } - if (!(cloud instanceof KubernetesCloud)) { - String msg = String.format("Slave cloud is not a KubernetesCloud, something is very wrong: %s", - getCloudName()); - LOGGER.log(Level.SEVERE, msg); - listener.fatalError(msg); - return; - } - KubernetesClient client = ((KubernetesCloud) cloud).connect(); - PodResource pods = client.pods().inNamespace(namespace).withName(name); - pods.delete(); - String msg = String.format("Terminated Kubernetes instance for slave %s", name); - LOGGER.log(Level.INFO, msg); - listener.getLogger().println(msg); + client.pods().inNamespace(namespace).withName(name).delete(); + } catch (KubernetesClientException e) { + String msg = String.format("Failed to delete pod for agent %s: %s", name, e.getMessage()); + LOGGER.log(Level.WARNING, msg, e); + listener.error(msg); computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline"))); - LOGGER.log(Level.INFO, "Disconnected computer {0}", name); - } catch (Exception e) { - LOGGER.log(Level.SEVERE, "Failed to terminate pod for slave " + name, e); + return; } + + String msg = String.format("Terminated Kubernetes instance for agent %s", name); + LOGGER.log(Level.INFO, msg); + listener.getLogger().println(msg); + computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline"))); + LOGGER.log(Level.INFO, "Disconnected computer {0}", name); } @Override From 6c6f3b474521e454ce7be14710e578bd3363c5c0 Mon Sep 17 00:00:00 2001 From: Carlos Sanchez Date: Thu, 10 Aug 2017 16:28:02 +0200 Subject: [PATCH 2/3] [JENKINS-45910] Delete pods in the cloud namespace when pod namespace is not defined Warn if pods have not been found, checking the correct way --- .../plugins/kubernetes/KubernetesSlave.java | 18 +++++++++++++----- .../pipeline/KubernetesPipelineTest.java | 3 +++ .../runWithOverriddenNamespace2.groovy | 1 + 3 files changed, 17 insertions(+), 5 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 6deb3b5a00..b46edb6db2 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java @@ -16,6 +16,7 @@ import org.kohsuke.stapler.DataBoundConstructor; import hudson.Extension; +import hudson.Util; import hudson.model.Computer; import hudson.model.Descriptor; import hudson.model.Label; @@ -82,9 +83,8 @@ public KubernetesSlave(PodTemplate template, String nodeDescription, String clou rs, template.getNodeProperties()); - // this.pod = pod; this.cloudName = cloudName; - this.namespace = template.getNamespace(); + this.namespace = Util.fixEmpty(template.getNamespace()); } public String getCloudName() { @@ -164,17 +164,25 @@ protected void _terminate(TaskListener listener) throws IOException, Interrupted return; } + String actualNamespace = getNamespace() == null ? client.getNamespace() : getNamespace(); try { - client.pods().inNamespace(namespace).withName(name).delete(); + Boolean delete = client.pods().inNamespace(actualNamespace).withName(name).delete(); + if (delete == null) { + String msg = String.format("Failed to delete pod for agent %s/%s: not found", actualNamespace, name); + LOGGER.log(Level.WARNING, msg); + listener.error(msg); + return; + } } catch (KubernetesClientException e) { - String msg = String.format("Failed to delete pod for agent %s: %s", name, e.getMessage()); + String msg = String.format("Failed to delete pod for agent %s/%s: %s", actualNamespace, name, + e.getMessage()); LOGGER.log(Level.WARNING, msg, e); listener.error(msg); computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline"))); return; } - String msg = String.format("Terminated Kubernetes instance for agent %s", name); + String msg = String.format("Terminated Kubernetes instance for agent %s/%s", actualNamespace, name); LOGGER.log(Level.INFO, msg); listener.getLogger().println(msg); computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline"))); diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java index fba10a6664..8e62cbbba6 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java @@ -246,6 +246,9 @@ public void runWithOverriddenNamespace() throws Exception { } @Test + /** + * Step namespace should have priority over anything else. + */ public void runWithOverriddenNamespace2() throws Exception { String overriddenNamespace = "kubernetes-plugin-overridden-namespace"; KubernetesClient client = cloud.connect(); diff --git a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/runWithOverriddenNamespace2.groovy b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/runWithOverriddenNamespace2.groovy index b126036ee5..9a45afc200 100644 --- a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/runWithOverriddenNamespace2.groovy +++ b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/runWithOverriddenNamespace2.groovy @@ -1,3 +1,4 @@ +// Step namespace should have priority over anything else. podTemplate(cloud: 'kubernetes-plugin-test', namespace: 'testns2', label: 'mypod', volumes: [emptyDirVolume(mountPath: '/my-mount')], containers: [ containerTemplate(name: 'jnlp', image: 'jenkinsci/jnlp-slave:2.62-alpine', args: '${computer.jnlpmac} ${computer.name}') ]) { From 53131665eae21301175da9cdaf1d89f1d5d63a21 Mon Sep 17 00:00:00 2001 From: Carlos Sanchez Date: Thu, 10 Aug 2017 18:10:53 +0200 Subject: [PATCH 3/3] [JENKINS-45910] Assert in tests that all pods are deleted --- .../plugins/kubernetes/KubernetesCloud.java | 11 +-- .../plugins/kubernetes/KubernetesSlave.java | 4 +- .../pipeline/PodTemplateStepExecution.java | 16 +++- .../kubernetes/KubernetesTestUtil.java | 86 ++++++++++++++++++- .../pipeline/ContainerExecDecoratorTest.java | 8 +- .../pipeline/KubernetesPipelineTest.java | 8 ++ 6 files changed, 115 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java index 0d2580c43a..e46ab0970c 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java @@ -35,6 +35,8 @@ import org.csanchez.jenkins.plugins.kubernetes.pipeline.PodTemplateStepExecution; import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume; import org.jenkinsci.plugins.durabletask.executors.OnceRetentionStrategy; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.QueryParameter; @@ -113,7 +115,7 @@ public class KubernetesCloud extends Cloud { .getProperty(PodTemplateStepExecution.class.getName() + ".defaultImage", "jenkinsci/jnlp-slave:alpine"); /** label for all pods started by the plugin */ - private static final Map POD_LABEL = ImmutableMap.of("jenkins", "slave"); + public static final Map DEFAULT_POD_LABELS = ImmutableMap.of("jenkins", "slave"); private static final String JNLPMAC_REF = "\\$\\{computer.jnlpmac\\}"; private static final String NAME_REF = "\\$\\{computer.name\\}"; @@ -324,8 +326,7 @@ public KubernetesClient connect() throws UnrecoverableKeyException, NoSuchAlgori new String[] { getDisplayName(), serverUrl }); client = new KubernetesFactoryAdapter(serverUrl, namespace, serverCertificate, credentialsId, skipTlsVerify, connectTimeout, readTimeout, maxRequestsPerHost).createClient(); - LOGGER.log(Level.FINE, "Connected to Kubernetes {0} URL {1}" + serverUrl, - new String[] { getDisplayName(), serverUrl }); + LOGGER.log(Level.FINE, "Connected to Kubernetes {0} URL {1}", new String[] { getDisplayName(), serverUrl }); return client; } @@ -497,7 +498,7 @@ private Pod getPodTemplate(KubernetesSlave slave, PodTemplate template) { private Map getLabelsMap(Set labelSet) { ImmutableMap.Builder builder = ImmutableMap. builder(); - builder.putAll(POD_LABEL); + builder.putAll(DEFAULT_POD_LABELS); if (!labelSet.isEmpty()) { for (LabelAtom label: labelSet) { builder.put(getIdForLabel(label), "true"); @@ -809,7 +810,7 @@ private boolean addProvisionedSlave(@Nonnull PodTemplate template, @CheckForNull templateNamespace = client.getNamespace(); } - PodList slaveList = client.pods().inNamespace(templateNamespace).withLabels(POD_LABEL).list(); + PodList slaveList = client.pods().inNamespace(templateNamespace).withLabels(DEFAULT_POD_LABELS).list(); List slaveListItems = slaveList.getItems(); Map labelsMap = getLabelsMap(template.getLabelSet()); 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 b46edb6db2..ba7274a116 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java @@ -166,8 +166,8 @@ protected void _terminate(TaskListener listener) throws IOException, Interrupted String actualNamespace = getNamespace() == null ? client.getNamespace() : getNamespace(); try { - Boolean delete = client.pods().inNamespace(actualNamespace).withName(name).delete(); - if (delete == null) { + Boolean deleted = client.pods().inNamespace(actualNamespace).withName(name).delete(); + if (!Boolean.TRUE.equals(deleted)) { String msg = String.format("Failed to delete pod for agent %s/%s: not found", actualNamespace, name); LOGGER.log(Level.WARNING, msg); listener.error(msg); diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java index c974e0c755..8d2301f694 100755 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java @@ -15,6 +15,7 @@ import hudson.AbortException; import hudson.model.Run; import hudson.slaves.Cloud; +import io.fabric8.kubernetes.client.KubernetesClient; import jenkins.model.Jenkins; public class PodTemplateStepExecution extends AbstractStepExecutionImpl { @@ -124,14 +125,25 @@ private PodTemplateCallback(PodTemplate podTemplate) { protected void finished(StepContext context) throws Exception { Cloud cloud = Jenkins.getInstance().getCloud(step.getCloud()); if (cloud == null) { - LOGGER.log(Level.FINE, "Cloud {0} no longer exists, cannot delete pod template {1}", + LOGGER.log(Level.WARNING, "Cloud {0} no longer exists, cannot delete pod template {1}", new Object[] { step.getCloud(), podTemplate.getName() }); return; } if (cloud instanceof KubernetesCloud) { + LOGGER.log(Level.INFO, "Removing pod template and deleting pod {1} from cloud {0}", + new Object[] { cloud.name, podTemplate.getName() }); KubernetesCloud kubernetesCloud = (KubernetesCloud) cloud; kubernetesCloud.removeTemplate(podTemplate); - kubernetesCloud.connect().pods().withName(podTemplate.getName()).delete(); + KubernetesClient client = kubernetesCloud.connect(); + Boolean deleted = client.pods().withName(podTemplate.getName()).delete(); + if (!Boolean.TRUE.equals(deleted)) { + LOGGER.log(Level.WARNING, "Failed to delete pod for agent {0}/{1}: not found", + new String[] { client.getNamespace(), podTemplate.getName() }); + return; + } + } else { + LOGGER.log(Level.WARNING, "Cloud is not a KubernetesCloud: {0} {1}", + new String[] { cloud.name, cloud.getClass().getName() }); } } } diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesTestUtil.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesTestUtil.java index aeed9b1459..7c458895fb 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesTestUtil.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesTestUtil.java @@ -24,6 +24,7 @@ package org.csanchez.jenkins.plugins.kubernetes; import static io.fabric8.kubernetes.client.Config.*; +import static java.util.logging.Level.*; import static org.hamcrest.Matchers.*; import static org.junit.Assume.*; @@ -33,22 +34,35 @@ import java.security.NoSuchAlgorithmException; import java.security.UnrecoverableKeyException; import java.security.cert.CertificateEncodingException; +import java.util.List; +import java.util.Map; import java.util.Optional; - -import io.fabric8.kubernetes.client.KubernetesClientException; -import org.junit.Assume; +import java.util.concurrent.ForkJoinPool; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.logging.Logger; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import io.fabric8.kubernetes.api.model.Cluster; import io.fabric8.kubernetes.api.model.Config; import io.fabric8.kubernetes.api.model.NamedCluster; import io.fabric8.kubernetes.api.model.NamedContext; import io.fabric8.kubernetes.api.model.NamespaceBuilder; +import io.fabric8.kubernetes.api.model.Pod; +import io.fabric8.kubernetes.api.model.PodList; import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientException; +import io.fabric8.kubernetes.client.Watch; +import io.fabric8.kubernetes.client.Watcher; +import io.fabric8.kubernetes.client.dsl.FilterWatchListDeletable; import io.fabric8.kubernetes.client.internal.KubeConfigUtils; import io.fabric8.kubernetes.client.utils.Utils; public class KubernetesTestUtil { + private static final Logger LOGGER = Logger.getLogger(KubernetesTestUtil.class.getName()); + public static final String TESTING_NAMESPACE = "kubernetes-plugin-test"; public static final String KUBERNETES_CONTEXT = System.getProperty("kubernetes.context", "minikube"); @@ -81,9 +95,73 @@ public static KubernetesCloud setupCloud() throws UnrecoverableKeyException, Cer // Run in our own testing namespace client.namespaces().createOrReplace( new NamespaceBuilder().withNewMetadata().withName(TESTING_NAMESPACE).endMetadata().build()); - } catch (KubernetesClientException e){ + } catch (KubernetesClientException e) { assumeNoException("Kubernetes cluster is not accessible", e); } return cloud; } + + /** + * Delete pods with matching labels + * + * @param client + * @param labels + * @param wait + * wait some time for pods to finish + * @return whether any pod was deleted + * @throws Exception + */ + public static boolean deletePods(KubernetesClient client, Map labels, boolean wait) + throws Exception { + + if (client != null) { + + // wait for 30 seconds for all pods to be terminated + if (wait) { + LOGGER.log(INFO, "Waiting for pods to terminate"); + ForkJoinPool forkJoinPool = new ForkJoinPool(1); + try { + forkJoinPool.submit(() -> IntStream.range(1, 1_000_000).anyMatch(i -> { + try { + FilterWatchListDeletable> pods = client.pods() + .withLabels(labels); + LOGGER.log(INFO, "Still waiting for pods to terminate: {0}", print(pods)); + boolean allTerminated = pods.list().getItems().isEmpty(); + if (allTerminated) { + LOGGER.log(INFO, "All pods are terminated: {0}", print(pods)); + } else { + LOGGER.log(INFO, "Still waiting for pods to terminate: {0}", print(pods)); + Thread.sleep(5000); + } + return allTerminated; + } catch (InterruptedException e) { + LOGGER.log(INFO, "Waiting for pods to terminate - interrupted"); + return true; + } + })).get(60, TimeUnit.SECONDS); + } catch (TimeoutException e) { + LOGGER.log(INFO, "Waiting for pods to terminate - timed out"); + // job not done in interval + } + } + + FilterWatchListDeletable> pods = client.pods() + .withLabels(labels); + if (!pods.list().getItems().isEmpty()) { + LOGGER.log(WARNING, "Deleting leftover pods: {0}", print(pods)); + if (Boolean.TRUE.equals(pods.delete())) { + return true; + } + + } + } + return false; + } + + private static List print(FilterWatchListDeletable> pods) { + return pods.list().getItems().stream() + .map(pod -> String.format("%s (%s)", pod.getMetadata().getName(), pod.getStatus().getPhase())) + .collect(Collectors.toList()); + } + } diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecoratorTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecoratorTest.java index a320c7e196..0fd1f5fbf7 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecoratorTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecoratorTest.java @@ -67,14 +67,12 @@ public class ContainerExecDecoratorTest { @BeforeClass public static void configureCloud() throws Exception { client = setupCloud().connect(); - deletePods(); + deletePods(client, labels, false); } @AfterClass - public static void deletePods() throws Exception { - if (client != null) { - client.pods().withLabel("class", labels.get("class")).delete(); - } + public static void after() throws Exception { + deletePods(client, labels, false); } @Before diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java index 8e62cbbba6..c3f18a59dd 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java @@ -43,6 +43,7 @@ import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep; +import org.junit.After; import org.junit.Before; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -104,6 +105,13 @@ public static void configureCloud() throws Exception { public void configureTemplates() throws Exception { cloud.getTemplates().clear(); cloud.addTemplate(buildBusyboxTemplate("busybox")); + deletePods(cloud.connect(), Collections.emptyMap(), false); + } + + @After + public void cleanup() throws Exception { + assertFalse("There are pods leftover after test execution, see previous logs", + deletePods(cloud.connect(), KubernetesCloud.DEFAULT_POD_LABELS, true)); } /**