Remove executor failed-pod-checks in favour of error categorization

[dejanzele]

Summary

The executor has two systems that match failed pods against operator config: failedpodchecks (decides whether a PodFailed pod is retry-eligible) and the error categorizer from #4713 (labels failures with a category). They overlap almost entirely in what they match on, but only the first can trigger a retry and only the second produces a category. This issue proposes deleting failedpodchecks and making the categorizer the single matching system, with retry eligibility expressed as a per-category flag. It can land now, independently of the retry-policy work in #4683.

Current state, verified on master (86c958e)

• failedPodChecks appears in no yaml anywhere in the repo: not in config/executor/config.yaml, _local/, e2e/, or the helm chart. With the zero-value config, PodRetryChecker.IsRetryable always returns false, so the subsystem is inert on every default deployment. At least one production deployment (ours) does populate it, so a migration recipe is part of this issue.
• For PodFailed pods the reporter classifies first (job_state_reporter.go:94-97) and then throws the classification away if failedpodchecks registers a retryable issue (:112-115). Every gated failure is classified for nothing.
• The scheduler only requeues runs that came back as lease returns (scheduler.go:974: requeue requires lastRun.Returned()). Any replacement for failedpodchecks must keep emitting PodLeaseReturned, or previously retryable failures become terminal.
• The categorizer matcher is a superset of failedpodchecks for pod-state matching (exit codes, termination messages, pod error message, conditions, per-container scoping) with two gaps: it never sees k8s events (PodEventCheck has no equivalent) and it has no retry-eligibility output.

Proposal: deprecate in two phases

Phase 1: classification unified, lease returns attributable, failedPodChecks deprecated in place

No new retry decision surface is introduced. An earlier revision of this proposal added an interim retryOn list to the categorizer config; it was dropped deliberately: any interim config concept that operators adopt becomes a long-lived support surface, and retry decisions should only ever be expressed once more, as the retry-policy API resource. So in phase 1 failedPodChecks keeps deciding retryability, unchanged.

What phase 1 does change, for every PodFailed pod:

• Classify once, use the result twice. Today the reporter classifies first and throws the result away whenever failedPodChecks registers a retryable issue. The reordered flow runs the issue check first and builds the terminal Failed event from the same classification.
• onPodError rules now see pod.Status.Message on the failed-pod path (previously only executor-captured issue messages), so admission and eviction failures classify correctly.
• Lease returns become attributable: the classification's category and subcategory are stamped onto the lease-return error envelope (the fields already exist on armadaevents.Error, no proto change), so retries driven by failedPodChecks show up in Lookout with a category once matching rules exist. This lets a deployment build its category taxonomy now, purely for observability, before any retry behavior moves.
• A latent race is fixed: with a populated failedPodChecks, two concurrent reporter goroutines could race between the issue check and registration, and the loser would emit a terminal Failed alongside the winner's lease return. The loser now treats the pod as handled.
• Populating kubernetes.failedPodChecks logs a deprecation notice pointing here: the key keeps working and will be superseded by retry policies.

Phase 2: retry policies take over the decision, then the package is deleted

