fix(controller): track executor state with LRU eviction instead of id-based skip

[shashankch292]

Purpose of this PR

Fixes a regression introduced in #2181 where executors with IDs above the MaxTrackedExecutorPerApp cap were silently dropped, and the state map was never pruned — causing Status.ExecutorState to report zero running executors on long-running drivers with executor churn.

Proposed changes:

• Remove the id-based skip in updateExecutorState that dropped any executor whose numeric ID exceeded MaxTrackedExecutorPerApp
• Add enforceExecutorStateCap: runs after the missing-pod pass; evicts terminated/UNKNOWN entries oldest-ID-first, and only drops live entries newest-ID-first (with a warning log) when the cap is fully occupied by live executors
• Break ID ties by entry name in both eviction sorts so eviction is deterministic across reconciles (entries are gathered from a map, so input order is otherwise random)
• Add util.ParseExecutorIDFromPodName to recover executor IDs from pod names for terminated entries whose pods are already gone

Change Category

• Bugfix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that could affect existing functionality)
• Documentation update

Rationale

PR #2181 introduced the cap by skipping any executor with ID > MaxTrackedExecutorPerApp. This had two failure modes on long-running drivers:

1. Invisible executors: new executors spawned after the ID threshold never appeared in Status.ExecutorState, so the operator's running-count was permanently wrong.
2. Stale entry crowding: terminated executor entries accumulated without bound, consuming the entire cap budget and leaving no room for live executors.

On applications with high executor churn (pods deleted, replacements spawn with higher IDs), the net effect was Status.ExecutorState showing zero running executors despite live pods.

The fix enforces the cap as a size bound at write time rather than a filter on IDs. Status.ExecutorState semantics shift from cumulative (every ID ever seen, up to cap) to instantaneous (at most cap entries at any time, preferring live over terminated). No CRD or schema changes; MaxTrackedExecutorPerApp default (1000) is unchanged.

Checklist

• I have conducted a self-review of my own code.
• I have updated documentation accordingly. (N/A — flag name/default/help text unchanged; Status.ExecutorState semantics covered in Rationale above.)
• I have added tests that prove my changes are effective or that my feature works.
• Existing unit tests pass locally with my changes.

Additional Notes

New test coverage (internal/controller/sparkapplication/controller_test.go):

• Live high-ID executor tracked correctly (regression for implement an upper bound limit to the number of tracked executor #2181 skip)
• Churn replacement executors reported Running after prior IDs terminated
• Cap=3 churn eviction reproduces the original crowding scenario
• All-live overflow: drops newest, keeps oldest
• Cap ≤ 0: no-op
• Mixed COMPLETED/FAILED/UNKNOWN eviction ordering
• Same parsed ID: eviction broken deterministically by name

New unit test (pkg/util/sparkpod_test.go): table-driven ParseExecutorIDFromPodName covering valid names, malformed names, and missing numeric suffix.

go test ./pkg/util/... ./internal/controller/sparkapplication/... passes. golangci-lint and gofmt clean.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vara-bonthu for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

🎉 Welcome to the Kubeflow Spark Operator! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

• If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
• Our team will review your PR soon!

Join the community:

• Slack: Join our #kubeflow-spark-on-kubernetes Slack channel.

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds bounded tracking for executor states by enforcing MaxTrackedExecutorPerApp after reconciliation, and introduces a pod-name-based executor ID parser as a fallback when pod labels are unavailable.

Changes:

• Added ParseExecutorIDFromPodName utility and unit tests for parsing trailing executor IDs from pod names.
• Reworked executor state tracking to enforce a cap via enforceExecutorStateCap (evict terminated first, then drop newest live if needed).
• Added controller-level tests covering churn, high executor IDs, mixed terminal/live maps, and no-cap behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.

File	Description
pkg/util/sparkpod.go	Adds trailing-digit executor ID parsing helper used for capped eviction ordering.
pkg/util/sparkpod_test.go	Adds table-driven tests for the new parsing helper.
internal/controller/sparkapplication/controller.go	Removes “skip by ID” logic and adds a dedicated cap-enforcement step with eviction strategy.
internal/controller/sparkapplication/controller_test.go	Adds multiple scenarios validating capped eviction behavior and churn handling.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.