Skip to content

Remove the last-logs annotation snapshot from the Jobs execution path #479

Description

@ayuskauskas

The nodewright.nvidia.com/last-logs Job annotation is a lot of machinery for very little remaining value. With the retry backoff in place and timed-out attempt pods retained as erroring, the failed attempt pod is still around with its full logs, so the snapshot is mostly duplicating evidence that already exists in a better form.

Scoped against feature/package-as-jobs at 4afdc73.

What comes out

operator/internal/controller/job_controller.go

  • snapshotFailureLogs and its call from handleActiveJob
  • stuckInitContainersnapshotFailureLogs is its only consumer
  • the annotationLastLogs and lastLogsMaxBytes consts

operator/internal/dal/

  • GetPodLogTail and tailAndSanitize, plus their tests and the generated mock. The snapshot is the only non-test consumer of GetPodLogTail.
  • The clientset kubernetes.Interface field and its nil guard. The clientset exists solely to read pod logs, which the controller-runtime client cannot do (log is a subresource stream).

Constructor plumbing — with the clientset gone, the clientset parameter can be dropped from dal.New, NewJobReconciler, NewPodReconciler, and NewSkyhookReconciler, along with the kubernetes.NewForConfig call in cmd/manager/main.go. That is the bulk of the deletion and touches several call sites and test fixtures.

RBACpods/log loses its only reader:

  • the //+kubebuilder:rbac:...pods/log... marker in skyhook_controller.go (then make manifests)
  • the rule in operator/config/rbac/role.yaml and its chart/templates/manager-rbac.yaml mirror, plus the comments in both that explain why pods/log is namespaced

Docsoperator/RELEASE_NOTES.md, docs/designs/2026-07-10-package-execution-as-jobs.md (log-visibility, TTL, and podFailurePolicy sections), and both the 1.31 and 1.27 rows in docs/kubernetes-support.md, which currently spend a paragraph each on when the snapshot does and does not fire.

What must stay

handleActiveJob's FailureTarget branch does two unrelated things. Only the snapshotFailureLogs call goes; recordStaleFailureTarget, failureTargetStale, failureTargetRemaining, and the failureTargetGrace requeue are the unreachable-node erroring signal and are unaffected.

Decide before deleting

snapshotFailureLogs already returns early when a genuine failed-attempt archive exists, so it only ever fires when there is no archive pod to read. Two cases are left after that:

  1. Interrupt Jobs. Package stages bound time per attempt via activeDeadlineSeconds on the pod template, so an expired attempt is Failed but not deleted and its logs survive — this is the case the issue's rationale covers. Interrupt Jobs still carry a Job-level deadline, where the Job controller deletes the active pod and the kubelet collects its logs. Worth confirming that losing evidence there is acceptable, or that interrupt Jobs should move to a per-attempt bound too.
  2. Never-started containers. The snapshot records the waiting reason/message rather than logs. That information is still on the pod object, so kubectl describe covers it as long as the pod exists.

Absorbed from #449 — the evidence gap removal does not close

#449 (closed as superseded) established that last-logs is already dead code for package Jobs, which strengthens the case for removing it — but it also documented a real gap that deleting the annotation does not fix. Recording it here so it is not lost with that issue.

Why it never fires for package stages. The snapshot is gated on the Job carrying FailureTarget. Since #402 removed the Job-level deadline, a package Job only reaches FailureTarget via BackoffLimitExceeded — after the final attempt has already failed. At that point one of two things is true, and neither produces a snapshot:

  1. The final attempt failed genuinely → podFailedGenuinely is true → snapshotFailureLogs returns early because a genuine archive already holds full logs.
  2. The final attempt was kubelet-refused → the snapshot proceeds, but podReplacementPolicy: Failed plus an exhausted budget means no Running/Pending pod is left to read, so target == nil and nothing is written.

Interrupt Jobs still reach FailureTarget with a live pod, which is why the mechanism appears to work and why this was not obvious. That matches the "interrupt Jobs are the residual case" note above — and means the package half of this feature can be deleted with no loss at all, because it never produced output.

The gap that outlives the removal. From validation Case 16 (version: "9.9.9", stageTimeout: 60s): the stage surfaced as erroring after 64s — correctly bounded — but the retained archive pod reads

phase=Failed reason=DeadlineExceeded
missing-init: {"terminated":{"exitCode":137,"reason":"ContainerStatusUnknown",
               "message":"The container could not be located when the pod was terminated"}}

The kubelet rewrote the container status on termination, so ImagePullBackOff and its message are gone. Pod status no longer names the problem, events age out, and the annotation that was meant to hold "<container>: ImagePullBackOff: <message>" was never written. The operator's answer to "why did this stage fail" is ContainerStatusUnknown.

Removing last-logs does not make this worse — the annotation was empty in all three validation cases that hit it — but it does mean #306 (surface ImagePullBackOff/ErrImagePull as erroring) becomes the only remaining path to a real answer for the unpullable-image case. Worth confirming that is acceptable as part of accepting this removal.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions