Repository: hazelcast/hazelcast-kubernetes (java)

Issue:
Make number of retries (to Kubernetes API) configurable
Currently the number of retries of calls to Kubernetes API is hardcoded to 10. It should be made configurable with a property and the default value should be reduced to 5.

Pull request description:
Add `kubernetes-api-retries` property
Make the number of retries configurable and change the default value to 3.

Before, the number of retires was hardcoded to 10, which is high and if there was a retry error, the stacktrace was not visible until the retries finished (after ~5 min). What's more, in Kubernetes it's common that the POD was restarted sooner than the retries finish, which resulted in no StackTrace at all. 

fix #130 

Patch:
diff --git a/README.md b/README.md
index 6b47a38..523060f 100644
--- a/README.md
+++ b/README.md
@@ -123,7 +123,8 @@ There are 4 properties to configure the plugin, all of them are optional.
  * `namespace`: Kubernetes Namespace where Hazelcast is running; if not specified, the value is taken from the environment variables `KUBERNETES_NAMESPACE` or `OPENSHIFT_BUILD_NAMESPACE`
  * `service-name`: service name used to scan only PODs connected to the given service; if not specified, then all PODs in the namespace are checked
  * `service-label-name`, `service-label-value`: service label and value used to tag services that should form the Hazelcast cluster together
- * `resolve-not-ready-addresses`: if set to `true`, it checks also the addresses of PODs which are not ready; `false` by default 
+ * `resolve-not-ready-addresses`: if set to `true`, it checks also the addresses of PODs which are not ready; `false` by default
+ * `kubernetes-api-retries`: number of retries in case of issues while connecting to Kubernetes API; defaults to `3` 
  * `kubernetes-master`: URL of Kubernetes Master; `https://kubernetes.default.svc` by default
  * `api-token`: API Token to Kubernetes API; if not specified, the value is taken from the file `/var/run/secrets/kubernetes.io/serviceaccount/token`
  * `ca-certificate`: CA Certificate for Kubernetes API; if not specified, the value is taken from the file `/var/run/secrets/kubernetes.io/serviceaccount/ca.crt`
diff --git a/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java b/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java
index 19cc2f3..d9848e2 100644
--- a/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java
+++ b/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java
@@ -35,6 +35,7 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
+import static com.hazelcast.kubernetes.KubernetesProperties.KUBERNETES_API_RETIRES;
 import static com.hazelcast.kubernetes.KubernetesProperties.KUBERNETES_API_TOKEN;
 import static com.hazelcast.kubernetes.KubernetesProperties.KUBERNETES_CA_CERTIFICATE;
 import static com.hazelcast.kubernetes.KubernetesProperties.KUBERNETES_MASTER_URL;
@@ -53,6 +54,7 @@ final class HazelcastKubernetesDiscoveryStrategy
 
     private static final String DEFAULT_MASTER_URL = "https://kubernetes.default.svc";
     private static final int DEFAULT_SERVICE_DNS_TIMEOUT_SECONDS = 5;
+    private static final int DEFAULT_KUBERNETES_API_RETRIES = 3;
 
     private final String namespace;
 
@@ -73,15 +75,11 @@ final class HazelcastKubernetesDiscoveryStrategy
         String serviceLabelValue = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, SERVICE_LABEL_VALUE, "true");
         namespace = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, NAMESPACE, getNamespaceOrDefault());
         Boolean resolveNotReadyAddresses = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, RESOLVE_NOT_READY_ADDRESSES, false);
+        int kubernetesApiRetries
+                = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, KUBERNETES_API_RETIRES, DEFAULT_KUBERNETES_API_RETRIES);
         String kubernetesMaster = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, KUBERNETES_MASTER_URL, DEFAULT_MASTER_URL);
