Skip to content

[7.x] Delete mounted indices after test case in ESRestTestCase #73555

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

Merged
merged 7 commits into from
Jun 2, 2021

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented May 31, 2021

This commit adds some clean up logic to ESRestTestCase so that searchable snapshots indices are deleted after test case executions, before the snapshot and repositories are wipe out.

@tlrx tlrx added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.14.0 v7.13.1 labels May 31, 2021
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 31, 2021
@elasticmachine
Copy link
Collaborator

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

@tlrx tlrx changed the title [7.x] Delete mounted indices after in searchable snapshots YAML tests [7.x] Delete mounted indices after searchable snapshots YAML tests May 31, 2021
@tlrx tlrx requested a review from original-brownbear May 31, 2021 12:38
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

I wonder if we should deal with this in ESRestTestCase instead. Otherwise we will have to manually add this cleanup to any new frozen-related REST tests and get extra noisy logging+failures when running other test failure?


- do:
indices.delete:
index: cold-docs
Copy link
Member

Choose a reason for hiding this comment

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

NIT in case my comment below does not make sense: I think index: [cold-docs, frozen-docs] should work here as well, no need for separate delete calls?

@tlrx
Copy link
Member Author

tlrx commented May 31, 2021

@original-brownbear ESRestTestCase already deletes all snapshots and repositories after test, and then delete all indices. For searchable snapshots we would like to delete mounted indices first and I found it easier to do it in tests directly. Do you prefer to add a preserveSearchableSnapshotsUponCompletion() and the logic to delete mounted indices in wipeCluster() ?

@original-brownbear
Copy link
Member

@tlrx

Do you prefer to add a preserveSearchableSnapshotsUponCompletion() and the logic to delete mounted indices in wipeCluster() ?

Yea I figured something along those lines would be nice to have and more in line with what we did for other similar resources in tests+rest tests right?

@tlrx
Copy link
Member Author

tlrx commented May 31, 2021

@original-brownbear indeed, let's do like you suggest, this is more in line with what we did for other resources (but I don't like it :))

@tlrx
Copy link
Member Author

tlrx commented Jun 1, 2021

@original-brownbear sorry - took me a bit of time to get this right. Can you have another look please? I'll forward port to master if it's LGTM.

@tlrx tlrx requested a review from original-brownbear June 1, 2021 08:31
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -591,6 +600,13 @@ private void wipeCluster() throws Exception {
}
}

// Clean up searchable snapshots indices before deleting snapshots and repositories
if (hasXPack() && nodeVersions.first().onOrAfter(Version.V_7_8_0)) {
if (preserveSearchableSnapshotsIndicesUponCompletion() == false) {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: could be joined into a single if with the outer if

@tlrx tlrx changed the title [7.x] Delete mounted indices after searchable snapshots YAML tests [7.x] Delete mounted indices after test case in ESRestTestCase Jun 2, 2021
@tlrx
Copy link
Member Author

tlrx commented Jun 2, 2021

Github Webhooks were down:
@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample
@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample
@elasticmachine run elasticsearch-ci/part-1

tlrx added a commit that referenced this pull request Jun 2, 2021
This commit adds some clean up logic to ESRestTestCase so 
that searchable snapshots indices are deleted after test case 
executions, before the snapshot and repositories are wipe out.

Backport of #73555
tlrx added a commit that referenced this pull request Jun 2, 2021
This commit adds some clean up logic to ESRestTestCase so 
that searchable snapshots indices are deleted after test case 
executions, before the snapshot and repositories are wipe out.

Backport of #73555
@tlrx tlrx merged commit cdb07e1 into elastic:7.x Jun 2, 2021
@tlrx tlrx deleted the delete-mounted-indices branch June 2, 2021 13:08
@tlrx
Copy link
Member Author

tlrx commented Jun 2, 2021

Thanks Armin!

tlrx added a commit that referenced this pull request Jun 4, 2021
In #73555 we updated ESRestTestCase so that it cleans 
up the searchable snapshots indices before cleaning up 
the snapshots and repositories. This test does not require 
to delete resources itself and can let ESRestTestCase do
 the clean up in the correct order.
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Jun 4, 2021
In elastic#73555 we updated ESRestTestCase so that it cleans 
up the searchable snapshots indices before cleaning up 
the snapshots and repositories. This test does not require 
to delete resources itself and can let ESRestTestCase do
 the clean up in the correct order.
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Jun 4, 2021
In elastic#73555 we updated ESRestTestCase so that it cleans 
up the searchable snapshots indices before cleaning up 
the snapshots and repositories. This test does not require 
to delete resources itself and can let ESRestTestCase do
 the clean up in the correct order.
tlrx added a commit that referenced this pull request Jun 4, 2021
In #73555 we updated ESRestTestCase so that it cleans 
up the searchable snapshots indices before cleaning up 
the snapshots and repositories. This test does not require 
to delete resources itself and can let ESRestTestCase do
 the clean up in the correct order.
tlrx added a commit that referenced this pull request Jun 4, 2021
In #73555 we updated ESRestTestCase so that it cleans 
up the searchable snapshots indices before cleaning up 
the snapshots and repositories. This test does not require 
to delete resources itself and can let ESRestTestCase do
 the clean up in the correct order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. >test Issues or PRs that are addressing/adding tests v7.13.2 v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants