Maas client tests and retry cases improvement

maas-client/CHANGELOG.md

@@ -1,5 +1,48 @@
 # This page contains notably changes of maas-client project.
 
+## Unreleased
+* `Features`
+  - Calls to maas-agent survive a database leader switchover. Retryable: `IOException`, 5xx, 429,
+    and 405 when the response reason names a database that cannot be written — maas-service reports
+    a read-only database that way, so the usual "fail fast on 4xx" rule does not hold here. 401 is
+    not retried, because the token source refreshes on its own schedule. See "Retry behaviour and
+    configuration" in README.
+  - New configuration: `maas.http.retry.max-total-duration-ms`, 60s by default, bounding a whole
+    call including retries. Attempt count and backoff growth derive from it, and each attempt is
+    capped by what is left, so the worst case a caller sees is that duration rather than the
+    duration plus one `maas.http.timeout`. `0` disables retries; an unreadable or negative value
+    warns and falls back to the default.
+* `Behaviour changes`
+  - **A call that fails with a retryable status now takes longer before failing.** It used to throw
+    on the first unexpected 5xx or 405; it is now retried within the configured duration.
+  - Failed calls to maas-agent throw `MaaSHttpException` instead of a bare `RuntimeException`. It
+    extends `MaaSException`, so existing `catch` blocks keep working — note the widening:
+    `catch (MaaSException)` used to mean a MaaS business error and now also catches transport
+    failures.
+  - **`deleteTopic` is not retried, on any options.** It used to be, as any other call. Its response
+    carries how many topics were deleted, and a repeat of a delete whose response was lost reports
+    zero for a topic that is already gone. Callers that need the old behaviour must retry themselves
+    and treat `false` as "not found" rather than "not deleted".
+  - The Kafka `watch-create` long poll is paced. A down maas-agent used to be re-polled as fast as
+    the socket could refuse the connection; the poll now backs off exponentially up to 30s with
+    jitter, so instances that lose the same agent do not all return at the same moment. It keeps
+    retrying for the life of the client.
+  - The watch poll window derives from `maas.http.timeout`, 25s with the defaults, instead of a
+    fixed 60s that outlasted the read timeout and made every quiet poll fail locally. Tuning
+    `maas.http.timeout` now moves it; below 2s the watch is not usable.
+  - `KafkaMaaSClient.watchTopicCreate` throws `IllegalStateException` after `close()`, and after the
+    watch thread has stopped on its own, instead of registering a callback that can never fire.
+  - Interrupting a thread during a retry wait restores the interrupt flag and aborts, instead of
+    swallowing `InterruptedException`.
+* `Fixed`
+  - `deleteTopic` threw `NullPointerException` and `search` threw `NoSuchElementException` when
+    maas-agent answered 200 with an empty body. They now report nothing deleted and no topics found.
+  - A response without a body no longer throws while the client reads it, in `getOrCreateTopic`,
+    `deleteTopic` and the error paths.
+* `Dependencies`
+  - New runtime dependency `dev.failsafe:failsafe`, which carries the retry policies. It has no
+    transitive dependencies, but services with dependency convergence rules will see it appear.
+
 ## 10.0.0
 * `Features`
   - **Breaking:** Removed _MaaSAPIClient.loadConfiguration_ from public API.

maas-client/README.md

