Skip to content

Commit 17f4146

Browse files
committed
[SPARK-59931] Simplify the K8s event messages of errors and SchedulingFailure
### What changes were proposed in this pull request? This PR aims to simplify the K8s event messages of errors and `SchedulingFailure`. - `EventUtils.describe` uses the message of the API server for a rejected request, like the K8s built-in controllers, instead of the fabric8 message with the request URL and the `Status` dump. It also no longer repeats a cause which reads the same. - The `SchedulingFailure` event describes the failure instead of carrying the stack trace. The resource status keeps the stack trace. - `docs/configuration.md` documents the event messages. ### Why are the changes needed? K8s events reach more readers than the resource status, e.g. via the built-in `view` ClusterRole and event exporters. Stack traces and request URLs are not actionable for them, and they push the actual cause out of the 1024-character event message. ### Does this PR introduce _any_ user-facing change? No, because K8s events have not been released yet. The resource status and the operator log are unchanged. ### How was this patch tested? Pass the CIs with the newly added and updated test cases. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5.5 Closes #920 from dongjoon-hyun/SPARK-59931. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
1 parent 99d746c commit 17f4146

9 files changed

Lines changed: 217 additions & 28 deletions

File tree

‎docs/configuration.md‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,9 @@ kubectl get events -A --field-selector reportingComponent=spark-kubernetes-opera
8080
```
8181

8282
Whenever a resource transitions into a new state, the operator publishes an event whose `reason`
83-
is the name of the new state and whose `message` is the state message. Repeated transitions into
83+
is the name of the new state and whose `message` is the state message, except that the event of
84+
`SchedulingFailure` leaves out the stack trace of the failure, see
85+
[Event messages](#event-messages). Repeated transitions into
8486
the same state are aggregated into a single `Event` object by incrementing its `count` and
8587
replacing its `message`. A repeat carrying the same `message` may be paced by
8688
[`minIntervalSeconds`](#event-frequency), while one carrying a new `message` is always published.
@@ -189,6 +191,29 @@ still holds the interval. An event which is republished, such as `SuspendHeld` o
189191
a failure which lasts, recovers with its next repeat after the interval, while one which is not,
190192
such as a state transition, is lost; the resource status remains the source of truth.
191193

194+
### Event messages
195+
196+
The `message` of a `Warning` event about an error, such as `ReconcileError` or
197+
`ClusterRequestFailed`, describes the exception and its innermost cause, without the stack trace,
198+
which the operator logs. For a request which the API server rejected, it is the message of the API
199+
server, as in the events of the built-in controllers, e.g. an RBAC denial which names the
200+
ServiceAccount of the operator, a rejection by an admission webhook or a resource quota, or a field
201+
value which the API server found invalid. When `SchedulingFailure` is caused by an exception, its
202+
event describes the exception alike, while its state message keeps the stack trace.
203+
204+
Events reach more readers than the status of the resources. The built-in `view`, `edit` and
205+
`admin` ClusterRoles grant reading the `events` of a namespace, but not the `SparkApplication` and
206+
`SparkCluster` resources, which the Helm chart does not aggregate into them, and event exporters
207+
often forward warnings out of the cluster, e.g. to chat or logging services. So keep credentials
208+
out of `spec.sparkConf` and pod templates, since an error may quote them, e.g. as an invalid value,
209+
and refer to Kubernetes Secrets instead, e.g. with `spark.kubernetes.driver.secretKeyRef.[EnvName]`
210+
and `spark.kubernetes.executor.secretKeyRef.[EnvName]`. To keep the details of failures in the
211+
resource status and the operator log only, exclude their reasons, e.g. as follows.
212+
213+
```properties
214+
spark.kubernetes.operator.events.excludedReasons=SchedulingFailure,ReconcileError,CleanupError,StatusUpdateFailed,ClusterRequestFailed,SuspendReleaseFailed,Kueue.*Failed
215+
```
216+
192217
## Metrics
193218

194219
Spark operator,

‎spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import org.apache.spark.k8s.operator.status.ApplicationState;
5353
import org.apache.spark.k8s.operator.status.ApplicationStateSummary;
5454
import org.apache.spark.k8s.operator.status.ApplicationStatus;
55+
import org.apache.spark.k8s.operator.utils.EventUtils;
5556
import org.apache.spark.k8s.operator.utils.ReconcilerUtils;
5657
import org.apache.spark.k8s.operator.utils.SparkAppStatusRecorder;
5758

@@ -162,8 +163,15 @@ public ReconcileProgress reconcile(
162163
.getStatus()
163164
.appendNewState(
164165
new ApplicationState(ApplicationStateSummary.SchedulingFailure, errorMessage));
165-
return attemptStatusUpdate(
166-
context, statusRecorder, updatedStatus, completeAndImmediateRequeue());
166+
// The status keeps the stack trace, while the event, which reaches more readers, only
167+
// describes the failure, like the other warning events.
168+
if (!statusRecorder.persistStatus(
169+
context,
170+
updatedStatus,
171+
Constants.SCHEDULE_FAILURE_MESSAGE + " " + EventUtils.describe(e))) {
172+
log.warn("Failed to persist status, will retry status update in next reconcile attempt");
173+
}
174+
return completeAndImmediateRequeue();
167175
}
168176
ApplicationStatus updatedStatus =
169177
context

‎spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,12 +199,15 @@ private ReconcileProgress failScheduling(
199199
log.error("Failed to request master resource.", e);
200200
}
201201
String msg = CLUSTER_SCHEDULE_FAILURE_MESSAGE + " StackTrace: " + buildGeneralErrorMessage(e);
202+
// The status keeps the stack trace, while the event, which reaches more readers, only
203+
// describes the failure, like the other warning events.
202204
statusRecorder.persistStatus(
203205
context,
204206
context
205207
.getResource()
206208
.getStatus()
207-
.appendNewState(new ClusterState(SchedulingFailure, msg)));
209+
.appendNewState(new ClusterState(SchedulingFailure, msg)),
210+
CLUSTER_SCHEDULE_FAILURE_MESSAGE + " " + EventUtils.describe(e));
208211
return completeAndImmediateRequeue();
209212
}
210213

‎spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/EventUtils.java‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import java.util.Set;
2323

24+
import io.fabric8.kubernetes.client.KubernetesClientException;
2425
import io.javaoperatorsdk.operator.api.event.EventRecord;
2526
import io.javaoperatorsdk.operator.api.event.EventType;
2627
import io.javaoperatorsdk.operator.api.event.ResourceEventRecorder;
@@ -146,8 +147,8 @@ public static EventType eventTypeOf(BaseStateSummary summary) {
146147
* <p>The reason doubles as the event key, so repeated warnings for the same reason on the same
147148
* resource collapse into a count increment on a single Event object instead of creating one
148149
* Event per occurrence. Messages therefore must not be part of the identity: they typically vary
149-
* between attempts, for instance because a {@code KubernetesClientException} message embeds the
150-
* request URL.
150+
* between attempts, for instance because a rejection by a resource quota reports its current
151+
* usage.
151152
*
152153
* @param recorder The event recorder bound to the resource.
153154
* @param reason A short CamelCase reason, as expected by Kubernetes.
@@ -193,8 +194,14 @@ public static void record(
193194
/**
194195
* Builds a concise single-line description of an exception, suitable for an event message. The
195196
* innermost cause is appended when the exception has one, since that is usually where the
196-
* actionable detail is. The full stack trace is intentionally omitted, it belongs in the
197-
* operator log.
197+
* actionable detail is, unless its message is the same. The full stack trace is intentionally
198+
* omitted, it belongs in the operator log.
199+
*
200+
* <p>A request which the API server answered with an error is described by the message of the
201+
* returned {@code Status}, like the events of the built-in controllers, rather than by the
202+
* message of fabric8, which wraps it with the request URL and a dump of the whole {@code
203+
* Status}. fabric8 rethrows such a failure as a copy of itself which carries the original as its
204+
* cause, so that cause is not repeated.
198205
*
199206
* @param throwable The exception to describe, may be null.
200207
* @return A description of the exception.
@@ -203,16 +210,31 @@ public static String describe(Throwable throwable) {
203210
if (throwable == null) {
204211
return "";
205212
}
206-
StringBuilder builder = new StringBuilder(describeSingle(throwable));
213+
String message = messageOf(throwable);
214+
String description = describeSingle(throwable, message);
207215
Throwable rootCause = rootCauseOf(throwable);
208-
if (rootCause != null) {
209-
builder.append(", caused by: ").append(describeSingle(rootCause));
216+
if (rootCause == null) {
217+
return description;
218+
}
219+
String rootCauseMessage = messageOf(rootCause);
220+
// A cause with the same message adds nothing but its type, e.g. the original of the copy which
221+
// fabric8 rethrows, or the request timeout which it wraps.
222+
if (StringUtils.isNotBlank(message) && message.equals(rootCauseMessage)) {
223+
return description;
224+
}
225+
return description + ", caused by: " + describeSingle(rootCause, rootCauseMessage);
226+
}
227+
228+
private static String messageOf(Throwable throwable) {
229+
if (throwable instanceof KubernetesClientException e
230+
&& e.getStatus() != null
231+
&& StringUtils.isNotBlank(e.getStatus().getMessage())) {
232+
return e.getStatus().getMessage();
210233
}
211-
return builder.toString();
234+
return throwable.getMessage();
212235
}
213236

214-
private static String describeSingle(Throwable throwable) {
215-
String message = throwable.getMessage();
237+
private static String describeSingle(Throwable throwable, String message) {
216238
String type = throwable.getClass().getSimpleName();
217239
return StringUtils.isBlank(message) ? type : type + ": " + message;
218240
}

‎spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/StatusRecorder.java‎

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,11 @@ protected StatusRecorder(
8484
* behavior.
8585
*
8686
* @param context Context of the resource for which status update should be performed.
87+
* @param currentStateEventMessage The message of the event about the current state, or null for
88+
* the message of that state.
8789
*/
88-
private void patchAndStatusWithVersionLocked(BaseContext<CR> context) {
90+
private void patchAndStatusWithVersionLocked(
91+
BaseContext<CR> context, String currentStateEventMessage) {
8992
CR resource = context.getResource();
9093
KubernetesClient client = context.getClient();
9194
ObjectNode newStatusNode = objectMapper.convertValue(resource.getStatus(), ObjectNode.class);
@@ -139,7 +142,7 @@ private void patchAndStatusWithVersionLocked(BaseContext<CR> context) {
139142
listener -> {
140143
listener.listenStatus(resource, prevStatus, resource.getStatus());
141144
});
142-
recordTransitionEvents(context, prevStatus, resource.getStatus());
145+
recordTransitionEvents(context, prevStatus, resource.getStatus(), currentStateEventMessage);
143146
}
144147

145148
/**
@@ -159,13 +162,17 @@ private void patchAndStatusWithVersionLocked(BaseContext<CR> context) {
159162
*
160163
* <p>The event type follows {@link EventUtils#eventTypeOf(BaseStateSummary)}. The state name is
161164
* used as both reason and dedup key, so an app that keeps re-entering the same state bumps the
162-
* count on one Event instead of accumulating one per attempt.
165+
* count on one Event instead of accumulating one per attempt. The message is the state message,
166+
* unless a message is given for the event about the current state.
163167
*
164168
* @param context Context of the resource whose status has just been patched.
165169
* @param prevStatus The status before the patch, may be null.
166170
* @param status The status that was persisted.
171+
* @param currentStateEventMessage The message of the event about the current state, or null for
172+
* the message of that state.
167173
*/
168-
private void recordTransitionEvents(BaseContext<CR> context, STATUS prevStatus, STATUS status) {
174+
private void recordTransitionEvents(
175+
BaseContext<CR> context, STATUS prevStatus, STATUS status, String currentStateEventMessage) {
169176
BaseState<?> currentState = status.getCurrentState();
170177
if (currentState == null
171178
|| (prevStatus != null && currentState.equals(prevStatus.getCurrentState()))) {
@@ -175,7 +182,10 @@ private void recordTransitionEvents(BaseContext<CR> context, STATUS prevStatus,
175182
for (BaseState<?> state : newStatesSince(prevStatus, status)) {
176183
Object summary = state.getCurrentStateSummary();
177184
if (summary instanceof BaseStateSummary stateSummary && !summary.equals(lastSummary)) {
178-
String message = state.getMessage();
185+
String message =
186+
currentStateEventMessage != null && state.equals(currentState)
187+
? currentStateEventMessage
188+
: state.getMessage();
179189
EventUtils.record(
180190
context.getEventRecorder(),
181191
EventUtils.eventTypeOf(stateSummary),
@@ -219,9 +229,28 @@ private List<BaseState<?>> newStatesSince(STATUS prevStatus, STATUS status) {
219229
* @return true if the status is successfully patched.
220230
*/
221231
public boolean persistStatus(BaseContext<CR> context, STATUS newStatus) {
232+
return persistStatus(context, newStatus, null);
233+
}
234+
235+
/**
236+
* Persists the new status of the resource to the Kubernetes cluster, like {@link
237+
* #persistStatus(BaseContext, BaseStatus)}, but publishes the given message in place of the
238+
* message of the current state in the event about the transition into it. It is meant for a
239+
* state message which is too detailed for an event, such as one which carries a stack trace,
240+
* since events typically reach more readers than the resource status, e.g. through the built-in
241+
* {@code view} ClusterRole or an event exporter.
242+
*
243+
* @param context The BaseContext containing the resource and client.
244+
* @param newStatus The new status to persist.
245+
* @param currentStateEventMessage The message of the event about the current state, or null for
246+
* the message of that state.
247+
* @return true if the status is successfully patched.
248+
*/
249+
public boolean persistStatus(
250+
BaseContext<CR> context, STATUS newStatus, String currentStateEventMessage) {
222251
try {
223252
context.getResource().setStatus(newStatus);
224-
patchAndStatusWithVersionLocked(context);
253+
patchAndStatusWithVersionLocked(context, currentStateEventMessage);
225254
return true;
226255
} catch (KubernetesClientException e) {
227256
log.error("Error while persisting status to {}", newStatus, e);

‎spark-operator/src/test/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStepTest.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,7 +1135,7 @@ void unsupportedKueueSpecFailsScheduling() {
11351135
Assertions.assertEquals(ReconcileProgress.completeAndImmediateRequeue(), progress);
11361136
Assertions.assertNull(getWorkload());
11371137
ArgumentCaptor<ApplicationStatus> captor = ArgumentCaptor.forClass(ApplicationStatus.class);
1138-
verify(recorder).persistStatus(any(), captor.capture());
1138+
verify(recorder).persistStatus(any(), captor.capture(), any());
11391139
Assertions.assertEquals(
11401140
ApplicationStateSummary.SchedulingFailure,
11411141
captor.getValue().getCurrentState().getCurrentStateSummary());
@@ -1638,16 +1638,25 @@ void driverMissingFromCacheWithUnbuildableSpecFailsScheduling() {
16381638
throw new SparkException(
16391639
"Please specify spark.kubernetes.file.upload.path property.");
16401640
});
1641-
when(recorder.persistStatus(any(), any())).thenReturn(true);
1641+
when(recorder.persistStatus(any(), any(), any())).thenReturn(true);
16421642

16431643
ReconcileProgress progress = appInitStep.reconcile(mockContext, recorder);
16441644

16451645
Assertions.assertEquals(ReconcileProgress.completeAndImmediateRequeue(), progress);
16461646
ArgumentCaptor<ApplicationStatus> captor = ArgumentCaptor.forClass(ApplicationStatus.class);
1647-
verify(recorder).persistStatus(any(), captor.capture());
1647+
ArgumentCaptor<String> eventMessage = ArgumentCaptor.forClass(String.class);
1648+
verify(recorder).persistStatus(any(), captor.capture(), eventMessage.capture());
16481649
Assertions.assertEquals(
16491650
ApplicationStateSummary.SchedulingFailure,
16501651
captor.getValue().getCurrentState().getCurrentStateSummary());
1652+
// The status keeps the stack trace, while the event only describes the failure
1653+
Assertions.assertTrue(
1654+
captor.getValue().getCurrentState().getMessage().contains("\tat "),
1655+
captor.getValue().getCurrentState().getMessage());
1656+
Assertions.assertEquals(
1657+
Constants.SCHEDULE_FAILURE_MESSAGE
1658+
+ " SparkException: Please specify spark.kubernetes.file.upload.path property.",
1659+
eventMessage.getValue());
16511660
Assertions.assertNotNull(getWorkload());
16521661
}
16531662

@@ -1804,7 +1813,7 @@ void kueueFlavorConflictFailsSchedulingAndReleasesWorkload() {
18041813
when(mockContext.getClient()).thenReturn(kubernetesClient);
18051814
when(mockContext.getEventRecorder()).thenReturn(eventRecorder);
18061815
createFlavor("spot-flavor", Map.of("pool", "spot"), List.of());
1807-
when(recorder.persistStatus(any(), any())).thenReturn(true);
1816+
when(recorder.persistStatus(any(), any(), any())).thenReturn(true);
18081817

18091818
appInitStep.reconcile(mockContext, recorder);
18101819
admitWorkload(Map.of("driver", Map.of("cpu", "spot-flavor")));
@@ -1815,7 +1824,7 @@ void kueueFlavorConflictFailsSchedulingAndReleasesWorkload() {
18151824
Assertions.assertNull(getWorkload());
18161825
verify(mockContext, never()).setKueuePodSetFlavors(any());
18171826
ArgumentCaptor<ApplicationStatus> captor = ArgumentCaptor.forClass(ApplicationStatus.class);
1818-
verify(recorder).persistStatus(any(), captor.capture());
1827+
verify(recorder).persistStatus(any(), captor.capture(), any());
18191828
Assertions.assertEquals(
18201829
ApplicationStateSummary.SchedulingFailure,
18211830
captor.getValue().getCurrentState().getCurrentStateSummary());

‎spark-operator/src/test/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStepTest.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -293,11 +293,19 @@ void rejectedRequestOfResourcesFailsScheduling() {
293293
ReconcileProgress.completeAndImmediateRequeue(),
294294
clusterInitStep.reconcile(mockContext, recorder));
295295
ArgumentCaptor<ClusterStatus> statusCaptor = ArgumentCaptor.forClass(ClusterStatus.class);
296-
verify(recorder).persistStatus(any(), statusCaptor.capture());
296+
ArgumentCaptor<String> eventMessage = ArgumentCaptor.forClass(String.class);
297+
verify(recorder).persistStatus(any(), statusCaptor.capture(), eventMessage.capture());
297298
Assertions.assertEquals(
298299
ClusterStateSummary.SchedulingFailure,
299300
statusCaptor.getValue().getCurrentState().getCurrentStateSummary());
300-
// The status already says why, so no event is needed
301+
// The status keeps the stack trace, which the SchedulingFailure event leaves out
302+
Assertions.assertTrue(
303+
statusCaptor.getValue().getCurrentState().getMessage().contains("\tat "),
304+
statusCaptor.getValue().getCurrentState().getMessage());
305+
Assertions.assertEquals(
306+
Constants.CLUSTER_SCHEDULE_FAILURE_MESSAGE + " KubernetesClientException: Invalid",
307+
eventMessage.getValue());
308+
// SchedulingFailure already says why, so no other event is needed
301309
verifyNoInteractions(eventRecorder);
302310
}
303311

@@ -1082,7 +1090,7 @@ void kueueFlavorConflictFailsSchedulingAndReleasesWorkload() {
10821090
verify(mockContext, never()).setKueuePodSetFlavors(any());
10831091
verify(mockContext, never()).getMasterServiceSpec();
10841092
ArgumentCaptor<ClusterStatus> captor = ArgumentCaptor.forClass(ClusterStatus.class);
1085-
verify(recorder).persistStatus(any(), captor.capture());
1093+
verify(recorder).persistStatus(any(), captor.capture(), any());
10861094
Assertions.assertEquals(
10871095
ClusterStateSummary.SchedulingFailure,
10881096
captor.getValue().getCurrentState().getCurrentStateSummary());

0 commit comments

Comments
 (0)