Repository navigation
OSAC-3132: migrate ExternalIP and ExternalIPAttachment feedback controllers to Bridge - #121
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>
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>
|
@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. |
WalkthroughChangesFeedback bridge migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant Bridge
participant ExternalIPAttachmentAPI
participant ExternalIPAPI
Reconciler->>Bridge: Reconcile request
Bridge->>ExternalIPAttachmentAPI: Fetch attachment
Bridge->>ExternalIPAttachmentAPI: Save synchronized state and address
Bridge->>ExternalIPAPI: Detach parent ExternalIP after deletion
🚥 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 (1)
osac-operator/internal/controller/externalipattachment_feedback_controller.go (1)
185-219: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse an
UpdateMaskfor status field updates.
ExternalIPsUpdateRequestsupportsUpdateMask, but these controllers send the fullObjectwithout it. Add anupdate_mask: ["status.attached", "status.state", "status.address"]field mask for the fields this sync writes so concurrent reconciles do not overwrite each other.🤖 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 `@osac-operator/internal/controller/externalipattachment_feedback_controller.go` around lines 185 - 219, Update syncAttachedOnParentExternalIP to include an UpdateMask in the ExternalIPsUpdateRequest, covering status.attached, status.state, and status.address. Keep the existing Object payload and attached-state synchronization unchanged, using the request’s field-mask builder or established API pattern.
🤖 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
`@osac-operator/internal/controller/externalipattachment_feedback_controller.go`:
- Around line 113-126: Update syncExternalIPAttachmentAddress to return an
error, propagating non-NotFound Get failures while preserving nil for missing
IDs, NotFound responses, and incomplete objects. In
newExternalIPAttachmentSyncUpdate, handle and return this error before
continuing so transient address-fetch failures trigger the existing retry
behavior.
---
Outside diff comments:
In
`@osac-operator/internal/controller/externalipattachment_feedback_controller.go`:
- Around line 185-219: Update syncAttachedOnParentExternalIP to include an
UpdateMask in the ExternalIPsUpdateRequest, covering status.attached,
status.state, and status.address. Keep the existing Object payload and
attached-state synchronization unchanged, using the request’s field-mask builder
or established API pattern.
🪄 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: 371fc806-f709-4cbc-9fd3-2bccdfedf301
📒 Files selected for processing (5)
osac-operator/internal/controller/externalip_feedback_controller.goosac-operator/internal/controller/externalipattachment_feedback_controller.goosac-operator/internal/controller/feedback/bridge.goosac-operator/internal/controller/feedback/bridge_test.goosac-operator/internal/controller/feedback_controller.go
💤 Files with no reviewable changes (1)
- osac-operator/internal/controller/feedback_controller.go
|
@tzvatot can you please take a look? |
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Clean 1:1 migration of the last two feedback controllers to Bridge. Logic preserved faithfully, delete/update path ordering matches the original, PostSaveOnDelete API change is safe (no prior callers), test coverage updated.
LGTM.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tzvatot, 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 |
OSAC-3597: Storage backend hooks
This branch was previously deployed
3 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).Ported from osac-operator PR #403 to the monorepo.
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.
Cleanup
Removed the
clone[M]/equal[M]generic helpers fromfeedback_controller.go— no remaining callers now that all feedback controllers use the Bridge.Migration complete
All feedback controllers now use
feedback.Bridge:IsNotFoundIsNotFound,PostSaveOnDeleteTest plan
make lint— 0 issuesmake test— all pass (controller coverage 72.2%, bridge coverage 94.0%)Jira: https://redhat.atlassian.net/browse/OSAC-3132
Summary by CodeRabbit