nvlink: use switch state machine to rotate certs#3249
Open
tmcroberts97 wants to merge 2 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
Summary by CodeRabbit
WalkthroughNvLinkManager and SwitchCertificateMonitor drop RMS-related wiring. Switch certificate reconfiguration now records switch-maintenance requests through the state controller, and the affected setup paths and tests were updated to match. ChangesSwitch certificate apply migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NvLinkManager
participant SwitchCertificateMonitor
participant ComponentManager
participant DB
participant SwitchStateController
NvLinkManager->>SwitchCertificateMonitor: new(..., component_manager)
SwitchCertificateMonitor->>ComponentManager: request_switch_maintenance_via_state_controller(...)
ComponentManager->>DB: set_switch_maintenance_requested(ReconfigureCertificate)
DB-->>ComponentManager: request results
ComponentManager->>SwitchStateController: record maintenance request
SwitchCertificateMonitor-->>NvLinkManager: Pending / applied_updates = 0
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
| format!("RMS ConfigureSwitchCertificate failed: {error}") | ||
| })?, | ||
| }; | ||
| db::switch::set_switch_maintenance_requested( |
Contributor
There was a problem hiding this comment.
do not call DB operation .. Instead use the Component manager
Contributor
Author
There was a problem hiding this comment.
But doesn't that call RMS? I thought we decided to use the switch state machine to do rotation.
| .insert(key, InFlightSwitchCertJob { job_id }); | ||
| Ok(SwitchCertApplyStatus::Pending) | ||
| } | ||
| self.request_certificate_reconfiguration_with_switch_state_controller(target, cancel_token) |
Contributor
There was a problem hiding this comment.
along with switch u need to call nmxc switch reconfigure also. Bcz it is in the scope of RACK
A seperate call is required
Contributor
There was a problem hiding this comment.
nmxc reconfigure is done ?
🔍 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
Comment on lines
+102
to
+106
| db::switch::set_switch_maintenance_requested( | ||
| txn, *switch_id, &initiator, operation, | ||
| ) | ||
| .await | ||
| .map_err(|error| ComponentManagerError::Internal(error.to_string()))?; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How is switch_maintenance_requested consumed/cleared by the state controller?
rg -nP 'switch_maintenance_requested' -C3 --type=rust
# Locate the state-controller/FSM reader of this field
rg -nP '(take|clear|consume|reset).*maintenance_requested|maintenance_requested.*(take|clear|None)' -C3 --type=rustRepository: NVIDIA/infra-controller
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files mentioning maintenance_requested or switch_maintenance_requested =="
rg -n --hidden --glob '!target' --glob '!**/node_modules/**' \
'switch_maintenance_requested|maintenance_requested' .
echo
echo "== Likely switch/state controller files =="
rg -n --hidden --glob '!target' --glob '!**/node_modules/**' \
'state controller|state_controller|switch.*state|maintenance' crates deploy book .github | head -n 300
echo
echo "== Database migration / schema references =="
rg -n --hidden --glob '!target' --glob '!**/node_modules/**' \
'switches.*switch_maintenance_requested|switch_maintenance_requested.*json|switch_maintenance_requested' \
crates db migrations sql . || trueRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== switch controller maintenance handler =="
sed -n '1,260p' crates/switch-controller/src/maintenance.rs
echo
echo "== switch controller ready handler =="
sed -n '1,180p' crates/switch-controller/src/ready.rs
echo
echo "== switch state-machine docs =="
sed -n '1,180p' docs/architecture/state_machines/switch.md
echo
echo "== switch configure-certificate docs =="
sed -n '220,290p' docs/architecture/state_machines/switch_configure_certificate.md
echo
echo "== switch db helper =="
sed -n '418,470p' crates/api-db/src/switch.rsRepository: NVIDIA/infra-controller
Length of output: 26502
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== switch maintenance API handler/tests =="
sed -n '1,220p' crates/api-core/src/handlers/switch.rs
echo
sed -n '1,260p' crates/api-core/src/tests/switch_state_controller/maintenance.rs
echo
echo "== switch model and DB mapping =="
sed -n '150,270p' crates/api-model/src/switch/mod.rs
echo
echo "== migration =="
sed -n '1,80p' crates/api-db/migrations/20260603120000_switch_maintenance_requested.sqlRepository: NVIDIA/infra-controller
Length of output: 21203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [
Path('crates/api-core/src/handlers/switch.rs'),
Path('crates/api-core/src/tests/switch_state_controller/maintenance.rs'),
Path('crates/api-model/src/switch/mod.rs'),
]:
print(p, p.exists(), p.stat().st_size if p.exists() else None, 'bytes', sum(1 for _ in p.open()) if p.exists() else None, 'lines')
PY
echo
echo "== migration =="
cat -n crates/api-db/migrations/20260603120000_switch_maintenance_requested.sqlRepository: NVIDIA/infra-controller
Length of output: 858
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path('crates/api-core/src/handlers/switch.rs'),
Path('crates/api-core/src/tests/switch_state_controller/maintenance.rs'),
Path('crates/api-model/src/switch/mod.rs'),
]
for p in paths:
if p.exists():
print(f"== {p} ==")
print(f"lines={sum(1 for _ in p.open())}")
PY
echo "== switch maintenance handler excerpts =="
rg -n -A4 -B4 'switch_maintenance_requested|set_switch_maintenance_requested|clear_switch_maintenance_requested|overwrit|already pending|already scheduled' crates/api-core/src/handlers/switch.rs crates/api-core/src/tests/switch_state_controller/maintenance.rs crates/api-model/src/switch/mod.rs crates/api-db/src/switch.rsRepository: NVIDIA/infra-controller
Length of output: 7387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== where switch maintenance is scheduled from API/component-manager =="
rg -n -A3 -B3 'set_switch_maintenance_requested|switch_maintenance_requested|maintenance requested' crates/api-core/src crates/component-manager/src crates/switch-controller/src docs/architecture/state_machines/switch.mdRepository: NVIDIA/infra-controller
Length of output: 10812
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== component_power_control scheduling path =="
rg -n -A5 -B5 'set_switch_maintenance_requested|component_power_control|SwitchMaintenanceOperation|ReconfigureCertificate' crates/api-core/src/handlers crates/api-core/src/tests crates/component-manager/src crates/api-db/src
echo
echo "== switch state controller tests mentioning maintenance requests =="
rg -n -A4 -B4 'maintenance_requested|ReconfigureCertificate|PowerOn|PowerOff|Reset|overwrit|already' crates/api-core/src/tests/switch_state_controller crates/switch-controller/srcRepository: NVIDIA/infra-controller
Length of output: 47089
Reject a second maintenance request while one is already pending. switch_maintenance_requested is a single DB slot, and the switch controller only clears it after consuming the request. A later set_switch_maintenance_requested call will overwrite the pending operation, so a concurrent PowerOn / PowerOff / Reset / ReconfigureCertificate request can disappear silently. Add an already pending check here, or model the field as an explicit queue if multiple outstanding requests are meant to be supported.
🤖 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/component-manager/src/component_manager.rs` around lines 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.
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