Skip to content

Conversation

@kkrik-es
Copy link
Contributor

Snapshot shard statistics seem to pick up unrelated files, leading to flakiness.

Fixes #120332

@kkrik-es kkrik-es added >test Issues or PRs that are addressing/adding tests auto-backport Automatically create backport pull requests when merged Team:StorageEngine :StorageEngine/Mapping The storage related side of mappings v8.18.1 v8.19.0 labels Feb 26, 2025
@kkrik-es kkrik-es self-assigned this Feb 26, 2025
@kkrik-es kkrik-es marked this pull request as ready for review February 26, 2025 07:45
@kkrik-es kkrik-es requested a review from martijnvg February 26, 2025 07:45
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

- match: { snapshot.snapshot: test_snapshot_2 }
- match: { snapshot.state : PARTIAL }
- match: { snapshot.shards.successful: 0 }
- match: { snapshot.shards.failed : 1 }
Copy link
Member

Choose a reason for hiding this comment

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

It is strange that that .security index is included. In both create snapshot requests, indices parameter is specified and the pattern doesn't match with the security index.

Copy link
Member

Choose a reason for hiding this comment

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

I think the reason system indices are included is because include_global_state defaults to true. I think we just need to set include_global_state to false in the create snapshot request and that should resolve the test failure as well and then we can keep these assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I think this is a more fundamental bug, we should be calling the reset-feature-state API to clean up system indices between tests, but it seems that the .security index is escaping that cleanup. I mean this is an ok fix assuming you don't really need these assertions, but there's definitely something bad happening here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right we actually kept the asserts and applied "include_global_state": false. That should do the trick, let's see.

Copy link
Contributor

Choose a reason for hiding this comment

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

++ sorry yeah also a valid workaround, but still there's some more fundamental problem here.

@kkrik-es kkrik-es merged commit 4269c73 into elastic:main Feb 26, 2025
17 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.18 Commit could not be cherrypicked due to conflicts
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 123458

@kkrik-es
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x
8.18

Questions ?

Please refer to the Backport tool documentation

@kkrik-es
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
9.0

Questions ?

Please refer to the Backport tool documentation

kkrik-es added a commit to kkrik-es/elasticsearch that referenced this pull request Feb 26, 2025
* Update 10_basic.yml

* Update muted-tests.yml

* Update 10_basic.yml

(cherry picked from commit 4269c73)

# Conflicts:
#	muted-tests.yml
kkrik-es added a commit to kkrik-es/elasticsearch that referenced this pull request Feb 26, 2025
* Update 10_basic.yml

* Update muted-tests.yml

* Update 10_basic.yml

(cherry picked from commit 4269c73)
elasticsearchmachine pushed a commit that referenced this pull request Feb 26, 2025
* Update 10_basic.yml

* Update muted-tests.yml

* Update 10_basic.yml

(cherry picked from commit 4269c73)

# Conflicts:
#	muted-tests.yml
elasticsearchmachine pushed a commit that referenced this pull request Feb 26, 2025
* Update 10_basic.yml

* Update muted-tests.yml

* Update 10_basic.yml

(cherry picked from commit 4269c73)

# Conflicts:
#	muted-tests.yml
elasticsearchmachine pushed a commit that referenced this pull request Feb 26, 2025
* Update 10_basic.yml

* Update muted-tests.yml

* Update 10_basic.yml

(cherry picked from commit 4269c73)
@kkrik-es kkrik-es deleted the fix/120332 branch February 26, 2025 14:06
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 backport pending :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine >test Issues or PRs that are addressing/adding tests v8.18.1 v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] XPackRestIT test {p0=snapshot/10_basic/Failed to snapshot indices with synthetic source} failing

4 participants