peter-toth commented on code in PR #853:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/853#discussion_r4057681791
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -181,24 +187,54 @@ public static void addOwnerReferenceSecondaryResource(
}
/**
- * Retrieves a Kubernetes resource by its desired state.
+ * Retrieves a Kubernetes resource by its desired state, reporting a
resource that could not be
+ * read as absent.
*
* @param client The KubernetesClient.
* @param desired The desired state of the resource.
* @param <T> The type of the resource, extending HasMetadata.
- * @return An Optional containing the retrieved resource, or empty if not
found.
+ * @return An Optional containing the retrieved resource, or empty if not
found or not readable.
*/
public static <T extends HasMetadata> Optional<T> getResource(
final KubernetesClient client, final T desired) {
- T resource = null;
try {
- resource = client.resource(desired).get();
+ return getResourceStrictly(client, desired);
+ } catch (KubernetesClientException e) {
+ log.warn("Failed to read the resource with responseCode={}, considering
it absent.",
+ e.getCode(), e);
+ return Optional.empty();
+ }
+ }
+
+ /**
+ * Retrieves a Kubernetes resource by its desired state, telling a missing
resource apart from a
+ * read the API server refused. A transient failure keeps reporting the
resource as absent, since
+ * the request did not reach a healthy API server and the create path, which
re-reads on an
+ * AlreadyExists conflict, still resolves the actual state.
+ *
+ * @param client The KubernetesClient.
+ * @param desired The desired state of the resource.
+ * @param <T> The type of the resource, extending HasMetadata.
+ * @return An Optional containing the retrieved resource, or empty if not
found or not reachable.
+ * @throws KubernetesClientException if the API server refused the read.
+ */
+ private static <T extends HasMetadata> Optional<T> getResourceStrictly(
+ final KubernetesClient client, final T desired) {
+ try {
+ return Optional.ofNullable(client.resource(desired).get());
} catch (KubernetesClientException e) {
if (e.getCode() == HTTP_NOT_FOUND) {
return Optional.empty();
}
+ if (isTransientError(e) || e.getCode() == HTTP_INTERNAL_ERROR) {
Review Comment:
**Finding 1.** 429 lands in neither branch, so it falls through to `throw
e`. That makes an API Priority and Fairness throttle fatal on a path this
method otherwise treats as retryable.
I drove `AppInitStep.reconcile` with a client whose driver-pod `get()`
throws 429 and whose `create()` succeeds:
```
main 9a61e23 : resulting state = DriverRequested
head 7989aba : resulting state = SchedulingFailure
```
`RestartConfig.restartPolicy` defaults to `Never` and
`RestartPolicy.attemptRestartOnState(Never, …)` is unconditionally false, so
that `SchedulingFailure` is the end of the application. All three `AppInitStep`
call sites run inside the `catch (Exception e)` that writes it.
The inconsistency is inside this same method. `getOrCreateSecondaryResource`
has a dedicated 429 branch on the create side, and `shouldBackoffBeforeRetry`
returns true for both 429 and a present `Retry-After`:
```java
} else if (e.getCode() == Constants.HTTP_TOO_MANY_REQUESTS) {
log.debug("Server returned 429 Too Many Requests, will retry
with backoff");
```
So the codebase already says 429 means "come back later". Your own javadoc
frames the throwing bucket as "the API server refused the read", and a throttle
is a deferral rather than a refusal, which is the same argument.
`Constants` is already imported here, so it is one clause. I applied exactly
this and re-ran the 429 arm, which then reaches `DriverRequested` like main:
```suggestion
if (isTransientError(e)
|| e.getCode() == HTTP_INTERNAL_ERROR
|| e.getCode() == Constants.HTTP_TOO_MANY_REQUESTS) {
```
`createsResourceWhenInitialReadDoesNotReachApiServer` should then take 429
in its `@ValueSource`. Its name is already a slight stretch for 500, where the
server did answer, and would be more so for 429 — something like
`createsResourceWhenInitialReadGetsNoTrustworthyAnswer` would cover all three.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -181,24 +187,54 @@ public static void addOwnerReferenceSecondaryResource(
}
/**
- * Retrieves a Kubernetes resource by its desired state.
+ * Retrieves a Kubernetes resource by its desired state, reporting a
resource that could not be
+ * read as absent.
*
* @param client The KubernetesClient.
* @param desired The desired state of the resource.
* @param <T> The type of the resource, extending HasMetadata.
- * @return An Optional containing the retrieved resource, or empty if not
found.
+ * @return An Optional containing the retrieved resource, or empty if not
found or not readable.
*/
public static <T extends HasMetadata> Optional<T> getResource(
final KubernetesClient client, final T desired) {
- T resource = null;
try {
- resource = client.resource(desired).get();
+ return getResourceStrictly(client, desired);
+ } catch (KubernetesClientException e) {
+ log.warn("Failed to read the resource with responseCode={}, considering
it absent.",
+ e.getCode(), e);
+ return Optional.empty();
+ }
+ }
+
+ /**
+ * Retrieves a Kubernetes resource by its desired state, telling a missing
resource apart from a
+ * read the API server refused. A transient failure keeps reporting the
resource as absent, since
+ * the request did not reach a healthy API server and the create path, which
re-reads on an
+ * AlreadyExists conflict, still resolves the actual state.
+ *
+ * @param client The KubernetesClient.
+ * @param desired The desired state of the resource.
+ * @param <T> The type of the resource, extending HasMetadata.
+ * @return An Optional containing the retrieved resource, or empty if not
found or not reachable.
+ * @throws KubernetesClientException if the API server refused the read.
+ */
+ private static <T extends HasMetadata> Optional<T> getResourceStrictly(
Review Comment:
**Finding 2.** `private` leaves the one `getResource` caller outside this
class with exactly the bug the description argues against.
`spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:208`
is that caller, unchanged on main and on #847's head:
```java
private boolean isMasterRequested(SparkClusterContext context) {
return ReconcilerUtils.getResource(context.getClient(),
context.getMasterStatefulSetSpec())
.isPresent();
}
```
Both of its call sites read a refused read as "the master was never
requested". The one that matters is the suspend branch at
`ClusterInitStep.java:71`:
```java
if (cluster.getSpec().isSuspend() && !isMasterRequested(context)) {
...
if (KueueWorkloadFactory.hasQueueName(cluster)) {
KueueWorkloadUtils.releaseWorkload(context.getClient(), cluster);
}
```
So a 403 on the `StatefulSet` read while a suspend is applied deletes the
Kueue `Workload` of a cluster whose master is running, dropping quota that is
still consumed. The Kueue-hold site at `ClusterInitStep.java:178` takes the
same false branch but is idempotent, so the suspend one is the fix.
`ReconcilerUtils` is in `…operator.utils` and `ClusterInitStep` in
`…reconciler.reconcilesteps`, so package-private will not reach it — it needs
`public`. `isMasterRequested` already runs inside `ClusterInitStep.reconcile`'s
`try`, so a throw lands in the existing `SchedulingFailure` path with no new
handling. A follow-up is fine if you would rather keep this PR to
`ReconcilerUtils`, but the helper has to be reachable for that follow-up to
exist.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -55,6 +55,7 @@
/** Utility class for reconciler operations. */
@Slf4j
+@SuppressWarnings("PMD.GodClass")
Review Comment:
**Finding 3.** The premise checks out. I removed the annotation and ran
`:spark-operator:pmdMain` on this head:
```
ReconcilerUtils.java:58: GodClass: Possible God Class (WMC=50, ATFD=25,
TCC=10.909%)
```
The default threshold is WMC 47, so base sat at 47 and this PR takes it to
50. The mechanism is fine — `GodClass` is class-scoped, there is nothing
narrower to suppress — but the effect is that the rule is off for this class
from here on, and the number is already moving in the wrong direction.
`TCC=10.909%` is the part worth acting on: these methods barely share state
because the class is four unrelated groups. `toUpdateControl`/`toDeleteControl`
convert control objects,
`getOrCreateSecondaryResource`/`getResource`/`getResourceStrictly`/`deleteResourceIfExists`
are CRUD helpers,
`isTransientError`/`shouldBackoffBeforeRetry`/`isFirstAttempt` classify
failures, and `clone` is a JSON deep copy with nothing to do with reconciling —
`ModelUtils` in `spark-operator-api` already owns the `objectMapper` it uses.
I tried moving `clone` out to see whether that alone gets under the
threshold and my quick edit tripped other PMD rules, so I am not claiming a
specific refactor fixes the number. The ask is only that the suppression not be
the permanent answer: a follow-up ticket to split the class, referenced from
the annotation, keeps the signal alive.
--
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]
