Support ServerMaintenanceApproval label
#618
Merged
Conversation
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
WalkthroughUpdates ServerMaintenance approval handling to accept either an annotation or a label on ServerClaim; controller checks both, clears the key during cleanup, and tests updated to set both metadata channels (annotation + label) using the trueValue constant. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Controller as ServerMaintenanceController
participant K8sAPI as Kubernetes API (ServerClaim)
participant Server as Server resource
Controller->>K8sAPI: Fetch ServerClaim
alt approval via annotation or label present
note right of Controller: checks annotation OR label for ServerMaintenanceApprovalKey
Controller->>K8sAPI: Update Server reference on ServerMaintenance (set Server)
Controller->>K8sAPI: Patch ServerMaintenance state -> InMaintenance
K8sAPI-->>Controller: Persisted updates (200)
Controller->>Server: (optional) ensure server reference consistency
else no approval found
Controller->>Controller: Log "waiting for approval"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🤖 Fix all issues with AI agents
In `@api/v1alpha1/servermaintenance_types.go`:
- Around line 16-17: Update the comment for ServerMaintenanceApprovalKey to
clarify semantics: state that the string
"metal.ironcore.dev/maintenance-approval" is the preferred label key for storing
server maintenance approval, but note that it is still read/written as a
deprecated annotation in controllers and tests (include "deprecated" and
migration/removal intent), and document which metadata form should be used going
forward so consumers know label is preferred and annotation support is legacy.
In `@internal/controller/servermaintenance_controller.go`:
- Around line 156-171: When cleaning up a ServerMaintenance object, ensure you
remove the approval key from both annotations and labels to avoid stale
auto-approvals: in the cleanup path that currently calls
updateServerRef/patchMaintenanceState (use the serverClaim and server variables
and the existing updateServerRef method), check for
metalv1alpha1.ServerMaintenanceApprovalKey in serverClaim.GetAnnotations() and
serverClaim.GetLabels() and remove it from both maps, persist the change via the
same update/patch flow (e.g., call updateServerRef or client.Update on the
ServerClaim) so the approval label/annotation is cleared during maintenance
teardown.
Comment on lines
156
to
+171
| annotations := serverClaim.GetAnnotations() | ||
| if _, ok := annotations[metalv1alpha1.ServerMaintenanceApprovalKey]; !ok { | ||
| log.V(1).Info("Server not approved for maintenance, waiting for approval", "Server", server.Name) | ||
| return ctrl.Result{}, nil | ||
| } | ||
| log.V(1).Info("Server approved for maintenance", "Server", server.Name) | ||
| if err = r.updateServerRef(ctx, log, maintenance, server); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| if modified, err := r.patchMaintenanceState(ctx, maintenance, metalv1alpha1.ServerMaintenanceStateInMaintenance); err != nil || modified { | ||
| return ctrl.Result{}, err | ||
| labels := serverClaim.GetLabels() | ||
| _, hasAnnotation := annotations[metalv1alpha1.ServerMaintenanceApprovalKey] | ||
| _, hasLabel := labels[metalv1alpha1.ServerMaintenanceApprovalKey] | ||
|
|
||
| if hasAnnotation || hasLabel { | ||
| log.V(1).Info("Server approved for maintenance", "Server", server.Name) | ||
| if err = r.updateServerRef(ctx, log, maintenance, server); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| if modified, err := r.patchMaintenanceState(ctx, maintenance, metalv1alpha1.ServerMaintenanceStateInMaintenance); err != nil || modified { | ||
| return ctrl.Result{}, err | ||
| } | ||
| } | ||
| log.V(1).Info("Server not approved for maintenance, waiting for approval", "Server", server.Name) | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
Clear approval label on cleanup to avoid stale auto-approvals.
With label-based approval now accepted, cleanup must also remove the approval label; otherwise a previously approved ServerClaim could silently auto-approve future maintenances.
🔧 Suggested fix (cleanup)
- metautils.DeleteLabels(serverClaim, []string{
- metalv1alpha1.ServerMaintenanceNeededLabelKey,
- })
+ metautils.DeleteLabels(serverClaim, []string{
+ metalv1alpha1.ServerMaintenanceNeededLabelKey,
+ metalv1alpha1.ServerMaintenanceApprovalKey,
+ })🤖 Prompt for AI Agents
In `@internal/controller/servermaintenance_controller.go` around lines 156 - 171,
When cleaning up a ServerMaintenance object, ensure you remove the approval key
from both annotations and labels to avoid stale auto-approvals: in the cleanup
path that currently calls updateServerRef/patchMaintenanceState (use the
serverClaim and server variables and the existing updateServerRef method), check
for metalv1alpha1.ServerMaintenanceApprovalKey in serverClaim.GetAnnotations()
and serverClaim.GetLabels() and remove it from both maps, persist the change via
the same update/patch flow (e.g., call updateServerRef or client.Update on the
ServerClaim) so the approval label/annotation is cleared during maintenance
teardown.
There was a problem hiding this comment.
@defo89, thank you! Happy to help ensure the cleanup flow is complete. 🚀
🧠 Learnings used
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconciliation must be based on current cluster state, not assumptions; handle NotFound errors correctly and gracefully manage partial failures
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status must reflect observed state, not desired state; conditions must be updated consistently and deterministically
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status updates must be performed via `Status().Update()` or `Status().Patch()` and separated from spec mutations
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/servermaintenance_controller.go`:
- Around line 156-171: The approval check currently treats any presence of
metalv1alpha1.ServerMaintenanceApprovalKey on serverClaim as approval; change it
to verify the value equals the trueValue constant instead. Replace the
hasAnnotation/hasLabel boolean presence tests with value lookups from
annotations and labels (e.g., val, ok :=
annotations[metalv1alpha1.ServerMaintenanceApprovalKey]) and only consider
approved when val == trueValue (same for labels). Keep the existing behavior
that on approval you call r.updateServerRef(ctx, log, maintenance, server) and
r.patchMaintenanceState(..., metalv1alpha1.ServerMaintenanceStateInMaintenance);
otherwise log the waiting-for-approval message and return. Ensure you reference
serverClaim, annotations, labels, metalv1alpha1.ServerMaintenanceApprovalKey,
trueValue, updateServerRef, and patchMaintenanceState when making the change.
Comment on lines
156
to
+171
| annotations := serverClaim.GetAnnotations() | ||
| if _, ok := annotations[metalv1alpha1.ServerMaintenanceApprovalKey]; !ok { | ||
| log.V(1).Info("Server not approved for maintenance, waiting for approval", "Server", server.Name) | ||
| return ctrl.Result{}, nil | ||
| } | ||
| log.V(1).Info("Server approved for maintenance", "Server", server.Name) | ||
| if err = r.updateServerRef(ctx, log, maintenance, server); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| if modified, err := r.patchMaintenanceState(ctx, maintenance, metalv1alpha1.ServerMaintenanceStateInMaintenance); err != nil || modified { | ||
| return ctrl.Result{}, err | ||
| labels := serverClaim.GetLabels() | ||
| _, hasAnnotation := annotations[metalv1alpha1.ServerMaintenanceApprovalKey] | ||
| _, hasLabel := labels[metalv1alpha1.ServerMaintenanceApprovalKey] | ||
|
|
||
| if hasAnnotation || hasLabel { | ||
| log.V(1).Info("Server approved for maintenance", "Server", server.Name) | ||
| if err = r.updateServerRef(ctx, log, maintenance, server); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| if modified, err := r.patchMaintenanceState(ctx, maintenance, metalv1alpha1.ServerMaintenanceStateInMaintenance); err != nil || modified { | ||
| return ctrl.Result{}, err | ||
| } | ||
| } | ||
| log.V(1).Info("Server not approved for maintenance, waiting for approval", "Server", server.Name) | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=go -C3 'ServerMaintenanceApprovalKey'Repository: ironcore-dev/metal-operator
Length of output: 9599
🏁 Script executed:
rg -n --type=go -C3 'trueValue'Repository: ironcore-dev/metal-operator
Length of output: 8859
🏁 Script executed:
rg -n --type=go -C3 '"true".*metalv1alpha1\|metalv1alpha1.*"true"'Repository: ironcore-dev/metal-operator
Length of output: 53
Check approval value explicitly—presence alone is insufficient.
The current code approves maintenance if the key exists, regardless of its value. This means setting ServerMaintenanceApprovalKey to "false" or any other value would still trigger approval. However, tests throughout the codebase (servermaintenance_controller_test.go, biossettings_controller_test.go, etc.) explicitly set this key to the trueValue constant ("true"), indicating the intent is value-based approval.
Since trueValue is already defined in this file (line 30), update the approval check to explicitly verify the value:
🔧 Proposed fix
- _, hasAnnotation := annotations[metalv1alpha1.ServerMaintenanceApprovalKey]
- _, hasLabel := labels[metalv1alpha1.ServerMaintenanceApprovalKey]
-
- if hasAnnotation || hasLabel {
+ approved := annotations[metalv1alpha1.ServerMaintenanceApprovalKey] == trueValue ||
+ labels[metalv1alpha1.ServerMaintenanceApprovalKey] == trueValue
+ if approved {
log.V(1).Info("Server approved for maintenance", "Server", server.Name)
if err = r.updateServerRef(ctx, log, maintenance, server); err != nil {
return ctrl.Result{}, err
}
if modified, err := r.patchMaintenanceState(ctx, maintenance, metalv1alpha1.ServerMaintenanceStateInMaintenance); err != nil || modified {
return ctrl.Result{}, err
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| annotations := serverClaim.GetAnnotations() | |
| if _, ok := annotations[metalv1alpha1.ServerMaintenanceApprovalKey]; !ok { | |
| log.V(1).Info("Server not approved for maintenance, waiting for approval", "Server", server.Name) | |
| return ctrl.Result{}, nil | |
| } | |
| log.V(1).Info("Server approved for maintenance", "Server", server.Name) | |
| if err = r.updateServerRef(ctx, log, maintenance, server); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| if modified, err := r.patchMaintenanceState(ctx, maintenance, metalv1alpha1.ServerMaintenanceStateInMaintenance); err != nil || modified { | |
| return ctrl.Result{}, err | |
| labels := serverClaim.GetLabels() | |
| _, hasAnnotation := annotations[metalv1alpha1.ServerMaintenanceApprovalKey] | |
| _, hasLabel := labels[metalv1alpha1.ServerMaintenanceApprovalKey] | |
| if hasAnnotation || hasLabel { | |
| log.V(1).Info("Server approved for maintenance", "Server", server.Name) | |
| if err = r.updateServerRef(ctx, log, maintenance, server); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| if modified, err := r.patchMaintenanceState(ctx, maintenance, metalv1alpha1.ServerMaintenanceStateInMaintenance); err != nil || modified { | |
| return ctrl.Result{}, err | |
| } | |
| } | |
| log.V(1).Info("Server not approved for maintenance, waiting for approval", "Server", server.Name) | |
| return ctrl.Result{}, nil | |
| annotations := serverClaim.GetAnnotations() | |
| labels := serverClaim.GetLabels() | |
| approved := annotations[metalv1alpha1.ServerMaintenanceApprovalKey] == trueValue || | |
| labels[metalv1alpha1.ServerMaintenanceApprovalKey] == trueValue | |
| if approved { | |
| log.V(1).Info("Server approved for maintenance", "Server", server.Name) | |
| if err = r.updateServerRef(ctx, log, maintenance, server); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| if modified, err := r.patchMaintenanceState(ctx, maintenance, metalv1alpha1.ServerMaintenanceStateInMaintenance); err != nil || modified { | |
| return ctrl.Result{}, err | |
| } | |
| } | |
| log.V(1).Info("Server not approved for maintenance, waiting for approval", "Server", server.Name) | |
| return ctrl.Result{}, nil |
🤖 Prompt for AI Agents
In `@internal/controller/servermaintenance_controller.go` around lines 156 - 171,
The approval check currently treats any presence of
metalv1alpha1.ServerMaintenanceApprovalKey on serverClaim as approval; change it
to verify the value equals the trueValue constant instead. Replace the
hasAnnotation/hasLabel boolean presence tests with value lookups from
annotations and labels (e.g., val, ok :=
annotations[metalv1alpha1.ServerMaintenanceApprovalKey]) and only consider
approved when val == trueValue (same for labels). Keep the existing behavior
that on approval you call r.updateServerRef(ctx, log, maintenance, server) and
r.patchMaintenanceState(..., metalv1alpha1.ServerMaintenanceStateInMaintenance);
otherwise log the waiting-for-approval message and return. Ensure you reference
serverClaim, annotations, labels, metalv1alpha1.ServerMaintenanceApprovalKey,
trueValue, updateServerRef, and patchMaintenanceState when making the change.
Nuckal777
approved these changes
Jan 27, 2026
defo89
added a commit
to sapcc/helm-charts
that referenced
this pull request
Jan 27, 2026
defo89
added a commit
to sapcc/helm-charts
that referenced
this pull request
Jan 27, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
ServerMaintenanceApprovallabel on ServerClaimsSummary by CodeRabbit
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.