Repository navigation
[SPARK-59576] Support K8s Normal events for all SparkApplication and SparkCluster state transitions - #831
[SPARK-59576] Support K8s Normal events for all SparkApplication and SparkCluster state transitions#831dongjoon-hyun wants to merge 2 commits into
Normal events for all SparkApplication and SparkCluster state transitions#831Conversation
|
cc @TQJADE |
…and `SparkCluster` state transitions
16239fd to
b34e30c
Compare
|
All tests passed. Could you review this PR when you have some time, please, @peter-toth ? |
There was a problem hiding this comment.
Thanks for the PR, @dongjoon-hyun!
StatusRecorder is the right seam for this, and moving from "current state is a failure" to "every state appended since the previous status" is what makes a single patch carrying two observations report both. I checked the parts that could go wrong and they hold: history keys stay monotonic across the restart trim at ApplicationStatus.java:193-202, so tailMap(prevLastKey + 1) cannot replay; every setLastObservedDriverStatus caller builds a fresh state rather than mutating the current one; and the new equals guard survives the statusCache JSON round trip - I removed it and publishesNoEventWhenTheCurrentStateDidNotChange is the one test that fails. The two items below are both about the classification table being a contract nothing pins.
Non-blocking
- 1.
eventTypeOfApplicationStatesmirrors the implementation instead of pinning it: it recomputesexpectedwith the sameisFailure() || == A || == Bexpression, so it cannot catch a classification change. I droppedDriverEvictedfromApplicationStateSummary.failuresand all 13 tests stayed green, while the docs table would have become wrong. [inline:EventUtilsTest.java:82]
Minor
- 2. The Warning-versus-Normal rule is not written down:
InitializedBelowThresholdExecutorsbeingNormalwhileRunningWithBelowThresholdExecutorsisWarningreads as arbitrary until you find the escalation atAppDriverTimeoutObserver.java:78. [inline:EventUtils.java:61]
| @Test | ||
| void eventTypeOfApplicationStates() { | ||
| for (ApplicationStateSummary summary : ApplicationStateSummary.values()) { | ||
| EventType expected = |
There was a problem hiding this comment.
Finding 1. expected is the same expression as EventUtils.eventTypeOf, so this loop asserts that the method agrees with itself. It pins the two NON_FAILURE_WARNING_STATES members, which is worth having, but it delegates the other half of the classification to isFailure() and therefore follows that set wherever it goes.
That matters because the classification is published as a contract: the eight Warning reasons are listed in the PR description and in docs/configuration.md:64-66. I checked what the test would catch by dropping DriverEvicted from ApplicationStateSummary.failures:
$ ./gradlew :spark-operator:test --tests "...EventUtilsTest"
tests="13" skipped="0" failures="0" errors="0"
Green, while DriverEvicted would now publish as Normal and the docs table would be wrong.
An explicit set makes the loop pin the documented table instead:
@Test
void eventTypeOfApplicationStates() {
Set<ApplicationStateSummary> expectedWarnings =
Set.of(
ApplicationStateSummary.SchedulingFailure,
ApplicationStateSummary.Failed,
ApplicationStateSummary.DriverEvicted,
ApplicationStateSummary.DriverStartTimedOut,
ApplicationStateSummary.DriverReadyTimedOut,
ApplicationStateSummary.ExecutorsStartTimedOut,
ApplicationStateSummary.RunningWithBelowThresholdExecutors,
ApplicationStateSummary.TerminatedWithoutReleaseResources);
for (ApplicationStateSummary summary : ApplicationStateSummary.values()) {
EventType expected =
expectedWarnings.contains(summary) ? EventType.WARNING : EventType.NORMAL;
assertThat(EventUtils.eventTypeOf(summary)).as(summary.name()).isEqualTo(expected);
}
}That also makes the two trailing RunningWithPartialCapacity / InitializedBelowThresholdExecutors assertions redundant, since the loop now covers them by exclusion. The same shape applies to eventTypeOfClusterStates at line 99, where Set.of(SchedulingFailure, Failed) is the whole table.
There was a problem hiding this comment.
Thank you, @peter-toth. I updated both tests to pin the documented table with explicit sets, and verified that dropping DriverEvicted from ApplicationStateSummary.failures now fails eventTypeOfApplicationStates.
| /** Maximum number of links followed when looking for the innermost cause of a failure. */ | ||
| private static final int MAX_CAUSE_DEPTH = 10; | ||
|
|
||
| /** States that are not failures but still deserve the attention of users. */ |
There was a problem hiding this comment.
Finding 2. "deserve the attention of users" does not say why InitializedBelowThresholdExecutors is not in this set. Both states mean the app is below its executor threshold, and the startup one is below the minimum required rather than merely under capacity, so on the wording alone it reads like the stronger candidate of the two.
The rule that actually separates them is elsewhere. AppDriverTimeoutObserver.java:78 escalates InitializedBelowThresholdExecutors to ExecutorsStartTimedOut, which is a failure and therefore already a Warning, so the startup case reports itself when it stops being transient. RunningWithBelowThresholdExecutors appears in no timeout branch: it persists silently, so nothing else would ever tell the user. RunningWithPartialCapacity is a third case, documented on the enum as a tolerable capacity level rather than a degradation.
Worth one sentence here, because this is the comment someone will read when they add a state:
/**
* States that are not failures but still deserve the attention of users, because nothing else
* reports them: a state that escalates to a failure on its own, such as
* {@code InitializedBelowThresholdExecutors} timing out into {@code ExecutorsStartTimedOut},
* warns through that failure instead and stays normal here.
*/There was a problem hiding this comment.
Thank you. I added the rule to the Javadoc of NON_FAILURE_WARNING_STATES, including why TerminatedWithoutReleaseResources is a warning (it is not meant for production use).
|
Merged to main |
…ns` to skip K8s events by reason ### What changes were proposed in this pull request? This PR aims to support `spark.kubernetes.operator.events.excludedReasons`, a comma-separated list of Java regular expressions. Events whose reason fully matches any of them are not published. ```properties spark.kubernetes.operator.events.excludedReasons=RunningHealthy,RunningWithPartialCapacity spark.kubernetes.operator.events.excludedReasons=Running.* ``` ### Why are the changes needed? Since SPARK-59576, the operator publishes K8s events for all state transitions. Some events, such as `RunningHealthy`, can be noisy for long-running resources. - #831 ### Does this PR introduce _any_ user-facing change? No behavior change by default because the default value is empty. ### How was this patch tested? Pass the CIs with the newly added test cases. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5 Closes #833 from dongjoon-hyun/SPARK-59586. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
This PR aims to publish K8s events for all
SparkApplicationandSparkClusterstate transitions, not only for failure states. The reason of each event is the new state name.SparkApplicationSparkClusterWarningSchedulingFailure,Failed,DriverEvicted,DriverStartTimedOut,DriverReadyTimedOut,ExecutorsStartTimedOut,RunningWithBelowThresholdExecutors,TerminatedWithoutReleaseResourcesSchedulingFailure,FailedNormalAn event is published only when the current state changes. Repeated transitions into the same state increment the count of one
Eventobject.Why are the changes needed?
To make the full lifecycle of Spark resources visible via
kubectl describeandkubectl get events.Does this PR introduce any user-facing change?
No by default. When
spark.kubernetes.operator.events.enabledistrue, the operator also publishes the newNormalandWarningevents above.How was this patch tested?
Pass the CIs with the newly added unit tests.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5