Skip to content

Conversation

@nielsbauman
Copy link
Contributor

The GeoIP expiration logic assumes that the md5 hashes of the databases change over time, as it deletes all database chunks from the .geoip_databases index. If the hashes do not change, no new database chunks are indexed and so the ingest nodes fail to download the databases as there are no chunks to download.
In this test suite, we don't have a way to change the database files served by the fixture, and thus change their md5 hashes.
Since we assume that in a real-world scenario the database files change over time, we are ok with skipping this part of the test.

See #128284 (comment) for a detailed investigation.

Closes #128284

@nielsbauman nielsbauman requested a review from Copilot November 6, 2025 10:02
@nielsbauman nielsbauman added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP and removed :Analytics/Aggregations Aggregations labels Nov 6, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Fixes a flaky test by removing the problematic assertion that assumed database files would be re-downloaded after resetting the validity setting. The test was failing because the GeoIP expiration logic deletes database chunks but doesn't re-download them when MD5 hashes remain unchanged, which is the case in the test fixture environment.

  • Removed the second half of testInvalidTimestamp that attempted to verify database re-download after resetting validity
  • Added a custom IndexSettingProvider to disable request cache for the .geoip_databases index, ensuring cache doesn't mask the deletion of database chunks
  • Unmuted the test in muted-tests.yml

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
muted-tests.yml Removed test mute entry for testInvalidTimestamp
GeoIpDownloaderIT.java Modified test to skip re-download verification, added cache-disabling plugin, and added assertion to verify chunk deletion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@nielsbauman nielsbauman marked this pull request as ready for review November 6, 2025 10:04
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Nov 6, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Copy link
Contributor

@szybia szybia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

my impression is that this doesn't seem too pressing to test, as a reviewer i'd be digging more into why we can't change the hashes, but 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v8.19.8 v9.1.8 v9.2.2 v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] GeoIpDownloaderCliIT testInvalidTimestamp failing

3 participants