[FIXED JENKINS-54770] Prevent multiple instances of KubernetesClient #397

fbelzunc · 2018-11-22T12:25:51Z

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

The current implementation tries to avoid the creation of a new KubernetesClient each time a connection is requested.

The implementation avoids the KubernetesClient to be recreated when saving the configuration in the case that any change was performed at Cloud level.

CC @escoem @Vlatombe @carlossg

added cache

ydubreuil

This looks good to me after comments are addressed

ydubreuil · 2018-11-22T13:01:46Z

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

+        CACHE_TTL = Integer.getInteger("org.csanchez.jenkins.plugins.kubernetes.clients.cacheTtl", 60);
+        clients = CacheBuilder.newBuilder()
+                .maximumSize(CACHE_SIZE)
+                .expireAfterWrite(CACHE_TTL, TimeUnit.MINUTES)


Could you add a listener on the cache with removalListener() to close all connections hold by a client when the client is evicted from the cache? The listener would basically just call KubernetesDefaultClient.close()

see https://google.github.io/guava/releases/16.0/api/docs/com/google/common/cache/CacheBuilder.html#removalListener(com.google.common.cache.RemovalListener) and https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/BaseClient.java#L76

This was address by the next commit

escoem · 2018-11-22T15:06:57Z

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

+        final Client c = clients.getIfPresent(cloudName);
+        if (c != null && validity == c.getValidity()) {
+            return c.getClient();
+        } else {


now we are closing connections in the listener, probably we should close also if c!=null && validity != c.getValidity() before creating a new KubernetesClient

it would be good to close existing client, because onRemoval could be called later in time

https://google.github.io/guava/releases/23.0/api/docs/com/google/common/cache/RemovalListener.html#onRemoval-com.google.common.cache.RemovalNotification-

Notifies the listener that a removal occurred at some point in the past.

carlossg

Added some comments, looks almost ready

Thanks!

carlossg · 2018-11-22T15:32:55Z

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

+                .maximumSize(CACHE_SIZE)
+                .expireAfterWrite(CACHE_TTL, TimeUnit.MINUTES)
+                .removalListener((RemovalListener<String, Client>) removalNotification -> {
+                    removalNotification.getValue().getClient().close();


see https://google.github.io/guava/releases/23.0/api/docs/com/google/common/cache/RemovalNotification.html

A notification of the removal of a single entry. The key and/or value may be null if they were already garbage collected.

so you need to check for null

carlossg · 2018-11-22T15:33:31Z

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

+        final Client c = clients.getIfPresent(cloudName);
+        if (c != null && validity == c.getValidity()) {
+            return c.getClient();
+        } else {


it would be good to close existing client, because onRemoval could be called later in time

https://google.github.io/guava/releases/23.0/api/docs/com/google/common/cache/RemovalListener.html#onRemoval-com.google.common.cache.RemovalNotification-

Notifies the listener that a removal occurred at some point in the past.

carlossg · 2018-11-22T15:49:11Z

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

@@ -46,6 +46,7 @@
 /**
 * @author <a href="mailto:nicolas.deloof@gmail.com">Nicolas De Loof</a>
 */
+@Deprecated


why deprecate it if you are still using it in a new method?

Because we don't want to have a public class creating KubernetesClients from outside the (protected) Provider. In the future, we will at least reduce the visibility of the factoryAdapter.

In this case, you should add @Restricted(NoExternalUse.class) to prevent any actual usage outside the plugin, and also add a short comment to motivate the deprecation to potential users.

carlossg · 2018-11-22T15:50:46Z

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


        LOGGER.log(Level.FINE, "Building connection to Kubernetes {0} URL {1} namespace {2}",
                new String[] { getDisplayName(), serverUrl, namespace });
-        client = new KubernetesFactoryAdapter(serverUrl, namespace, serverCertificate, credentialsId, skipTlsVerify,
-                connectTimeout, readTimeout, maxRequestsPerHost).createClient();
+        KubernetesClient client = KubernetesClientProvider.createClient(name, serverUrl, namespace, serverCertificate, credentialsId, skipTlsVerify,


NIT I would just use KubernetesClientProvider.createClient(this) instead of passing 8 parameters

Vlatombe · 2018-11-23T08:33:37Z

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

+
+    static KubernetesClient createClient(final String cloudName, final String serviceAddress, final String namespace, @CheckForNull final String caCertData,
+                                                @CheckForNull final String credentials, final boolean skipTlsVerify, final int connectTimeout, final int readTimeout, final int maxRequestsPerHost) throws NoSuchAlgorithmException, UnrecoverableKeyException,
+            KeyStoreException, IOException, CertificateEncodingException, ExecutionException {


ExecutionException is not used afaics

Vlatombe · 2018-11-23T08:39:19Z

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

@@ -46,6 +46,7 @@
 /**
 * @author <a href="mailto:nicolas.deloof@gmail.com">Nicolas De Loof</a>
 */
+@Deprecated


In this case, you should add @Restricted(NoExternalUse.class) to prevent any actual usage outside the plugin, and also add a short comment to motivate the deprecation to potential users.

Vlatombe · 2018-11-23T08:41:04Z

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

@@ -421,12 +421,12 @@ public void setPodRetention(PodRetention podRetention) {
     */
    @SuppressFBWarnings({ "IS2_INCONSISTENT_SYNC", "DC_DOUBLECHECK" })
    public KubernetesClient connect() throws UnrecoverableKeyException, NoSuchAlgorithmException, KeyStoreException,
-            IOException, CertificateEncodingException {
+            IOException, CertificateEncodingException, ExecutionException {


ExecutionException is not required (here, and in other references across the PR)

Vlatombe · 2018-11-23T10:35:15Z

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

@@ -643,6 +646,7 @@ public FormValidation doTestConnection(@QueryParameter String name, @QueryParame

                // test listing pods
                client.pods().list();
+                client.close();


Use the try-with-resources construct instead. https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

Vlatombe

lgtm

fbelzunc and others added 9 commits November 22, 2018 11:03

Create KubernetesClientHelper

5ef0807

added cache

c722364

Merge pull request #1 from escoem/kubernetes-client-service

5fe5a32

added cache

Exception management + some clean-up

eb7ea27

Deprecate KubernetesFactoryAdapter

faabe76

some more clean-up

e551797

clean-up

7164594

Remove KubernetesClient

f0c708f

Update surefire

f4e67c1

ydubreuil suggested changes Nov 22, 2018

View reviewed changes

Add a removalListener to close the wiped out connections

dd235c6

escoem reviewed Nov 22, 2018

View reviewed changes

Add comment about cache expiration

2df234a

carlossg reviewed Nov 22, 2018

View reviewed changes

Vlatombe reviewed Nov 23, 2018

View reviewed changes

fbelzunc added 3 commits November 23, 2018 11:21

Address different comments on the PR

dad3c28

Resolve merge conflicts

c7f99c0

Address comments

ebd2ebe

carlossg approved these changes Nov 23, 2018

View reviewed changes

carlossg changed the title ~~[FIXED JENKINS-54770] Manage KubernetesClient~~ [FIXED JENKINS-54770] Prevent multiple instances of KubernetesClient Nov 23, 2018

Vlatombe reviewed Nov 23, 2018

View reviewed changes

fbelzunc added 2 commits November 23, 2018 11:47

Use the try-with-resources Statement

4532cd6

Correcly implement The try-with-resources Statement

1c0a64d

ydubreuil approved these changes Nov 23, 2018

View reviewed changes

Vlatombe approved these changes Nov 23, 2018

View reviewed changes

Fix findbugs

f536cb7

carlossg merged commit 656b807 into jenkinsci:master Nov 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED JENKINS-54770] Prevent multiple instances of KubernetesClient #397

[FIXED JENKINS-54770] Prevent multiple instances of KubernetesClient #397

fbelzunc commented Nov 22, 2018

ydubreuil left a comment

ydubreuil Nov 22, 2018

fbelzunc Nov 22, 2018

escoem Nov 22, 2018

carlossg Nov 22, 2018

carlossg left a comment

carlossg Nov 22, 2018

carlossg Nov 22, 2018

carlossg Nov 22, 2018

escoem Nov 23, 2018

Vlatombe Nov 23, 2018

carlossg Nov 22, 2018

Vlatombe Nov 23, 2018

Vlatombe Nov 23, 2018

Vlatombe Nov 23, 2018 •

edited

Loading

Vlatombe Nov 23, 2018

Vlatombe left a comment

[FIXED JENKINS-54770] Prevent multiple instances of KubernetesClient #397

[FIXED JENKINS-54770] Prevent multiple instances of KubernetesClient #397

Conversation

fbelzunc commented Nov 22, 2018

ydubreuil 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

carlossg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vlatombe Nov 23, 2018 • edited Loading

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

Vlatombe Nov 23, 2018 •

edited

Loading