Skip to content

fix: Add IbPortDown alert for machines with down IB ports#519

Open
hasayesh wants to merge 1 commit intoNVIDIA:mainfrom
hasayesh:nvbug-5866723
Open

fix: Add IbPortDown alert for machines with down IB ports#519
hasayesh wants to merge 1 commit intoNVIDIA:mainfrom
hasayesh:nvbug-5866723

Conversation

@hasayesh
Copy link
Contributor

@hasayesh hasayesh commented Mar 11, 2026

Description

When the IB Fabric Monitor detects ports not in Active state, it now sets a PreventAllocations health alert on the affected machine. This prevents Carbide from attempting to allocate instances on machines with degraded IB connectivity, avoiding SRE alerts.

  • Add HealthProbeId::ib_port_down() and HealthProbeAlert::ib_port_down()
  • Detect ports not in Active state during IB fabric monitoring
  • Set/clear IbPortDown health alert via health report overrides
  • Update existing test to expect health alert blocking

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Related Issues (Optional)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

@hasayesh hasayesh requested a review from a team as a code owner March 11, 2026 04:25
@github-actions
Copy link

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-03-11 04:27:25 UTC | Commit: eb88f65

@github-actions
Copy link

github-actions bot commented Mar 11, 2026

🛡️ Vulnerability Scan

🚨 Found 72 vulnerability(ies)
📊 vs main: 72 (no change)

Severity Breakdown:

  • 🔴 Critical/High: 72
  • 🟡 Medium: 0
  • 🔵 Low/Info: 0

🔗 View full details in Security tab

🕐 Last updated: 2026-03-17 01:41:37 UTC | Commit: e4e83f8

@hasayesh hasayesh requested a review from Matthias247 March 11, 2026 16:51
@Matthias247
Copy link
Contributor

It needs to take the SKU into account. There's hosts which intentionally have multiple ports disconnected. And if they would all have PreventAllocations set, none of them would be usable anymore.

@hasayesh hasayesh requested a review from wminckler March 11, 2026 22:22
@hasayesh
Copy link
Contributor Author

This is what I suggest:
Update the L40 machines to have 2 IB's in SKU
Then we go from most to least specific:
Then if the machine has been assigned an instance-type use that
If not use SKU
if SKU does not exist (during early deployment) no check.

Please let me know if this is reasonable.

@hasayesh
Copy link
Contributor Author

This is what I suggest: Update the L40 machines to have 2 IB's in SKU Then we go from most to least specific: Then if the machine has been assigned an instance-type use that If not use SKU if SKU does not exist (during early deployment) no check.

Please let me know if this is reasonable.

OK my bad, json shows the properly populated inactive. Will update as such. Thanks.

///
/// Returns:
/// - `Ok(None)` if no SKU is assigned, SKU not found, or SKU has no infiniband_devices defined
/// (in these cases, IB port down monitoring should be skipped)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we don't want to monitor if we don't know the SKU of the machine?

What if we just returned an empty set in this case? That would mean "this machine does not expect any ports to be inactive", and we'd alert if the port is down. I feel that would be a more "safe" default if we don't know about the machine SKU, than avoiding alerting altogether.

(Maybe this question is for @Matthias247 who suggested checking the SKU. How do you feel about the default behavior if the SKU isn't found?)

Copy link
Contributor Author

@hasayesh hasayesh Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the two flags:

bom_validation.enabled
allow_allocation_on_validation_failure

If the operator explicitly skips bom_validation.enabled the point is not to do it correct? To me they both should be enabled, if the strict behavior is required otherwise what is the point of these?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're talking about different things.

I'm talking about what the fallback behavior is if we see an IB port down, but we're in an environment without BOM validation. I don't think we should skip alerting if that's the case.

To me, skipping BOM validation doesn't mean you don't care about whether IB ports are down. Unless I'm missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just returned an empty set in this case? That would mean "this machine does not expect any ports to be inactive", and we'd alert if the port is down.

I know of a few deployments which don't have SKUs/BOMs set up, but where not all ports are connected. For these the alarms would go off.
If we wanted to avoid that, then only taking into account SKUs if actually defined is ok.
But maybe this would also be a good way to "motivate" operators to setup SKUs?

I'd leave the decision up to @ajf and SRE teams.

Copy link
Contributor

@Matthias247 Matthias247 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at the implementation in-depth. I think @kensimon already did look deeper or can support more.

One question is probably whether the health alert is set in the IB monitor or in the main state handler. E.g. we currently set the "dpu heartbeattimeout" one in the main state handler. Both will work, and I personally don't have a big favorite (but maybe @kensimon or @chet might have?).

If you implement the optimization pointed out in the other comment then the check inside the monitor might be a bit more efficient since we don't need to load SKU data in the state handler.

When the IB Fabric Monitor detects ports not in Active state, it now
sets a PreventAllocations health alert on the affected machine. This
prevents Carbide from attempting to allocate instances on machines with
degraded IB connectivity, avoiding SRE alerts.
- Add HealthProbeId::ib_port_down() and HealthProbeAlert::ib_port_down()
- Detect ports not in Active state during IB fabric monitoring
- Set/clear IbPortDown health alert via health report overrides
- Update existing test to expect health alert blocking

    ## Type of Change
    <!-- Check one that best describes this PR -->
    - [ ] **Add** - New feature or capability
    - [ ] **Change** - Changes in existing functionality
    - [x] **Fix** - Bug fixes
    - [ ] **Remove** - Removed features or deprecated functionality
    - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.)

    ## Related Issues (Optional)
    <!-- If applicable, provide GitHub Issue. -->

    ## Breaking Changes
    - [ ] This PR contains breaking changes

    <!-- If checked above, describe the breaking changes and migration steps
    -->

    ## Testing
    <!-- How was this tested? Check all that apply -->
    - [x] Unit tests added/updated
    - [x] Integration tests added/updated
    - [x] Manual testing performed
    - [ ] No testing required (docs, internal refactor, etc.)

    ## Additional Notes
    <!-- Any additional context, deployment notes, or reviewer guidance -->

Signed-off-by: Hamid Asayesh <hasayesh@nvidia.com>

Apply suggestion from @kensimon

Co-authored-by: Ken Simon <ken@kensimon.io>
Signed-off-by: Hamid Asayesh <162524665+hasayesh@users.noreply.github.com>

Apply suggestion from @kensimon

Signed-off-by: Hamid Asayesh <162524665+hasayesh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants