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

Convert flaky yml tests to EsRestTestCases #63634

Merged
merged 8 commits into from
Oct 27, 2020

Conversation

andreidan
Copy link
Contributor

We've had several failures in the yml ILM tests because ILM doesn't run in a deterministic way
and we can't wait for it to run in the yml test. The latest failure, field [indices.another_index.phase] is null,
is another symptom.

This PR converts the ILM yml tests that rely on ILM having run (eg. successful explain api
output tests for managed indices, valid and invalid move to step api tests that specify the
correct current step) into ESRestTestCase tests that make use of the assertBusy primitive.

Resolves #47275
Relates to #53488

This use case is covered in the `TimeseriesLifecycleActionIT` in several
tests like: `testMoveToAllocateStep` or `testMoveToRolloverStep`
The yml "Test Invalid Move To Step With Invalid Next Step" worked based on
assuming the current step is a particular one. As we can't control the
timing of ILM and we can't busy assert in yml test, this converts the
test to a java test and makes use of `assertBusy`
This converts the explain lifecycle yml tests that depende on ILM having run
at least once to a java integration test that makes use of `assertBusy`.
@andreidan andreidan added >test Issues or PRs that are addressing/adding tests :Data Management/ILM+SLM Index and Snapshot lifecycle management v8.0.0 v7.10.1 labels Oct 13, 2020
@andreidan andreidan requested a review from dakrone October 13, 2020 17:16
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/ILM+SLM)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Oct 13, 2020
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, I left some really minor comments, thanks!

@@ -228,6 +242,15 @@ public static void createIndexWithSettings(RestClient client, String index, Stri
ensureGreen(index);
}

public static void createIndexWithSettingsNoAlias(RestClient client, String index, Settings.Builder settings) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of adding a helper (it's also strange that it's called "NoAlias" since the default should always be no alias), we should change the other createIndexWithSettings to make the alias @Nullable?

Copy link
Contributor Author

@andreidan andreidan Oct 14, 2020

Choose a reason for hiding this comment

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

I've renamed the method createIndexWithSettings (c399088) with signature:

    public static void createIndexWithSettings(RestClient client, String index, Settings.Builder settings) throws IOException {

I think piggybacking this use case on the helper that has the alias and useWriteIndex parameters by making both @Nullable over complicates the logic (and we'd end up passing 2 null parameters).

What do you think?

public static void createIndexWithSettingsNoAlias(RestClient client, String index, Settings.Builder settings) throws IOException {
Request request = new Request("PUT", "/" + index);
request.setJsonEntity("{\n \"settings\": " + Strings.toString(settings.build())
+ "}");
Copy link
Member

Choose a reason for hiding this comment

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

Super minor, but probably no need for a newline here :)

assertManagedIndex(explain.get(secondIndex));
assertUnmanagedIndex(explain.get(unmanagedIndex));

Map<String, Object> explainIndexWithMissingPolicy = explain.get(indexWithMissingPolicy);
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 we could probably put this one is a second assertBusy so that we don't end up doing two explains when we only need to do one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's only one explain API call in this assertBusy. I believe it's ok to leave it in one assertBusy call.

@andreidan
Copy link
Contributor Author

@elasticmachine update branch

@andreidan andreidan merged commit 6afd042 into elastic:master Oct 27, 2020
@andreidan andreidan added v7.11.0 and removed v7.10.1 labels Oct 27, 2020
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Oct 27, 2020
The yml "Test Invalid Move To Step With Invalid Next Step" worked based on
assuming the current step is a particular one. As we can't control the
timing of ILM and we can't busy assert in yml test, this converts the
test to a java test and makes use of `assertBusy`

This converts the explain lifecycle yml tests that depende on ILM having run
at least once to a java integration test that makes use of `assertBusy`.

(cherry picked from commit 6afd042)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
andreidan added a commit that referenced this pull request Oct 27, 2020
The yml "Test Invalid Move To Step With Invalid Next Step" worked based on
assuming the current step is a particular one. As we can't control the
timing of ILM and we can't busy assert in yml test, this converts the
test to a java test and makes use of `assertBusy`

This converts the explain lifecycle yml tests that depende on ILM having run
at least once to a java integration test that makes use of `assertBusy`.

(cherry picked from commit 6afd042)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Nov 4, 2020
The yml "Test Invalid Move To Step With Invalid Next Step" worked based on
assuming the current step is a particular one. As we can't control the
timing of ILM and we can't busy assert in yml test, this converts the
test to a java test and makes use of `assertBusy`

This converts the explain lifecycle yml tests that depende on ILM having run
at least once to a java integration test that makes use of `assertBusy`.

(cherry picked from commit 6afd042)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
andreidan added a commit that referenced this pull request Nov 4, 2020
The yml "Test Invalid Move To Step With Invalid Next Step" worked based on
assuming the current step is a particular one. As we can't control the
timing of ILM and we can't busy assert in yml test, this converts the
test to a java test and makes use of `assertBusy`

This converts the explain lifecycle yml tests that depende on ILM having run
at least once to a java integration test that makes use of `assertBusy`.

(cherry picked from commit 6afd042)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] IndexLifecycleRestIT {yaml=ilm/40_explain_lifecycle/Test All Indexes Lifecycle Explain} failure
4 participants