fix database manager with multiple pipelines #12862

kaisecheng · 2021-04-29T20:22:00Z

This PR goes together with plugin PR logstash-plugins/logstash-filter-geoip#181
To the reviewer: I rewrote the whole flow of geoip database service. So, it is easier to review the raw file.
Please check here

Background

Currently, each geoip pipeline creates one DatabaseManager to download database. To prevent duplicate download, this PR changes DatabaseManager to singleton to make sure only one instance manages database and one scheduler downloads database.

Acceptance criteria

in multiple pipelines, only one set of database is downloaded
the scheduler fetches new database if there is a new release, and checks database age every day
when there is new release, database manager should push the new database path to all geoip plugins
when Logstash fail to touch infra endpoint for 30 days and user is using EULA version, Logstash see the database as expired and trigger the expiry action of the geoip plugin
when user hasn't set the database path (online mode) and couldn't download database, Logstash fallback to CC version indefinitely until Logstash can download database again, and should not experience any interruption
Logstash only keep two versions of datatabse, CC and the latest EULA. Old EULA database should be removed once a new version is available
when user provides database path (offline mode), Logstash should not access endpoint or download database

metadata example

database type	update at	gz md5	directory name	is eula
ASN	1620246514	2f09b8db7a7f993d642e450d3ebcf339	1620245143	true
City	1620246514	678ec92e897050fdb32ad56ffb91e487	1620245143	true

Fixed: #12856

…oad GeoIP database

change download location

x-pack/lib/filters/geoip/database_manager.rb

kaisecheng · 2021-05-06T12:45:55Z

x-pack/lib/filters/geoip/database_manager.rb

+
+      case
+      when days_without_update >= 30
+        if @states[database_type].is_eula && @states[database_type].plugins.size > 0


(5) this check ensures Logstash can use CC database indefinitely. we only notify plugins to do expiry action when the plugin is using EULA and age >= 30

kaisecheng · 2021-05-06T12:48:12Z

x-pack/lib/filters/geoip/database_manager.rb

+            "Please check the network settings and allow Logstash accesses the internet to download the latest database, "\
+            "or switch to offline mode (:database => PATH_TO_YOUR_DATABASE) to use a self-managed database "\
+            "which you can download from https://dev.maxmind.com/geoip/geoip2/geolite2/ ")
+          @states[database_type].plugins.dup.each { |plugin| plugin.expire_action if plugin }


(4) the actual expiry logic is in plugin

…ple_pipelines

…ple_pipelines � Conflicts: � x-pack/lib/filters/geoip/database_manager.rb � x-pack/lib/filters/geoip/database_metadata.rb � x-pack/lib/filters/geoip/util.rb � x-pack/spec/filters/geoip/database_manager_spec.rb � x-pack/spec/filters/geoip/database_metadata_spec.rb

…ple_pipelines # Conflicts: # x-pack/lib/filters/geoip/database_manager.rb # x-pack/spec/filters/geoip/database_manager_spec.rb

jsvd

this review suggests some changes so that the implementation is done in a more Ruby-like way, namely adopting some patterns like the Observer and Singleton patterns.

x-pack/lib/filters/geoip/database_manager.rb

x-pack/lib/filters/geoip/database_metadata.rb

x-pack/lib/filters/geoip/database_manager.rb

x-pack/lib/filters/geoip/database_metadata.rb

jsvd · 2021-06-09T11:32:05Z

A couple of notes of things we should address but that don't block this PR:

The periodic check will always print the same message once the Database is deleted:

[2021-06-09T12:10:17,893][WARN ][logstash.filters.geoip   ][pipeline_17] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}

This message gets printed every day, and in that mindset it's confusing to say "geoip plugin will stop / will tag", as it's been doing it already for perhaps days or weeks

with multiple pipelines and a single database manager, the log entries are very confusing, as only 1 pipeline is identified (likely the last to start), but we print N messages for N pipelines using the geoip filter. A few examples:

