fix: health service health-report now uses coalescing queue#569
Merged
Conversation
Signed-off-by: ianisimov <ianisimov@nvidia.com>
🛡️ Vulnerability Scan🚨 Found 74 vulnerability(ies) Severity Breakdown:
🔗 View full details in Security tab 🕐 Last updated: 2026-03-13 23:45:03 UTC | Commit: 462c3a7 |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-03-13 23:45:03 UTC | Commit: 462c3a7 |
Matthias247
approved these changes
Mar 13, 2026
poroh
approved these changes
Mar 14, 2026
ajf
pushed a commit
to ajf/infra-controller
that referenced
this pull request
Jun 1, 2026
…VIDIA#569) Signed-off-by: Chet Nichols III <chetn@nvidia.com>
10 tasks
chet
added a commit
to chet/bare-metal-manager-core
that referenced
this pull request
Jun 2, 2026
Brings `InfiniBandPartition` in line with the proto-modeling changes. The handler keeps its REST surface; Create and Update route through `ToProto` on the API request types and the entity gains the full round-trip plus a delete-request builder. The entity also picks up the typed `Labels` field per the `InstanceType` reference in NVIDIA#540 -- it reaches `Labels.ToProto()` / `Labels.FromProto(...)` directly. Primary callouts are: - API request side: `APIInfiniBandPartitionCreateRequest.ToProto(ibp)` and `APIInfiniBandPartitionUpdateRequest.ToProto(ibp)`; both source corresponding fields via `ibp.ToProto()`. - Entity side: `InfiniBandPartition.ToProto`, `FromProto`, and `ToDeletionRequestProto`. There's also a new `InfiniBandPartition.Validate()` so site-driven `FromProto` callers can do `FromProto` + `Validate` -- mirroring the MachineCapability pattern (which went in at NVIDIA#569). - `InfiniBandPartitionStatus` is now a typed string with its own `.FromProto(state)` and `.Message()` methods, replacing the `InfiniBandPartitionStatusFromProto(state)` helper that split a single proto state into `(status, message)`. The workflow activity caller is just a two-liner now. - Handler simplified to one-liners, and the inline `cwssaws` / `model/util` imports drop. As part of the new typed `InfiniBandPartitionStatus` (and the already-typed `Labels`), the typing propagates through the API and DAO layers, but nothing changes on the wire -- JSON serialization of `type X string` is identical to `string`, the OpenAPI spec still just says "string", and the SDK is unaffected. Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
added a commit
to chet/bare-metal-manager-core
that referenced
this pull request
Jun 2, 2026
Brings `InfiniBandPartition` in line with the proto-modeling changes. The handler keeps its REST surface; Create and Update route through `ToProto` on the API request types and the entity gains the full round-trip plus a delete-request builder. The entity also picks up the typed `Labels` field per the `InstanceType` reference in NVIDIA#540 -- it reaches `Labels.ToProto()` / `Labels.FromProto(...)` directly. Primary callouts are: - API request side: `APIInfiniBandPartitionCreateRequest.ToProto(ibp)` and `APIInfiniBandPartitionUpdateRequest.ToProto(ibp)`; both source corresponding fields via `ibp.ToProto()`. - Entity side: `InfiniBandPartition.ToProto`, `FromProto`, and `ToDeletionRequestProto`. There's also a new `InfiniBandPartition.Validate()` so site-driven `FromProto` callers can do `FromProto` + `Validate` -- mirroring the MachineCapability pattern (which went in at NVIDIA#569). - `InfiniBandPartitionStatus` is now a typed string with its own `.FromProto(state)` and `.Message()` methods, replacing the `InfiniBandPartitionStatusFromProto(state)` helper that split a single proto state into `(status, message)`. The workflow activity caller is just a two-liner now. - Handler simplified to one-liners, and the inline `cwssaws` / `model/util` imports drop. As part of the new typed `InfiniBandPartitionStatus` (and the already-typed `Labels`), the typing propagates through the API and DAO layers, but nothing changes on the wire -- JSON serialization of `type X string` is identical to `string`, the OpenAPI spec still just says "string", and the SDK is unaffected. Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
added a commit
that referenced
this pull request
Jun 3, 2026
…lPartition (#2122) ## Description Brings `NVLinkLogicalPartition` in line with the proto-modeling changes. Create and Update route through `ToProto` on the API request types. Primary callouts are: - API request side: `APINVLinkLogicalPartitionCreateRequest.ToProto` and `APINVLinkLogicalPartitionUpdateRequest.ToProto`, each sourcing corresponding fields via the entity's `ToProto()`. - Entity side: `NVLinkLogicalPartition.ToProto`, `FromProto`, and `ToDeletionRequestProto`. Metadata now lives in the entity's `ToProto()`. - There's also a new `NVLinkLogicalPartition.Validate()` so site-driven `FromProto` callers can do `FromProto` + `Validate` -- mirroring the MachineCapability workflow (which went in at [#569](NVIDIA/infra-controller-rest#569)). - `NVLinkLogicalPartitionStatus` is now a typed string with its own `.FromProto(state)` and `.Message()` methods, replacing the `wfutil.GetNVLinkLogicalPartitionStatus(state)` helper that split a single proto state into `(status, message)`. `UpdateNVLinkLogicalPartitionStatusInDB` is retained but now takes a typed `*NVLinkLogicalPartitionStatus`. - Handler simplified to one-liners on create / update / delete. As part of the new typed `NVLinkLogicalPartitionStatus`, the typing propagates through the API and DAO layers, but nothing changes on the wire -- JSON serialization of `type X string` is identical to `string`, the OpenAPI spec still just says "string", and the SDK is unaffected. Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com> ## Type of Change <!-- Check one that best describes this PR --> - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [x] **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 - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes <!-- Any additional context, deployment notes, or reviewer guidance --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Tests** * Updated NVLink Logical Partition test fixtures for consistency across handler and database model tests. * **Refactor** * Improved NVLink Logical Partition status handling with stronger type safety and enhanced validation for partition names and status values. * Streamlined workflow request payload generation with improved proto conversion utilities. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
added a commit
that referenced
this pull request
Jun 3, 2026
…rtition (#2116) ## Description This is NVIDIA/infra-controller-rest#594, re-opened over here (and had already gone through @coderabbitai feedback loops, curious to see what happens here). Brings `InfiniBandPartition` in line with the proto-modeling changes. The handler keeps its REST surface; Create and Update route through `ToProto` on the API request types and the entity gains the full round-trip plus a delete-request builder. The entity also picks up the typed `Labels` field per the `InstanceType` reference in #540 -- it reaches `Labels.ToProto()` / `Labels.FromProto(...)` directly. Primary callouts are: - API request side: `APIInfiniBandPartitionCreateRequest.ToProto(ibp)` and `APIInfiniBandPartitionUpdateRequest.ToProto(ibp)`; both source corresponding fields via `ibp.ToProto()`. - Entity side: `InfiniBandPartition.ToProto`, `FromProto`, and `ToDeletionRequestProto`. There's also a new `InfiniBandPartition.Validate()` so site-driven `FromProto` callers can do `FromProto` + `Validate` -- mirroring the MachineCapability pattern (which went in at #569). - `InfiniBandPartitionStatus` is now a typed string with its own `.FromProto(state)` and `.Message()` methods, replacing the `InfiniBandPartitionStatusFromProto(state)` helper that split a single proto state into `(status, message)`. The workflow activity caller is just a two-liner now. - Handler simplified to one-liners, and the inline `cwssaws` / `model/util` imports drop. As part of the new typed `InfiniBandPartitionStatus` (and the already-typed `Labels`), the typing propagates through the API and DAO layers, but nothing changes on the wire -- JSON serialization of `type X string` is identical to `string`, the OpenAPI spec still just says "string", and the SDK is unaffected. Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com> ## Type of Change <!-- Check one that best describes this PR --> - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [x] **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 - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes <!-- Any additional context, deployment notes, or reviewer guidance --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Corrected InfiniBand partition status handling in test scenarios. * **Refactor** * Improved internal type safety for InfiniBand partition status management. * Enhanced validation for InfiniBand partition operations and request parameters. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Chet Nichols III <chetn@nvidia.com>
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
3 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.
Description
Health Service used single unbounded queue, which meant if API is too slow to respond it grew and leaked memory.
This PR adds coalescing queue which only stores last reports per
id. This automatically throttle Sink to only produce reports as fast as API can handle it.It also intoroduces workers count, so it can submit from multiple threads (mostly usefull if there is more than 1 API instance).
Type of Change
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes
Criterion shows following results:
sink_health_override/enqueue/report time: [978.53 µs 988.74 µs 1.0003 ms] thrpt: [19.994 Melem/s 20.228 Melem/s 20.439 Melem/s] change: time: [−1.3991% −0.4695% +0.4203%] (p = 0.32 > 0.05) thrpt: [−0.4186% +0.4717% +1.4190%] No change in performance detected. Found 15 outliers among 100 measurements (15.00%) 1 (1.00%) low mild 3 (3.00%) high mild 11 (11.00%) high severe sink_health_override/drain/report time: [354.57 ns 355.91 ns 357.43 ns] thrpt: [16.787 Melem/s 16.858 Melem/s 16.922 Melem/s] change: time: [−0.3900% −0.0445% +0.3375%] (p = 0.81 > 0.05) thrpt: [−0.3363% +0.0445% +0.3915%] No change in performance detected. Found 11 outliers among 100 measurements (11.00%) 2 (2.00%) low mild 7 (7.00%) high mild 2 (2.00%) high severe sink_health_override/drain_convert/v time: [154.91 µs 155.33 µs 155.84 µs] thrpt: [38.500 Kelem/s 38.627 Kelem/s 38.733 Kelem/s] change: time: [+0.3760% +0.9159% +1.4849%] (p = 0.00 < 0.05) thrpt: [−1.4632% −0.9075% −0.3745%] Change within noise threshold. Found 12 outliers among 100 measurements (12.00%) 6 (6.00%) high mild 6 (6.00%) high severeWhich shows what slowest par is the
try_into()part. Attempts to optimize it lead only to marginal increase in performance (10-20%), but made convert much more complicated, so i we assume what38Kelem/sis fine.