fix duplicate listener names by stopping existing listener #77
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Eeshu-Yadav, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the system's listener management by implementing a robust dependency tracking mechanism for routes and TLS secrets. It ensures that listeners are only activated when all their necessary resources are available, thereby improving system stability and resource handling. The changes also refine the process of updating existing listeners.
Highlights
- Dependency Tracking for Listeners: Introduced ListenerDependencies to track required route and TLS secret names for listeners.
- Listener Queuing and Auto-start: Listeners with unresolved dependencies are now queued and automatically started once their dependencies become available.
- Dependency Extraction: Logic added to extract dependency names from filter chains and HTTP Connection Manager (HCM) configurations.
- Listener Management Refinement: Improved error logging and ensured static listener behavior is preserved, including explicitly stopping old listeners before starting new ones with the same name.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds dependency tracking for listeners, ensuring they are only started when their required resources like routes and TLS secrets are available. The change in orion-lib/src/listeners/listeners_manager.rs refactors the listener update logic to explicitly stop an existing listener before starting a new one with the same name. This is an excellent improvement as it fixes a potential race condition where the new listener could fail to bind to its address. The new code is clearer, more explicit, and more robust. The changes look good.
|
My understanding is that this PR addresses this issue: github.com//issues/74 . |
|
Sure will update that
|
Eeshu-Yadav
changed the title
Eeshu-Yadav
force-pushed
the
bugfix/listener-duplicate-name-74
branch
from
98f7b2b to
32887fb
Compare
Eeshu-Yadav
force-pushed
the
bugfix/listener-duplicate-name-74
branch
2 times, most recently
from
cb81287 to
b06f779
Compare
|
@hzxuzhonghu kindly review |
Eeshu-Yadav
force-pushed
the
bugfix/listener-duplicate-name-74
branch
2 times, most recently
from
df805b6 to
5e23e34
Compare
Eeshu-Yadav
force-pushed
the
bugfix/listener-duplicate-name-74
branch
from
5e23e34 to
72dd529
Compare
Eeshu-Yadav
force-pushed
the
bugfix/listener-duplicate-name-74
branch
2 times, most recently
from
88fdf2e to
f272970
Compare
True... but at the same time, you could have a very long running connection that will stop you from doing any updates. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dawid-nowak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Make sense lets create another issue to track |
- Check if listener with same name exists before starting new one - Stop the existing listener to prevent conflicts - Improve logging for duplicate listener detection Signed-off-by: Eeshu-Yadav <[email protected]>
Eeshu-Yadav
force-pushed
the
bugfix/listener-duplicate-name-74
branch
from
f272970 to
03dafea
Compare
… versions Enhance the existing multiple listener versions approach with configurable cleanup policies and resource management, following Envoy's patterns for graceful listener updates. Key improvements: - Add ListenerManagerConfig with configurable cleanup policies - Implement automatic cleanup of old listener versions - Support CountBasedOnly, TimeBasedOnly, and Hybrid cleanup strategies - Prevent resource leaks while maintaining connection continuity - Add comprehensive test coverage for cleanup behaviors Benefits: - Graceful LDS-style updates without connection interruption - Long-running connections don't block listener deployments - Configurable behavior for different production environments - Automatic resource management prevents memory/handle leaks Configuration options: - max_versions_per_listener: limit concurrent versions (default: 2) - cleanup_policy: define cleanup strategy (default: CountBasedOnly(2)) - cleanup_interval: background cleanup frequency (default: 60s) Fixes: kmesh-net#98 Related: kmesh-net#74, kmesh-net#77 Signed-off-by: Eeshu-Yadav <[email protected]>
|
@hzxuzhonghu so what is the plan here ? are we ok to merge it while there is ongoing work on feat: allow multiple versions of the same listener #99 ? Otherwise we should close this PR |
|
I want to see a overall proposal, as we know envoy is already making much effort on listener hot reloading. This pr is very aggressive. |
|
I will close this and let's continue the work on PR99 |
5 participants
Fix issue where adding a listener with the same name as an existing listener doesn't properly stop the old one, causing conflicts.
Changes:
Fixes #74