NO-JIRA: Refactor connection checker to fix redundant looping - #3099
NO-JIRA: Refactor connection checker to fix redundant looping#3099tpantelis wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@tpantelis: This pull request explicitly references no jira issue. 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. |
Summary by CodeRabbit
WalkthroughThe connection checker now accepts configuration, applies configurable timing, performs synchronous checks, and stops through context cancellation. Controller wiring uses the new configuration. Tests cover repeated checks and shutdown paths. ChangesConnection checker lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PodNetworkConnectivityCheckController
participant ConnectionChecker
participant EndpointCheck
PodNetworkConnectivityCheckController->>ConnectionChecker: construct with ConnectionCheckerConfig
ConnectionChecker->>ConnectionChecker: apply check period and timeout
ConnectionChecker->>EndpointCheck: execute one check per interval
EndpointCheck-->>ConnectionChecker: return check result
PodNetworkConnectivityCheckController->>ConnectionChecker: stop or cancel context
ConnectionChecker-->>PodNetworkConnectivityCheckController: exit Run
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors)
✅ Passed checks (20 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tpantelis 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 |
![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 `@pkg/cmd/checkendpoints/controller/connection_checker.go`:
- Around line 105-116: Update the internal goroutine in connectionChecker.Run to
select on both c.stop and ctx2.Done(). Ensure it exits when either Stop() is
called or the parent context is cancelled, while preserving the existing
cancel() behavior.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5491a332-19f7-434f-b151-d9ec567355fb
📒 Files selected for processing (3)
pkg/cmd/checkendpoints/controller/connection_checker.gopkg/cmd/checkendpoints/controller/connection_checker_test.gopkg/cmd/checkendpoints/controller/pod_network_connectivity_check_controller.go
The previous implementation had two layers of periodic execution - Run() used wait.UntilWithContext() to call checkConnection() periodically, but checkConnection() also had its own ticker and infinite loop. The code was simplified to have checkConnection() execute once per call, relying on wait.UntilWithContext() for periodic execution. Also, the previous Run() method launched wait.UntilWithContext() in a goroutine and then blocked on <-ctx2.Done(). This is redundant because wait.UntilWithContext() already blocks until the context is cancelled. The new implementation calls wait.UntilWithContext() directly, which properly blocks the Run() method until the context is cancelled or Stop() is called. Unit tests were added to cover the Run() functionality. To facilitate this, the check period was made configurable by introducing a ConnectionCheckerConfig struct to replace the long parameter list in NewConnectionChecker(). Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
70f509e to
39cfe66
Compare
|
@tpantelis: all tests passed! 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. |
2 participants
The previous implementation had two layers of periodic execution -
Run()usedwait.UntilWithContext()to callcheckConnection()periodically, butcheckConnection()also had its own ticker and infinite loop. The code was simplified to havecheckConnection()execute once per call, relying onwait.UntilWithContext()for periodic execution.Also, the previous
Run()method launchedwait.UntilWithContext()in a goroutine and then blocked on<-ctx2.Done(). This is redundant becausewait.UntilWithContext()already blocks until the context is cancelled. The new implementation callswait.UntilWithContext()directly, which properly blocks theRun()method until the context is cancelled orStop()is called.Unit tests were added to cover the
Run()functionality. To facilitate this, the check period was made configurable by introducing aConnectionCheckerConfigstruct to replace the long parameter list inNewConnectionChecker().