[AI] OSAC-2126: Scale accepted but NodePool unchanged when ClusterOrder phase is Failed - #151
Conversation
…ase is Failed Co-authored-by: osac-dev-bot <osac-automation@redhat.com>
|
@osac-jira-ai-issue-solver[bot]: This pull request references OSAC-2126 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. |
|
AI Session Costs
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughCluster updates now validate the existing cluster state before applying ChangesCluster update validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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: 1
🤖 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 `@fulfillment-service/internal/servers/private_clusters_server.go`:
- Around line 235-238: Move the FAILED/DELETE_FAILED terminal-state guard from
the pre-write validateClusterStateForSpecUpdate check into the atomic
spec-update condition used by GenericServer.Update or its DAO layer. Ensure the
masked spec fields are written only when the existing cluster state is
non-terminal, preventing concurrent status transitions from allowing a spec
write; do not rely on the client-supplied lock.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 76dd576f-ace2-4aa2-91b8-cfe455b23774
📒 Files selected for processing (2)
fulfillment-service/internal/servers/private_clusters_server.gofulfillment-service/internal/servers/private_clusters_server_test.go
| err = s.validateClusterStateForSpecUpdate(ctx, request) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect GenericServer update behavior and DAO write conditions.
rg -n -C 12 --type go 'func \(.*\*GenericServer.*\) Update\(' .
rg -n -C 8 --type go 'SetLock\(|SetVersion\(|Transaction|transaction|Compare|Condition|WHERE' .
# Confirm whether a cluster update can enforce:
# status.state NOT IN (FAILED, DELETE_FAILED) in the same operation as the spec write.Repository: osac-project/osac
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow inspection to the relevant private_clusters_server.go file and nearby functions.
wc -l fulfillment-service/internal/servers/private_clusters_server.go
sed -n '200,260p' fulfillment-service/internal/servers/private_clusters_server.go
sed -n '430,490p' fulfillment-service/internal/servers/private_clusters_server.go
# Locate the referenced helper and GenericServer Update implementation files by name/path.
rg -n --type go 'func \(.*\*PrivateClusters.*\) .*Update|func \(.*\*GenericServer.*\) Update\(|validateClusterStateForSpecUpdate|DELETE_FAILED|ClusterState' internal/servers fulfillment-service/internal -g '*.go' | head -n 200Repository: osac-project/osac
Length of output: 30677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generic DAO and Update call path.
sed -n '600,660p' fulfillment-service/internal/servers/generic_server.go
rg -n --type go 'func \(.*Generic.*\) Update|SetId\(id\)\.Do|SetLock|SetVersion|Condition|Update\(|Tx|Transaction|Version' fulfillment-service/internal/servers/generic_*\*.go -g '*.go' -C 5
# Inspect all ClusterState terminal-state guards around spec updates in the repository.
rg -n --type go 'ClusterState_CLUSTER_STATE_FAILED|ClusterState_CLUSTER_STATE_DELETE_FAILED|validateClusterStateForSpecUpdate' fulfillment-service/internal -g '*.go' -C 4
# Inspect cluster reconciler terminal-state behavior.
sed -n '160,225p' fulfillment-service/internal/controllers/cluster/cluster_reconciler_function.goRepository: osac-project/osac
Length of output: 1965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Continue inspecting the generic update call path after locked GET.
sed -n '660,740p' fulfillment-service/internal/servers/generic_server.go
# Find the exact generic server files and inspect DAO call sites.
git ls-files 'fulfillment-service/internal/servers/*generic*.go' 'fulfillment-service/internal/*dao*.go' 'fulfillment-service/internal/**/dao*.go'
rg -n --type go 'SetLock\(true\)|SetVersion|Condition|tx\.Update|Update\(.*Version|SetId\(.*\)\.Do' fulfillment-service/internal/servers fulfillment-service/internal -g '*.go' -C 3 --max-count 200
# Inspect the full spec-state guard with tests.
sed -n '1020,1180p' fulfillment-service/internal/servers/private_clusters_server_test.goRepository: osac-project/osac
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get exact generic server files and read the relevant Update method lines.
git ls-files 'fulfillment-service/internal/servers/*generic*server.go' 'fulfillment-service/internal/servers/generic*.go'
FILE=$(git ls-files 'fulfillment-service/internal/servers/generic_server.go' | head -n 1)
sed -n '607,730p' "$FILE"
# Search only server and core files for the same methods using path prefixes.
rg -n --type go 'func \(.*GenericServer|SetLock\(true\)|SetVersion|Condition|Get\(\)\.SetId|Update\(\)\.SetObject|Update\(\)\.Condition' fulfillment-service/internal/servers fulfillment-service/internal/internal fulfillment-service/internal/databases fulfillment-service/internal/common --glob '*.go' -C 4 --max-count 300
# Read cluster state terminal-state tests around the new guard.
sed -n '1020,1140p' fulfillment-service/internal/servers/private_clusters_server_test.goRepository: osac-project/osac
Length of output: 50373
Make the terminal-state predicate cover the spec write.
validateClusterStateForSpecUpdate checks the cluster state in a pre-write lookup before GenericServer.Update. GenericServer.Update then rereads the object and applies the masked spec fields, so a concurrent status-only update that transitions the cluster to FAILED or DELETE_FAILED after the pre-write check can still persist spec changes. Move this predicate into the same condition used for the spec update, for example through a conditional DAO update on the existing state, instead of relying on a client-supplied lock.
🤖 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 `@fulfillment-service/internal/servers/private_clusters_server.go` around lines
235 - 238, Move the FAILED/DELETE_FAILED terminal-state guard from the pre-write
validateClusterStateForSpecUpdate check into the atomic spec-update condition
used by GenericServer.Update or its DAO layer. Ensure the masked spec fields are
written only when the existing cluster state is non-terminal, preventing
concurrent status transitions from allowing a spec write; do not rely on the
client-supplied lock.
There was a problem hiding this comment.
Valid concern. Changed validateClusterStateForSpecUpdate to acquire a FOR UPDATE row lock (SetLock(true)) when reading the cluster state. Since the gRPC interceptor chain wraps the entire handler in a single database transaction, this lock is held when GenericServer.Update later performs its own locked read and write — preventing a concurrent status transition from bypassing the terminal-state check. The DAO's UpdateRequest does not support conditional WHERE clauses, so a conditional DAO update was not feasible; the row-level lock within the same transaction achieves the same atomicity guarantee.
Addressed in 1d776d2.
|
Consider switching from a blocklist (FAILED, DELETE_FAILED) to an allowlist that mirrors the reconciler guard exactly. The reconciler at An allowlist approach keeps the API and reconciler in sync and is future-proof if new states are added: state := existingCluster.GetStatus().GetState()
if state != privatev1.ClusterState_CLUSTER_STATE_PROGRESSING &&
state != privatev1.ClusterState_CLUSTER_STATE_READY {
return grpcstatus.Errorf(
grpccodes.InvalidArgument,
"cannot update cluster spec when cluster state is %s",
state,
)
} |
Co-authored-by: osac-dev-bot <osac-automation@redhat.com>
Meta-comment (CodeRabbit review trigger) — no action needed. Addressed in 1d776d2. |
|
In comment, @coderabbitai[bot] said:
CodeRabbit bot auto-response — no action needed. Addressed in 1d776d2. |
|
In comment, @coderabbitai[bot] said:
CodeRabbit bot walkthrough/summary — no action needed. Addressed in 1d776d2. |
|
In comment, @coderabbitai[bot] said:
CodeRabbit bot review summary — no action needed. Addressed in 1d776d2. |
Co-authored-by: osac-dev-bot <osac-automation@redhat.com>
Switched from blocklist (FAILED, DELETE_FAILED) to allowlist (PROGRESSING, READY) matching the reconciler guard at cluster_reconciler_function.go:175-178. Added test cases for DELETING and UNSPECIFIED states being rejected. The previous concern about UNSPECIFIED breaking creation flow was unfounded — validateClusterStateForSpecUpdate is only called in Update(), not Create(). Addressed in bd70dab. |
Co-authored-by: osac-dev-bot <osac-automation@redhat.com>
|
/lgtm |
|
/retest |
|
Re-triggered failed runs:
|
Co-authored-by: osac-dev-bot <osac-automation@redhat.com>
|
CI failures addressed in cf0fba6. |
|
/lgtm |
5 participants
Resolves OSAC-2126
Summary
FAILEDorDELETE_FAILED, returningInvalidArgumentinstead of silently accepting changes that the reconciler will never applyFixes OSAC-2126
Test plan
ginkgo run -r internalpasses (servers suite requires PostgreSQL container)osac edit clusters <id>to increase node_sets size — should return InvalidArgument errorSummary by CodeRabbit
Bug Fixes
Tests