[2021-06-09T12:12:02,296][INFO ][logstash.filters.geoip   ][pipeline_17] Using geoip database {:path=>"/Users/joaoduarte/elastic/logstash/.bare/data/plugins/filters/geoip/1623237108/GeoLite2-City.mmdb", :healthy_database=>true}
[2021-06-09T12:12:02,297][INFO ][logstash.filters.geoip   ][pipeline_17] Using geoip database {:path=>"/Users/joaoduarte/elastic/logstash/.bare/data/plugins/filters/geoip/1623237108/GeoLite2-City.mmdb", :healthy_database=>true}
[2021-06-09T12:12:02,297][INFO ][logstash.filters.geoip   ][pipeline_17] Using geoip database {:path=>"/Users/joaoduarte/elastic/logstash/.bare/data/plugins/filters/geoip/1623237108/GeoLite2-City.mmdb", :healthy_database=>true}
[2021-06-09T12:12:02,297][INFO ][logstash.filters.geoip   ][pipeline_17] Using geoip database {:path=>"/Users/joaoduarte/elastic/logstash/.bare/data/plugins/filters/geoip/1623237108/GeoLite2-City.mmdb", :healthy_database=>true}
[2021-06-09T12:12:02,297][INFO ][logstash.filters.geoip   ][pipeline_17] Using geoip database {:path=>"/Users/joaoduarte/elastic/logstash/.bare/data/plugins/filters/geoip/1623237108/GeoLite2-City.mmdb", :healthy_database=>true}

And:

[2021-06-09T12:21:26,613][WARN ][logstash.filters.geoip   ][pipeline_3] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}
[2021-06-09T12:21:26,613][WARN ][logstash.filters.geoip   ][pipeline_3] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}
[2021-06-09T12:21:26,614][WARN ][logstash.filters.geoip   ][pipeline_3] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}
[2021-06-09T12:21:26,614][WARN ][logstash.filters.geoip   ][pipeline_3] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}
[2021-06-09T12:21:26,614][WARN ][logstash.filters.geoip   ][pipeline_3] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}
[2021-06-09T12:21:26,614][WARN ][logstash.filters.geoip   ][pipeline_3] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}
[2021-06-09T12:21:26,614][WARN ][logstash.filters.geoip   ][pipeline_3] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}
[2021-06-09T12:21:26,614][WARN ][logstash.filters.geoip   ][pipeline_3] geoip plugin will stop filtering and will tag all events with the '_geoip_expired_database' tag. {:healthy_database=>false}

Or even in some cases there's only 1 message, but still gets identified with a pipeline (rufus changed to execute every 30 seconds):

