[JENKINS-73224] Do not try to create a pod which already exists #1561

amuniz · 2024-05-28T13:36:38Z

See JENKINS-73224 for details.

Easier to review hiding whitespace changes: https://github.com/jenkinsci/kubernetes-plugin/pull/1561/files?w=1

Testing done

Automated test added.

Submitter checklist

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

See [JENKINS-73224](https://issues.jenkins.io/browse/JENKINS-73224) for details.

amuniz · 2024-05-28T13:39:37Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineRJRTest.java

+                        .build());
+            }
+        }
+    }


Without the fix this test fails with the log in the linked ticket.

...java/org/csanchez/jenkins/plugins/kubernetes/pipeline/AbstractKubernetesPipelineRJRTest.java

amuniz · 2024-05-28T13:42:09Z

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

+            // the pod might already exist and the creating logic must be skipped.
+            Pod existingPod =
+                    client.pods().inNamespace(namespace).withName(podName).get();
+            if (existingPod == null) {


Everything inside this if branch is untouched (except formatting, which I was forced to do by Spotless)

Vlatombe

LGTM

jglick · 2024-05-28T15:33:23Z

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

-            listener.getLogger().printf("Created Pod: %s %s/%s%n", cloudName, namespace, podName);
-            Metrics.metricRegistry().counter(MetricNames.PODS_CREATED).inc();
-
-            node.getRunListener().getLogger().printf("Created Pod: %s %s/%s%n", cloudName, namespace, podName);
            kubernetesComputer.setLaunching(true);


Could we fix this field please, so it is not persisting an AtomicBoolean?

Does it need to be an AtomicBoolean?

Ah, I see #1479 (comment)

Fixed in 3b78a0b

jglick · 2024-05-28T15:33:41Z

(disabling automerge while I review too)

…ugin/pull/1479/files#r1430305858

amuniz · 2024-05-28T15:56:46Z

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

@@ -68,7 +67,7 @@ public class KubernetesLauncher extends JNLPLauncher {

    private static final Logger LOGGER = Logger.getLogger(KubernetesLauncher.class.getName());

-    private final AtomicBoolean launched = new AtomicBoolean(false);
+    private volatile boolean launched = false;


FTR: this is not directly related or needed for the goal of this PR, but it was requested by @jglick in a review.

(but consider #1561 (comment))

jglick · 2024-05-28T16:07:13Z

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

+                LOGGER.log(INFO, () -> "Pod already exists: " + cloudName + " " + namespace + "/" + podName);
+                listener.getLogger().printf("Pod already exists: %s %s/%s%n", cloudName, namespace, podName);


So how does this actually differ from

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

Lines 106 to 110 in 3b78a0b

if (launched) {

LOGGER.log(INFO, "Agent has already been launched, activating: {0}", node.getNodeName());

computer.setAcceptingTasks(true);

return;

}

and do we even still need the launching field if we have this logic?

That is another face of the same behaviour, just under a slightly different timing (a lot narrower than the one being fixed in this PR).

This PR catches the case there the controller is restarted between the pod creating call and the end of the waiting for loop. While the existing launched check would only catch a restart right after the node save and the end of the method, which is quite unlikely to happen, but possible I guess.

do we even still need the launching field if we have this logic?

Not sure. https://github.com/amuniz/kubernetes-plugin/blob/f10083a42e708a6bff990c6ccb306ce618b8bccd/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesComputer.java#L201 is never used from this plugin sources, but it's public and it could be used somewhere else, so I would stay on the safe side and keep it.

is never used from this plugin sources, but it's public and it could be used somewhere else

Well, if it is not used it can be deleted. (I usually just do a GH org-wide search.)

Then there is stuff like https://github.com/amuniz/kubernetes-plugin/blob/3b78a0ba311af4c091b94c070b9a8c8b6ab6768b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesLauncher.java#L199-L200 which it would not make sense to do twice, but there is no current defense against race conditions. It seems there are deeper bugs waiting here.

jglick · 2024-05-28T16:11:20Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineRJRTest.java

+        // restart
+        rjr.stopJenkins();
+        rjr.startJenkins();
+        // update k8s to make a node suitable to schedule (add disktype=special to the node)


A little scary to have the test even temporarily patch the Node! Would be safer to do something to the namespace like a quota 🤔 Fine enough for Kind/CI, could just become a problem if we try to run tests on a permanent cluster.

This was the easiest way I found, and with the finally it's granted to be back to the original state.

So long as the JVM terminates normally, yes.

jglick · 2024-05-28T16:12:26Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/steps/AssertBuildLogMessage.java

+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.RealJenkinsRule;
+
+public class AssertBuildLogMessage implements RealJenkinsRule.Step {


BTW you can also make these things static methods in some util class; a bit less boilerplate.

Yeah, but less reusable then...

How is that less reusable?

When writing a new test it seems natural to go to the steps package and look for generic steps to use in the new test, however inspecting other test classes looking for static methods seems less discoverable to me at least. That will likely end up with re-writing the same (or very similar) static methods in multiple test classes.

inspecting other test classes looking for static methods

I meant reusable static methods in a utility class in the steps package. Or just a single class in the main test package called Steps. Etc.

jglick

Looks to fix the stated bug. I am pretty sure there are other things wrong here but perhaps they rarely matter.

amuniz added 2 commits May 28, 2024 15:32

[JENKINS-73224] Do not try to create a pod which already exists

367fbc1

See [JENKINS-73224](https://issues.jenkins.io/browse/JENKINS-73224) for details.

Merge branch 'master' into JENKINS-73224

08f553f

amuniz requested a review from a team as a code owner May 28, 2024 13:36

amuniz commented May 28, 2024

View reviewed changes

...java/org/csanchez/jenkins/plugins/kubernetes/pipeline/AbstractKubernetesPipelineRJRTest.java Show resolved Hide resolved

amuniz commented May 28, 2024

View reviewed changes

Extract common test logic

c5ce8c2

Vlatombe approved these changes May 28, 2024

View reviewed changes

Vlatombe added the bug Bug Fixes label May 28, 2024

Vlatombe enabled auto-merge (squash) May 28, 2024 15:28

jglick reviewed May 28, 2024

View reviewed changes

jglick disabled auto-merge May 28, 2024 15:33

amuniz added 2 commits May 28, 2024 17:54

Unused imports

b65787a

Back to plain boolean. See https://github.com/jenkinsci/kubernetes-pl…

3b78a0b

…ugin/pull/1479/files#r1430305858

amuniz commented May 28, 2024

View reviewed changes

jglick reviewed May 28, 2024

View reviewed changes

jglick approved these changes May 28, 2024

View reviewed changes

Vlatombe approved these changes May 29, 2024

View reviewed changes

Vlatombe merged commit c06f753 into jenkinsci:master May 29, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73224] Do not try to create a pod which already exists #1561

[JENKINS-73224] Do not try to create a pod which already exists #1561

amuniz commented May 28, 2024 •

edited

Loading

amuniz May 28, 2024

amuniz May 28, 2024

Vlatombe left a comment

jglick May 28, 2024

amuniz May 28, 2024

amuniz May 28, 2024

amuniz May 28, 2024

jglick commented May 28, 2024

amuniz May 28, 2024

jglick May 28, 2024

jglick May 28, 2024

amuniz May 28, 2024

amuniz May 28, 2024

jglick May 28, 2024

jglick May 28, 2024

jglick May 28, 2024

amuniz May 28, 2024

jglick May 28, 2024

jglick May 28, 2024

amuniz May 28, 2024

jglick May 28, 2024

amuniz May 29, 2024

jglick May 29, 2024 •

edited

Loading

jglick left a comment

		LOGGER.log(INFO, () -> "Pod already exists: " + cloudName + " " + namespace + "/" + podName);
		listener.getLogger().printf("Pod already exists: %s %s/%s%n", cloudName, namespace, podName);

	if (launched) {
	LOGGER.log(INFO, "Agent has already been launched, activating: {0}", node.getNodeName());
	computer.setAcceptingTasks(true);
	return;
	}

[JENKINS-73224] Do not try to create a pod which already exists #1561

[JENKINS-73224] Do not try to create a pod which already exists #1561

Conversation

amuniz commented May 28, 2024 • edited Loading

Testing done

Submitter checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

jglick commented May 28, 2024

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

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 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

amuniz commented May 28, 2024 •

edited

Loading

jglick May 29, 2024 •

edited

Loading