-        String apiToken = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, KUBERNETES_API_TOKEN, null);
-        if (apiToken == null) {
-            apiToken = readAccountToken();
-        }
-        String caCertificate = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, KUBERNETES_CA_CERTIFICATE, null);
-        if (caCertificate == null) {
-            caCertificate = readCaCertificate();
-        }
+        String apiToken = getApiToken(properties);
+        String caCertificate = caCertificate(properties);
 
         logger.info("Kubernetes Discovery properties: { "
                 + "service-dns: " + serviceDns + ", "
@@ -92,9 +90,10 @@ final class HazelcastKubernetesDiscoveryStrategy
                 + "service-label-value: " + serviceLabelValue + ", "
                 + "namespace: " + namespace + ", "
                 + "resolve-not-ready-addresses: " + resolveNotReadyAddresses + ", "
+                + "kubernetes-api-retries: " + kubernetesApiRetries + ", "
                 + "kubernetes-master: " + kubernetesMaster + "}");
 
-        client = buildKubernetesClient(namespace, kubernetesMaster, apiToken, caCertificate);
+        client = buildKubernetesClient(namespace, kubernetesMaster, apiToken, caCertificate, kubernetesApiRetries);
         if (serviceDns != null) {
             endpointResolver = new DnsEndpointResolver(logger, serviceDns, port, serviceDnsTimeout);
         } else {
@@ -104,6 +103,22 @@ final class HazelcastKubernetesDiscoveryStrategy
         logger.info("Kubernetes Discovery activated resolver: " + endpointResolver.getClass().getSimpleName());
     }
 
