Skip to content

fix(flow): find untracked hosts when expected inventory is empty - #4909

Open
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:gh-issue-4356
Open

fix(flow): find untracked hosts when expected inventory is empty#4909
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:gh-issue-4356

Conversation

@chet

@chet chet commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Flow keeps a drift report comparing the hosts Cloud expects with the hosts Core discovers. As it stood, removing the final expected host made Flow stop asking Core for machines altogether. Hosts still visible to Core then disappeared from the report, while DPU records could sometimes be reported as unexpected hosts.

So, machine sync now asks Core what is present even when Cloud expects zero hosts, and it builds the report from HOST machine records only. If Core cannot answer, Flow keeps the last reliable report instead of replacing it with an empty one.

Key updates include:

  • Untracked hosts remain visible after the final expected host is removed.
  • DPUs are excluded from host matching and drift reporting.
  • A valid empty response and a failed Core request remain meaningfully different.

Related issues

This supports #4356

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Flow keeps a drift report comparing the hosts Cloud expects with the hosts Core discovers. As it stood, removing the final expected host made Flow stop asking Core for machines altogether. Hosts still visible to Core then disappeared from the report, while DPU records could sometimes be reported as unexpected hosts.

So, machine sync now asks Core what is present even when Cloud expects zero hosts, and it builds the report from `HOST` machine records only. If Core cannot answer, Flow keeps the last reliable report instead of replacing it with an empty one.

Key updates include:

- Untracked hosts remain visible after the final expected host is removed.
- DPUs are excluded from host matching and drift reporting.
- A valid empty response and a failed Core request remain meaningfully different.

Tests added!

This supports NVIDIA#4356

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
@chet
chet requested a review from a team as a code owner August 13, 2026 00:37
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5bc4a63f-be9b-4840-9416-38a6f80330bd

📥 Commits

Reviewing files that changed from the base of the PR and between 8529d0b and cd59638.

📒 Files selected for processing (4)
  • rest-api/flow/internal/scheduler/jobs/inventorysync/actual_sync_machine.go
  • rest-api/flow/internal/scheduler/jobs/inventorysync/actual_sync_machine_test.go
  • rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go
  • rest-api/flow/internal/scheduler/jobs/inventorysync/inventory_test.go
💤 Files with no reviewable changes (1)
  • rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go

Summary by CodeRabbit

  • Bug Fixes

    • Inventory synchronization now processes host machines only, preventing DPU records from being linked incorrectly.
    • Machines discovered without an expected match are reported as missing.
    • Synchronization continues querying inventory when no expected compute components exist.
    • Existing drift information is preserved when inventory retrieval fails.
  • Tests

    • Expanded coverage for host filtering, machine synchronization, orphan detection, persistence, and inventory cleanup scenarios.
    • Updated test documentation to clarify host-only machine matching.

Walkthrough

Machine synchronization now filters NICo inventory to HOST machine details before linking, counting, and drift detection. The flow queries NICo when expected compute components are empty and preserves existing drift when the actual-inventory RPC fails.

Changes

Inventory synchronization

Layer / File(s) Summary
Host filtering and machine synchronization
rest-api/flow/internal/scheduler/jobs/inventorysync/actual_sync_machine.go
The sync flow always queries NICo, retains HOST details, counts hosts, and passes filtered details to syncMachineIDs.
Host-scoped drift detection and integration
rest-api/flow/internal/scheduler/jobs/inventorysync/actual_sync_machine.go, rest-api/flow/internal/scheduler/jobs/inventorysync/inventory_test.go
Drift detection and machine-ID linking use host details. The DPU linkage test documents and validates host-only filtering.
Synchronization and failure regression coverage
rest-api/flow/internal/scheduler/jobs/inventorysync/actual_sync_machine_test.go, rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go
Tests cover host/DPU filtering, linking, persisted IDs, orphan drift, empty inventories, RPC failures, and related test cleanup.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Mergeability Score: ⚪ Minimal · up to cd596

This localized inventory-sync fix is merge-ready after normal checks and review; no actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary fix: finding untracked hosts when expected inventory is empty.
Description check ✅ Passed The description directly explains the synchronization bug, the HOST filtering fix, failure handling, and related tests.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@chet

chet commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full_review, thanks!

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

@chet Full review requested for #4909.

✅ Action performed

Full review finished.

@github-actions

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-08-13 00:41:27 UTC | Commit: cd59638

@thossain-nv thossain-nv added the rest-api Add this label when an issue or PR concerns NICo REST API label Aug 13, 2026 — with ChatGPT Codex Connector
@chet
chet requested review from kunzhao-nv and spydaNVIDIA August 13, 2026 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rest-api Add this label when an issue or PR concerns NICo REST API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants