Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMPROVEMENT] Start searching for the new available port from the last allocated port instead of starting from port 0 #8598

Open
james-munson opened this issue May 19, 2024 · 0 comments
Labels
component/longhorn-instance-manager Longhorn instance manager (interface between control and data plane) kind/improvement Request for improvement of existing function require/backport Require backport. Only used when the specific versions to backport have not been definied. require/doc Require updating the longhorn.io documentation require/manual-test-plan Require adding/updating manual test cases if they can't be automated

Comments

@james-munson
Copy link
Contributor

Is your improvement request related to a feature? Please describe (馃憤 if you like this request)

There are known bugs where Longhorn talks to the wrong engine or replica, using a stale IP/port assignment that has been re-issued to another component, but still remembered as having belonged to the former one. See, for instance, #8587 for a recent example.

Describe the solution you'd like

It's true that just like freed memory, the holder of that outdated address should have zeroed it out to avoid mistakenly using it. But it is also true that immediate re-assignment makes it more likely to be hit in error if there is a race between clearing up old address maps and creating new ones.
The bitmap code used to track assigned port ranges always starts a bit 0, meaning that a recently freed bit is very likely to be found and re-used quickly. The suggestion is to revise bitmap.go to act more like a ring buffer: remember the offset of the most recent allocation, and start there in the search for the next to allocate, only returning to start again at zero after wrapping around.
Besides making an accidental late reference much less likely, it will make it much easier to follow the logs, since a given IP/port is unlikely to mean two different targets over time.

Describe alternatives you've considered

I tried this idea out on @PhanLe1010 and @ejweber, and they thought it has value.

Additional context

FWIW, I would do this to all instances of bitmap.go, which is currently copied in

  • longhorn-instance-manager/pkg/util/bitmap.go
  • longhorn-spdk-engine/pkg/util/bitmap.go
  • backing-image-manager/pkg/util/bitmap.go

It might also be worthwhile to move it into go-common-libs and have only one copy of the code.

@james-munson james-munson added require/doc Require updating the longhorn.io documentation require/manual-test-plan Require adding/updating manual test cases if they can't be automated kind/improvement Request for improvement of existing function require/backport Require backport. Only used when the specific versions to backport have not been definied. labels May 19, 2024
@derekbit derekbit added the component/longhorn-instance-manager Longhorn instance manager (interface between control and data plane) label May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/longhorn-instance-manager Longhorn instance manager (interface between control and data plane) kind/improvement Request for improvement of existing function require/backport Require backport. Only used when the specific versions to backport have not been definied. require/doc Require updating the longhorn.io documentation require/manual-test-plan Require adding/updating manual test cases if they can't be automated
Projects
None yet
Development

No branches or pull requests

2 participants