OCPBUGS-81503: Use merge patches instead of full updates for BMH and agent#10092
OCPBUGS-81503: Use merge patches instead of full updates for BMH and agent#10092alegacy wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@alegacy: This pull request references Jira Issue OCPBUGS-81503, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci
Bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alegacy 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe controller now persists partial changes using RFC6902-style merge patches instead of full resource updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
internal/controller/controllers/bmh_agent_controller_test.go (1)
1200-1208: Add one regression that proves field-preserving behavior.These assertions only prove that the write path changed. They do not verify the bug this PR fixes: preserving live-object fields that this binary does not know about. One regression that seeds an extra field on the stored object and verifies reconcile leaves it intact would lock that in.
Also applies to: 3426-3436, 3706-3707
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/controllers/bmh_agent_controller_test.go` around lines 1200 - 1208, The tests set up Patch expectations but don't assert that reconcile preserves unknown/live fields; add a regression in bmh_agent_controller_test.go that seeds an extra (non-modeled) field on the stored resource (e.g., add a custom JSON field in the BareMetalHost or Agent object body/annotations) before invoking the reconcile path used in the test, run the existing reconcile logic (the same test flow that triggers mockClient.EXPECT().Patch(...).DoAndReturn(...) for BareMetalHost and Agent) and assert after reconcile that the extra field remains unchanged; reference the existing mockClient.EXPECT().Patch handlers and the BareMetalHost/Agent objects used in those expectations to find where to seed the extra field and where to add the post-reconcile assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/controllers/bmh_agent_controller.go`:
- Around line 221-225: The current patch call in handleReconcileResult uses
r.Client.Patch(ctx, obj, client.MergeFrom(patchBase)) which drops optimistic
locking; instead enforce resourceVersion-based optimistic concurrency by
updating the full object: set
obj.SetResourceVersion(patchBase.GetResourceVersion()) and call
r.Client.Update(ctx, obj) (handle conflicts as needed). Locate
handleReconcileResult and replace the Patch call using
client.MergeFrom(patchBase) with the Update approach so the original
resourceVersion from patchBase is preserved and Update will surface conflicts.
---
Nitpick comments:
In `@internal/controller/controllers/bmh_agent_controller_test.go`:
- Around line 1200-1208: The tests set up Patch expectations but don't assert
that reconcile preserves unknown/live fields; add a regression in
bmh_agent_controller_test.go that seeds an extra (non-modeled) field on the
stored resource (e.g., add a custom JSON field in the BareMetalHost or Agent
object body/annotations) before invoking the reconcile path used in the test,
run the existing reconcile logic (the same test flow that triggers
mockClient.EXPECT().Patch(...).DoAndReturn(...) for BareMetalHost and Agent) and
assert after reconcile that the extra field remains unchanged; reference the
existing mockClient.EXPECT().Patch handlers and the BareMetalHost/Agent objects
used in those expectations to find where to seed the extra field and where to
add the post-reconcile assertion.
🪄 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: f36c44ce-d10c-40ad-a5bb-c599876d6e67
📒 Files selected for processing (2)
internal/controller/controllers/bmh_agent_controller.gointernal/controller/controllers/bmh_agent_controller_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10092 +/- ##
=======================================
Coverage 44.27% 44.28%
=======================================
Files 416 416
Lines 72549 72558 +9
=======================================
+ Hits 32123 32132 +9
Misses 37521 37521
Partials 2905 2905
🚀 New features to boost your workflow:
|
|
Looks fine to me w/ coderabbit's suggestion 👍 |
alegacy
force-pushed
the
fix/use-patch-instead-of-update
branch
from
7d52c0b to
bbba150
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/controllers/bmh_agent_controller.go (1)
277-350: Consider a helper for the patch-base lifecycle.The repeated
DeepCopy()+handleReconcileResult(...)sequence is easy to miss the next time a mutating reconcile step is added. Wrapping “snapshot → mutate → apply” in one helper would make stale-base mistakes harder.As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/controllers/bmh_agent_controller.go` around lines 277 - 350, The code repeatedly takes snapshots (bmhPatchBase/agentPatchBase = obj.DeepCopy()), runs a mutating step (e.g. r.reconcileBMH, r.reconcileAgentSpec, r.reconcileAgentInventory, r.ensureMCSCert, r.reconcileDay2SpokeBMH) and then calls r.handleReconcileResult, which is error-prone; introduce a helper (e.g. applyWithPatchBase) that accepts the target runtime.Object (BMH/Agent), a mutation callback (func() ReconcileResult or similar) and the logger/context, and inside the helper do obj.DeepCopy(), invoke the mutation, then call r.handleReconcileResult with the original and snapshot; replace each manual DeepCopy + call sequence (references: bmhPatchBase, agentPatchBase, DeepCopy(), handleReconcileResult, reconcileBMH, reconcileAgentSpec, reconcileAgentInventory, ensureMCSCert, reconcileDay2SpokeBMH) with calls to this helper to centralize the snapshot→mutate→apply pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/controllers/bmh_agent_controller.go`:
- Around line 277-350: The code repeatedly takes snapshots
(bmhPatchBase/agentPatchBase = obj.DeepCopy()), runs a mutating step (e.g.
r.reconcileBMH, r.reconcileAgentSpec, r.reconcileAgentInventory,
r.ensureMCSCert, r.reconcileDay2SpokeBMH) and then calls
r.handleReconcileResult, which is error-prone; introduce a helper (e.g.
applyWithPatchBase) that accepts the target runtime.Object (BMH/Agent), a
mutation callback (func() ReconcileResult or similar) and the logger/context,
and inside the helper do obj.DeepCopy(), invoke the mutation, then call
r.handleReconcileResult with the original and snapshot; replace each manual
DeepCopy + call sequence (references: bmhPatchBase, agentPatchBase, DeepCopy(),
handleReconcileResult, reconcileBMH, reconcileAgentSpec,
reconcileAgentInventory, ensureMCSCert, reconcileDay2SpokeBMH) with calls to
this helper to centralize the snapshot→mutate→apply pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fcda195-bd43-465e-80ed-a9ceedc036d1
📒 Files selected for processing (2)
internal/controller/controllers/bmh_agent_controller.gointernal/controller/controllers/bmh_agent_controller_test.go
…Agent The BMAC controller used client.Update() to write BMH and Agent objects back to the API server. This sends the entire object, which risks overwriting fields set by other controllers (e.g., siteconfig) — if assisted-service is compiled against an older version of the BMH CRD that lacks newer fields. Switch handleReconcileResult to use client.Patch with client.MergeFrom, which sends only the delta between the pre-mutation snapshot and the current state. This ensures fields unknown to assisted-service's Go structs are left untouched on the server. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Allain Legacy <alegacy@redhat.com>
alegacy
force-pushed
the
fix/use-patch-instead-of-update
branch
from
bbba150 to
f4d23a5
Compare
|
Firstly, thank you @alegacy for this contribution! We were thinking to implement patch as deferred method in the reconcile loop, and I think it would make perfect sense to take advantage of this issue to implement it. I have drafted a PR with the help of Claude minions #10095 to convey the idea. Feel free to comment/contribute to it or take inspiration to complete this PR. @CrystalChun please share your thoughts FYI @carbonin as we were talking about it |
Tested your fix in my lab system. Solves the same problem. I'm happy with your implementation if we can proceed with that as per our offline discussion. |
|
/hold ...will close once the other PR is merged. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@alegacy: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Superseded by #10095 |
|
@alegacy: This pull request references Jira Issue OCPBUGS-81503. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. 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. |
4 participants
The BMAC controller used client.Update() to write BMH and Agent objects back to the API server. This sends the entire object, which risks overwriting fields set by other controllers (e.g., siteconfig) — if assisted-service is compiled against an older version of the BMH CRD that lacks newer fields.
Switch handleReconcileResult to use client.Patch with client.MergeFrom, which sends only the delta between the pre-mutation snapshot and the current state. This ensures fields unknown to assisted-service's Go structs are left untouched on the server.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist