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.

Sign up for GitHub

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

Try to stabilise tests by checking for cluster health early #7184

Merged
merged 6 commits into from
Sep 27, 2023
Merged

Try to stabilise tests by checking for cluster health early #7184

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c7325f
Try to stabilise tests by checking for cluster health early
pebrc Sep 26, 2023
93c28e7
Use existing test step
pebrc Sep 27, 2023
7aee85e
keep health check in k8s checks for upgrades
pebrc Sep 27, 2023
adae648
Do not check health on creation when breakage is expected
pebrc Sep 27, 2023
2647d5c
debugging test failure
pebrc Sep 27, 2023
a2477cc
Rename builder func to reflect what it does
pebrc Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions test/e2e/test/elasticsearch/steps_creation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ func (b Builder) CreationTestSteps(k *test.K8sClient) test.StepList {
if b.Elasticsearch.Spec.Version != createdEs.Spec.Version {
return fmt.Errorf("expected version %s but got %s", b.Elasticsearch.Spec.Version, createdEs.Spec.Version)
}
// TODO this is incomplete
return nil
// check cluster health here already to make sure any dependent builders can rely on a ready ES
// this is special casing Elasticsearch a little bit in the interest of test stability see https://github.com/elastic/cloud-on-k8s/issues/7172
return clusterHealthIs(b, k, esv1.ElasticsearchGreenHealth)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we already have a dedicated step, no real advantage compared to your proposal except that we would have a dedicated entry in the logs:

=== RUN   TestAgentConfigRef/Elasticsearch_cluster_should_be_created
Retries (5m0s timeout): .
    --- PASS: TestAgentConfigRef/Elasticsearch_cluster_should_be_created (0.57s)
=== RUN   TestAgentConfigRef/ES_cluster_health_should_eventually_be_green
Retries (5m0s timeout): ......
    --- PASS: TestAgentConfigRef/ES_cluster_health_should_eventually_be_green (41.40s)
=== RUN   TestAgentConfigRef/Creating_an_Agent_should_succeed
    --- PASS: TestAgentConfigRef/Creating_an_Agent_should_succeed (1.48s)
Suggested change
// check cluster health here already to make sure any dependent builders can rely on a ready ES
// this is special casing Elasticsearch a little bit in the interest of test stability see https://github.com/elastic/cloud-on-k8s/issues/7172
return clusterHealthIs(b, k, esv1.ElasticsearchGreenHealth)
return nil
}),
},
// check cluster health here already to make sure any dependent builders can rely on a ready ES
// this is special casing Elasticsearch a little bit in the interest of test stability see https://github.com/elastic/cloud-on-k8s/issues/7172
CheckClusterHealth(b, k),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think my motivation was to have this check in the create steps. While the existing check runs in the check steps. By having it in the create steps I want to introduce a delay so that the Agent creation (and other resource creations) does not run before ES is ready. I could remove the dedicated step from the check steps to avoid the duplication.

Copy link
Collaborator Author

@pebrc pebrc Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I misunderstood your proposal! I think that makes sense actually.

}),
},
})
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/test/helper/yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ func (yd *YAMLDecoder) ToBuilders(reader *bufio.Reader, transform BuilderTransfo
builders = append(builders, builder)
}

// sort to make sure the ES builders come first to minimise the chance of test failures as in https://github.com/elastic/cloud-on-k8s/issues/7172
sortBuilders(builders)
return builders, nil
}

Expand Down