OCPBUGS-20056: EndpointAccessibleController: Retry the whole list endpoints and send request loop 3 times#855
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/severity-moderate
|
@tchap: This pull request references Jira Issue OCPBUGS-20056, which is invalid:
Comment 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. |
|
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)
WalkthroughController sync now retries full endpoint-list fetch plus parallel HTTP reachability checks up to configurable attempts with per-request context timeouts, retry intervals, and an injectable HTTP client; resync jitter base made configurable. Tests moved to in-process httptest servers and cover retry, stale-endpoint, and request-timeout scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
tchap
force-pushed
the
EndpointUnavailable-retry
branch
3 times, most recently
from
089d5bc to
a1f718e
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tchap: This pull request references Jira Issue OCPBUGS-20056, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (ksiddiqu@redhat.com), skipping review request. 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. |
tchap
force-pushed
the
EndpointUnavailable-retry
branch
12 times, most recently
from
ed6281c to
795cde0
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/libs/endpointaccessible/endpoint_accessible_controller.go`:
- Around line 142-153: The code currently returns the raw error when
c.endpointListFn() fails for non-NotFound cases, which bypasses the whole-cycle
retry/status update path; change the non-NotFound branch to mirror the NotFound
handling: build an OperatorStatus for c.availableConditionName with
ConditionFalse, set a Reason like "ListError" and the error text via
err.Error(), then call c.operatorClient.ApplyOperatorStatus(ctx,
c.controllerInstanceName, status) and return that result instead of returning
err directly so transient lister/cache failures update operator status and enter
the same retry path.
- Around line 112-119: The production path uses buildTLSClient() which
hard-codes Timeout: 5 * time.Second and thus overrides the computed
requestTimeout (default 10s) so client-level timeout wins and breaks
configurable retry behavior; fix by either removing the fixed Timeout in
buildTLSClient() to rely on context deadlines or change the call site (where
client is nil) to pass the computed requestTimeout into
buildTLSClient(requestTimeout) and apply that value to the client's Timeout;
update the buildTLSClient function signature and any callers to accept and use
requestTimeout (or remove the Timeout field entirely) so the requestTimeout
variable takes effect for production calls that create the http client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aabf9163-8091-402f-895c-249924e7eb8c
📒 Files selected for processing (2)
pkg/libs/endpointaccessible/endpoint_accessible_controller.gopkg/libs/endpointaccessible/endpoint_accessible_controller_test.go
tchap
force-pushed
the
EndpointUnavailable-retry
branch
from
795cde0 to
37b3d01
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/libs/endpointaccessible/endpoint_accessible_controller.go`:
- Around line 145-148: The select that waits between retries currently ignores
the ctx.Done() branch and continues, so when ctx is canceled the controller
still runs another endpointListFn() and probe; update the select in the retry
loop inside the sync logic to exit the sync when ctx.Done() is selected (e.g.,
return from the enclosing function or break out of the retry loop) so that no
further endpointListFn() calls or probe passes occur after cancellation and you
don't immediately report EndpointUnavailable during shutdown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7514ce8-5dab-4adb-a476-6002444e71c1
📒 Files selected for processing (2)
pkg/libs/endpointaccessible/endpoint_accessible_controller.gopkg/libs/endpointaccessible/endpoint_accessible_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/libs/endpointaccessible/endpoint_accessible_controller_test.go
|
@tchap: The following tests 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. |
The reason for this change is to get rid of intermittent Available=false, even during upgrades. This is achieved by retrying the whole fetch endpoints and send requests loop 3 times, including a short sleep between each attempt.
tchap
force-pushed
the
EndpointUnavailable-retry
branch
from
37b3d01 to
ad9a720
Compare
|
@tchap: This pull request references Jira Issue OCPBUGS-20056, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (ksiddiqu@redhat.com), skipping review request. 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. |
|
|
||
| defaultRequestTimeout = 10 * time.Second | ||
| defaultRetryInterval = 5 * time.Second | ||
| defaultAttemptCount = 3 |
There was a problem hiding this comment.
Not 100% sure about these values. It effectively means there is an inertial of 40 seconds before an actual Available=false issue surfaces. But there is always a trade-off.
2 participants
The reason for this change is to get rid of intermittent
Available=false, possibly even during upgrades.This is achieved by retrying the whole fetch endpoints and send requests loop 3 times, including a short sleep between each attempt.
Mostly trying to improve CI, though. A failing job example.