When scheduler-side retry policies land (a default policy first, per-queue attachment later, see #4683), each failedPodChecks entry translates into an onCategory Retry rule referencing categories the deployment established during phase 1. Once the affected deployments have moved, internal/executor/podchecks/failedpodchecks/ and its config types are deleted along with the executor-side failed-pod retry decision. The lease-return mechanism itself stays: pending-pod-checks and infra issue handling still emit lease returns; only the failed-pod retryability decision moves to the scheduler. During the overlap window a failure class should be handled by exactly one system, failedPodChecks or a policy, never both, or the two budgets stack (the executor burns maxAttemptedRuns first, then the engine applies its own limits).

Because only the matcher changes and the lease-return mechanism stays, there is no behavior change to attempt accounting, anti-affinity, the Lookout "Lease Returned" display, or pod cleanup timing. Default deployments see byte-identical event streams since both failedPodChecks and errorCategories ship empty or disabled.

Why config rather than deriving retryability from the static internal category: the classifier never emits internal (those constants are stamped at error-construction sites, not by rule matching), and internal means "Armada's fault", which is not the same as "worth another attempt" (max-runs-exceeded is internal and deliberately terminal). Retryability here encodes operator-known infra flakes, and only the operator can name those.

Migration for deployments that populate failedPodChecks

Each of the three check lists translates to categorizer rules; the categories can be added in phase 1 for attribution, and the retry decision follows in phase 2 as policy rules:

• podStatuses: [{regexp, reason}] becomes onPodError: <regexp>. As part of phase 1 the failed-pod path passes pod.Status.Message into classification, so onPodError matches the same text the old check did. The reason filter has no direct equivalent. In practice the regex carries the signal.
• failedContainerStatuses: [{containerNameRegexp, messageRegexp}] becomes onTerminationMessage: <messageRegexp> plus containerName. The categorizer scopes by exact container name. Deployments using real regexes there should enumerate names, or we add a regex variant if someone needs it.
• podEvents: [{regexp, reason, type}] has no categorizer equivalent, because the categorizer deliberately does not consume k8s events (lossy, TTL-bound). If a deployment relies on event matching for retry decisions, this issue needs an onPodEvents matcher added to the categorizer first. Events are already fetched at the single call site, so the addition is cheap. Whether we need it depends on what the affected configs actually contain, which should be settled before the deletion PR merges.

Deployment ordering: phase 1 is safe for everyone (the old key keeps working, with a warning; default deployments are untouched since both failedPodChecks and errorCategories ship empty or disabled). A deployment must finish moving its retry rules into policies before upgrading past the phase 2 deletion, where a leftover failedPodChecks key becomes silently ignored (config loading logs an unused-key warning only) and previously retryable failures would become terminal. The phase 2 release notes must call this out loudly.

Relationship to retry policies (#4683)

The retryable flag is an interim bridge and is designed to be deleted. It lives only in executor yaml, no proto field, no schema, and retryability at category granularity maps one-to-one onto a future onCategory retry policy rule. When the retry-policy series lands, each retryable category becomes a policy rule, the flag is deprecated with a config warning, and the executor-side retry decision (FailedStartingUp, DetectAndRegisterFailedPodIssue) is removed entirely.

Out of scope

Pending-pod-checks stays. It is a live, shipped-by-default watchdog whose decisions are time-based (deadlines for node assignment, kubelet updates, init containers, per-check grace periods). The categorizer is stateless and trigger-less, so folding detection into it would be a redesign, not a simplification. The one piece of pending-pod-checks that retry policies will eventually replace is its per-check Retry|Fail action field. That gets its own proposal when the time comes.

Implementation notes

• The existing test at pod_issue_handler_test.go:270-275 pins a race that is currently unreachable: with retryable categories enabled, two concurrent reporter goroutines can race between the issue check and registration, and the loser would emit a terminal Failed alongside the winner's lease return. The change must treat "issue already registered" as handled and add a concurrent-registration test.
• The lease-return issue message must carry the causal text (termination message or pod status message), since it surfaces in the Lookout run error and in the max-runs-exceeded final error.
• Optionally, the classification can be stamped onto the lease-return error so retried failures are visible by category in Lookout. The fields already exist on the error envelope from Error Categorization #4713, so this needs no proto change either way.

[dejanzele]

We audited the one known production config that populates failedPodChecks. All entries translate to pod-status message and container matchers: the event checks there match kubelet admission and eviction failures whose text also appears in pod.Status.Message, which the proposed change feeds into classification. So the onPodEvents matcher is not needed for this migration and stays deferred as an additive extension if a future deployment genuinely depends on event-only signals.

[Demiserular]

very principled approach @dejanzele , In my opinion there are three key concerns to address before proceeding:

1. verify that armadaevents.Error actually has category/subcategory fields (the claim of "no proto change" needs validation).
2. add the onPodEvents matcher to the categorizer in Phase 1 as insurance since the categorizer deliberately doesn't consume k8s events and this gap could break production deployments silently in Phase 2.
3. ensure the race fix in registerIssue correctly handles both retryable and terminal existing-issue cases, not just treating "already registered" as handled.

I want you opinion on these