viirya commented on code in PR #860:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/860#discussion_r4065830351
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -297,20 +315,53 @@ public static boolean isFirstAttempt(Context<?> context) {
}
/**
- * Whether the given failure is transport level rather than a decision by
the API server.
+ * Whether the given failure is expected to clear without anyone acting on
it, so that a caller
+ * may wait it out rather than report it. A broken connection and an
overloaded or proxied server
+ * qualify; a rejection by a reachable API server and a client side
rejection do not.
*
* @param e The failure to classify.
- * @return True if the request did not reach a healthy API server, false
otherwise.
+ * @return True if the failure is expected to clear on its own, false
otherwise.
*/
public static boolean isTransientError(KubernetesClientException e) {
- // code 0 is fabric8's sentinel for network-level failures (timeouts,
connection resets, etc.)
return switch (e.getCode()) {
- case 0, HTTP_CLIENT_TIMEOUT, HTTP_BAD_GATEWAY,
- HTTP_UNAVAILABLE, HTTP_GATEWAY_TIMEOUT -> true;
+ case NO_RESPONSE_CODE -> brokeOnTheWayToTheApiServer(e);
+ case HTTP_CLIENT_TIMEOUT, HTTP_BAD_GATEWAY, HTTP_UNAVAILABLE,
HTTP_GATEWAY_TIMEOUT -> true;
default -> false;
};
}
+ /**
+ * Whether the given failure left the request unanswered, so that asking
again may yet work. It
+ * is defined as the complement of the two status-less failures that
repeating cannot change: a
+ * rejection the client raised before sending anything, which carries no
cause at all, and an
+ * answer that arrived but could not be used, which carries the parsing or
certificate failure
+ * that rejected it. Anything else that carries a cause counts as unanswered.
+ *
+ * <p>Naming what cannot work, rather than what can, keeps this from
tracking the exception types
+ * of whichever HTTP client is plugged in. A connection the peer closes
mid-response is the case
+ * that matters: the client in use reports it with a type of its own which
is not even an {@link
+ * java.io.IOException}, so any list of recognized connection failures would
silently miss it.
+ *
+ * @param e The failure to inspect.
+ * @return True if the request went unanswered, false otherwise.
+ */
+ private static boolean brokeOnTheWayToTheApiServer(KubernetesClientException
e) {
+ // Bounded walk: a cyclic cause chain must not hang the reconciler. The
marker is nested, since
+ // fabric8 wraps the failure and its own HTTP client wraps it again.
+ Throwable cause = e.getCause();
+ for (int depth = 0; cause != null && depth < MAX_CAUSE_DEPTH; depth++) {
+ // A handshake that failed over a certificate, a host name or a trust
store is the one
+ // answer-less failure that repeating cannot fix, so it is grouped with
the unusable answers.
+ if (cause instanceof JsonProcessingException
+ || cause instanceof SSLHandshakeException
+ || cause instanceof SSLPeerUnverifiedException) {
+ return false;
Review Comment:
[P2] Preserve retryability for TLS handshake timeouts
Could we distinguish certificate validation failures from transient
handshake failures here? Netty's `SslHandshakeTimeoutException` extends
`SSLHandshakeException`, so this condition also rejects a TLS handshake timeout
caused by a temporarily unresponsive API server or proxy.
`getResourceStrictly()` then propagates the initial GET failure, and the CREATE
path skips its retry/backoff branch; `AppInitStep` can still turn that
temporary failure into `SchedulingFailure`. Please narrow the permanent-failure
check and add a handshake-timeout regression test while retaining coverage for
certificate rejection.
Reference: [Netty's exception definition at the pinned
version](https://github.com/netty/netty/blob/netty-4.2.18.Final/handler/src/main/java/io/netty/handler/ssl/SslHandshakeTimeoutException.java).
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -297,20 +315,53 @@ public static boolean isFirstAttempt(Context<?> context) {
}
/**
- * Whether the given failure is transport level rather than a decision by
the API server.
+ * Whether the given failure is expected to clear without anyone acting on
it, so that a caller
+ * may wait it out rather than report it. A broken connection and an
overloaded or proxied server
+ * qualify; a rejection by a reachable API server and a client side
rejection do not.
*
* @param e The failure to classify.
- * @return True if the request did not reach a healthy API server, false
otherwise.
+ * @return True if the failure is expected to clear on its own, false
otherwise.
*/
public static boolean isTransientError(KubernetesClientException e) {
- // code 0 is fabric8's sentinel for network-level failures (timeouts,
connection resets, etc.)
return switch (e.getCode()) {
- case 0, HTTP_CLIENT_TIMEOUT, HTTP_BAD_GATEWAY,
- HTTP_UNAVAILABLE, HTTP_GATEWAY_TIMEOUT -> true;
+ case NO_RESPONSE_CODE -> brokeOnTheWayToTheApiServer(e);
+ case HTTP_CLIENT_TIMEOUT, HTTP_BAD_GATEWAY, HTTP_UNAVAILABLE,
HTTP_GATEWAY_TIMEOUT -> true;
default -> false;
};
}
+ /**
+ * Whether the given failure left the request unanswered, so that asking
again may yet work. It
+ * is defined as the complement of the two status-less failures that
repeating cannot change: a
+ * rejection the client raised before sending anything, which carries no
cause at all, and an
+ * answer that arrived but could not be used, which carries the parsing or
certificate failure
+ * that rejected it. Anything else that carries a cause counts as unanswered.
+ *
+ * <p>Naming what cannot work, rather than what can, keeps this from
tracking the exception types
+ * of whichever HTTP client is plugged in. A connection the peer closes
mid-response is the case
+ * that matters: the client in use reports it with a type of its own which
is not even an {@link
+ * java.io.IOException}, so any list of recognized connection failures would
silently miss it.
+ *
+ * @param e The failure to inspect.
+ * @return True if the request went unanswered, false otherwise.
+ */
+ private static boolean brokeOnTheWayToTheApiServer(KubernetesClientException
e) {
+ // Bounded walk: a cyclic cause chain must not hang the reconciler. The
marker is nested, since
+ // fabric8 wraps the failure and its own HTTP client wraps it again.
+ Throwable cause = e.getCause();
+ for (int depth = 0; cause != null && depth < MAX_CAUSE_DEPTH; depth++) {
+ // A handshake that failed over a certificate, a host name or a trust
store is the one
+ // answer-less failure that repeating cannot fix, so it is grouped with
the unusable answers.
+ if (cause instanceof JsonProcessingException
+ || cause instanceof SSLHandshakeException
+ || cause instanceof SSLPeerUnverifiedException) {
+ return false;
+ }
+ cause = cause.getCause();
+ }
+ return e.getCause() != null;
Review Comment:
[P2] Exclude thread interruption from retryable failures
This fallback also classifies thread interruption as a transient transport
failure. In fabric8, `OperationSupport.waitForResult()` restores the interrupt
flag and wraps `InterruptedException` in `InterruptedIOException`, which can
reach this method inside a code `-1` `KubernetesClientException`. The create
path will then issue additional GET/CREATE attempts, while `backoffSleep()`
immediately exits because the thread remains interrupted. Could we detect
actual interruption in the cause chain and stop retrying it? Please avoid
excluding every `InterruptedIOException`, since `SocketTimeoutException`
extends that class and should remain retryable.
Reference: [fabric8's interruption
handling](https://github.com/fabric8io/kubernetes-client/blob/v7.8.0/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/OperationSupport.java#L489-L510).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
