Add Taints and Tolerations for Server and ServerClaim#878
Merged
Conversation
github-actions
Bot
added
api-change
documentation
xkonni
force-pushed
the
feat/taints
branch
2 times, most recently
from
f97911f to
ee9e961
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Kubernetes-style taints to Server and tolerations to ServerClaim, with API types, CRD schema and docs, generated apply/deepcopy helpers, matching utilities, and controller checks plus tests enforcing tolerations during claim binding. ChangesServer Taint/Toleration Scheduling
Sequence Diagram(s)sequenceDiagram
participant ServerClaimController
participant isServerClaimable
participant tolerates
participant ServerTaints
participant ClaimTolerations
ServerClaimController->>isServerClaimable: check if server claimable
isServerClaimable->>tolerates: tolerates(server.Spec.Taints, claim.Spec.Tolerations)
tolerates->>ServerTaints: filter NoBind taints (skip Evict)
tolerates->>ClaimTolerations: verify each NoBind taint matches a toleration
ClaimTolerations-->>tolerates: true if all NoBind taints covered, false otherwise
alt Tolerations match taints
tolerates-->>isServerClaimable: true
isServerClaimable-->>ServerClaimController: server is claimable
else Tolerations do not match
tolerates-->>isServerClaimable: false
isServerClaimable->>isServerClaimable: log rejection, return false
isServerClaimable-->>ServerClaimController: server not claimable
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 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 `@docs/api-reference/api.md`:
- Around line 1856-1857: Update the documentation for the taint effects so that
TaintEffectEvict is described as reserved/non-functional rather than actively
evicting claims: replace the current sentence that says "TaintEffectEvict causes
existing ServerClaims bound to the server to be evicted..." with wording that
explicitly states TaintEffectEvict is reserved/placeholder and does not
currently trigger eviction, and keep TaintEffectNoBind description as-is;
reference the symbols TaintEffectEvict and TaintEffectNoBind when making the
change so the intent is clear to readers.
In `@internal/controller/serverclaim_controller_test.go`:
- Around line 530-559: The test "should claim a server with a NoBind taint when
the claim has a matching toleration" creates a ServerClaim (variable claim) but
never deletes it; add cleanup after the assertion that the claim is bound by
calling the k8s client to delete the ServerClaim (e.g.,
Expect(k8sClient.Delete(ctx, claim)).To(Succeed())) and optionally wait for
deletion with an Eventually/Object(check) to ensure the claim is removed; place
this cleanup immediately after the
Eventually(Object(claim)).Should(HaveField("Status.Phase",
Equal(metalv1alpha1.PhaseBound))) line so the created claim is removed at the
end of the test.
- Around line 518-523: The negative assertions using
Consistently(Object(server)) and Consistently(Object(claim)) need explicit
duration and polling interval to avoid flakiness—update those calls to include a
timeout and polling interval (e.g., Consistently(..., 2*time.Second,
100*time.Millisecond)) so the controller has time to reconcile; for the positive
test that creates a ServerClaim (the block that asserts Status.Phase becomes
PhaseBound) add explicit cleanup to mirror the negative test by deleting the
created ServerClaim at the end of that test (ensure you reference the same claim
variable used when creating it) so resources are removed after the test
completes.
🪄 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: 21978aab-929d-44f9-b01e-acd8592d9606
⛔ Files ignored due to path filters (2)
dist/chart/templates/crd/metal.ironcore.dev_serverclaims.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_servers.yamlis excluded by!**/dist/**
📒 Files selected for processing (13)
api/v1alpha1/server_types.goapi/v1alpha1/serverclaim_types.goapi/v1alpha1/taint_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/metal.ironcore.dev_serverclaims.yamlconfig/crd/bases/metal.ironcore.dev_servers.yamldocs/api-reference/api.mdinternal/controller/serverclaim_controller.gointernal/controller/serverclaim_controller_test.gointernal/controller/taint.gointernal/controller/taint_test.gothird_party/expansion/expand.gothird_party/expansion/expand_test.go
afritzler
requested changes
May 12, 2026
Member
|
@coderabbitai do you think it is a good idea to reuse the k8s.io |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Arguments for reusing
Arguments against (and why custom types are the better fit here):
Verdict: The custom types defined in this PR ( |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/v1alpha1/common_types.go (1)
348-351: ⚡ Quick winAdd
// +kubebuilder:default=Equalto clarifyOperatorfield behavior.The
Operatorfield is optional but currently relies on controller-side fallback (empty operator treated asEqual). Adding the default marker makes this behavior explicit in the API schema, matching Kubernetes conventions and improving clarity for API consumers.Suggested change
// Operator represents the key's relationship to the value. + // +kubebuilder:default=Equal // +optional Operator TolerationOperator `json:"operator,omitempty"`🤖 Prompt for 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. In `@api/v1alpha1/common_types.go` around lines 348 - 351, Add the kubebuilder default marker to the Operator field so the API schema documents the default behavior: add the comment "// +kubebuilder:default=Equal" immediately above the Operator declaration for the Operator TolerationOperator `json:"operator,omitempty"` field in the struct (where Operator is defined) so codegen will set the schema default to Equal.
🤖 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.
Nitpick comments:
In `@api/v1alpha1/common_types.go`:
- Around line 348-351: Add the kubebuilder default marker to the Operator field
so the API schema documents the default behavior: add the comment "//
+kubebuilder:default=Equal" immediately above the Operator declaration for the
Operator TolerationOperator `json:"operator,omitempty"` field in the struct
(where Operator is defined) so codegen will set the schema default to Equal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e343064-8865-4e50-84ce-e383ee370ef5
⛔ Files ignored due to path filters (2)
dist/chart/templates/crd/metal.ironcore.dev_serverclaims.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_servers.yamlis excluded by!**/dist/**
📒 Files selected for processing (16)
api/v1alpha1/applyconfiguration/api/v1alpha1/serverclaimspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/taint.goapi/v1alpha1/applyconfiguration/api/v1alpha1/toleration.goapi/v1alpha1/applyconfiguration/utils.goapi/v1alpha1/common_types.goapi/v1alpha1/server_types.goapi/v1alpha1/serverclaim_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/metal.ironcore.dev_serverclaims.yamlconfig/crd/bases/metal.ironcore.dev_servers.yamldocs/api-reference/api.mdinternal/controller/serverclaim_controller.gointernal/controller/serverclaim_controller_test.gointernal/controller/taint.gointernal/controller/taint_test.go
✅ Files skipped from review due to trivial changes (5)
- api/v1alpha1/applyconfiguration/api/v1alpha1/serverclaimspec.go
- api/v1alpha1/applyconfiguration/utils.go
- api/v1alpha1/applyconfiguration/api/v1alpha1/taint.go
- docs/api-reference/api.md
- api/v1alpha1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/controller/serverclaim_controller.go
- config/crd/bases/metal.ironcore.dev_servers.yaml
- api/v1alpha1/serverclaim_types.go
- config/crd/bases/metal.ironcore.dev_serverclaims.yaml
- api/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.go
- internal/controller/taint_test.go
- internal/controller/taint.go
- api/v1alpha1/server_types.go
stefanhipfel
approved these changes
May 12, 2026
afritzler
changed the title
Introduces Taint and Toleration types modelled after the Kubernetes taint/toleration API. TaintEffect supports NoBind (blocks binding, default) and Evict (reserved for future eviction logic). Adds Taints []Taint to ServerSpec and Tolerations []Toleration to ServerClaimSpec. Closes #850 Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com>
Adds tolerates() helper that checks whether a claim's tolerations cover all NoBind taints on a server. Evict taints are skipped (future work). Wires the check into isServerClaimable() so tainted servers are not bound to claims without matching tolerations. Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com>
... and add generated files Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com>
Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/taints_test.go (1)
12-119: ⚡ Quick winConsider adding a test case for empty
Taint.Effect.The test suite is comprehensive, but there is no test case covering a taint with an empty
Effectfield. If taints with emptyEffectare possible (and should default toNoBind), adding a test case would verify the expected behavior and catch any regression.Suggested test entry:
Entry("taint with empty effect treated as NoBind", []metalv1alpha1.Taint{ {Key: "dedicated", Value: "gpu", Effect: ""}, }, []metalv1alpha1.Toleration{ {Key: "dedicated", Operator: metalv1alpha1.TolerationOperatorExists}, }, true, ),However, if the API layer guarantees that
Taint.Effectis always populated (via defaulting or validation), then this test case is unnecessary.🤖 Prompt for 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. In `@internal/controller/taints_test.go` around lines 12 - 119, Add a test Entry to the DescribeTable in internal/controller/taints_test.go to cover a taint with an empty Effect and assert it is treated like TaintEffectNoBind; specifically add an Entry that calls tolerates with a taint {Key: "dedicated", Value: "gpu", Effect: ""} and a toleration {Key: "dedicated", Operator: metalv1alpha1.TolerationOperatorExists} expecting true so the tolerates(taints, tolerations) behavior for empty Taint.Effect is verified; place it alongside the other Entry cases in the same DescribeTable that uses the tolerates function and metalv1alpha1.Taint/metalv1alpha1.Toleration types.
🤖 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.
Nitpick comments:
In `@internal/controller/taints_test.go`:
- Around line 12-119: Add a test Entry to the DescribeTable in
internal/controller/taints_test.go to cover a taint with an empty Effect and
assert it is treated like TaintEffectNoBind; specifically add an Entry that
calls tolerates with a taint {Key: "dedicated", Value: "gpu", Effect: ""} and a
toleration {Key: "dedicated", Operator: metalv1alpha1.TolerationOperatorExists}
expecting true so the tolerates(taints, tolerations) behavior for empty
Taint.Effect is verified; place it alongside the other Entry cases in the same
DescribeTable that uses the tolerates function and
metalv1alpha1.Taint/metalv1alpha1.Toleration types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f6ef323-1db4-48e9-9427-fbf66314c298
⛔ Files ignored due to path filters (2)
dist/chart/templates/crd/metal.ironcore.dev_serverclaims.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_servers.yamlis excluded by!**/dist/**
📒 Files selected for processing (16)
api/v1alpha1/applyconfiguration/api/v1alpha1/serverclaimspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.goapi/v1alpha1/applyconfiguration/api/v1alpha1/taint.goapi/v1alpha1/applyconfiguration/api/v1alpha1/toleration.goapi/v1alpha1/applyconfiguration/utils.goapi/v1alpha1/common_types.goapi/v1alpha1/server_types.goapi/v1alpha1/serverclaim_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/metal.ironcore.dev_serverclaims.yamlconfig/crd/bases/metal.ironcore.dev_servers.yamldocs/api-reference/api.mdinternal/controller/serverclaim_controller.gointernal/controller/serverclaim_controller_test.gointernal/controller/taints.gointernal/controller/taints_test.go
✅ Files skipped from review due to trivial changes (6)
- api/v1alpha1/applyconfiguration/api/v1alpha1/taint.go
- api/v1alpha1/applyconfiguration/utils.go
- api/v1alpha1/applyconfiguration/api/v1alpha1/serverclaimspec.go
- api/v1alpha1/applyconfiguration/api/v1alpha1/serverspec.go
- api/v1alpha1/zz_generated.deepcopy.go
- api/v1alpha1/applyconfiguration/api/v1alpha1/toleration.go
🚧 Files skipped from review as they are similar to previous changes (8)
- config/crd/bases/metal.ironcore.dev_serverclaims.yaml
- internal/controller/serverclaim_controller.go
- api/v1alpha1/serverclaim_types.go
- api/v1alpha1/common_types.go
- config/crd/bases/metal.ironcore.dev_servers.yaml
- api/v1alpha1/server_types.go
- docs/api-reference/api.md
- internal/controller/serverclaim_controller_test.go
Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com>
Member
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
afritzler
approved these changes
May 13, 2026
nagadeesh-nagaraja
pushed a commit
that referenced
this pull request
May 13, 2026
* feat: add Taints and Tolerations API types for Server and ServerClaim Introduces Taint and Toleration types modelled after the Kubernetes taint/toleration API. TaintEffect supports NoBind (blocks binding, default) and Evict (reserved for future eviction logic). Adds Taints []Taint to ServerSpec and Tolerations []Toleration to ServerClaimSpec. Closes #850 Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com> * feat: enforce taint/toleration check during server claim binding Adds tolerates() helper that checks whether a claim's tolerations cover all NoBind taints on a server. Evict taints are skipped (future work). Wires the check into isServerClaimable() so tainted servers are not bound to claims without matching tolerations. Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com> * address coderabbit review ... and add generated files Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com> * address review Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com> * address review, again Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com> --------- Signed-off-by: Konstantin Koslowski <konstantin.koslowski@sap.com> Signed-off-by: Nagadeesh Nagaraja <nagdeesh@gmail.com>
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.
Closes #850
Summary
Introduces a taint/toleration mechanism for bare-metal server scheduling,
modelled after the Kubernetes taint/toleration API.
TaintandTolerationtypes toapi/v1alpha1TaintEffectsupportsNoBind(default, blocks binding) andEvict(reserved for future eviction logic)
Server.spec.taints— set by operators or external systems to restrictwhich claims can bind to a server
ServerClaim.spec.tolerations— set on a claim to opt in to tainted serversisServerClaimable()now checks that allNoBindtaints on a server arecovered by a matching toleration on the claim
Behavior
A server with a
NoBindtaint will not be bound to a claim unless the claimcarries a matching toleration. The server remains
Available— taints onlyaffect binding, not server lifecycle.
Evicttaints are defined in the API but not yet enforced. They are ignoredduring binding and reserved for a future "drain and evict" flow.
Maintenance operations are not affected by taints.
Example
Summary by CodeRabbit
New Features
CRD / API
Docs
Tests