+    private String getApiToken(Map<String, Comparable> properties) {
+        String apiToken = getOrNull(properties, KUBERNETES_SYSTEM_PREFIX, KUBERNETES_API_TOKEN);
+        if (apiToken == null) {
+            apiToken = readAccountToken();
+        }
+        return apiToken;
+    }
+
+    private String caCertificate(Map<String, Comparable> properties) {
+        String caCertificate = getOrNull(properties, KUBERNETES_SYSTEM_PREFIX, KUBERNETES_CA_CERTIFICATE);
+        if (caCertificate == null) {
+            caCertificate = readCaCertificate();
+        }
+        return caCertificate;
+    }
+
     private String getNamespaceOrDefault() {
         String namespace = System.getenv("KUBERNETES_NAMESPACE");
         if (namespace == null) {
@@ -240,8 +255,8 @@ final class HazelcastKubernetesDiscoveryStrategy
     }
 
     private KubernetesClient buildKubernetesClient(String namespace, String kubernetesMaster, String accessToken,
-                                                   String caCertificate) {
-        return new KubernetesClient(namespace, kubernetesMaster, accessToken, caCertificate);
+                                                   String caCertificate, int retries) {
+        return new KubernetesClient(namespace, kubernetesMaster, accessToken, caCertificate, retries);
     }
 
     @SuppressFBWarnings("DMI_HARDCODED_ABSOLUTE_FILENAME")
diff --git a/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategyFactory.java b/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategyFactory.java
index f8fa8ca..d8f9308 100644
--- a/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategyFactory.java
+++ b/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategyFactory.java
@@ -44,6 +44,7 @@ public class HazelcastKubernetesDiscoveryStrategyFactory
                 KubernetesProperties.SERVICE_LABEL_VALUE,
                 KubernetesProperties.NAMESPACE,
                 KubernetesProperties.RESOLVE_NOT_READY_ADDRESSES,
+                KubernetesProperties.KUBERNETES_API_RETIRES,
                 KubernetesProperties.KUBERNETES_MASTER_URL,
                 KubernetesProperties.KUBERNETES_API_TOKEN,
                 KubernetesProperties.KUBERNETES_CA_CERTIFICATE,
diff --git a/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java b/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java
index b2141be..6a9ba01 100644
--- a/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java
+++ b/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java
@@ -43,7 +43,6 @@ import static java.util.Collections.emptyList;
 class KubernetesClient {
     private static final ILogger LOGGER = Logger.getLogger(KubernetesClient.class);
 
-    private static final int RETRIES = 10;
     private static final List<String> NON_RETRYABLE_KEYWORDS = asList(
             "\"reason\":\"Forbidden\"",
             "\"reason\":\"Unauthorized\"",
@@ -53,14 +52,16 @@ class KubernetesClient {
     private final String kubernetesMaster;
     private final String apiToken;
     private final String caCertificate;
+    private final int retries;
 
     private boolean isNoPublicIpAlreadyLogged;
 
-    KubernetesClient(String namespace, String kubernetesMaster, String apiToken, String caCertificate) {
+    KubernetesClient(String namespace, String kubernetesMaster, String apiToken, String caCertificate, int retries) {
         this.namespace = namespace;
         this.kubernetesMaster = kubernetesMaster;
         this.apiToken = apiToken;
         this.caCertificate = caCertificate;
+        this.retries = retries;
     }
 
     /**
@@ -459,7 +460,7 @@ class KubernetesClient {
                                          .get())
                         .asObject();
             }
-        }, RETRIES, NON_RETRYABLE_KEYWORDS);
+        }, retries, NON_RETRYABLE_KEYWORDS);
     }
 
     @SuppressWarnings("checkstyle:magicnumber")
diff --git a/src/main/java/com/hazelcast/kubernetes/KubernetesProperties.java b/src/main/java/com/hazelcast/kubernetes/KubernetesProperties.java
index ef1fa52..bad8ca7 100644
--- a/src/main/java/com/hazelcast/kubernetes/KubernetesProperties.java
+++ b/src/main/java/com/hazelcast/kubernetes/KubernetesProperties.java
@@ -91,6 +91,12 @@ public final class KubernetesProperties {
      */
     public static final PropertyDefinition RESOLVE_NOT_READY_ADDRESSES = property("resolve-not-ready-addresses", BOOLEAN);
 
+    /**
+     * <p>Configuration key: <tt>kubernetes-api-retries</tt></p>
+     * Defines the number of retries to Kubernetes API. Defaults to: 3.
+     */
+    public static final PropertyDefinition KUBERNETES_API_RETIRES = property("kubernetes-api-retries", INTEGER);
+
     /**
      * <p>Configuration key: <tt>kubernetes-master</tt></p>
      * Defines an alternative address for the kubernetes master. Defaults to: <tt>https://kubernetes.default.svc</tt>


Test patch:
diff --git a/src/test/java/com/hazelcast/kubernetes/KubernetesClientTest.java b/src/test/java/com/hazelcast/kubernetes/KubernetesClientTest.java
index 6ac7fb8..e715cf0 100644
--- a/src/test/java/com/hazelcast/kubernetes/KubernetesClientTest.java
+++ b/src/test/java/com/hazelcast/kubernetes/KubernetesClientTest.java
@@ -47,6 +47,7 @@ public class KubernetesClientTest {
     private static final String TOKEN = "sample-token";
     private static final String CA_CERTIFICATE = "sample-ca-certificate";
     private static final String NAMESPACE = "sample-namespace";
+    private static final int RETRIES = 3;
 
     @Rule
     public WireMockRule wireMockRule = new WireMockRule(wireMockConfig().dynamicPort());
@@ -56,7 +57,7 @@ public class KubernetesClientTest {
     @Before
     public void setUp() {
         String kubernetesMasterUrl = String.format("http://%s:%d", KUBERNETES_MASTER_IP, wireMockRule.port());
-        kubernetesClient = new KubernetesClient(NAMESPACE, kubernetesMasterUrl, TOKEN, CA_CERTIFICATE);
+        kubernetesClient = new KubernetesClient(NAMESPACE, kubernetesMasterUrl, TOKEN, CA_CERTIFICATE, RETRIES);
         stubFor(get(urlMatching("/api/.*")).atPriority(5)
                                            .willReturn(aResponse().withStatus(401).withBody("\"reason\":\"Forbidden\"")));
     }