Merge pull request #192 from jenkinsci/delete-errors

[JENKINS-45910] Delete pods in the cloud namespace when pod namespace is not defined

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<String, String> POD_LABEL = ImmutableMap.of("jenkins", "slave");
+    public static final Map<String, String> 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<String, String> getLabelsMap(Set<LabelAtom> labelSet) {
         ImmutableMap.Builder<String, String> builder = ImmutableMap.<String, String> 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<Pod> slaveListItems = slaveList.getItems();
 
         Map<String, String> labelsMap = getLabelsMap(template.getLabelSet());

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;
 
@@ -12,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;
@@ -22,10 +27,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;
 
 /**
@@ -80,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() {
@@ -131,35 +133,60 @@ 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;
         }
 
+        String actualNamespace = getNamespace() == null ? client.getNamespace() : getNamespace();
         try {
-            Cloud cloud = getCloud();
-            if (cloud == null) {
-                String msg = String.format("Slave cloud no longer exists: %s", getCloudName());
+            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.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);
+                listener.error(msg);
                 return;
             }
-            KubernetesClient client = ((KubernetesCloud) cloud).connect();
-            PodResource<Pod, DoneablePod> 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);
+        } catch (KubernetesClientException e) {
+            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")));
-            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/%s", actualNamespace, 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

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() });
             }
         }
     }

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<String, String> 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<Pod, PodList, Boolean, Watch, Watcher<Pod>> 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<Pod, PodList, Boolean, Watch, Watcher<Pod>> 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<String> print(FilterWatchListDeletable<Pod, PodList, Boolean, Watch, Watcher<Pod>> pods) {
+        return pods.list().getItems().stream()
+                .map(pod -> String.format("%s (%s)", pod.getMetadata().getName(), pod.getStatus().getPhase()))
+                .collect(Collectors.toList());
+    }
+
 }

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

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));
     }
 
     /**
@@ -246,6 +254,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();

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}')
 ]) {