Repository navigation
Conversation
Delegates to feedback.Bridge with IsNotFound set to match the sentinel ErrExternalIPNotFound error. The Fetch callback preserves the original error-wrapping behavior (gRPC NotFound and nil object both wrap into ErrExternalIPNotFound). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
|
@vladikr: This pull request references OSAC-3132 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 task 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 APPROVED This pull-request has been approved by: vladikr The full list of commands accepted by this bot can be found here. The pull request process is described 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 (4)
💤 Files with no reviewable changes (1)
WalkthroughExternalIP and ExternalIPAttachment feedback controllers now delegate reconciliation to ChangesFeedback bridge migration
Estimated code review effort: 4 (Complex) | ~45 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/feedback/bridge_test.go (1)
408-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the remote record actually reaches the hook.
This is the only test exercising the new parameter, and it discards it. Since
syncDeleteFnalready setsSUBNET_STATE_DELETING, verifying the hook receives that mutated remote locks in the contract the signature change exists for.♻️ Suggested assertion
- bridge.PostSaveOnDelete = func(_ context.Context, _ *v1alpha1.Subnet, _ *privatev1.Subnet) error { + bridge.PostSaveOnDelete = func(_ context.Context, _ *v1alpha1.Subnet, remote *privatev1.Subnet) error { + Expect(remote).NotTo(BeNil()) + Expect(remote.GetStatus().GetState()).To(Equal(privatev1.SubnetState_SUBNET_STATE_DELETING)) Expect(trk.saveCalls).To(Equal(1))🤖 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/feedback/bridge_test.go` around lines 408 - 415, Update the PostSaveOnDelete callback in the deletion test to retain the received *privatev1.Subnet argument and assert that its state is SUBNET_STATE_DELETING before completing the existing assertions. Keep the current hook invocation and finalizer checks unchanged.internal/controller/externalipattachment_feedback_controller.go (1)
185-219: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid unguarded full-object updates on the shared parent
ExternalIP.
syncAttachedOnParentExternalIPmutatesexternalIP.Status.Attachedand then callsExternalIPsUpdateRequest_builder{Object: externalIP}without a field mask or lock, so concurrent updates to the same parent, including feedback controller saves writing other status fields, can clobber each other’s changes. Use a partial/status update path, or enable locking/resource-version checks before sending the object.🤖 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/externalipattachment_feedback_controller.go` around lines 185 - 219, The syncAttachedOnParentExternalIP function must avoid unguarded full-object updates when changing the shared parent ExternalIP status. Replace the ExternalIPsUpdateRequest_builder update with a partial/status-only update or add locking/resource-version checks so concurrent status changes are not overwritten, while preserving the existing attached-state comparison and error behavior.
🤖 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/controller/externalipattachment_feedback_controller.go`:
- Around line 114-123: Update the feedback callback around
syncExternalIPAttachmentAddress and syncAttachedOnParentExternalIP to fetch the
parent ExternalIP once, reuse that result for both synchronization steps, and
propagate any fetch error from address synchronization so SyncUpdate returns the
error and triggers a retry. Preserve the existing Ready-phase handling and
logging behavior for parent-attachment failures.
---
Outside diff comments:
In `@internal/controller/externalipattachment_feedback_controller.go`:
- Around line 185-219: The syncAttachedOnParentExternalIP function must avoid
unguarded full-object updates when changing the shared parent ExternalIP status.
Replace the ExternalIPsUpdateRequest_builder update with a partial/status-only
update or add locking/resource-version checks so concurrent status changes are
not overwritten, while preserving the existing attached-state comparison and
error behavior.
In `@internal/controller/feedback/bridge_test.go`:
- Around line 408-415: Update the PostSaveOnDelete callback in the deletion test
to retain the received *privatev1.Subnet argument and assert that its state is
SUBNET_STATE_DELETING before completing the existing assertions. Keep the
current hook invocation and finalizer checks unchanged.
🪄 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: d10c03ae-e0a8-412d-9260-d6523dd00660
📒 Files selected for processing (5)
internal/controller/externalip_feedback_controller.gointernal/controller/externalipattachment_feedback_controller.gointernal/controller/feedback/bridge.gointernal/controller/feedback/bridge_test.gointernal/controller/feedback_controller.go
💤 Files with no reviewable changes (1)
- internal/controller/feedback_controller.go
| return func(ctx context.Context, obj *v1alpha1.ExternalIPAttachment, remote *privatev1.ExternalIPAttachment) error { | ||
| syncExternalIPAttachmentState(ctx, obj, remote) | ||
| syncExternalIPAttachmentAddress(ctx, eipClient, remote) | ||
|
|
||
| if obj.Status.Phase == v1alpha1.ExternalIPAttachmentPhaseReady { | ||
| if err := syncAttachedOnParentExternalIP(ctx, eipClient, remote, true); err != nil { | ||
| ctrllog.FromContext(ctx).Error(err, "Failed to set attached on parent ExternalIP, will retry") | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Parent ExternalIP is fetched twice, and the address-sync fetch failure is silently dropped.
On a Ready attachment this issues two eipClient.Get RPCs for the same parent in a single pass. Worse, syncExternalIPAttachmentAddress logs-and-returns on error, so SyncUpdate returns nil, the Bridge saves the attachment without externalIpAddress, and nothing requeues — the address can stay unset until an unrelated event fires. Fetch the parent once and propagate the error so the reconcile retries.
🔧 Sketch: single fetch, propagated error
return func(ctx context.Context, obj *v1alpha1.ExternalIPAttachment, remote *privatev1.ExternalIPAttachment) error {
syncExternalIPAttachmentState(ctx, obj, remote)
- syncExternalIPAttachmentAddress(ctx, eipClient, remote)
-
- if obj.Status.Phase == v1alpha1.ExternalIPAttachmentPhaseReady {
- if err := syncAttachedOnParentExternalIP(ctx, eipClient, remote, true); err != nil {
- ctrllog.FromContext(ctx).Error(err, "Failed to set attached on parent ExternalIP, will retry")
- return err
- }
- }
- return nil
+ ready := obj.Status.Phase == v1alpha1.ExternalIPAttachmentPhaseReady
+ // fetch the parent once, reuse for address + attached sync
+ return syncParentExternalIP(ctx, eipClient, remote, ready)
}🤖 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/externalipattachment_feedback_controller.go` around lines
114 - 123, Update the feedback callback around syncExternalIPAttachmentAddress
and syncAttachedOnParentExternalIP to fetch the parent ExternalIP once, reuse
that result for both synchronization steps, and propagate any fetch error from
address synchronization so SyncUpdate returns the error and triggers a retry.
Preserve the existing Ready-phase handling and logging behavior for
parent-attachment failures.
Delegates to feedback.Bridge with IsNotFound for the sentinel error, and PostSaveOnDelete for clearing the parent ExternalIP's attached flag after the attachment's DELETING state is persisted. SyncUpdate captures eipClient for setting attached=true on Ready and syncing the parent's address to the attachment. Bridge API change: PostSaveOnDelete now receives the remote proto in addition to the K8s object, so callbacks can reference proto spec fields (e.g. the parent ExternalIP ID). Also removes the clone/equal generic helpers from feedback_controller.go since all feedback controllers now use the Bridge (which calls proto.Clone/proto.Equal directly). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
0f77785 to
6d39f1e
Compare
|
moving to osac-project/osac#121 |
This branch was previously deployed
2 participants
Summary
Final migration PR — ExternalIP and ExternalIPAttachment feedback controllers now use the
feedback.Bridge. This completes the migration of all feedback controllers (excluding BareMetalInstance, which is a fundamentally different signal-only pattern).IsNotFoundfor sentinelErrExternalIPNotFoundIsNotFound,PostSaveOnDelete, and two gRPC clientsBridge API change
PostSaveOnDeletesignature updated fromfunc(ctx, obj O) errortofunc(ctx, obj O, remote R) error. The remote proto is now passed so callbacks can reference proto spec fields — ExternalIPAttachment needs the parent ExternalIP ID fromremote.GetSpec().GetExternalIp()to clear the parent's attached flag.Bridge tests updated to match.
ExternalIPAttachment details
This controller is the most complex feedback controller:
eipClientto setattached=trueon the parent ExternalIP when the attachment reaches Ready, and to sync the parent's addressattached=falseon the parent ExternalIP after the attachment's DELETING state is persisted — this is the cross-resource side effect that motivated thePostSaveOnDeletehook in PR OSAC-3132: extract feedback.Bridge and migrate Subnet feedback controller #387Cleanup
Removed the
clone[M]/equal[M]generic helpers fromfeedback_controller.go— they have no remaining callers now that all feedback controllers use the Bridge (which callsproto.Clone/proto.Equaldirectly).Migration complete
All feedback controllers now use
feedback.Bridge:IsNotFoundIsNotFound,PostSaveOnDeleteRelated bugs found during migration
Test plan
make lint— 0 issuesmake test— all pass (controller coverage 72.3%, bridge coverage 94.0%)Jira: https://redhat.atlassian.net/browse/OSAC-3132
Summary by CodeRabbit