[JENKINS-73056] cancel queue Item that really matches the pod causing…

… the cancelling (#1539) Co-authored-by: Yacine Smaoui <yacine.smaoui@cariad.technology> Co-authored-by: Vincent Latombe <vincent@latombe.net>
jenkinsci · Apr 30, 2024 · c646b71 · c646b71
1 parent 4017ba2
commit c646b71
Show file tree

Hide file tree

Showing 10 changed files with 158 additions and 129 deletions.
diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
@@ -76,6 +76,9 @@ public class PodTemplate extends AbstractDescribableImpl<PodTemplate> implements
     public static final Integer DEFAULT_SLAVE_JENKINS_CONNECTION_TIMEOUT =
             Integer.getInteger(PodTemplate.class.getName() + ".connectionTimeout", 1000);
 
+    public static final String JENKINS_LABEL = "jenkins/label";
+    public static final String JENKINS_LABEL_DIGEST = "jenkins/label-digest";
+
     /**
      * Digest function that is used to compute the kubernetes label "jenkins/label-digest"
      * Not used for security.
@@ -477,22 +480,6 @@ public Map<String, String> getLabelsMap() {
         return labelsMap;
     }
 
-    static String sanitizeLabel(String input) {
-        String label = input;
-        int max = 63;
-        // Kubernetes limit
-        // a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must
-        // start and end with an alphanumeric character (e.g. 'MyValue',  or 'my_value',  or '12345', regex used
-        // for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')
-        if (label.length() > max) {
-            label = label.substring(label.length() - max);
-        }
-        label = label.replaceAll("[^_.a-zA-Z0-9-]", "_")
-                .replaceFirst("^[^a-zA-Z0-9]", "x")
-                .replaceFirst("[^a-zA-Z0-9]$", "x");
-        return label;
-    }
-
     @DataBoundSetter
     public void setLabel(String label) {
         this.label = Util.fixEmptyAndTrim(label);
@@ -503,14 +490,13 @@ private void recomputeLabelDerivedFields() {
         this.labelSet = Label.parse(label);
         Map<String, String> tempMap = new HashMap<>();
         if (label == null) {
-            tempMap.put("jenkins/label", DEFAULT_LABEL);
-            tempMap.put("jenkins/label-digest", "0");
+            tempMap.put(JENKINS_LABEL, DEFAULT_LABEL);
+            tempMap.put(JENKINS_LABEL_DIGEST, "0");
         } else {
             MessageDigest labelDigestFunction = getLabelDigestFunction();
             labelDigestFunction.update(label.getBytes(StandardCharsets.UTF_8));
-            tempMap.put("jenkins/label", sanitizeLabel(label));
-            tempMap.put(
-                    "jenkins/label-digest", String.format("%040x", new BigInteger(1, labelDigestFunction.digest())));
+            tempMap.put(JENKINS_LABEL, PodTemplateUtils.sanitizeLabel(label));
+            tempMap.put(JENKINS_LABEL_DIGEST, String.format("%040x", new BigInteger(1, labelDigestFunction.digest())));
         }
         labelsMap = Collections.unmodifiableMap(tempMap);
     }

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java
@@ -757,6 +757,32 @@ public static boolean validateImage(String image) {
         return image != null && image.matches("\\S+");
     }
 
+    /**
+     * <p>Sanitizes the input string to create a valid Kubernetes label.
+     * <p>The input string is truncated to a maximum length of 57 characters,
+     * and any characters that are not alphanumeric or hyphens are replaced with underscores. If the input string starts with a non-alphanumeric
+     * character, it is replaced with 'x'.
+     *
+     * @param  input  the input string to be sanitized
+     * @return        the sanitized and validated label
+     * @throws AssertionError if the generated label is not valid
+     */
+    public static String sanitizeLabel(String input) {
+        int max = /* Kubernetes limit */ 63 - /* hyphen */ 1 - /* suffix */ 5;
+        String label;
+        if (input.length() > max) {
+            label = input.substring(input.length() - max);
+        } else {
+            label = input;
+        }
+        label = label.replaceAll("[^_a-zA-Z0-9-]", "_")
+                .replaceFirst("^[^a-zA-Z0-9]", "x")
+                .replaceFirst("[^a-zA-Z0-9]$", "x");
+
+        assert PodTemplateUtils.validateLabel(label) : label;
+        return label;
+    }
+
     private static List<EnvVar> combineEnvVars(Container parent, Container template) {
         Map<String, EnvVar> combinedEnvVars = new HashMap<>();
         Stream.of(parent.getEnv(), template.getEnv())

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodUtils.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodUtils.java
@@ -19,24 +19,28 @@
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.Util;
-import hudson.model.Queue;
+import hudson.model.Label;
 import io.fabric8.kubernetes.api.model.ContainerStatus;
 import io.fabric8.kubernetes.api.model.ObjectMeta;
 import io.fabric8.kubernetes.api.model.Pod;
 import io.fabric8.kubernetes.api.model.PodStatus;
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.fabric8.kubernetes.client.KubernetesClientException;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import java.util.Map;
+import java.util.Optional;
 import java.util.function.Predicate;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 import jenkins.model.Jenkins;
 import org.apache.commons.lang.StringUtils;
+import org.csanchez.jenkins.plugins.kubernetes.pipeline.PodTemplateStepExecution;
 
 public final class PodUtils {
+    private PodUtils() {}
+
     private static final Logger LOGGER = Logger.getLogger(PodUtils.class.getName());
 
     public static final Predicate<ContainerStatus> CONTAINER_IS_TERMINATED =
@@ -66,52 +70,56 @@ public static List<ContainerStatus> getContainers(Pod pod, Predicate<ContainerSt
     }
 
     /**
-     * Cancel queue items matching the given pod.
-     * It uses the annotation "runUrl" added to the pod to do the matching.
-     *
-     * It uses the current thread context to list item queues,
+     * <p>Cancel queue items matching the given pod.
+     * <p>The queue item has to have a task url matching the pod "runUrl"-annotation
+     * and the queue item assigned label needs to match the label jenkins/label of the pod.
+     * <p>It uses the current thread context to list item queues,
      * so make sure to be in the right context before calling this method.
      *
      * @param pod The pod to cancel items for.
      * @param reason The reason the item are being cancelled.
      */
     public static void cancelQueueItemFor(Pod pod, String reason) {
-        Queue q = Jenkins.get().getQueue();
-        boolean cancelled = false;
-        ObjectMeta metadata = pod.getMetadata();
+        var metadata = pod.getMetadata();
         if (metadata == null) {
             return;
         }
-        Map<String, String> annotations = metadata.getAnnotations();
+        String podName = metadata.getName();
+        String podNamespace = metadata.getNamespace();
+        String podDisplayName = podNamespace + "/" + podName;
+        var annotations = metadata.getAnnotations();
         if (annotations == null) {
-            LOGGER.log(Level.FINE, "Pod .metadata.annotations is null: {0}/{1}", new Object[] {
-                metadata.getNamespace(), metadata.getName()
-            });
+            LOGGER.log(Level.FINE, () -> "Pod " + podDisplayName + " .metadata.annotations is null");
             return;
         }
-        String runUrl = annotations.get("runUrl");
+        var runUrl = annotations.get(PodTemplateStepExecution.POD_ANNOTATION_RUN_URL);
         if (runUrl == null) {
-            LOGGER.log(Level.FINE, "Pod .metadata.annotations.runUrl is null: {0}/{1}", new Object[] {
-                metadata.getNamespace(), metadata.getName()
-            });
+            LOGGER.log(Level.FINE, () -> "Pod " + podDisplayName + " .metadata.annotations.runUrl is null");
             return;
         }
-        for (Queue.Item item : q.getItems()) {
-            Queue.Task task = item.task;
-            if (runUrl.equals(task.getUrl())) {
-                LOGGER.log(Level.FINE, "Cancelling queue item: \"{0}\"\n{1}", new Object[] {
-                    task.getDisplayName(), !StringUtils.isBlank(reason) ? "due to " + reason : ""
-                });
-                q.cancel(item);
-                cancelled = true;
-                break;
-            }
-        }
-        if (!cancelled) {
-            LOGGER.log(Level.FINE, "No queue item found for pod: {0}/{1}", new Object[] {
-                metadata.getNamespace(), metadata.getName()
-            });
+        var labels = metadata.getLabels();
+        if (labels == null) {
+            LOGGER.log(Level.FINE, () -> "Pod " + podDisplayName + " .metadata.labels is null");
+            return;
         }
+        var jenkinsLabel = labels.get(PodTemplate.JENKINS_LABEL);
+        var queue = Jenkins.get().getQueue();
+        Arrays.stream(queue.getItems())
+                .filter(item -> item.getTask().getUrl().equals(runUrl))
+                .filter(item -> Optional.ofNullable(item.getAssignedLabel())
+                        .map(Label::getName)
+                        .map(name -> PodTemplateUtils.sanitizeLabel(name).equals(jenkinsLabel))
+                        .orElse(false))
+                .findFirst()
+                .ifPresentOrElse(
+                        item -> {
+                            LOGGER.log(
+                                    Level.FINE,
+                                    () -> "Cancelling queue item: \"" + item.task.getDisplayName() + "\"\n"
+                                            + (!StringUtils.isBlank(reason) ? "due to " + reason : ""));
+                            queue.cancel(item);
+                        },
+                        () -> LOGGER.log(Level.FINE, () -> "No queue item found for pod " + podDisplayName));
     }
 
     @CheckForNull

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
@@ -41,6 +41,8 @@ public class PodTemplateStepExecution extends AbstractStepExecutionImpl {
     private static final long serialVersionUID = -6139090518333729333L;
 
     private static final String NAME_FORMAT = "%s-%s";
+    public static final String POD_ANNOTATION_BUILD_URL = "buildUrl";
+    public static final String POD_ANNOTATION_RUN_URL = "runUrl";
 
     private static /* almost final */ boolean VERBOSE =
             Boolean.parseBoolean(System.getProperty(PodTemplateStepExecution.class.getName() + ".verbose"));
@@ -70,9 +72,15 @@ public boolean start() throws Exception {
         PodTemplateContext podTemplateContext = getContext().get(PodTemplateContext.class);
         String parentTemplates = podTemplateContext != null ? podTemplateContext.getName() : null;
 
-        String label = step.getLabel();
-        if (label == null) {
-            label = labelify(run.getExternalizableId());
+        String label;
+        String podTemplateLabel = step.getLabel();
+        if (podTemplateLabel == null) {
+            var sanitized = PodTemplateUtils.sanitizeLabel(run.getExternalizableId()) + "-"
+                    + RandomStringUtils.random(5, "bcdfghjklmnpqrstvwxz0123456789");
+            assert PodTemplateUtils.validateLabel(sanitized) : sanitized;
+            label = sanitized;
+        } else {
+            label = podTemplateLabel;
         }
 
         // Let's generate a random name based on the user specified to make sure that we don't have
@@ -121,8 +129,8 @@ public boolean start() throws Exception {
             newTemplate.setInheritYamlMergeStrategy(step.isInheritYamlMergeStrategy());
             String url = cloud.getJenkinsUrlOrNull();
             if (url != null) {
-                newTemplate.getAnnotations().add(new PodAnnotation("buildUrl", url + run.getUrl()));
-                newTemplate.getAnnotations().add(new PodAnnotation("runUrl", run.getUrl()));
+                newTemplate.getAnnotations().add(new PodAnnotation(POD_ANNOTATION_BUILD_URL, url + run.getUrl()));
+                newTemplate.getAnnotations().add(new PodAnnotation(POD_ANNOTATION_RUN_URL, run.getUrl()));
             }
         }
         newTemplate.setImagePullSecrets(step.getImagePullSecrets().stream()
@@ -190,17 +198,6 @@ private static KubernetesCloud resolveCloud(final String cloudName) throws Abort
         return cloud;
     }
 
-    static String labelify(String input) {
-        int max = /* Kubernetes limit */ 63 - /* hyphen */ 1 - /* suffix */ 5;
-        if (input.length() > max) {
-            input = input.substring(input.length() - max);
-        }
-        input = input.replaceAll("[^_a-zA-Z0-9-]", "_").replaceFirst("^[^a-zA-Z0-9]", "x");
-        String label = input + "-" + RandomStringUtils.random(5, "bcdfghjklmnpqrstvwxz0123456789");
-        assert PodTemplateUtils.validateLabel(label) : label;
-        return label;
-    }
-
     /**
      * Check if the current Job is permitted to use the cloud.
      *

diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateTest.java
@@ -1,6 +1,5 @@
 package org.csanchez.jenkins.plugins.kubernetes;
 
-import static org.csanchez.jenkins.plugins.kubernetes.PodTemplate.*;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.empty;
@@ -32,18 +31,4 @@ public void copyConstructor() throws Exception {
         pt.setIdleMinutes(99);
         assertEquals(xs.toXML(pt), xs.toXML(new PodTemplate(pt)));
     }
-
-    @Test
-    public void sanitizeLabels() {
-        assertEquals("label1", sanitizeLabel("label1"));
-        assertEquals("label1_label2", sanitizeLabel("label1 label2"));
-        assertEquals(
-                "el1_label2_verylooooooooooooooooooooooooooooonglabelover63chars",
-                sanitizeLabel("label1 label2 verylooooooooooooooooooooooooooooonglabelover63chars"));
-        assertEquals("xfoo_bar", sanitizeLabel(":foo:bar"));
-        assertEquals("xfoo_barx", sanitizeLabel(":foo:bar:"));
-        assertEquals(
-                "l2_verylooooooooooooooooooooooooooooonglabelendinginunderscorex",
-                sanitizeLabel("label1 label2 verylooooooooooooooooooooooooooooonglabelendinginunderscore_"));
-    }
 }
diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java
@@ -24,12 +24,29 @@
 
 package org.csanchez.jenkins.plugins.kubernetes;
 
-import static java.util.Arrays.*;
-import static java.util.Collections.*;
-import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.*;
+import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.combine;
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.parseFromYaml;
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.sanitizeLabel;
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.substitute;
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.unwrap;
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.validateLabel;
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.*;
-import static org.junit.Assert.*;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasEntry;
+import static org.hamcrest.Matchers.hasItems;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import hudson.model.Node;
 import hudson.tools.ToolLocationNodeProperty;
@@ -944,4 +961,25 @@ public void shouldIgnoreContainerEmptyArgs() {
         assertEquals(List.of("arg1", "arg2"), result.getArgs());
         assertEquals(List.of("parent command"), result.getCommand());
     }
+
+    @Test
+    public void shouldSanitizeJenkinsLabel() {
+        assertEquals("foo", sanitizeLabel("foo"));
+        assertEquals("foo_bar__3", sanitizeLabel("foo bar #3"));
+        assertEquals("This_Thing", sanitizeLabel("This/Thing"));
+        assertEquals("xwhatever", sanitizeLabel("/whatever"));
+        assertEquals(
+                "xprolix-for-the-sixty-three-character-limit-in-kubernetes",
+                sanitizeLabel("way-way-way-too-prolix-for-the-sixty-three-character-limit-in-kubernetes"));
+        assertEquals("label1", sanitizeLabel("label1"));
+        assertEquals("label1_label2", sanitizeLabel("label1 label2"));
+        assertEquals(
+                "bel2_verylooooooooooooooooooooooooooooonglabelover63chars",
+                sanitizeLabel("label1 label2 verylooooooooooooooooooooooooooooonglabelover63chars"));
+        assertEquals("xfoo_bar", sanitizeLabel(":foo:bar"));
+        assertEquals("xfoo_barx", sanitizeLabel(":foo:bar:"));
+        assertEquals(
+                "ylooooooooooooooooooooooooooooonglabelendinginunderscorex",
+                sanitizeLabel("label1 label2 verylooooooooooooooooooooooooooooonglabelendinginunderscore_"));
+    }
 }
diff --git a/...java/org/csanchez/jenkins/plugins/kubernetes/pipeline/AbstractKubernetesPipelineTest.java b/...java/org/csanchez/jenkins/plugins/kubernetes/pipeline/AbstractKubernetesPipelineTest.java
@@ -50,6 +50,7 @@
 import org.csanchez.jenkins.plugins.kubernetes.KubernetesComputer;
 import org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil;
 import org.csanchez.jenkins.plugins.kubernetes.PodTemplate;
+import org.csanchez.jenkins.plugins.kubernetes.PodUtils;
 import org.csanchez.jenkins.plugins.kubernetes.model.KeyValueEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.model.SecretEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
@@ -81,6 +82,7 @@ public abstract class AbstractKubernetesPipelineTest {
     public LoggerRule logs = new LoggerRule()
             .recordPackage(KubernetesCloud.class, Level.FINE)
             .recordPackage(NoDelayProvisionerStrategy.class, Level.FINE)
+            .record(PodUtils.class, Level.FINE)
             .record(NodeProvisioner.class, Level.FINE)
             .record(KubernetesAgentErrorCondition.class, Level.FINE);
 

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
@@ -844,4 +844,12 @@ private <R extends Run> R assertBuildStatus(R run, Result... status) throws Exce
         MatcherAssert.assertThat(msg, run.getResult(), Matchers.is(oneOf(status)));
         return run;
     }
+
+    @Test
+    public void cancelOnlyRelevantQueueItem() throws Exception {
+        r.waitForMessage("cancelled pod item by now", b);
+        r.createOnlineSlave(Label.get("special-agent"));
+        r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b));
+        r.assertLogContains("ran on special agent", b);
+    }
 }