[2021-06-09T12:28:25,285][ERROR][logstash.filters.geoip.databasemanager][pipeline_2] Failed to open TCP connection to geoip.elastic.co:443 (initialize: name or service not known) {:cause=>#<SocketError: Failed to open TCP connection to geoip.elastic.co:443 (initialize: name or service not known)>}
[2021-06-09T12:28:55,274][ERROR][logstash.filters.geoip.databasemanager][pipeline_2] Failed to open TCP connection to geoip.elastic.co:443 (initialize: name or service not known) {:cause=>#<SocketError: Failed to open TCP connection to geoip.elastic.co:443 (initialize: name or service not known)>}
[2021-06-09T12:29:25,559][ERROR][logstash.filters.geoip.databasemanager][pipeline_2] Failed to open TCP connection to geoip.elastic.co:443 (initialize: name or service not known) {:cause=>#<SocketError: Failed to open TCP connection to geoip.elastic.co:443 (initialize: name or service not known)>}

jsvd

LGTM

jsvd

LGTM
tested manually multiple scenarios, online and offline, with > 20 pipelines.
Logstash + the plugin were always able to either download or delete the database correctly.

kaisecheng · 2021-06-09T12:03:12Z

The periodic check will always print the same message once the Database is deleted

I will update plugin fail_filter to only print msg when @heathy_database turn from true to false

with multiple pipelines and a single database manager, the log entries are very confusing, as only 1 pipeline is identified (likely the last to start)

Can you explain your setup? I have tested 10x pipelines. Each pipeline prints the corresponding msg. How do you set it to n pipelines but only one pipeline is identified?

kaisecheng · 2021-06-09T14:36:21Z

I found two issues.

if the database is expired before Logstash start, the fail path doesn't run because the fail action is taken before observable adds plugin
observable is not thread-safe. add_observer and delete_observer should work with lock

fix database expired before startup fix fallback to CC database issue

jsvd · 2021-06-15T13:01:47Z

One final remark: when restarting logstash when offline after it deleted an expired database, we can't see any information that the plugins will not enrich data. if you're familiar with the implementation you can figure out that the following line tells you that:

[2021-06-15T13:57:36,392][INFO ][logstash.filters.geoip   ][pipeline_12] Using geoip database {:path=>nil}

Can we also throw a warning in this situation? the user should see that they're starting a new pipeline that won't enrich data

jsvd

LGTM 🚀

jsvd · 2021-06-15T16:21:07Z

@kaisecheng I don't see master failing on the tests that this PR is failing in, can you do a check and find if these changes are causing an intengration test to fail?

kaisecheng · 2021-06-15T16:28:24Z

@kaisecheng I don't see master failing on the tests that this PR is failing in, can you do a check and find if these changes are causing an intengration test to fail?

the integration test failed because we don't have a match geoip plugin release.
We need to first ~~merge master and then release the plugin gem~~ release the plugin gem and trigger the test in this branch

…ple_pipelines

…bump version to 7.2.1

kaisecheng · 2021-06-16T14:33:44Z

jenkins test this please

kaisecheng · 2021-06-16T21:40:51Z

jenkins test this please

This PR adds support to geoip multiple pipelines which have a single instance to manage database download to avoid individual download per pipeline

* master: (41 commits) Test: resolve integration failure due ECS mode (elastic#13044) Feat: event factory support (elastic#13017) Doc: Add geoip database API to node stats (elastic#13019) Add geoip database metrics to /node/stats API (elastic#13004) ecs: on-by-default plus docs (elastic#12830) ispec: fix cross-spec leak from fatal error integration specs (elastic#13002) Fix UBI source URL (elastic#13008) update fpm to allow pkg creation on jdk11+jruby 9.2 (elastic#13005) Add unit test to grant that production aliases correspond to a published RubyGem (elastic#12993) Fix logstash.bat not setting exit code (elastic#12948) Use the OS separator to invoke gradlew from Rake script (elastic#13000) Allow per-pipeline config of ECS Compatibility mode via Central Management (elastic#12861) Update jinja2 dependency in docker build (elastic#12994) fix database manager with multiple pipelines (elastic#12862) Fix Reflections stack traces when process yml files in classpath and debug is enabled (elastic#12991) Fix/log4j routing to avoid create spurious file (elastic#12965) Deps: update JRuby to 9.2.19.0 (elastic#12989) Doc: Add tip for checking for existing field (elastic#12899) Added test to cover the installation of aliased plugins (elastic#12967) CI: Update logstash_release.json after 7.3.12 (elastic#12986) ...

kaisecheng mentioned this pull request Apr 29, 2021

fix database manager with multiple pipelines and expiry action logstash-plugins/logstash-filter-geoip#181

Merged

roaksoax added the status:work-in-progress label Apr 29, 2021

kaisecheng added 3 commits May 4, 2021 13:45

change DatabaseManager to singleton to ensure only one instance downl…

5b83225

…oad GeoIP database

DatabaseManager ensures single download job

94ee10c

refactor DatabaseManager

222d807

kaisecheng force-pushed the geoip_multiple_pipelines branch from 5e2091a to 222d807 Compare May 4, 2021 16:32

kaisecheng added 6 commits May 4, 2021 20:56

log age check only when subscriber exist

8646194

update log message

1d2ab08

add bypass

950fe8c

change metadata format

481bb22

change download location

log trace

18a9cac

comment

4d8be13

kaisecheng commented May 5, 2021

View reviewed changes

x-pack/lib/filters/geoip/database_manager.rb Show resolved Hide resolved

kaisecheng marked this pull request as ready for review May 5, 2021 22:09

roaksoax added status:needs-review and removed status:work-in-progress labels May 5, 2021

kaisecheng added 2 commits May 6, 2021 12:15

add test case

0dd4c9d

fix test

f4cf2a5

kaisecheng assigned elasticsearch-bot May 6, 2021

kaisecheng commented May 6, 2021

View reviewed changes

jsvd self-requested a review May 6, 2021 12:51

jsvd unassigned elasticsearch-bot May 6, 2021

kaisecheng added 3 commits May 10, 2021 15:09

Merge branch 'master' of github.com:elastic/logstash into geoip_multi…

580824b

…ple_pipelines

update log message

021d48d

kaisecheng mentioned this pull request May 17, 2021

[META] GeoIP filter download service for MaxMind license change #12560

Closed

18 tasks

kaisecheng added 2 commits June 2, 2021 10:41

ensure database path return the result after initial download finished

c885b82

Merge branch 'master' of github.com:elastic/logstash into geoip_multi…

fe26d4b

…ple_pipelines # Conflicts: # x-pack/lib/filters/geoip/database_manager.rb # x-pack/spec/filters/geoip/database_manager_spec.rb

jsvd reviewed Jun 2, 2021

View reviewed changes

jsvd approved these changes Jun 9, 2021

View reviewed changes

roaksoax added status:approved and removed status:needs-review labels Jun 9, 2021

kaisecheng added 5 commits June 10, 2021 11:29

remove observable

fb2b592

fix database expired before startup fix fallback to CC database issue

add test case

ba9bbd9

remove thread context pipeline.id

73b7a4f

adjust log message

59880b6

adjust log message

cda5900

kaisecheng added 2 commits June 15, 2021 15:03

fix test

79c15de

adjust log message

b9b248c

kaisecheng requested a review from jsvd June 15, 2021 14:46

jsvd approved these changes Jun 15, 2021

View reviewed changes

kaisecheng added 2 commits June 16, 2021 13:30

Merge branch 'master' of github.com:elastic/logstash into geoip_multi…

bb22ab8

…ple_pipelines

7.2.0 was yanked due to missing jars dependency in manual publish. …

dfca338

…bump version to 7.2.1

kaisecheng merged commit 42c4bba into elastic:master Jun 16, 2021

kaisecheng mentioned this pull request Jun 16, 2021

[7.x] fix database manager with multiple pipelines (#12862) #12995

Merged

kaisecheng added the v7.14.0 label Jun 16, 2021

kaisecheng added a commit that referenced this pull request Jun 17, 2021

fix database manager with multiple pipelines (#12862) (#12995)

72e8dc8

This PR adds support to geoip multiple pipelines which have a single instance to manage database download to avoid individual download per pipeline

kaisecheng added the geoip label Jul 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix database manager with multiple pipelines #12862

fix database manager with multiple pipelines #12862

kaisecheng commented Apr 29, 2021 •

edited

Loading

kaisecheng May 6, 2021

kaisecheng May 6, 2021

jsvd left a comment

jsvd commented Jun 9, 2021

jsvd left a comment

jsvd left a comment

kaisecheng commented Jun 9, 2021

kaisecheng commented Jun 9, 2021

jsvd commented Jun 15, 2021

jsvd left a comment

jsvd commented Jun 15, 2021

kaisecheng commented Jun 15, 2021 •

edited

Loading

kaisecheng commented Jun 16, 2021

kaisecheng commented Jun 16, 2021

fix database manager with multiple pipelines #12862

fix database manager with multiple pipelines #12862

Conversation

kaisecheng commented Apr 29, 2021 • edited Loading

Background

Acceptance criteria

metadata example

kaisecheng May 6, 2021

Choose a reason for hiding this comment

kaisecheng May 6, 2021

Choose a reason for hiding this comment

jsvd left a comment

Choose a reason for hiding this comment

jsvd commented Jun 9, 2021

jsvd left a comment

Choose a reason for hiding this comment

jsvd left a comment

Choose a reason for hiding this comment

kaisecheng commented Jun 9, 2021

kaisecheng commented Jun 9, 2021

jsvd commented Jun 15, 2021

jsvd left a comment

Choose a reason for hiding this comment

jsvd commented Jun 15, 2021

kaisecheng commented Jun 15, 2021 • edited Loading

kaisecheng commented Jun 16, 2021

kaisecheng commented Jun 16, 2021

kaisecheng commented Apr 29, 2021 •

edited

Loading

kaisecheng commented Jun 15, 2021 •

edited

Loading