Transition Server to Maintenance in case of pending BIOS settings exist
#581
Conversation
WalkthroughAdds HTTP DELETE endpoint to mock Redfish server for reverting resource overrides, fixes a logging typo in BIOS settings controller, and introduces test coverage for the maintenance workflow when pending BIOS settings exist, including HTTP PATCH and DELETE interactions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/controller/biossettings_controller_test.go (1)
1124-1128: Consider adding a timeout to the HTTP client.While the mock server is local and should respond quickly, adding a timeout to the HTTP client provides better test hygiene and prevents potential hangs.
🔎 Suggested improvement
- client := &http.Client{} + client := &http.Client{Timeout: 10 * time.Second}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/v1alpha1/biossettings_types.gobmc/mock/server/server.gointernal/controller/biossettings_controller.gointernal/controller/biossettings_controller_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/controller/biossettings_controller.go (1)
api/v1alpha1/biossettings_types.go (3)
BIOSSettingsStateBlocked(65-65)BIOSSettings(142-148)BIOSSettingsStatePending(63-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run test e2e
- GitHub Check: Run linter
- GitHub Check: Run tests
- GitHub Check: Run test chart
🔇 Additional comments (7)
bmc/mock/server/server.go (1)
160-172: LGTM! Clean DELETE handler implementation.The implementation correctly handles DELETE requests by removing overrides and returning 204 No Content. The mutex locking ensures thread safety, and the idempotent behavior (no error if override doesn't exist) aligns with HTTP DELETE semantics.
api/v1alpha1/biossettings_types.go (1)
64-65: LGTM! Well-documented new state.The
BIOSSettingsStateBlockedstate is appropriately placed in the state machine and clearly documented. This state enables proper handling of servers with pre-existing pending BIOS changes.internal/controller/biossettings_controller_test.go (2)
28-31: LGTM! Constants for mock server configuration.Extracting the mock server IP and port into constants improves maintainability and consistency across tests.
1104-1227: Good test coverage for the blocked state flow.The test comprehensively validates the new blocked state behavior:
- Sets up pending BIOS settings via HTTP PATCH
- Verifies transition to Blocked state
- Confirms state persists while pending settings exist
- Clears pending settings and verifies transition to InProgress/Applied
The test follows existing patterns in the file and provides confidence in the new functionality.
internal/controller/biossettings_controller.go (3)
275-276: LGTM! Blocked state case added to state machine.The new case correctly delegates to
handleBlockedStatefor processing.
299-316: Core fix: Transition to Blocked instead of failing.This is the key fix for the PR. When pending BIOS settings are detected, the controller now transitions to
Blockedstate with a requeue interval, allowing it to wait for the pending changes to be cleared rather than failing immediately. The condition is properly updated to track the reason for blocking.
403-419: Well-structured blocked state handler.The
handleBlockedStatefunction follows the established patterns in the controller:
- Checks if pending settings still exist
- Stays in Blocked state with requeue if pending settings persist
- Transitions back to Pending when cleared, which will reset conditions and restart the flow
The transition to Pending correctly leverages the existing
updateStatusbehavior that clears conditions when entering Pending state, ensuring a fresh start for the settings application.
xkonni
force-pushed
the
bug/fix-pending-biossettings
branch
from
7b7970a to
f795f1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/controller/biossettings_controller_test.go (1)
1146-1158: Variable shadowing:serverClaimshadows outer scope variable.At line 1146,
serverClaim :=creates a new variable that shadows theserverClaimdeclared at line 899 in the outerBeforeEachscope. While this works correctly since the outer variable isn't used in this test, it's cleaner to use assignment (=) instead to avoid confusion.♻️ Suggested fix
- serverClaim := &metalv1alpha1.ServerClaim{ + serverClaim = &metalv1alpha1.ServerClaim{internal/controller/biossettings_controller.go (1)
403-419: LGTM - Clean implementation of the Blocked state handler.The handler correctly:
- Rechecks pending BIOS settings on each reconciliation
- Stays in
Blockedstate with requeue if settings are still pending- Transitions back to
Pendingwhen cleared, allowing the normal flow to resumeThe transition to
Pending(rather thanInProgress) is the right choice as it ensures the full validation flow runs again after the blockage clears.Consider adding a condition update when transitioning out of
Blockedstate to provide better observability/traceability. For example:// When transitioning out of Blocked: log.V(1).Info("Pending settings cleared - transitioning back to Pending state") // Optionally update the existing BIOSPendingSettingConditionCheck condition // to indicate the blockage was resolvedThis is optional since the existing
BIOSPendingSettingConditionCheckcondition already captures that pending settings were found.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/v1alpha1/biossettings_types.gobmc/mock/server/server.gointernal/controller/biossettings_controller.gointernal/controller/biossettings_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v1alpha1/biossettings_types.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/controller/biossettings_controller.go (1)
api/v1alpha1/biossettings_types.go (3)
BIOSSettingsStateBlocked(65-65)BIOSSettings(142-148)BIOSSettingsStatePending(63-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run test e2e
- GitHub Check: Run linter
- GitHub Check: Run test chart
- GitHub Check: Run tests
🔇 Additional comments (6)
bmc/mock/server/server.go (2)
84-85: LGTM - DELETE method routing added correctly.The routing to
handleRedfishDELETEfollows the existing pattern for other HTTP methods.
160-194: LGTM - DELETE handler implementation is correct.The handler correctly:
- Validates the resource exists (via
fetchCurrentDataForPath)- Rejects collection resources
- Removes the override to revert to embedded JSON
Minor note:
fetchCurrentDataForPathstores data ins.overrides(line 576) before DELETE removes it, which is a slight inefficiency but acceptable for a test mock server.internal/controller/biossettings_controller_test.go (2)
28-31: LGTM - Constants extracted for test configuration.Extracting
mockServerIPandmockServerPortas constants improves maintainability and reduces magic values throughout the test file.
1104-1231: Good test coverage for the new Blocked state workflow.The test comprehensively covers:
- Setting up pending BIOS settings via HTTP PATCH
- Verifying transition to
Blockedstate- Confirming the state remains
Blockedduring retry cycles- Clearing pending settings via HTTP DELETE
- Verifying successful transition to
InProgress/AppliedThe use of explicit HTTP calls to the mock server effectively simulates real-world scenarios where pending BIOS changes exist on the BMC.
internal/controller/biossettings_controller.go (2)
275-276: LGTM - Blocked state routing added to state machine.The new case properly routes
BIOSSettingsStateBlockedto the newhandleBlockedStatehandler, maintaining the existing pattern.
299-315: LGTM - Pending settings handling improved.Good changes:
- Fixed log key typo:
"TaskCount"(was likely"TaskCound")- Transitions to
Blockedstate instead of failing when pending BIOS settings are detectedThe
RequeueAfter: r.ResyncIntervalensures the controller will retry after the configured interval, giving external processes time to clear pending settings.
- if pending settings exist, create maintenance object and fail - added unit test and `delete` method for bmc mock
xkonni
force-pushed
the
bug/fix-pending-biossettings
branch
from
f795f1a to
8a5414e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bmc/mock/server/server.go (1)
160-194: DELETE handler correctly implements override removal.The implementation properly reverts overrides to restore embedded JSON data. The logic flow is sound:
- Validates the resource exists and isn't a collection
- Removes the override entry under mutex protection
- Returns 204 No Content (correct status for successful DELETE)
The DELETE behavior is idempotent, succeeding even when no override exists, which aligns with REST best practices.
Optional: Consider extracting duplicate error handling.
The error handling block (lines 168-179) is identical to the PATCH handler (lines 117-128). While not critical for a mock server, extracting this into a helper function would improve maintainability:
func handleFetchError(w http.ResponseWriter, r *http.Request, err error) bool { switch { case strings.Contains(err.Error(), "resource not found"): http.NotFound(w, r) return true case strings.Contains(err.Error(), "corrupt embedded JSON"): http.Error(w, err.Error(), http.StatusInternalServerError) return true default: http.Error(w, err.Error(), http.StatusInternalServerError) return true } }Then both handlers could use:
base, err := fetchCurrentDataForPath(s, urlPath) if err != nil { handleFetchError(w, r, err) return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bmc/mock/server/server.gointernal/controller/biossettings_controller.gointernal/controller/biossettings_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/biossettings_controller.go
- internal/controller/biossettings_controller_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run test e2e
- GitHub Check: Run linter
- GitHub Check: Run tests
- GitHub Check: Run test chart
🔇 Additional comments (1)
bmc/mock/server/server.go (1)
84-85: LGTM! DELETE routing properly integrated.The DELETE method routing follows the established pattern and correctly dispatches to the new handler.
| const ( | ||
| mockServerIP = "127.0.0.1" | ||
| mockServerPort = 8000 | ||
| ) |
There was a problem hiding this comment.
Can we move this to the suite setup?
| }) | ||
|
|
||
| It("Should fail and create maintenance when pending settings exist", func(ctx SpecContext) { | ||
| // Ensure BMC is in Enabled state |
There was a problem hiding this comment.
Please use a By(...) statement instead of commenting the code.
| } | ||
| resp, err := client.Do(req) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Use a DeferCleanup() function here to close the body.
|
|
||
| clearResp, err := client.Do(clearReq) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| defer clearResp.Body.Close() |
afritzler
changed the title
Server to Maintenance in case of pending BIOS settings exist
When the metal operator tries to apply biossettings to a server with pending changes, it fails and releases the server immediately. no maintenance object is created. the server can then be claimed and integrated into production, making it hard to apply the biossettings at a later time.
This change creates a maintenance object when pending changes exist and sets the biossettings state to failed.
Changes:
- if pending settings exist, create maintenance object and fail
- added unit test and
deletemethod for bmc mockSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.