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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 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 Merge Risk: ⚪ Minimal · up to The change simplifies connection-check scheduling and makes its check period configurable while adding coverage for Run behavior. No actionable merge-blocking risk remains beyond normal checks and review. 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, 1 warning)
✅ Passed checks (10 passed)
Full details: Title checkExplanation The title describes the connection checker refactor, uses the imperative mood, and is 61 characters long. It does not use an affected-component prefix, but the requirement applies only when the title is scoped. Full details: Pr QualityExplanation The change is non-trivial and behavioral, although its scope is small: 3 files and 184 non-vendor changed lines, well below 7000. The description explains the redundant loops and the refactor, and it mentions unit tests. However, it does not identify automated CNO CI lanes/jobs or platforms. It also omits user-facing impact and upgrade/rollback considerations for the changed checker behavior and public constructor. The commit title says it fixes redundant looping, but neither the title nor body contains a bug or issue link. Resolution Update the PR description with explicit Why, What, and Testing/How to verify it sections. In Testing, name the automated CNO CI jobs or lanes and the platforms they cover. Describe the user-facing impact and upgrade/rollback considerations for this behavioral and public API change. Add a valid bug or issue link in the title or description. Full details: Commit Message QualityExplanation The PR has one self-contained, non-merge commit. Its body explains the reason for the refactor, and the subject is descriptive. However, the commit changes only Full details: Unit Tests For Go ChangesExplanation PASS: The pull request modifies production Go files under pkg/ and also modifies pkg/cmd/checkendpoints/controller/connection_checker_test.go. No bindata/ template changes are present. This satisfies the custom check requirement for accompanying test-file changes. Full details: E2e Tests For Feature ChangesExplanation The PR changes non-test Go source under Resolution Add or modify appropriate files under Full details: Docs For Feature And Behavior ChangesExplanation PASS. The diff contains only internal Full details: Stale Project Docs And ConfigExplanation No project documentation or configuration became stale. The PR changes only Go source and test files. The applicable files have no diff from the parent, and searches found no references to Full details: Go And Test Code QualityExplanation PASS. The committed diff adds no prohibited logging calls, bare duration values, nested error shadowing, IPv4-only handling, or unprotected new map/slice access. The added tests use Gomega polling and do not use bare t.Fatal/t.Fatalf, time.Sleep, or os.Setenv. The existing Full details: Ai-Generated Code SmellExplanation The PR adds two obvious comment-slop comments in Full details: Stable And Deterministic Test NamesExplanation PASS — The pull request adds standard Go
✨ Finishing Touches🧪 Generate unit tests (beta)
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
70f509e to
39cfe66
Compare
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>
39cfe66 to
912060b
Compare
|
@tpantelis: 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. |
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().