[JENKINS-35246] Kubernetes agents not getting deleted in Jenkins after pods are deleted #217

Vlatombe · 2017-09-05T12:52:33Z

This fixes the provisioning lifecycle for KubernetesSlave. The problem
was ProvisioningCallback was adding the node to Jenkins but it is
expected to be done by NodeProvisioner once the future completes.

This could lead in cases of short node usage in cases where the node would
get readded to Jenkins after the termination had occurred.

Moving the provisioning logic to a ComputerLauncher honors the lifecycle
properly since ComputerLauncher#launch is called after the
NodeProvisioner has finished its job, eliminating risk of race
condition.

https://issues.jenkins-ci.org/browse/JENKINS-35246

marvinthepa · 2017-09-05T18:58:47Z

pom.xml

@@ -212,6 +212,9 @@
            at io.fabric8.kubernetes.client.utils.Serialization.<clinit>(Serialization.java:37)
          -->
          <pluginFirstClassLoader>true</pluginFirstClassLoader>
+          <systemProperties>
+            <hudson.slaves.NodeProvisioner.initialDelay>0</hudson.slaves.NodeProvisioner.initialDelay>


marvinthepa · 2017-09-06T07:35:30Z

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

-    public Cloud getCloud() {
-        return Jenkins.getInstance().getCloud(getCloudName());
+    @Nonnull
+    public KubernetesCloud getCloud() {


I have to admit that I don't understand enough about this, but could this be a binary compatibility issue?

I restored the original method.

marvinthepa

Just a few minor things..

marvinthepa · 2017-09-08T10:43:41Z

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

+
+    public PodTemplate getTemplate() {
+        return template;
+    }

    public KubernetesSlave(PodTemplate template, String nodeDescription, KubernetesCloud cloud, String labelStr)


Maybe I am wrong, but isn't this unused now and could/should be marked @Deprecated?

marvinthepa · 2017-09-08T10:45:58Z

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

-            computer.disconnect(OfflineCause.create(new Localizable(HOLDER, "offline")));
-            return;
-        }
+        KubernetesCloud cloud = getKubernetesCloud();


What happens here if the cloud no longer exists, e.g. because it was deleted by a user?

marvinthepa · 2017-09-08T10:51:25Z

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

@@ -0,0 +1,465 @@
+package org.csanchez.jenkins.plugins.kubernetes;


File is missing a license header, AFAIK @carlossg (or CloudBees?) requires them.

marvinthepa · 2017-09-08T12:16:48Z

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

@@ -24,68 +24,18 @@

 package org.csanchez.jenkins.plugins.kubernetes;

-import static org.csanchez.jenkins.plugins.kubernetes.KubernetesCloud.*;


You still have a few unused imports here.

Vlatombe · 2017-09-11T07:24:18Z

Thanks, @marvinthepa, all fixed now.

…provisioning)

This fixes the provisioning lifecycle for KubernetesSlave. The problem was ProvisioningCallback was adding the node to Jenkins but it is expected to be done by NodeProvisioner once the future completes. This could lead in cases of short node usage in cases where the node would get readded to Jenkins after the termination had occurred. Moving the provisioning logic to a ComputerLauncher honors the lifecycle properly since ComputerLauncher#launch is called after the NodeProvisioner has finished its job, eliminating risk of race condition.

…termination call.

marvinthepa · 2017-09-19T15:41:59Z

@Vlatombe is it possible that the changes from this pull request lead to the fact that a slave can now connect to the master before all containers in that pod have successfully started?

I have noticed this behavior when testing another PR, and this one looks a bit suspicious in this regard.. I have to admit that I didn't check closely, though.

Vlatombe · 2017-09-19T16:27:10Z

@marvinthepa It's possible since the launch is now asynchronous. Setting computer.setAcceptingTasks(false); at the beginning of the launcher and computer.setAcceptingTasks(true); at the end should do the trick, since the logic already checks that all containers within the pod are launched.

Prevent to start builds that might try to use containers that haven't started (or will never start due to errors). This reverts a change in behavior introduced in jenkinsci#217.

dpyoung2 · 2017-09-26T13:27:30Z

Hello,
Will this fix be included in the next release of the plugin? If yes, when is the next release?

Prevent to start builds that might try to use containers that haven't started (or will never start due to errors). This reverts a change in behavior introduced in #217.

Vlatombe force-pushed the provisioning_lifecycle_fix branch from 893de2a to b7682b3 Compare September 5, 2017 13:01

carlossg changed the title ~~Move ProvisioningCallback logic to KubernetesLauncher~~ [JENKINS-35246] Kubernetes agents not getting deleted in Jenkins after pods are deleted Sep 5, 2017

Vlatombe force-pushed the provisioning_lifecycle_fix branch 2 times, most recently from d50c07a to 6f56cf9 Compare September 5, 2017 14:24

marvinthepa reviewed Sep 5, 2017

View reviewed changes

marvinthepa reviewed Sep 6, 2017

View reviewed changes

carlossg approved these changes Sep 7, 2017

View reviewed changes

marvinthepa reviewed Sep 11, 2017

View reviewed changes

Vlatombe force-pushed the provisioning_lifecycle_fix branch from ce28918 to 1c0f29a Compare September 11, 2017 07:23

Vlatombe added 11 commits September 11, 2017 11:06

Update jenkins-test-harness to fix TestExtension exceptions during tests

f3ec20b

Set hudson.slaves.NodeProvisioner.initialDelay=0 for hpi:run (faster …

357355f

…provisioning)

Fix findbugs issues

7e22d56

Retain binary compatibility

92da633

Remove unused imports

38abc14

Add missing header

f05c8dc

Flag unused constructor as deprecated

da505ea

Comment

83d067b

Add logging in case the cloud has been removed when slave is getting …

536eb78

…termination call.

SonarLint

05ef5e4

Vlatombe force-pushed the provisioning_lifecycle_fix branch from 1c0f29a to 05ef5e4 Compare September 11, 2017 09:09

carlossg merged commit aea6ecd into jenkinsci:master Sep 11, 2017

marvinthepa mentioned this pull request Sep 21, 2017

Only allow tasks after all containers in pod are ready #230

Merged

Vlatombe deleted the provisioning_lifecycle_fix branch December 23, 2020 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-35246] Kubernetes agents not getting deleted in Jenkins after pods are deleted #217

[JENKINS-35246] Kubernetes agents not getting deleted in Jenkins after pods are deleted #217

Vlatombe commented Sep 5, 2017 •

edited by carlossg

Loading

marvinthepa Sep 5, 2017

marvinthepa Sep 6, 2017

Vlatombe Sep 7, 2017

marvinthepa left a comment

marvinthepa Sep 8, 2017

marvinthepa Sep 8, 2017

marvinthepa Sep 8, 2017

marvinthepa Sep 8, 2017

Vlatombe commented Sep 11, 2017

marvinthepa commented Sep 19, 2017

Vlatombe commented Sep 19, 2017

dpyoung2 commented Sep 26, 2017

		@@ -0,0 +1,465 @@
		package org.csanchez.jenkins.plugins.kubernetes;

		@@ -24,68 +24,18 @@

		package org.csanchez.jenkins.plugins.kubernetes;

		import static org.csanchez.jenkins.plugins.kubernetes.KubernetesCloud.*;

[JENKINS-35246] Kubernetes agents not getting deleted in Jenkins after pods are deleted #217

[JENKINS-35246] Kubernetes agents not getting deleted in Jenkins after pods are deleted #217

Conversation

Vlatombe commented Sep 5, 2017 • edited by carlossg Loading

marvinthepa Sep 5, 2017

Choose a reason for hiding this comment

marvinthepa Sep 6, 2017

Choose a reason for hiding this comment

Vlatombe Sep 7, 2017

Choose a reason for hiding this comment

marvinthepa left a comment

Choose a reason for hiding this comment

marvinthepa Sep 8, 2017

Choose a reason for hiding this comment

marvinthepa Sep 8, 2017

Choose a reason for hiding this comment

marvinthepa Sep 8, 2017

Choose a reason for hiding this comment

marvinthepa Sep 8, 2017

Choose a reason for hiding this comment

Vlatombe commented Sep 11, 2017

marvinthepa commented Sep 19, 2017

Vlatombe commented Sep 19, 2017

dpyoung2 commented Sep 26, 2017

Vlatombe commented Sep 5, 2017 •

edited by carlossg

Loading