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

Fix testSnapshotWithStuckNode #102398

Conversation

DaveCTurner
Copy link
Contributor

This test sometimes relies on repository cleanup to remove all but the
index.latest and index-N blobs, but in fact repo cleanup will leave
behind the index-(N-1) blob too. This commit relaxes the test to
account for this, but then strengthens it to assert that the blobs left
in the repo are exactly the ones we expect.

Closes #101573

This test sometimes relies on repository cleanup to remove all but the
`index.latest` and `index-N` blobs, but in fact repo cleanup will leave
behind the `index-(N-1)` blob too. This commit relaxes the test to
account for this, but then strengthens it to assert that the blobs left
in the repo are exactly the ones we expect.

Closes elastic#101573
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.12.0 labels Nov 21, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team label Nov 21, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Contributor

@arteam arteam left a comment

Choose a reason for hiding this comment

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

Nice work! I spent a lot of time on this test in the last iteration and couldn't make the assertion work consistently.

// Expect two or three files to remain in the repository:
// (1) index-latest
// (2) index-(N+1)
// (3) index-N (maybe: a fully successful deletion removes this, but cleanup does not, see #100718)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it this one https://github.com/elastic/elasticsearch/pull/101718/files ?
Does not seem related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, #100718

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed one digit 🤦

@DaveCTurner DaveCTurner merged commit 8b13e1d into elastic:main Nov 21, 2023
14 checks passed
@DaveCTurner DaveCTurner deleted the 2023/11/21/testSnapshotWithStuckNode-lenience branch November 21, 2023 09:46
@DaveCTurner DaveCTurner restored the 2023/11/21/testSnapshotWithStuckNode-lenience branch June 17, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team >test Issues or PRs that are addressing/adding tests v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] DedicatedClusterSnapshotRestoreIT testSnapshotWithStuckNode failing
4 participants