draft - #4653
Open
spydaNVIDIA wants to merge 1 commit into
Open
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThe PR adds switch health-history storage with deduplication and 250-record retention. The switch controller computes aggregate health once, records metrics, and queues deferred persistence through a PostgreSQL write operation. Integration tests cover insertion, retention, deduplication, and object-ID updates. ChangesSwitch health history
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant handle_object_state
participant record_metrics
participant PersistSwitchHealthHistory
participant PostgreSQL
handle_object_state->>handle_object_state: Aggregate switch health once
handle_object_state->>record_metrics: Pass aggregate report
handle_object_state->>PersistSwitchHealthHistory: Queue switch ID and report
PersistSwitchHealthHistory->>PostgreSQL: Persist deduplicated history row
PostgreSQL->>PostgreSQL: Retain newest 250 rows per switch
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/switch-controller/src/handler.rs (1)
75-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the aggregate report into the deferred write.
record_metricsborrowsaggregate_healthbefore the history write. Changerecord_health_historyto take ownership and move the report intoPersistSwitchHealthHistory. This removes one full clone per state-handler iteration.🤖 Prompt for 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. In `@crates/switch-controller/src/handler.rs` around lines 75 - 78, Update record_health_history to take ownership of aggregate_health and move it directly into PersistSwitchHealthHistory, rather than cloning it. Adjust the state-handler flow so record_metrics borrows the report before the deferred history write while preserving existing behavior.Source: Coding guidelines
crates/switch-controller/src/lib.rs (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestrict the write-operation API to this crate.
PersistSwitchHealthHistoryhas no external callers. Changewrite_opsand its struct fields topub(crate)sohandler.rscan construct it without exposing this implementation detail.🤖 Prompt for 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. In `@crates/switch-controller/src/lib.rs` at line 39, Restrict the write-operation API to crate scope: change the write_ops module declaration in crates/switch-controller/src/lib.rs:39-39 to pub(crate), and change the fields of PersistSwitchHealthHistory in crates/switch-controller/src/write_ops.rs:32-35 to pub(crate) so handler.rs can construct it without exposing the implementation publicly.Source: Coding guidelines
🤖 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 `@crates/api-db/migrations/20260805120000_switch_health_history.sql`:
- Around line 16-28: Ensure the 250-row retention limit is enforced when
histories are merged through object-ID updates, not only after inserts. In
crates/api-db/migrations/20260805120000_switch_health_history.sql lines 16-28,
extend the retention trigger to cover object_id updates if retaining
trigger-based enforcement; in crates/api-db/src/health_history.rs lines 198-200,
delete excess rows for new_object_id within the same update_object_ids
transaction. Add a test covering the merge of two histories that each contain
250 records.
In `@crates/api-db/src/health_history.rs`:
- Around line 160-174: Make the HealthHistoryTableId::Switch persistence query
atomically deduplicate identical reports per object_id, using per-object
serialization or a database uniqueness constraint with an atomic insert rather
than the current NOT EXISTS check. Add an integration test using two database
connections that concurrently submit the same health report and verifies only
one row is inserted.
---
Nitpick comments:
In `@crates/switch-controller/src/handler.rs`:
- Around line 75-78: Update record_health_history to take ownership of
aggregate_health and move it directly into PersistSwitchHealthHistory, rather
than cloning it. Adjust the state-handler flow so record_metrics borrows the
report before the deferred history write while preserving existing behavior.
In `@crates/switch-controller/src/lib.rs`:
- Line 39: Restrict the write-operation API to crate scope: change the write_ops
module declaration in crates/switch-controller/src/lib.rs:39-39 to pub(crate),
and change the fields of PersistSwitchHealthHistory in
crates/switch-controller/src/write_ops.rs:32-35 to pub(crate) so handler.rs can
construct it without exposing the implementation publicly.
🪄 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: add99e78-76e7-451c-915c-c039b518cf11
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/api-db/migrations/20260805120000_switch_health_history.sqlcrates/api-db/src/health_history.rscrates/switch-controller/Cargo.tomlcrates/switch-controller/src/handler.rscrates/switch-controller/src/lib.rscrates/switch-controller/src/write_ops.rs
Comment on lines
+16
to
+28
| CREATE OR REPLACE FUNCTION switch_health_history_keep_limit() | ||
| RETURNS TRIGGER AS | ||
| $body$ | ||
| BEGIN | ||
| DELETE FROM switch_health_history WHERE object_id=NEW.object_id AND id NOT IN (SELECT id from switch_health_history where object_id=NEW.object_id ORDER BY id DESC LIMIT 250); | ||
| RETURN NULL; | ||
| END; | ||
| $body$ | ||
| LANGUAGE plpgsql; | ||
|
|
||
| CREATE TRIGGER t_switch_health_history_keep_limit | ||
| AFTER INSERT ON switch_health_history | ||
| FOR EACH ROW EXECUTE PROCEDURE switch_health_history_keep_limit(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply retention after object-ID updates.
The trigger runs only after INSERT. update_object_ids changes object_id without invoking the trigger. If both object IDs have 250 records, one rename can leave 500 records under the new ID.
Enforce the 250-record limit in the object-ID update transaction, or extend retention to object-ID updates. Add a test that merges two full histories.
crates/api-db/migrations/20260805120000_switch_health_history.sql#L16-L28: retain rows after anobject_idchange if trigger-based enforcement is kept.crates/api-db/src/health_history.rs#L198-L200: delete excess rows fornew_object_idafter the rename within the same transaction.
📍 Affects 2 files
crates/api-db/migrations/20260805120000_switch_health_history.sql#L16-L28(this comment)crates/api-db/src/health_history.rs#L198-L200
🤖 Prompt for 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.
In `@crates/api-db/migrations/20260805120000_switch_health_history.sql` around
lines 16 - 28, Ensure the 250-row retention limit is enforced when histories are
merged through object-ID updates, not only after inserts. In
crates/api-db/migrations/20260805120000_switch_health_history.sql lines 16-28,
extend the retention trigger to cover object_id updates if retaining
trigger-based enforcement; in crates/api-db/src/health_history.rs lines 198-200,
delete excess rows for new_object_id within the same update_object_ids
transaction. Add a test covering the merge of two histories that each contain
250 records.
Comment on lines
+160
to
+174
| HealthHistoryTableId::Switch => "WITH new_history_record as( | ||
| SELECT $1 as object_id, | ||
| $2::jsonb as health, | ||
| $3 as health_hash, | ||
| $4 as time | ||
| ), | ||
| last_history_record as( | ||
| SELECT health_hash FROM switch_health_history | ||
| WHERE object_id = $1 | ||
| ORDER BY id DESC | ||
| LIMIT 1 | ||
| ) | ||
| INSERT INTO switch_health_history (object_id, health, health_hash, time) | ||
| SELECT * FROM new_history_record | ||
| WHERE NOT EXISTS (SELECT health_hash FROM last_history_record WHERE last_history_record.health_hash = new_history_record.health_hash);", |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect persistence call paths and any existing serialization of switch writes.
rg -n -C5 '\bPersistSwitchHealthHistory\b|\bhealth_history::persist\s*\(|\bDbWriteBatch\b|handle_object_state' \
crates/switch-controller crates/state-controller crates/api-dbRepository: NVIDIA/infra-controller
Length of output: 22063
Make switch health deduplication atomic.
Concurrent transactions can both read the same latest hash and insert duplicate rows. Serialize persistence per object_id, or enforce deduplication with a database constraint and an atomic insert. Add a two-connection integration test for concurrent identical reports.
🤖 Prompt for 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.
In `@crates/api-db/src/health_history.rs` around lines 160 - 174, Make the
HealthHistoryTableId::Switch persistence query atomically deduplicate identical
reports per object_id, using per-object serialization or a database uniqueness
constraint with an atomic insert rather than the current NOT EXISTS check. Add
an integration test using two database connections that concurrently submit the
same health report and verifies only one row is inserted.
Contributor
|
title is "draft" and description is empty |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes