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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maxmind geoip provider to support in process db reload upon mmdb file update #30250

Closed
nezdolik opened this issue Oct 16, 2023 · 8 comments · Fixed by #32490
Closed

Maxmind geoip provider to support in process db reload upon mmdb file update #30250

nezdolik opened this issue Oct 16, 2023 · 8 comments · Fixed by #32490
Assignees
Labels
area/geoip Any work related to geoip filter area/http_filter enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue

Comments

@nezdolik
Copy link
Member

Title: Maxmind geoip provider to support in process db reload upon mmdb file update

Description:
Maxmind geoip provider currently does not support in process reload of mmdb files, in case such files have been updated externally. Envoy needs to be restarted to have the latest content from mmdb file. High level approach on how this can be supported in process (assuming low churn of updates, Maxmind publishes new versions on weekly basis):

  • Create Filesystem::Watcher object for each mmdb file path and watch for file update events.
  • Add boolean flag into provider if any lookup is currently in progress
  • Whenever update callback in provider is triggered by Filesystem::Watcher:
    ** Check if there is any lookup in progress. If not, mutex lock the mddb object and reload it with MMDB_open call. If lookup is in progress, wait until is not and do the same logic (as in first part). That means reload callback will need to run on separate thread.
    ** Another option add one boolean flag if specific db needs reload and check it in the beginning of subsequent lookup and reload db if needed.
    There should exist single instance of geoip provider based on current implementation (one provider per one config hash).
@nezdolik nezdolik added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Oct 16, 2023
@nezdolik
Copy link
Member Author

cc @ravenblackx @alyssawilk

@nezdolik
Copy link
Member Author

Also asked if MMDB_open watches for file update, i don't think it is, but there is always hope: maxmind/libmaxminddb#322

@kyessenov kyessenov added area/http_filter and removed triage Issue requires triage labels Oct 17, 2023
@ravenblackx
Copy link
Contributor

Could you use a mutex-guarded shared_ptr and thereby allow requests to read from the old instance until the new one is loaded, then switch the 'source' shared_ptr? Seems like your first option risks a starvation (if there's always someone reading then the update never gets a chance), and the second assumes there is only one reader thread, which I don't think is the case (unless you mean still doing the mutex lock thing and just having one of the threads take on the role of updater).

I don't know how long the load operation is; blocking worker threads for the duration of a load may also be an issue?

@nezdolik
Copy link
Member Author

I will check how long the load operation takes (also with prod version of database). Thank you for suggestion @ravenblackx

Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 19, 2023
@nezdolik
Copy link
Member Author

lively ping

@nezdolik nezdolik added the area/geoip Any work related to geoip filter label Nov 21, 2023
@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 22, 2023
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 22, 2023
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 29, 2023
@nezdolik nezdolik reopened this Jan 11, 2024
@phlax phlax added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Jan 11, 2024
@nezdolik nezdolik self-assigned this Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/geoip Any work related to geoip filter area/http_filter enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants