Skip to content

Fix worker identity in partial YARN logs - #279

Open
mnkj0021 wants to merge 8 commits into
NVIDIA:devfrom
mnkj0021:fix/276-preserve-worker-identity
Open

mnkj0021 wants to merge 8 commits into
NVIDIA:devfrom
mnkj0021:fix/276-preserve-worker-identity

Conversation

@mnkj0021

@mnkj0021 mnkj0021 commented Oct 1, 2026 •

Copy link
Copy Markdown

Preserve real YARN worker identities in partial-log reports.

  • NodeManager registration carries the reported host identity to container starts without leaking one worker's identity into another file.
  • Cross-file inference is only used when a directory has one unambiguous registered host; ambiguous multi-worker layouts remain unresolved instead of borrowing the wrong capacity.
  • ResourceManager assignment keeps the assigned host in the node inventory even when registration evidence is absent, while explicitly treating that node as unregistered/incomplete for final accounting.
  • A later NodeManager registration without instance-type metadata preserves an instance type already learned from the ResourceManager.
  • Existing incomplete-accounting/final-cost safeguards remain in place.
  • Regression coverage includes NM-only identity, RM assignment without registration, split logs, co-located workers, later conflicting registration, instance-type preservation, and mapped-but-unregistered nodes.

Validation completed before the latest human-review follow-up:

  • Python 3.12 full yarn-resource-cost unittest suite: 73/73 passed on the prior review-fix head
  • py_compile: passed
  • git diff --check: passed

The current head is b0920a5. The latest license-header and YARN Python workflows are action_required and need NVIDIA maintainer/vetter approval, so this description does not claim a fresh CI pass on the final review-fix head.

Fixes #276

Signed-off-by: mnkj0021 <98058999+mnkj0021@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Fixes worker identity tracking in resource cost logs.

The PR appears safe to merge; no new actionable issue or outstanding previous finding was identified.

Summary

The PR preserves reported worker identities in partial YARN logs and creates unknown-capacity nodes for RM allocations without registration evidence.

  • Adds registration-evidence warnings and incomplete accounting for affected containers.
  • Adds regressions for split logs, shared directories, and unregistered allocations.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Logs[YARN logs] --> Registration[Registration host and capacity]
  Logs --> Allocation[RM allocation or NM start]
  Registration --> Reconcile[Reconcile container node]
  Allocation --> Reconcile
  Reconcile --> Report[Completeness and cost report]
Loading

Reviews (5) · Last reviewed commit: "test(yarn): cover registration and host-..."

assignment = RM_ASSIGN_RE.search(line)
if assignment:
host = assignment.group("host")
assigned_node = evidence.nodes.setdefault(host, Node(host))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the host in the node inventory without treating this placeholder as registration evidence? With assignment maxima and a node-class map, this now produces a final cost even though no node registration was logged.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed on the current head b0920a5. RM-assignment placeholders remain in the node inventory, but final accounting now treats a used node without registration evidence as incomplete even if a node-class map makes it priceable. I added regression coverage for the mapped-but-unregistered case. Leaving this thread open for your confirmation.

container = evidence.containers.setdefault(
candidate.container_id, candidate
)
if container is candidate and node_id == path_node_id:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we track whether this file has reported a host instead of comparing node_id with the directory name? If the directory is worker-a, a later worker-b registration can relabel A’s container while leaving A’s capacity on it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed on b0920a5. NodeManager parsing now tracks an explicit reported host per file rather than comparing the mutable node_id with the directory name, so a later registration for another worker cannot relabel an already-attributed container. Added a regression for that sequence. Leaving this open for your review.

nm_hosts_by_dir.setdefault(file.parent, set()).add(node_id)
for container in unresolved_nm_containers.pop(file, []):
container.node_id = node_id
node.instance_type = _node_instance_type(line)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we preserve the existing instance type when this NM line doesn’t include one? Otherwise it clears a class already reported by the RM and can prevent pricing.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed on b0920a5. An NM registration now replaces instance_type only when that registration line actually reports one, so an RM-provided instance type is preserved otherwise. Added a focused regression. Leaving the thread open for confirmation.

@greptile-apps

greptile-apps Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Want your agent to iterate on Greptile's feedback? Try greploops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Preserve worker identity in YARN node reports for partial logs

2 participants