fix(platform-connectors): recover after GPU replacement - #1587
Conversation
Signed-off-by: Billard <82095453+iacker@users.noreply.github.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Kubernetes connector now ignores ChangesGPU replacement recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR makes a localized GPU recovery behavior change with no actionable merge-blocking risk remaining beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant HealthyGPUEvent
participant processNodeEvents
participant recoveryEntities
participant NodeCondition
HealthyGPUEvent->>processNodeEvents: submit healthy GPU event
processNodeEvents->>recoveryEntities: derive recovery entities
recoveryEntities-->>processNodeEvents: return stable GPU and PCI identities
processNodeEvents->>NodeCondition: remove matching fault message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test 4a21647 |
|
🌿 Fern Docs Preview: https://nvidia-preview-pull-request-1587.docs.buildwithfern.com/nvsentinel |
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
|
||
| hasStableGPUIdentity := false | ||
|
|
||
| for _, entity := range event.EntitiesImpacted { |
There was a problem hiding this comment.
Do we need two for loops? If we loop over EntitiesImpacted just once we can collect all the "stable identities", and on exit it that is not empty then return?
Signed-off-by: Billard <82095453+iacker@users.noreply.github.com>
|
yeah, good call, one loop is enough. Done in 2fe000d. I collect the entities and set hasStableGPUIdentity := false
entities := make([]*protos.Entity, 0, len(event.EntitiesImpacted))
for _, entity := range event.EntitiesImpacted {
if strings.EqualFold(entity.EntityType, "GPU") || strings.EqualFold(entity.EntityType, "PCI") {
hasStableGPUIdentity = true
}
if !strings.EqualFold(entity.EntityType, "GPU_UUID") {
entities = append(entities, entity)
}
}
if !hasStableGPUIdentity {
return event.EntitiesImpacted
}
return entitiesOnly cost is we build the slice even when we end up not using it, which felt cheaper than walking the list twice.
|
2 participants
Summary
Fixes #1585.
GPU recovery now ignores
GPU_UUIDwhen a stable GPU index or PCI address is present. This lets a healthy event clear the old node condition after a physical GPU replacement, while preserving UUID-only and non-GPU matching behavior.Type of Change
Component(s) Affected
Testing
Checklist
Summary by CodeRabbit