Add early returns on resource changes in `ServerReconciler` by nagadeesh-nagaraja · Pull Request #539 · ironcore-dev/metal-operator

nagadeesh-nagaraja · 2025-11-25T19:03:09Z

Proposed Changes

Server Reconciler avoid double reconciliation logic. This helps avoid overloading BMC, multiple reconcile during sequencial l update

Summary by CodeRabbit

New Features
- System information now includes BIOS version tracking.
Improvements
- Enhanced server status updates and boot configuration management.
- Improved detection and handling of server configuration changes.
- Better server state transition management with more efficient update processing.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Nuckal777 · 2025-11-27T10:12:40Z

Thanks, could you please remove the changes of #540 from this PR?

nagadeesh-nagaraja · 2025-11-27T10:51:12Z

Thanks, could you please remove the changes of #540 from this PR?

I will rebase once that PR merges. this PR will fail unit tests without the test fixes in the other PR :D

will remove this commit once PR 549 merges

This reverts commit 099672c.

afritzler · 2025-12-09T07:50:02Z

+	if modified {
+		return ctrl.Result{}, nil
+	}


Why not doing this right after the ensureServerStateTransition call?

I would suggest doing something like that here:

if modified, err := r.ensureServerStateTransition(ctx, log, bmcClient, server); err != nil || modified { return ctrl.Result{}, err }

because we like to update the status field before ending the reconcile... as use the status field to make decision in beginning of reconcile loop, i wanted to update the status before next reconcile.
I know updating status might modify the CRD by itself.. but I think that next improvement to tackle, what do you think?

afritzler · 2026-01-15T15:39:51Z

@coderabbitai review

coderabbitai · 2026-01-15T15:39:58Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2026-01-15T15:40:04Z

Walkthrough

Extends SystemInfo with a BIOSVersion field in the BMC layer and refactors the server controller to use CreateOrPatch pattern with modification signaling, enabling callers to detect and conditionally requeue based on whether reconciliation steps mutated server state.

Changes

Cohort / File(s)	Summary
BMC Layer `bmc/bmc.go`, `bmc/redfish.go`	Added `BIOSVersion` field to `SystemInfo` struct and populated it in `GetSystemInfo()` from upstream system data.
Server Controller `internal/controller/server_controller.go`	Refactored reconciliation methods to return `(bool, error)` instead of `error` only, replacing direct `Patch()` calls with `CreateOrPatch()` to detect mutations. Affected functions: `ensureServerBootConfigRef()`, `extractServerDetailsFromRegistry()`, `applyBootConfigurationAndIgnitionForDiscovery()`, `updateServerStatusFromSystemInfo()`, `ensureInitialBootConfigurationIsDeleted()`, `patchServerState()`, `ensureServerPowerState()`, and status update functions. Controller now immediately requeues on detected modifications during state transitions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

api-change

Suggested reviewers

Nuckal777

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description is incomplete and contains grammatical errors. It lacks detailed explanation of changes, fixes section, and structured information compared to the template.	Expand description with specific changes made, clarify the problem being solved, include a 'Fixes #' section, and correct grammatical issues ('consmeticImp' branch name, 'sequencial l update').

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: adding early returns on resource changes in ServerReconciler to avoid unnecessary reconciliation cycles.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 5

🤖 Fix all issues with AI agents

In `@internal/controller/server_controller.go`:
- Around line 1044-1067: The updatePowerOffCondition function is using the
PoweringOnCondition constant while setting reason "ServerPowerOff" and message
"Server is powering off", which is semantically inconsistent; change the
condition type used in updatePowerOffCondition (the call to acc.UpdateSlice)
from PoweringOnCondition to a clearly named constant such as
PoweringOffCondition (or a generic PowerStateCondition) and ensure that the
chosen constant is declared and used consistently (and updated in any related
readers/guards) so the condition type matches the reason/message set by
updatePowerOffCondition.
- Around line 239-243: Fix the typo in the comment inside the Server state
transition block: change "there there as an event to process the new state" to
"there is an event to process the new state" near the modified check (the block
that returns ctrl.Result{}, nil when modified is true) so the comment reads
clearly.
- Around line 407-409: The current return ignores the modification flag from
ensureInitialBootConfigurationIsDeleted; change the return to propagate the
modified boolean (i.e., return modified, err) so callers receive the true/false
modification signal when err is nil, matching the file's existing pattern and
enabling the expected early-return behavior in the surrounding function.
- Around line 1019-1042: The updatePowerOnCondition function is incorrectly
using controllerutil.CreateOrPatch (which doesn't update the status
subresource); change it to use the Status writer: deep-copy the original server
(e.g., serverBase := server.DeepCopy()), mutate only server.Status.Conditions
with conditionutils.NewAccessor and acc.UpdateSlice (as you already do), then
call r.Status().Patch(ctx, server, client.MergeFrom(serverBase)) and handle the
error; return true when the patch succeeds (or after comparing pre/post
condition slices to detect a real change) and return false on no-op or error.
Apply the same pattern to updatePowerOffCondition.
- Around line 367-384: Replace status-only CreateOrPatch calls with
Status().Patch using the same merge-from pattern used elsewhere: take a copy of
the existing server object (e.g., serverBase), modify
server.Status.NetworkInterfaces and server.Status.State, then call
r.Status().Patch(ctx, server, client.MergeFrom(serverBase)) instead of
controllerutil.CreateOrPatch; apply this change for the occurrence in the block
updating server status (currently using controllerutil.CreateOrPatch) and the
other listed occurrences (around lines noted in the review), keeping the
surrounding error handling and the subsequent call to
r.invalidateRegistryEntryForServer unchanged.

♻️ Duplicate comments (1)

internal/controller/server_controller.go (1)

557-614: Same issue: CreateOrPatch used for status-only updates.

This function updates exclusively status fields (BIOSVersion, PowerState, SerialNumber, Processors, Storages, etc.) but uses CreateOrPatch which operates on the main resource, not the status subresource. This is the same issue flagged earlier and should use Status().Patch() instead.

🧹 Nitpick comments (1)

internal/controller/server_controller.go (1)
907-924: Redundant status assignment before CreateOrPatch.

Line 911 sets server.Status.State = state but this assignment is immediately overwritten by the CreateOrPatch mutate function at line 914. The assignment at line 911 should be removed.

Additionally, this function updates a status field via CreateOrPatch which has the same concern noted earlier.
Suggested fix
 func (r *ServerReconciler) patchServerState(ctx context.Context, server *metalv1alpha1.Server, state metalv1alpha1.ServerState) (bool, error) {
 	if server.Status.State == state {
 		return false, nil
 	}
-	server.Status.State = state

 	opResult, err := controllerutil.CreateOrPatch(ctx, r.Client, server, func() error {
 		server.Status.State = state
 		return nil
 	})

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 493d587 and 9f791d3.

📒 Files selected for processing (3)

bmc/bmc.go
bmc/redfish.go
internal/controller/server_controller.go

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified

Files:

bmc/bmc.go
bmc/redfish.go
internal/controller/server_controller.go

🧠 Learnings (8)