Conversation
…rnetes CRD validation fails Reconcilers now transition a resource to FAILED, with a clear condition message, when the Kubernetes API rejects a Create/Patch as invalid, instead of retrying forever. When the generated Kubernetes CRD spec fails admission validation (e.g. an out-of-range field), the error is permanent — retrying the reconcile will never succeed. Resources were left stuck in STARTING/PROGRESSING indefinitely with no useful signal to the user about what went wrong. - Detect Kubernetes "Invalid" API errors on Create/Patch across all hub-backed reconcilers (compute instances, clusters, bare metal instances, networking resources) and mark the resource FAILED with a ValidationFailed condition carrying the API error message - For compute instances specifically, distinguish permanent validation errors from transient Kubernetes API errors (e.g. connection issues) so transient failures keep retrying without prematurely marking the resource as failed Added unit tests covering: CRD validation failures on Create and on Patch resulting in a FAILED state with the expected condition, and transient (non-validation) Kubernetes errors continuing to be retried without changing state. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
Address unresolved CodeRabbit feedback from PR osac-project#903: - Skip the Update() call when updateMask has no paths, matching the existing "only send an update if there are actual changes" comment that previously had no guard - Log the swallowed updateErr at warn level when both reconcileErr and updateErr are non-nil, so operators see when status persistence fails after a reconciliation error Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Marc Sluiter <msluiter@redhat.com>
|
@slintes: This pull request references OSAC-2398 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: slintes The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughKubernetes Invalid errors now transition multiple reconciled resources to FAILED with validation messages instead of being retried. ComputeInstance additionally distinguishes transient errors, skips empty updates, and tests failure and recovery paths. ChangesValidation failure handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ 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}
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 `@internal/controllers/cluster/cluster_reconciler_function.go`:
- Around line 226-229: Update the Invalid-error handling in the reconciler’s
validation and Patch paths to avoid calling setFailed(err), which permanently
suppresses future reconciliation. Record the failed spec identity/generation
using the existing suppression mechanism, and skip retries only when the current
invalid payload exactly matches that recorded failure; allow reconciliation
after the user changes the spec.
- Around line 478-493: Update setFailed to call updateCondition with
CLUSTER_CONDITION_TYPE_FAILED instead of CLUSTER_CONDITION_TYPE_PROGRESSING,
while preserving the FAILED state transition and existing status, reason, and
error message.
In
`@internal/controllers/computeinstance/computeinstance_reconciler_function_test.go`:
- Around line 1996-2336: Add a recovery-path spec in the “Kubernetes validation
error handling” Describe block using the existing two-reconcile pattern from
“should create CR on second reconcile after hub is persisted.” Run f.run first
with an invalid instance type and verify FAILED, then update the same
computeInstance to a valid specification and reconcile again against a client
that succeeds. Assert the instance leaves FAILED and reaches the expected
successful state, preserving the existing Invalid-error coverage.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 750f6268-a6e7-4037-a2c5-70d42491a513
📒 Files selected for processing (11)
internal/controllers/baremetalinstance/baremetalinstance_reconciler_function.gointernal/controllers/cluster/cluster_reconciler_function.gointernal/controllers/computeinstance/computeinstance_reconciler_function.gointernal/controllers/computeinstance/computeinstance_reconciler_function_test.gointernal/controllers/externalip/external_ip_reconciler_function.gointernal/controllers/externalipattachment/external_ip_attachment_reconciler_function.gointernal/controllers/externalippool/external_ip_pool_reconciler_function.gointernal/controllers/natgateway/nat_gateway_reconciler_function.gointernal/controllers/securitygroup/securitygroup_reconciler_function.gointernal/controllers/subnet/subnet_reconciler_function.gointernal/controllers/virtualnetwork/virtual_network_reconciler_function.go
| if apierrors.IsInvalid(err) { | ||
| t.setFailed(err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'cluster_reconciler_function.go' . || true
echo "== file stats =="
file="$(fd 'cluster_reconciler_function.go' . | head -n 1)"
wc -l "$file"
echo "== relevant sections =="
sed -n '1,280p' "$file" | nl -ba
echo "== setFailed and state transitions =="
sed -n '440,510p' "$file" | nl -ba
echo "== occurrences of setFailed/state constants near file =="
rg -n "setFailed|CLUSTER_STATE_FAILED|PROGRESSING|READY|state\\s*!=" "$file"Repository: osac-project/fulfillment-service
Length of output: 415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./internal/controllers/cluster/cluster_reconciler_function.go"
echo "== relevant sections =="
awk 'NR>=1 && NR<=280 {printf "%7d %s\n", NR, $0}' "$file"
echo "== setFailed and state transitions =="
awk 'NR>=440 && NR<=510 {printf "%7d %s\n", NR, $0}' "$file"
echo "== occurrences of setFailed/state constants near file =="
grep -nE "setFailed|CLUSTER_STATE_FAILED|PROGRESSING|READY|state !=|if state|SetClusterState|ClusterStatus|ValidationFailed|Failed" "$file" || trueRepository: osac-project/fulfillment-service
Length of output: 13966
Do not treat Invalid errors as terminal FAILED states.
setFailed() sets state to FAILED, but update() returns immediately for any state that is not PROGRESSING or READY before syncing the spec. If a Kubernetes validation failure sends the cluster into FAILED, future re-validations are skipped even after the user fixes the spec. Store the failed spec identity/generation for safe suppression instead, and only block retries that have exactly the same invalid payload. This also applies to the Patch path at lines 243-246.
🤖 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/controllers/cluster/cluster_reconciler_function.go` around lines 226
- 229, Update the Invalid-error handling in the reconciler’s validation and
Patch paths to avoid calling setFailed(err), which permanently suppresses future
reconciliation. Record the failed spec identity/generation using the existing
suppression mechanism, and skip retries only when the current invalid payload
exactly matches that recorded failure; allow reconciliation after the user
changes the spec.
Source: Learnings
There was a problem hiding this comment.
[AI-Assisted] Valid concern. The state guard at lines 167-171 is pre-existing code — our PR adds a new path INTO that terminal state via setFailed(), but the guard itself predates this change.
Fixing recovery properly requires either removing the state guard (which was intentional — comment on line 164 says "Do nothing if the cluster is in a terminal failure state") or adding spec-generation tracking to skip retries only for the same invalid payload. Both are design changes beyond this bugfix scope.
Note that only the cluster reconciler has this issue — all other reconcilers have no state guard and naturally recover when the spec is corrected (verified by new recovery-path test in c725398).
Our PR still improves cluster behavior: previously IsInvalid errors retried silently forever with no error message. Now the user at least sees FAILED state with ValidationFailed reason and the specific validation error.
There was a problem hiding this comment.
@slintes, agreed. The current change introduces a new terminal path into the pre-existing state guard, so recovery after a corrected invalid spec remains a valid—but separately scoped—design issue.
Keeping the user-visible FAILED status and ValidationFailed condition is still an improvement over silent retries. I’ll leave this finding unresolved for follow-up rather than blocking #973. Would you like me to open a GitHub issue for generation/fingerprint-based retry suppression in internal/controllers/cluster/cluster_reconciler_function.go?
🐇💭
✏️ Learnings added
Learnt from: slintes
Repo: osac-project/fulfillment-service PR: 973
File: internal/controllers/cluster/cluster_reconciler_function.go:226-229
Timestamp: 2026-07-28T11:45:01.601Z
Learning: In `internal/controllers/cluster/cluster_reconciler_function.go`, the intentional terminal-state guard skips reconciliation unless a Cluster is in PROGRESSING or READY. PR `#973` introduces FAILED state on Kubernetes Invalid errors, so corrected Cluster specs cannot currently recover; address this cluster-specific behavior in a follow-up through spec generation or payload-fingerprint-aware failure suppression rather than assuming it behaves like the other reconcilers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ygalblum
Repo: osac-project/fulfillment-service PR: 957
File: internal/controllers/externalip/external_ip_reconciler_function.go:411-420
Timestamp: 2026-07-25T01:22:32.127Z
Learning: In Go hub-resource reconciler implementations, do not treat a resource status of FAILED after a Kubernetes Invalid error as a terminal state that suppresses future reconciliations. If the Kubernetes API rejected the resource due to an Invalid validation error, the resource spec may have changed and must be retried. Implement “safe suppression” only by comparing the currently observed failed spec identity (e.g., desired-spec generation, revision, or a computed fingerprint) against the identity of the version that failed validation; only suppress if they match exactly, otherwise allow reconciliation to proceed.
…lure Verify that compute instance reconciler retries and creates the K8s CR after a previous IsInvalid failure, proving no state guard blocks recovery when the spec is corrected. Addresses CodeRabbit review feedback on PR osac-project#973. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Marc Sluiter <msluiter@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@internal/controllers/computeinstance/computeinstance_reconciler_function_test.go`:
- Around line 2337-2432: Extract the repeated test fixture setup from the four
specs in this Describe into shared BeforeEach initialization and small helpers
such as newFakeClient(interceptor.Funcs) and newComputeInstance. Centralize
scheme registration, hubCache, hubsClient, computeInstancesClient, function
construction, and the common compute instance fields; leave each spec
responsible only for its interceptor behavior and instance-type core variation.
- Around line 2434-2451: The recovery assertions after the second f.run call are
incomplete. In the recovery test, assert that
computeInstance.GetStatus().GetState() is no longer
COMPUTE_INSTANCE_STATE_FAILED and that CONFIGURATION_APPLIED is no longer
ValidationFailed, while preserving the existing CR creation checks.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f3cefe7-6569-4d59-bb32-9bfb041ed6de
📒 Files selected for processing (1)
internal/controllers/computeinstance/computeinstance_reconciler_function_test.go
…specs Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Marc Sluiter <msluiter@redhat.com>
|
[AI-Assisted] This PR has been migrated to the mono repo: osac-project/osac#115. Closing in favor of that PR. |
This branch was previously deployed
4 participants
Summary
IsInvalid) onCreate/Patchacross all hub-backed reconcilers, marking resources as FAILED with aValidationFailedcondition instead of retrying indefinitelyUpdate()call whenupdateMaskhas no paths (implements the existing "only send an update if there are actual changes" comment)updateErrin compute instance reconciler — when bothreconcileErrandupdateErrare non-nil, log the droppedupdateErrat warn level so operators see when status persistence failsContext
Replaces #957, #917, and #856. Original work by @ygalblum, rebased onto current main and extended with improvements from unresolved CodeRabbit feedback on #903.
Jira
OSAC-2398
Test plan
ginkgo run -r internal/controllers— all 18 suites pass)gofmtandgo buildcleanIsInvalidtests from original PR cover all reconcilersAssisted-by: Claude Code noreply@anthropic.com
🤖 Generated with Claude Code
Summary by CodeRabbit