nvlink: use switch state machine to rotate certs#3249
Open
tmcroberts97 wants to merge 5 commits into
Open
Conversation
Instead of rotating the certs via RMS directly in the switch cert monitor, exercise the switch state machine. Signed-off-by: Thomas McRoberts <tmcroberts@nvidia.com>
|
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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughNvLinkManager and SwitchCertificateMonitor now use ComponentManager state-controller maintenance requests instead of RMS certificate jobs. Rack maintenance scheduling is transactional and idempotent, API handlers delegate to shared scheduling paths, and tests verify persisted maintenance state. ChangesSwitch certificate apply migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NvLinkManager
participant SwitchCertificateMonitor
participant ComponentManager
participant Database
NvLinkManager->>SwitchCertificateMonitor: start(..., component_manager)
SwitchCertificateMonitor->>ComponentManager: request_rack_maintenance_via_state_controller(ConfigureNmxCluster)
ComponentManager->>Database: lock rack and persist maintenance request
Database-->>ComponentManager: scheduling outcome
ComponentManager-->>SwitchCertificateMonitor: Scheduled or AlreadyPending
SwitchCertificateMonitor-->>NvLinkManager: Pending status
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/nvlink-manager/src/switch_cert_monitor.rs (1)
820-856: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: a transaction around a single statement adds no atomicity benefit here.
set_switch_maintenance_requestedis a singleUPDATE; wrapping it in an explicitbegin/commit(with onlybeginmade cancellable) is heavier than necessary. Since the helper accepts a&mut PgConnection, you could acquire a pooled connection directly and drop the transaction ceremony. Non-blocking.🤖 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/nvlink-manager/src/switch_cert_monitor.rs` around lines 820 - 856, The transaction wrapper in request_certificate_reconfiguration_with_switch_state_controller is unnecessary for a single set_switch_maintenance_requested update and only adds overhead/cancellation asymmetry. Replace the explicit self.db_pool.begin()/txn.commit() flow with a pooled connection path that passes a mutable connection directly into db::switch::set_switch_maintenance_requested, while keeping the same error handling and tracing::info context.
🤖 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.
Nitpick comments:
In `@crates/nvlink-manager/src/switch_cert_monitor.rs`:
- Around line 820-856: The transaction wrapper in
request_certificate_reconfiguration_with_switch_state_controller is unnecessary
for a single set_switch_maintenance_requested update and only adds
overhead/cancellation asymmetry. Replace the explicit
self.db_pool.begin()/txn.commit() flow with a pooled connection path that passes
a mutable connection directly into db::switch::set_switch_maintenance_requested,
while keeping the same error handling and tracing::info context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 899df5a8-0215-40ee-9f44-4a6b455d24a9
📒 Files selected for processing (5)
crates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/nvl_instance.rscrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/switch_cert_monitor.rs
💤 Files with no reviewable changes (3)
- crates/api-core/src/setup.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/nvlink-manager/src/lib.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
..to configure NVSwitch certs. Signed-off-by: Thomas McRoberts <tmcroberts@nvidia.com>
Contributor
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 `@crates/component-manager/src/component_manager.rs`:
- Around line 102-106: The maintenance request path in component_manager::update
should reject a new request when switch_maintenance_requested is already set,
since set_switch_maintenance_requested currently overwrites the single pending
DB slot. Add an explicit “already pending” guard before calling
db::switch::set_switch_maintenance_requested, returning an appropriate
ComponentManagerError if a PowerOn, PowerOff, Reset, or ReconfigureCertificate
request is already queued; alternatively, change the switch maintenance model to
a queue only if multiple outstanding requests are intended.
🪄 Autofix (Beta)
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: b10a4146-2dcb-4804-9f2e-e802cd8ede13
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/api-core/src/handlers/component_manager.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/component-manager/src/component_manager.rscrates/nvlink-manager/Cargo.tomlcrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/switch_cert_monitor.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/api-core/src/setup.rs
- crates/nvlink-manager/src/lib.rs
- crates/nvlink-manager/src/switch_cert_monitor.rs
...instead of using the configure_switch-certificate flow. Also introduce a rack maintenance scheduler to ensure atomic and idempotnt rack maintenance. Signed-off-by: Thomas McRoberts <tmcroberts@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/rack.rs`:
- Around line 68-78: Update find_by_id_for_update to filter out soft-deleted
racks by adding a deleted IS NULL condition to its SQL query, while preserving
the existing ID lookup and row-locking behavior.
In `@crates/component-manager/src/component_manager.rs`:
- Around line 150-168: Move the credential_manager.set_credentials call out of
the transaction and execute it only after the locked transaction commits with
Scheduled, preserving the existing single-writer check performed by
find_by_id_for_update. Ensure failures use an explicit post-commit recovery
strategy, such as clearing maintenance_requested in a follow-up transaction or
the established retry mechanism, without holding the database lock across the
external await.
🪄 Autofix (Beta)
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: 4062d56d-0248-4ed7-bacf-57dcc9b1fb16
📒 Files selected for processing (8)
crates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/tests/nvl_instance.rscrates/api-db/src/rack.rscrates/api-model/src/rack.rscrates/component-manager/Cargo.tomlcrates/component-manager/src/component_manager.rscrates/nvlink-manager/src/switch_cert_monitor.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-core/src/handlers/component_manager.rs
10 tasks
10 tasks
..after vault write failure. If the writing of the RMS token to vault fails, attempt to cancel the just submitted maintenance request to leave the rack in a cleaner state. Signed-off-by: Thomas McRoberts <tmcroberts@nvidia.com>
Signed-off-by: Thomas McRoberts <tmcroberts@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/rack.rs (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
query_scalarfor the single-column existence check.This query selects only
idand discards its value;query_scalar::<_, RackId>(query)withfetch_optionalexpresses that contract more directly and avoids the single-element tuple mapping.Proposed refactor
- sqlx::query_as::<_, (RackId,)>(query) + sqlx::query_scalar::<_, RackId>(query) .bind(rack_id) .fetch_optional(txn)🤖 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/rack.rs` around lines 61 - 69, Update lock_for_update to use sqlx::query_scalar::<_, RackId>(query) for the single-column id lookup, retaining the existing bind, fetch_optional, error mapping, and row-is-present boolean result without tuple mapping.Source: Learnings
🤖 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.
Nitpick comments:
In `@crates/api-db/src/rack.rs`:
- Around line 61-69: Update lock_for_update to use sqlx::query_scalar::<_,
RackId>(query) for the single-column id lookup, retaining the existing bind,
fetch_optional, error mapping, and row-is-present boolean result without tuple
mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef32ae0e-c49e-486e-9b90-f9008f44f63b
📒 Files selected for processing (4)
crates/api-core/src/handlers/rack.rscrates/api-db/src/rack.rscrates/component-manager/src/component_manager.rscrates/nvlink-manager/src/switch_cert_monitor.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/api-core/src/handlers/rack.rs
- crates/component-manager/src/component_manager.rs
- crates/nvlink-manager/src/switch_cert_monitor.rs
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.
Instead of rotating the certs via RMS directly in the switch cert monitor, exercise the switch state machine.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes