From 36a6cf8cad00d8f18377ff71c358f2257509e93f Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Thu, 8 Oct 2020 13:13:07 +0200 Subject: [PATCH 1/3] Make sure containerCap defaults to Integer.MAX_VALUE when starting from an empty KubernetesCloud --- .../jenkins/plugins/kubernetes/KubernetesCloud.java | 7 +++++-- .../jenkins/plugins/kubernetes/KubernetesCloudTest.java | 6 +++++- .../config.xml | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-) rename src/test/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest/{minRetentionTimeoutReadResolve => emptyKubernetesCloudReadResolve}/config.xml (98%) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java index e2c0a6ec85..fd2dc7cc7b 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java @@ -121,7 +121,7 @@ public class KubernetesCloud extends Cloud { private String jenkinsTunnel; @CheckForNull private String credentialsId; - private int containerCap = Integer.MAX_VALUE; + private Integer containerCap = Integer.MAX_VALUE; private int retentionTimeout = DEFAULT_RETENTION_TIMEOUT_MINUTES; private int connectTimeout = DEFAULT_CONNECT_TIMEOUT_SECONDS; private int readTimeout = DEFAULT_READ_TIMEOUT_SECONDS; @@ -994,7 +994,10 @@ private Object readResolve() { if (podLabels == null && labels != null) { setPodLabels(PodLabel.fromMap(labels)); } - + // if unset, should default to no limit + if (containerCap == null) { + containerCap = Integer.MAX_VALUE; + } return this; } diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java index 6dd269c5ce..d0920a339f 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java @@ -357,9 +357,13 @@ public void minRetentionTimeout() { @Test @LocalData - public void minRetentionTimeoutReadResolve() { + public void emptyKubernetesCloudReadResolve() { KubernetesCloud cloud = j.jenkins.clouds.get(KubernetesCloud.class); assertEquals(KubernetesCloud.DEFAULT_RETENTION_TIMEOUT_MINUTES, cloud.getRetentionTimeout()); + assertEquals(Integer.MAX_VALUE, cloud.getContainerCap()); + assertEquals(KubernetesCloud.DEFAULT_MAX_REQUESTS_PER_HOST, cloud.getMaxRequestsPerHost()); + assertEquals(PodRetention.getKubernetesCloudDefault(), cloud.getPodRetention()); + assertEquals(KubernetesCloud.DEFAULT_WAIT_FOR_POD_SEC, cloud.getWaitForPodSec()); } public HtmlInput getInputByName(DomElement root, String name) { diff --git a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest/minRetentionTimeoutReadResolve/config.xml b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest/emptyKubernetesCloudReadResolve/config.xml similarity index 98% rename from src/test/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest/minRetentionTimeoutReadResolve/config.xml rename to src/test/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest/emptyKubernetesCloudReadResolve/config.xml index 9f4d13773a..b2308d5598 100644 --- a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest/minRetentionTimeoutReadResolve/config.xml +++ b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest/emptyKubernetesCloudReadResolve/config.xml @@ -1,7 +1,7 @@ - 2.190.1 + 2.222.4 DEVELOPMENT 2 NORMAL From bd5d865fc82bcabe6984b6af81a42b02ab8434a4 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 9 Oct 2020 10:00:03 +0200 Subject: [PATCH 2/3] Reviews --- .../plugins/kubernetes/KubernetesCloud.java | 35 ++++++------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java index fd2dc7cc7b..eda1420b70 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java @@ -121,7 +121,7 @@ public class KubernetesCloud extends Cloud { private String jenkinsTunnel; @CheckForNull private String credentialsId; - private Integer containerCap = Integer.MAX_VALUE; + private Integer containerCap; private int retentionTimeout = DEFAULT_RETENTION_TIMEOUT_MINUTES; private int connectTimeout = DEFAULT_CONNECT_TIMEOUT_SECONDS; private int readTimeout = DEFAULT_READ_TIMEOUT_SECONDS; @@ -381,33 +381,21 @@ public void setCredentialsId(String credentialsId) { } public int getContainerCap() { - return containerCap; + return containerCap != null ? containerCap : Integer.MAX_VALUE; } @DataBoundSetter public void setContainerCapStr(String containerCapStr) { - if (containerCapStr.equals("")) { - setContainerCap(Integer.MAX_VALUE); - } else { - setContainerCap(Integer.parseInt(containerCapStr)); - } + setContainerCap(containerCapStr.equals("") ? null : Integer.parseInt(containerCapStr)); } - @DataBoundSetter - public void setContainerCap(int containerCap) { - if (containerCap < 0) { - this.containerCap = Integer.MAX_VALUE; - } else { - this.containerCap = containerCap; - } + public void setContainerCap(Integer containerCap) { + this.containerCap = (containerCap != null && containerCap >= 0) ? containerCap : null; } public String getContainerCapStr() { - if (containerCap == Integer.MAX_VALUE) { - return ""; - } else { - return String.valueOf(containerCap); - } + // serialized Integer.MAX_VALUE means no limit + return (containerCap == null || containerCap == Integer.MAX_VALUE) ? "" : String.valueOf(containerCap); } public int getReadTimeout() { @@ -586,6 +574,7 @@ public synchronized Collection provision(@CheckForN * */ private boolean addProvisionedSlave(@Nonnull PodTemplate template, @CheckForNull Label label, int scheduledCount) throws Exception { + int containerCap = getContainerCap(); if (containerCap == 0) { return false; } @@ -603,7 +592,7 @@ private boolean addProvisionedSlave(@Nonnull PodTemplate template, @CheckForNull if (allActiveSlavePods != null && containerCap <= allActiveSlavePods.size() + scheduledCount) { LOGGER.log(Level.INFO, "Maximum number of concurrently running agent pods ({0}) reached for Kubernetes Cloud {4}, not provisioning: {1} running or pending in namespace {2} with Kubernetes labels {3}", - new Object[] { containerCap, allActiveSlavePods.size() + scheduledCount, templateNamespace, getLabels(), name }); + new Object[] {containerCap, allActiveSlavePods.size() + scheduledCount, templateNamespace, getLabels(), name }); return false; } @@ -725,7 +714,7 @@ public boolean equals(Object o) { return skipTlsVerify == that.skipTlsVerify && addMasterProxyEnvVars == that.addMasterProxyEnvVars && capOnlyOnAlivePods == that.capOnlyOnAlivePods && - containerCap == that.containerCap && + Objects.equals(containerCap, that.containerCap) && retentionTimeout == that.retentionTimeout && connectTimeout == that.connectTimeout && readTimeout == that.readTimeout && @@ -994,10 +983,6 @@ private Object readResolve() { if (podLabels == null && labels != null) { setPodLabels(PodLabel.fromMap(labels)); } - // if unset, should default to no limit - if (containerCap == null) { - containerCap = Integer.MAX_VALUE; - } return this; } From 85455273f54e15e4029a7f8e4b281ef7dbcee236 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 9 Oct 2020 14:43:10 +0200 Subject: [PATCH 3/3] Revert some ws --- .../csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java index eda1420b70..eca6a20d17 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java @@ -592,7 +592,7 @@ private boolean addProvisionedSlave(@Nonnull PodTemplate template, @CheckForNull if (allActiveSlavePods != null && containerCap <= allActiveSlavePods.size() + scheduledCount) { LOGGER.log(Level.INFO, "Maximum number of concurrently running agent pods ({0}) reached for Kubernetes Cloud {4}, not provisioning: {1} running or pending in namespace {2} with Kubernetes labels {3}", - new Object[] {containerCap, allActiveSlavePods.size() + scheduledCount, templateNamespace, getLabels(), name }); + new Object[] { containerCap, allActiveSlavePods.size() + scheduledCount, templateNamespace, getLabels(), name }); return false; } @@ -983,6 +983,7 @@ private Object readResolve() { if (podLabels == null && labels != null) { setPodLabels(PodLabel.fromMap(labels)); } + return this; }