Move BMCSettings mock redfish_local method into go mock server#790
Open
nagadeesh-nagaraja wants to merge 6 commits intomainfrom
Open
Move BMCSettings mock redfish_local method into go mock server#790nagadeesh-nagaraja wants to merge 6 commits intomainfrom
nagadeesh-nagaraja wants to merge 6 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds BMC Settings support to the Redfish mock server. It introduces a new BMC Settings endpoint with a corresponding attribute registry, implements runtime attribute loading and application logic, and refactors existing BMC reset/settings workflows to use HTTP PATCH requests instead of direct in-memory mutations. Changes
Sequence DiagramsequenceDiagram
participant Client as Test Client
participant MockSrv as Mock Server
participant BMCMgr as BMC Manager Resource
participant BMCSet as BMC Settings Resource
Client->>MockSrv: PATCH /Managers/BMC/Settings (Attributes + SettingsApplyTime=Immediate)
MockSrv->>MockSrv: applyBMCSettings(pending update)
activate MockSrv
MockSrv->>MockSrv: Load registry, filter noRebootBMCSettings
MockSrv->>MockSrv: Check if immediate attrs in noRebootBMCSettings
MockSrv->>BMCMgr: Apply immediate attrs to current Manager.Attributes
MockSrv->>MockSrv: Remove immediate attrs from pending update
MockSrv->>BMCSet: Store remaining attrs in Settings.Attributes
deactivate MockSrv
MockSrv-->>Client: 204 No Content
Note over Client,BMCSet: Later: BMC Reset Triggered
Client->>MockSrv: Reset BMC (GracefulRestart)
MockSrv->>MockSrv: doBMCReset()
activate MockSrv
MockSrv->>MockSrv: setLocked(false)
MockSrv->>MockSrv: applyPendingBMCSettings()
MockSrv->>BMCSet: Load pending Settings.Attributes
MockSrv->>BMCMgr: Copy pending to current Manager.Attributes
MockSrv->>BMCSet: Clear Settings.Attributes
deactivate MockSrv
MockSrv-->>Client: Reset complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bmc/mock/server/data/Registries/index.json (1)
5-15:⚠️ Potential issue | 🟡 MinorUpdate
Members@odata.countto reflect the actual member count.The
Members@odata.countfield is1but there are now 3 members in the array. This inconsistency could cause issues if Redfish clients rely on the count for iteration or validation.Proposed fix
- "Members@odata.count": 1, + "Members@odata.count": 3,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/data/Registries/index.json` around lines 5 - 15, The Members@odata.count value is incorrect (currently 1) and must match the actual number of entries in the Members array; update the "Members@odata.count" field in Registries/index.json to 3 so it reflects the three "@odata.id" entries in "Members" (e.g., "/redfish/v1/Registries/Base.1.5.0", "/redfish/v1/Registries/BiosAttributeRegistry.v1_0_0", "/redfish/v1/Registries/BMCAttributeRegistry").
🧹 Nitpick comments (1)
bmc/redfish_kube.go (1)
238-246: Resolve the settings URI from the selected manager instead of hardcoding it.Both methods ignore the requested BMC UUID and assume the settings resource is always
/redfish/v1/Managers/BMC/Settings. The local implementation inbmc/redfish_local.goalready follows the manager-advertised@Redfish.Settings.SettingsObject; reusing that pattern here would also let you send the same@Redfish.SettingsApplyTimepayload.Also applies to: 295-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/redfish_kube.go` around lines 238 - 246, The patch hardcodes "/redfish/v1/Managers/BMC/Settings" in SetBMCAttributesImmediately (in RedfishKubeBMC) instead of resolving the manager's advertised settings URI; update this method (and the corresponding apply/settings method around the 295-300 region) to locate the selected manager by the requested BMC UUID, read its `@Redfish.Settings.SettingsObject` (the pattern used in the local implementation in bmc/redfish_local.go), use that SettingsObject URI as the PATCH target, and support sending the same `@Redfish.SettingsApplyTime` payload as the local implementation does.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bmc/mock/server/server.go`:
- Around line 56-57: The immediate-apply whitelist variable noRebootBMCSettings
is missing the "composite" attribute listed as ResetRequired:false in the BMC
attribute registry; update the slice to include "composite" (e.g., var
noRebootBMCSettings = []string{"abc", "composite"}) so PATCHes for composite are
applied immediately; optionally note for future work to derive this list
programmatically from the BMCAttributeRegistry index to avoid drift.
- Around line 459-473: The reset path (doBMCReset) flips the resource lock via
setLocked but applyPendingBMCSettings/applyBMCSettings never checks that lock,
allowing PATCHes during the reset sleep to be accepted and applied; modify
applyPendingBMCSettings and the PATCH handling code to check the same lock state
(e.g., call an isLocked(bmcPath) helper or inspect the overrides[bmcPath] locked
flag) and return/reject with an error when the resource is locked, so any
incoming PATCHes while doBMCReset has setLocked(..., true) are refused rather
than queued/applied.
In `@bmc/redfish_kube.go`:
- Around line 244-250: The PATCH response from c.Patch in bmc/redfish_kube.go
(the resp returned by c.Patch("/redfish/v1/Managers/BMC/Settings", payload)) is
not being closed; after checking err != nil add a defer resp.Body.Close() to
ensure the response body is closed and connection-pool resources are freed
(place the defer immediately after the nil-error check in the same block where
resp is available).
---
Outside diff comments:
In `@bmc/mock/server/data/Registries/index.json`:
- Around line 5-15: The Members@odata.count value is incorrect (currently 1) and
must match the actual number of entries in the Members array; update the
"Members@odata.count" field in Registries/index.json to 3 so it reflects the
three "@odata.id" entries in "Members" (e.g.,
"/redfish/v1/Registries/Base.1.5.0",
"/redfish/v1/Registries/BiosAttributeRegistry.v1_0_0",
"/redfish/v1/Registries/BMCAttributeRegistry").
---
Nitpick comments:
In `@bmc/redfish_kube.go`:
- Around line 238-246: The patch hardcodes "/redfish/v1/Managers/BMC/Settings"
in SetBMCAttributesImmediately (in RedfishKubeBMC) instead of resolving the
manager's advertised settings URI; update this method (and the corresponding
apply/settings method around the 295-300 region) to locate the selected manager
by the requested BMC UUID, read its `@Redfish.Settings.SettingsObject` (the
pattern used in the local implementation in bmc/redfish_local.go), use that
SettingsObject URI as the PATCH target, and support sending the same
`@Redfish.SettingsApplyTime` payload as the local implementation does.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcf2b889-3a5e-4ded-a4a0-9f63d2f35f36
📒 Files selected for processing (13)
bmc/mock/server/data/Managers/BMC/Settings/index.jsonbmc/mock/server/data/Managers/BMC/index.jsonbmc/mock/server/data/Registries/BMCAttributeRegistry/index.jsonbmc/mock/server/data/Registries/index.jsonbmc/mock/server/server.gobmc/mockup.gobmc/redfish_kube.gobmc/redfish_local.gointernal/controller/biossettings_controller_test.gointernal/controller/bmcsettings_controller_test.gointernal/controller/bmcsettingsset_controller_test.gointernal/controller/helper.gointernal/controller/suite_test.go
💤 Files with no reviewable changes (1)
- bmc/mockup.go
afritzler
requested changes
Apr 8, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bmc/mock/server/server.go (1)
536-540:⚠️ Potential issue | 🟠 MajorReject settings PATCHes while the BMC is reset-locked.
Returning
nilhere still letshandlePatchpersist the body to/Managers/BMC/Settingsand answer 204, so writes made during the simulated offline window are queued and then applied byapplyPendingBMCSettings(). The lock is therefore not actually blocking mutations.🔒 Minimal fix
// If the BMC is mid-reset (locked), leave the immediate settings in attrs - // so they are written to the pending Settings resource and picked up by - // applyPendingBMCSettings once the reset completes. + // reject the PATCH so writes are not accepted while the BMC is offline. if s.isLocked(bmcBase) { - return nil + return errLocked }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bmc/mock/server/server.go` around lines 536 - 540, The branch that checks s.isLocked(bmcBase) currently returns nil which allows handlePatch to continue and persist the PATCH body; change it to reject the request immediately when the BMC is reset-locked by returning an error/HTTP 423-style response from the same check so handlePatch does not write to /Managers/BMC/Settings or return 204; update the code around s.isLocked(bmcBase) to short-circuit with an appropriate error value (so pending writes are not queued) and ensure applyPendingBMCSettings remains the mechanism that applies queued changes after unlock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bmc/mock/server/server.go`:
- Around line 56-85: The current loadNoRebootAttrs silently returns nil on
dataFS.ReadFile or json.Unmarshal errors causing silent behavior changes; update
loadNoRebootAttrs (used by init() to set noRebootSettings and
noRebootBMCSettings) to fail fast by panicking (or returning an error and making
init handle it) when ReadFile or Unmarshal fail, including the registryPath and
the underlying error in the panic/log message so the process exits with a clear
diagnostic instead of silently emptying the allowlist.
- Around line 623-642: The helper resetAttributesFromEmbeddedLocked currently
only replaces Attributes and can leave stale fields (like resourceLock) behind;
instead, read the embedded JSON via dataFS.ReadFile, unmarshal into a
map[string]any (as already done into defaults), and replace the entire in-memory
resource with that defaults map (i.e. assign overrides[filePath] = defaults)
rather than setting only current[attributesKey]; update usages of
loadResourceLocked/overrides accordingly so the full embedded resource (not just
Attributes) is restored in resetAttributesFromEmbeddedLocked.
---
Duplicate comments:
In `@bmc/mock/server/server.go`:
- Around line 536-540: The branch that checks s.isLocked(bmcBase) currently
returns nil which allows handlePatch to continue and persist the PATCH body;
change it to reject the request immediately when the BMC is reset-locked by
returning an error/HTTP 423-style response from the same check so handlePatch
does not write to /Managers/BMC/Settings or return 204; update the code around
s.isLocked(bmcBase) to short-circuit with an appropriate error value (so pending
writes are not queued) and ensure applyPendingBMCSettings remains the mechanism
that applies queued changes after unlock.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8adadbed-ab98-4f6f-a429-3b808fa2faa0
📒 Files selected for processing (3)
bmc/mock/server/server.gointernal/controller/biossettings_controller_test.gointernal/controller/bmcsettings_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/biossettings_controller_test.go
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.
Proposed Changes
Move mocked unit test methods into go mock server
Fixes #789
Summary by CodeRabbit
New Features
Bug Fixes
Chores