[JENKINS-73056] cancel queue Item that really matches the pod causing the cancelling #1539

ysmaoui · 2024-04-25T13:43:21Z

JENKINS-73056

Testing done

Submitter checklist

Give feedback

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue
Options

ysmaoui · 2024-04-25T13:45:34Z

fixes https://issues.jenkins.io/browse/JENKINS-73056

jglick

Idea seems right though implementation could be refined.

Deserves test coverage, which I think ought to be feasible here; it seems this method is called for several reasons, the most straightforward of which would be that the API request to create the pod in the first place was rejected, for example because you specified well-formed YAML which fails schema validation or some such constraint. So something like

parallel k8s: {
  catchError(buildResult: 'SUCCESS') {
    podTemplate(yaml: '…invalid…') {
      node(POD_LABEL) {
        sh 'false should never run'
      }
    }
    echo 'cancelled pod item by now'
  }
}, unrelated: {
  node('special-agent') {}
}

should pass if the test waits for cancelled pod item by now message and then creates a static agent named special-agent, if I follow the bug report correctly.

jglick · 2024-04-25T14:17:33Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodUtils.java

+                String whyOnQueue = item.getWhy();
+                if(whyOnQueue.contains(metadata.getName())) {


This seems fragile. We can probably match based on the label request instead?

I tried printing a couple of "assignedLabels" for items on the queue ( return value of Queue.Item.getAssignedLabel() )

some of these assigned labels match the exactly the pod label "jenkins/label"
but some other, have that - at the beginning that I don't really where it comes from

but the assignedLabel value and the why String have the same value

and the issue where jenkins/label does not match the assignedLabel String is in the case when the pod Name is using the fallback DEFAULT_AGENT_PREFIX ( because the given label does not match the regex set here https://github.com/jenkinsci/kubernetes-plugin/blob/40d4fb21bd8046082ecd88fc5cf860f0db8e54ee/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java#L313C14-L313C23 )

and the 'x' in the jenkins/label comes from here

kubernetes-plugin/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

Line 198 in 40d4fb2

input = input.replaceAll("[^_a-zA-Z0-9-]", "_").replaceFirst("^[^a-zA-Z0-9]", "x");

so, the question I have is why the queue.item assignedLabel not the pod name but the value of the label in jenkins/label instead..

ysmaoui · 2024-04-25T21:51:09Z

Hi @jglick

to be honest I am not very familiar of how to run tests for this plugin, But this is the pipeline that I use to reproduce the bug: ( the failing pod is where the image cannot be pulled - wrong registry )

String incorrectImage = "does.not.exist/jenkins/inbound-agent:3206.vb_15dcf73f6a_9-3-jdk11"
// on dev
String correctImage = "jenkins/inbound-agent:3206.vb_15dcf73f6a_9-3-jdk11"

String getPodYaml(String image){
  return """
apiVersion: v1
kind: Pod
spec:
  containers:
  - name: jnlp
    image: ${image}
    imagePullPolicy: "Always"
    tty: false
    workingDir: "/home/jenkins/agent"
    resources:
      requests:
        memory: 100Mi
        cpu: 500m
  restartPolicy: "Never"
  volumes:
  - emptyDir:
      medium: ""
    name: "workspace-volume"
"""
}

parallel (
  failingPod: {
    stage("failing pod") {
      podTemplate(yaml: getPodYaml(incorrectImage)) {
        node(POD_LABEL) {
          sh 'false should never run'
        }
      }
    }
  },
  correctPods: {
   // keep spawning pods until eventually one of them catches the cancel queue item from the failing pod 
    List l = (1..10).toList()
    l.each { num ->
      stage("correctPod-${num}") {
        podTemplate(yaml: getPodYaml(correctImage)) {
          node(POD_LABEL) {
            echo("correctPod${num}")
          }
        }
      }
    }
  }
)

and when executed this is what happens:

I need probably some assistance in writing and executing such a test scenario for the plugin, can you help?

( excluding the failing pod branch from the pipeline results in no failures in the "correct-pods" branches )

* Add KubernetesPipelineTest#cancelOnlyRelevantQueueItem to cover the use case * Remove duplicated sanitization method from `PodTemplate#sanitizeLabel` * Extract labels and annotations to constant for reuse

Vlatombe · 2024-04-29T08:53:14Z

Thank you for your contribution, made some follow-ups and took the opportunity to clean up sanitization. Implemented the test proposed by @jglick

ysmaoui · 2024-04-29T10:26:00Z

Thank you for your contribution, made some follow-ups and took the opportunity to clean up sanitization. Implemented the test proposed by @jglick

thanks for the help @Vlatombe

it seems spotbugs is still complaining, and I tried to fix it here: https://github.com/ysmaoui/kubernetes-plugin/pull/1/files
if you approve, I would like to merge the fix here

…-pointer-bug spotbugs-detected possible nullpointer-exception

…queue-item-when-a-pod-fails

Vlatombe

LGTM, pinging @jglick for a second pair of eyes

jglick · 2024-04-29T13:16:57Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java

+     * @throws AssertionError if the generated label is not valid
+     */
+    public static String sanitizeLabel(String input) {
+        int max = /* Kubernetes limit */ 63 - /* hyphen */ 1 - /* suffix */ 5;


So this is actually from labelify not from the function previous called sanitizeLabel?

I think labelify and the previous sanitizeLabel methods are now merged into this new sanitizeLabel method of the PodTemplateUtils class

Yes, both were doing very similar job, there is now only one method.

This actually changes a behavior for labels that contains a .. For example label release-1.2.3 resulted in jenkins/label: release-1.2.3. Now it results in jenkins/label: release-1_2_3.

Indeed the previous version handled [^_.a-zA-Z0-9-] which is gone here. Seems like a regression; @Dohbedoh can you please make sure this is filed as an issue (if not a patch)?

changes a behavior

But is that breaking anything?

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodUtils.java

jglick

Sorry, no, runUrl Javadoc does not seem to match reality.

This reverts commit baa1004.

ysmaoui · 2024-04-30T06:21:18Z

thank you @Vlatombe and @jglick for reviewing and all the help :)

can we merge?

cancel queue Item that really matches the pod causing the cancelling

3ce404c

ysmaoui requested a review from a team as a code owner April 25, 2024 13:43

jglick reviewed Apr 25, 2024

View reviewed changes

ysmaoui marked this pull request as draft April 27, 2024 10:31

ysmaoui force-pushed the bugfix/JENKINS-73056-cancel-only-relevant-queue-item-when-a-pod-fails branch 7 times, most recently from 4c7d18c to 56c8cb3 Compare April 28, 2024 20:39

match the queue Item with the jenkins/label of the pod

c5889c6

ysmaoui force-pushed the bugfix/JENKINS-73056-cancel-only-relevant-queue-item-when-a-pod-fails branch from 56c8cb3 to c5889c6 Compare April 28, 2024 21:06

Vlatombe changed the title ~~cancel queue Item that really matches the pod causing the cancelling~~ [JENKINS-73056] cancel queue Item that really matches the pod causing the cancelling Apr 29, 2024

[JENKINS-73056] Clean up sanitization, add a test for the case

297530a

* Add KubernetesPipelineTest#cancelOnlyRelevantQueueItem to cover the use case * Remove duplicated sanitization method from `PodTemplate#sanitizeLabel` * Extract labels and annotations to constant for reuse

Vlatombe added the bug Bug Fixes label Apr 29, 2024

spotbugs-detected possible nullpointer-exception

c22162b

Merge pull request #1 from ysmaoui/bugfix/JENKINS-73056-spotbugs-null…

b889be7

…-pointer-bug spotbugs-detected possible nullpointer-exception

ysmaoui marked this pull request as ready for review April 29, 2024 12:29

Vlatombe added 2 commits April 29, 2024 14:34

Merge branch 'master' into bugfix/JENKINS-73056-cancel-only-relevant-…

97fc278

…queue-item-when-a-pod-fails

Fix imports

f130ebc

Vlatombe requested a review from jglick April 29, 2024 12:37

Vlatombe approved these changes Apr 29, 2024

View reviewed changes

jglick approved these changes Apr 29, 2024

View reviewed changes

jglick requested changes Apr 29, 2024

View reviewed changes

ysmaoui force-pushed the bugfix/JENKINS-73056-cancel-only-relevant-queue-item-when-a-pod-fails branch from b294ac7 to 1ce44e4 Compare April 29, 2024 13:45

ysmaoui requested review from jglick and Vlatombe April 29, 2024 13:52

removed unnecessary runUrl matching

baa1004

ysmaoui force-pushed the bugfix/JENKINS-73056-cancel-only-relevant-queue-item-when-a-pod-fails branch from 1ce44e4 to baa1004 Compare April 29, 2024 13:54

Yacine Smaoui added 2 commits April 29, 2024 15:55

Revert "removed unnecessary runUrl matching"

db7318e

This reverts commit baa1004.

Add filtering with runUrl of the items on the queue

6851c27

Vlatombe approved these changes Apr 29, 2024

View reviewed changes

jglick approved these changes Apr 29, 2024

View reviewed changes

Vlatombe merged commit c646b71 into jenkinsci:master Apr 30, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73056] cancel queue Item that really matches the pod causing the cancelling #1539

[JENKINS-73056] cancel queue Item that really matches the pod causing the cancelling #1539

ysmaoui commented Apr 25, 2024 •

edited by Vlatombe

Loading

Submitter checklist

ysmaoui commented Apr 25, 2024

jglick left a comment

jglick Apr 25, 2024

ysmaoui Apr 25, 2024

ysmaoui Apr 25, 2024 •

edited

Loading

ysmaoui Apr 25, 2024

ysmaoui commented Apr 25, 2024 •

edited

Loading

Vlatombe commented Apr 29, 2024

ysmaoui commented Apr 29, 2024

Vlatombe left a comment

jglick Apr 29, 2024

ysmaoui Apr 29, 2024

Vlatombe Apr 29, 2024

Dohbedoh Jul 30, 2024

jglick Jul 31, 2024

Vlatombe Aug 12, 2024

jglick left a comment

ysmaoui commented Apr 30, 2024

		String whyOnQueue = item.getWhy();
		if(whyOnQueue.contains(metadata.getName())) {

[JENKINS-73056] cancel queue Item that really matches the pod causing the cancelling #1539

[JENKINS-73056] cancel queue Item that really matches the pod causing the cancelling #1539

Conversation

ysmaoui commented Apr 25, 2024 • edited by Vlatombe Loading

Testing done

Submitter checklist

ysmaoui commented Apr 25, 2024

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysmaoui Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysmaoui commented Apr 25, 2024 • edited Loading

Vlatombe commented Apr 29, 2024

ysmaoui commented Apr 29, 2024

Vlatombe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

ysmaoui commented Apr 30, 2024

ysmaoui commented Apr 25, 2024 •

edited by Vlatombe

Loading

ysmaoui Apr 25, 2024 •

edited

Loading

ysmaoui commented Apr 25, 2024 •

edited

Loading