feat(expected-machine): per host BMC vendor override - #2912
Open
s3rj1k wants to merge 1 commit into
Open
Conversation
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
June 26, 2026 10:49
d38e985 to
04c592a
Compare
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:
WalkthroughAdds a machine-level Redfish BMC vendor override through the RPC, storage, Redfish client, and admin CLI paths. The override can now be set, cleared, shown, and propagated into Redfish client creation. ChangesVendor override flow
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/admin-cli/src/machine/vendor_override/args.rs`:
- Around line 43-47: The `vendor` argument on `VendorOverrideArgs` currently
accepts any string and only fails back to auto-detection later, so invalid
values slip through parse time. Update the `clap` field in `VendorOverrideArgs`
to validate against the exact `RedfishVendor` variant names (case-sensitive) by
using a `ValueEnum` or a custom parser/validator. Ensure `--vendor` is rejected
immediately for typos and only the supported vendor names are accepted.
In `@crates/api-core/src/handlers/machine.rs`:
- Around line 325-333: Validate the incoming bmc_vendor_override in
machine::update before calling db::machine::update_bmc_vendor_override: reject
any non-empty value that redfish_vendor_from_str does not accept and return
NicoError::InvalidArgument instead of persisting it. Keep the existing
empty/absent handling, but only pass through vendor names that map to a known
Redfish vendor so typos cannot be stored and later silently ignored when the
client is built.
🪄 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: 97cbfb50-bbec-4b40-8551-a3ed7fcd7472
📒 Files selected for processing (23)
crates/admin-cli/src/machine/mod.rscrates/admin-cli/src/machine/vendor_override/args.rscrates/admin-cli/src/machine/vendor_override/cmd.rscrates/admin-cli/src/machine/vendor_override/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/api.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/machine.rscrates/api-db/migrations/20260625120000_machine_bmc_vendor_override.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/redfish/src/libredfish/conv.rscrates/redfish/src/libredfish/mod.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine/mod.rscrates/utils/src/redfish.rsdocs/manuals/nico-admin-cli/commands/machine/machine-vendor-override-clear.mddocs/manuals/nico-admin-cli/commands/machine/machine-vendor-override-set.mddocs/manuals/nico-admin-cli/commands/machine/machine-vendor-override-show.mddocs/manuals/nico-admin-cli/commands/machine/machine-vendor-override.mddocs/manuals/nico-admin-cli/commands/machine/machine.md
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
July 1, 2026 08:33
04c592a to
304b943
Compare
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
July 1, 2026 08:49
304b943 to
fd86083
Compare
Contributor
|
/ok to test fd86083 |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2912.docs.buildwithfern.com/infra-controller |
🔐 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-07-08 07:51:52 UTC | Commit: fd86083 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
July 8, 2026 09:47
fd86083 to
a026f88
Compare
Contributor
Author
rebased and regenerated protos |
Collaborator
|
conflicts |
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
July 9, 2026 20:17
a026f88 to
148ab9c
Compare
Contributor
Author
rebased |
Collaborator
|
/ok to test 148ab9c |
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
July 9, 2026 21:05
148ab9c to
c6614cf
Compare
Contributor
Author
RBAC tests have failed, fixed (hopefully) and rebased, sorry about that |
wminckler
reviewed
Jul 16, 2026
| -- Add bmc_vendor_override to machines so an operator can pin the Redfish BMC | ||
| -- vendor for a machine. NULL means automatic detection. The value is a | ||
| -- RedfishVendor variant name passed down into libredfish as the forced vendor. | ||
| ALTER TABLE machines ADD COLUMN bmc_vendor_override text; |
Contributor
There was a problem hiding this comment.
wouldn't this feature be better off as part of expected machines?
Contributor
Author
There was a problem hiding this comment.
Sorry, for taking so long, was vacationing, yes, expected machines are better, thanks!
I've updated PR and did rebase on latest HEAD.
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
July 27, 2026 23:41
c6614cf to
e83d7d4
Compare
s3rj1k
changed the title
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
3 times, most recently
from
July 28, 2026 22:40
6546633 to
909e561
Compare
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
thossain-nv
removed
the
rest-api
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
July 29, 2026 10:47
909e561 to
0f88c5a
Compare
Contributor
Author
|
Kindly asking to enable testing on this PR, it touches RPC and has a tendency to get in conflict quite fast. |
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
July 31, 2026 00:07
0f88c5a to
0a39d62
Compare
wminckler
reviewed
Aug 6, 2026
| #[serde(default)] | ||
| pub host_lifecycle_profile: Option<HostLifecycleProfile>, | ||
| /// Operator pinned Redfish BMC vendor, a `RedfishVendor` variant name such as | ||
| /// `Dell`. Absent or empty means automatic detection. |
Contributor
There was a problem hiding this comment.
empty should be rejected. empty != optional in rust (in theory)
wminckler
reviewed
Aug 6, 2026
| .data | ||
| .bmc_vendor_override | ||
| .as_deref() | ||
| .filter(|name| !name.is_empty()) |
Contributor
There was a problem hiding this comment.
as I mentioned earlier, empty should be rejected
wminckler
reviewed
Aug 6, 2026
| /// sweep, so without negative caching the common path would query Postgres | ||
| /// on every client construction. | ||
| cache: moka::future::Cache<IpAddr, Option<RedfishVendor>>, | ||
| } |
Contributor
|
/ok to test 0a39d62 |
Contributor
|
I think you need to sign your commits too |
Contributor
Author
hmm, forgot after rebase, will do after test finishes |
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
2 times, most recently
from
August 13, 2026 22:35
47f08af to
d2e1995
Compare
Pin a Redfish BMC vendor on the expected machine record. Resolved inside `create_client` so every BMC client honors it, and recorded by Site Explorer so it also drives the firmware config key, the restart path and the console transport. Probe and factory-bootstrap clients stay unpinnable, since forcing a vendor there deadlocks credential rotation. Unpinned hosts are unchanged. Signed-off-by: s3rj1k <evasive.gyron@gmail.com>
s3rj1k
force-pushed
the
feat/machine-bmc-vendor-override
branch
from
August 18, 2026 10:45
d2e1995 to
7cc4239
Compare
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
5 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.
Pin a Redfish BMC vendor on the expected machine record. Resolved inside
create_clientso every BMC client honors it, and recorded by Site Explorer soit also drives the firmware config key, the restart path and the console
transport. Probe and factory-bootstrap clients stay unpinnable, since forcing a
vendor there deadlocks credential rotation. Unpinned hosts are unchanged.
Related issues
...
Type of Change
Breaking Changes
Testing
Additional Notes
...