@@ -58,6 +58,62 @@ MaaSClient client = new MaaSAPIClientImpl(() -> M2MManager.getInstance().getToke
 ```
 
 
+## Retry behaviour and configuration
+
+Every call to maas-agent is retried before giving up, bounded by a single
+setting: the maximum total duration of the call.
+
+| Property | Default | Meaning |
+|---|---|---|
+| `maas.http.timeout` | `30` (seconds) | connect/read/write timeout of a **single** attempt |
+| `maas.http.retry.max-total-duration-ms` | `60000` | how long one call may take in **total**, retries included. `0` disables retries |
+
+`max-total-duration-ms` is the only retry knob: the attempt count and the pauses
+between attempts are derived from it. The first pause is 1s, each next one
+doubles, and the cap is a quarter of the total — with the default 60s that gives
+1s, 2s, 4s, 8s, 15s, 15s, roughly six attempts when each attempt fails fast. If
+attempts hang instead, fewer of them fit into the same duration. Backoff carries
++/-20% jitter so concurrent callers do not retry in lockstep.
+
+Each attempt is additionally bounded by what is left of the total duration, so
+the worst case a caller sees is that total duration itself rather than the total
+duration plus one `maas.http.timeout`.
+
+The 60s default is meant to outlast a database leader switchover while still
+failing fast enough to react to a real outage.
+
+The watch endpoint (`watch-create`) is excluded: it is a long poll with its own
+loop and its own backoff. Its window is derived from `maas.http.timeout` and stays
+below it — maas-service holds the request open for the whole window and then answers
+with an empty list, which the client has to be able to receive. With the default 30s
+timeout the window is 25s.
+
+`deleteTopic` is excluded as well, on any options: its response carries how many
+topics were deleted, and a repeat of a delete whose response was lost reports zero
+for a topic that is already gone. `getOrCreateTopic` is retried on any options —
+maas-service resolves the classifier before it looks at `onTopicExists`, so a
+repeated create returns the registration the first attempt made.
+
+Which responses are retried:
+
+| Response | Retried | Why |
+|---|---|---|
+| `IOException` | yes | connection refused/reset while the agent is being rescheduled |
+| 5xx | yes | includes the `500` maas-agent returns when it cannot reach maas-service at all |
+| 429 | yes | throttling |
+| **405** | **only when the `reason` names a database that cannot be written** | maas-service maps PostgreSQL error `25006` (READ ONLY SQL TRANSACTION) to `405`, so a write against a demoted Patroni node during a switchover arrives as `405`, not as `5xx`. A plain `405` — a route removed on the server, an ingress rejecting the method — is permanent and fails fast |
+| 401 | no | `CachingTokenSource` refreshes on its own polling interval, so a retry within the backoff reads the same token, and `M2MInterceptor` has already made its own 401 round trip by then |
+| other 4xx | no | permanent client errors, failed on the first attempt |
+
+The 405 entry is deliberate: the usual "retry 5xx, fail fast on 4xx" rule does not
+survive a database leader switchover here.
+
+The `reason` of the error envelope is what decides, not the error code: every
+maas-service error carries the same code, so the envelope alone says nothing. The
+match is loose — the reason has to mention a database together with `read-only`
+or `not active` — so a reworded message on the server still counts, while a `405`
+about a read-only *field* does not.
+
 ## Kafka client usage example
 All MaaS operations for Kafka is collected in [KafkaMaaSClient](https://github.com/Netcracker/qubership-maas-client/blob/main/client/src/main/java/com/netcracker/cloud/maas/client/api/kafka/KafkaMaaSClient.java). To obtain *new* instance of MaaS Kafka client just call: 
 ```java

maas-client/client/pom.xml

@@ -26,6 +26,10 @@
             <artifactId>lombok</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>dev.failsafe</groupId>
+            <artifactId>failsafe</artifactId>
+        </dependency>
         <!-- needed because of control-plane websock client implemented using okhttp client -->
         <dependency>
             <groupId>com.squareup.okhttp3</groupId>

maas-client/client/src/main/java/com/netcracker/cloud/maas/client/api/MaaSException.java

@@ -1,7 +1,13 @@
 package com.netcracker.cloud.maas.client.api;
 
 public class MaaSException extends RuntimeException {
+
     public MaaSException(String format, Object...args) {
         super(String.format(format, args));
     }
+
+    /** For a message that is already built and must not go through String.format. */
+    protected MaaSException(String message, Throwable cause) {
+        super(message, cause);
+    }
 }

maas-client/client/src/main/java/com/netcracker/cloud/maas/client/api/MaaSHttpException.java

@@ -0,0 +1,13 @@
+package com.netcracker.cloud.maas.client.api;
+
+/** A call to maas that did not succeed: an unexpected status code or a transport failure. */
+public class MaaSHttpException extends MaaSException {
+
+    public static MaaSHttpException of(String message) {
+        return new MaaSHttpException(message, null);
+    }
+
+    public MaaSHttpException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}

maas-client/client/src/main/java/com/netcracker/cloud/maas/client/impl/Env.java

@@ -35,6 +35,7 @@ public class Env {
     public static final String PROP_TENANT_MANAGER_URL = "maas.client.tenant-manager.url";
     public static final String PROP_TENANT_MANAGER_RECONNECT_TIMEOUT = "maas.client.tenant-manager.reconnect-timeout";
     public static final String PROP_HTTP_TIMEOUT = "maas.http.timeout";
+    public static final String PROP_HTTP_RETRY_MAX_TOTAL_DURATION_MS = "maas.http.retry.max-total-duration-ms";
 
     public static String apiUrl() {
         return apiUrl(M2MClient.isK8sM2mEnabled());
@@ -109,6 +110,37 @@ public static Duration httpTimeout() {
         );
     }
 
+    static final long DEFAULT_HTTP_RETRY_MAX_TOTAL_DURATION_MS = 60_000L;
+
+    /**
+     * How long one call may take in total, retries included. Zero leaves a single
+     * attempt; an unreadable or negative value falls back to the default with a warning.
+     */
+    public static Duration httpRetryMaxTotalDuration() {
+        return Duration.ofMillis(
+                stringProperty(PROP_HTTP_RETRY_MAX_TOTAL_DURATION_MS)
+                        .map(Env::parseRetryDurationMillis)
+                        .orElse(DEFAULT_HTTP_RETRY_MAX_TOTAL_DURATION_MS)
+        );
+    }
+
+    private static long parseRetryDurationMillis(String raw) {
+        long millis;
+        try {
+            millis = Long.parseLong(raw.trim());
+        } catch (NumberFormatException e) {
+            log.warn("Ignoring '{}={}': not a number of milliseconds, using {}ms",
+                    PROP_HTTP_RETRY_MAX_TOTAL_DURATION_MS, raw, DEFAULT_HTTP_RETRY_MAX_TOTAL_DURATION_MS);
+            return DEFAULT_HTTP_RETRY_MAX_TOTAL_DURATION_MS;
+        }
+        if (millis < 0) {
+            log.warn("Ignoring '{}={}': negative, using {}ms",
+                    PROP_HTTP_RETRY_MAX_TOTAL_DURATION_MS, raw, DEFAULT_HTTP_RETRY_MAX_TOTAL_DURATION_MS);
+            return DEFAULT_HTTP_RETRY_MAX_TOTAL_DURATION_MS;
+        }
+        return millis;
+    }
+
     public static String url2ws(String url) {
         return url.replaceAll("^http(s?):", "ws$1:");
     }
@@ -202,8 +234,11 @@ private static Optional<String> microProfileConfigOptional(String key) {
             Method getOptionalValue = config.getClass().getMethod("getOptionalValue", String.class, Class.class);
             return (Optional<String>) getOptionalValue.invoke(config, key, String.class);
         } catch (ClassNotFoundException e) {
+            // MicroProfile Config is an optional dependency
             return Optional.empty();
         } catch (Throwable e) {
+            // Throwable, not Exception: a broken config provider fails class initialisation with
+            // an Error, and the caller must still fall back to system properties and environment
             log.trace("MicroProfile Config not available or lookup failed for '{}'", key, e);
             return Optional.empty();
         }