fix(flow): preserve expected component descriptions - #4713
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughThe mirror carries Core descriptions into normalized machine, NVSwitch, and power-shelf components. Reconciliation manages the reserved description entry, preserves unrelated metadata, persists updates, and clears removed descriptions. Unit and database-backed tests cover the lifecycle. ChangesExpected description synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change preserves expected component descriptions while retaining existing metadata, with no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Core
participant Reconciliation
participant updateMirroredComponent
participant Database
Core->>Reconciliation: provide expected component description
Reconciliation->>updateMirroredComponent: apply expected_description
updateMirroredComponent->>Database: set or remove reserved metadata key
Database-->>updateMirroredComponent: preserve runtime and operator metadata
updateMirroredComponent-->>Reconciliation: persist mirrored component
``
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 4 | ❌ 1</summary>
### ❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
| :----------------: | :--------- | :------------------------------------------------------------------------------------ | :--------------------------------------------------------------------------------- |
| Docstring Coverage | ⚠️ Warning | Docstring coverage is 40.91% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
<details>
<summary>✅ Passed checks (4 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :-------------------------------------------------------------------------------------------------------------------------------- |
| Title check | ✅ Passed | The title clearly identifies the primary change: preserving expected component descriptions in Flow. |
| Description check | ✅ Passed | The description directly explains description propagation, ownership, merging, clearing, tests, and validation for the changeset. |
| Linked Issues check | ✅ Passed | The changes satisfy issue `#4353` by preserving descriptions for all component types and retaining runtime and operator metadata. |
| Out of Scope Changes check | ✅ Passed | The code and tests remain within the linked issue scope of expected description propagation, ownership, merging, and clearing. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches</summary>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-5296370324"} --> Create PR with unit tests
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->
---
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
|
@coderabbitai full review. |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.go`:
- Line 446: Update the inventory reconciliation update logic around the
description column so it never replaces the complete stored JSONB map with the
stale snapshot value. Use an atomic JSONB expression against the database column
to update only expected_description, deleting that key when Core clears it,
while preserving unrelated runtime metadata such as nvos_ip. Add an interleaving
integration test covering a runtime metadata update during reconciliation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d60ad4bc-7b8c-4fd8-8cab-313b65fbe667
📒 Files selected for processing (3)
rest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component.gorest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_component_test.gorest-api/flow/internal/scheduler/jobs/inventorysync/expected_mirror_db_test.go
|
@coderabbitai full review. |
|
✅ Action performedFull review finished. |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-12 17:24:45 UTC | Commit: a4d9d45 |
1643cc9 to
a4d9d45
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4713.docs.buildwithfern.com/infra-controller |
a4d9d45 to
14209d1
Compare
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
14209d1 to
d4ef351
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full review. |
|
✅ Action performedFull review finished. |
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Makes sense to me, Description as a JSON makes this a bit confusing. @kunzhao-nv Can you take a look as well?
| labelComponentSlotID = "slot_id" | ||
| labelComponentTrayIdx = "tray_idx" | ||
| labelComponentHostID = "host_id" | ||
| expectedDescriptionKey = "expected_description" |
There was a problem hiding this comment.
Should this be metadata_description?
There was a problem hiding this comment.
The source is Metadata.description but this is really the description for all the Expected* components...
Depends what we want the name to reflect.
2 participants
Summary
Preserve Core descriptions for
ExpectedMachine,ExpectedSwitch, andExpectedPowerShelfwhen Flow mirrors expected inventory.The Core-owned value is stored under the reserved
expected_descriptionkey so expected reconciliation can create, update, or clear it without replacing runtime- or operator-owned description entries.Fixes #4353.
Root cause
The Core API detail models already exposed descriptions, but the Flow normalization and persistence paths omitted them. Consequently, new mirrored components lost their descriptions and existing components never received description changes.
The description column is also shared with runtime metadata such as the NVSwitch
nvos_ip, so replacing the entire JSON map would have caused data loss.Changes
ExpectedMachine,ExpectedSwitch, andExpectedPowerShelf.expected_descriptionkey.expected_descriptionwhen Core clears the value.Validation
make test-flowgo vet ./flow/internal/scheduler/jobs/inventorysyncgit diff --checkThe full Flow suite was rerun after rebasing onto current upstream
main.Note
This is an attempt at fully automated issue resolution using Codex.