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

Skip HealthNodeUpgradeIT for some rolling upgrades #119698

Merged

Conversation

PeteGillinElastic
Copy link
Member

This excludes the HealthNodeUpgradeIT test for the rolling upgrade tests which use a cluster with a mix of either 8.5.3 or 8.6.2 nodes, which serve the health endpoint at _internal/_health, and 8.last nodes, which serve it at _health_report. There is no sensible and reliable way to test the endpoint in such clusters.

Closes #118157
Closes #118158

This excludes the `HealthNodeUpgradeIT` test for the rolling upgrade
tests which use a cluster with a mix of either 8.5.3 or 8.6.2 nodes,
which serve the health endpoint at `_internal/_health`, and 8.last
nodes, which serve it at `_health_report`. There is no sensible and
reliable way to test the endpoint in such clusters.

Closes elastic#118157
Closes elastic#118158
@PeteGillinElastic PeteGillinElastic marked this pull request as ready for review January 8, 2025 09:21
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jan 8, 2025
@PeteGillinElastic PeteGillinElastic added :Data Management/Health >test Issues or PRs that are addressing/adding tests and removed needs:triage Requires assignment of a team area label labels Jan 8, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Jan 8, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@PeteGillinElastic
Copy link
Member Author

N.B. Before this PR, the test appears to have been muted entirely. After this PR, it is muted only for these two BWC rolling upgrade versions. So this does add some missing coverage.

@gmarouli
Copy link
Contributor

gmarouli commented Jan 8, 2025

I was looking into the test:

if (clusterHasFeature("health.supports_health")) {
            assertBusy(() -> {
                Response response = client().performRequest(new Request("GET", "_cat/tasks"));
                String tasks = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8);
                assertThat(tasks, Matchers.containsString("health-node"));
            });
            assertBusy(() -> {
                String path = clusterHasFeature("health.supports_health_report_api") ? "_health_report" : "_internal/_health";
                Response response = client().performRequest(new Request("GET", path));
                Map<String, Object> health_report = entityAsMap(response.getEntity());
                assertThat(health_report.get("status"), equalTo("green"));
            });
        }

And I am wondering, instead of adding these explicit mutes, shouldn't we just use clusterHasFeature("health.supports_health_report_api)" in the if statement?

@gmarouli gmarouli self-requested a review January 8, 2025 10:27
@PeteGillinElastic
Copy link
Member Author

I was looking into the test:

if (clusterHasFeature("health.supports_health")) {
            assertBusy(() -> {
                Response response = client().performRequest(new Request("GET", "_cat/tasks"));
                String tasks = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8);
                assertThat(tasks, Matchers.containsString("health-node"));
            });
            assertBusy(() -> {
                String path = clusterHasFeature("health.supports_health_report_api") ? "_health_report" : "_internal/_health";
                Response response = client().performRequest(new Request("GET", path));
                Map<String, Object> health_report = entityAsMap(response.getEntity());
                assertThat(health_report.get("status"), equalTo("green"));
            });
        }

And I am wondering, instead of adding these explicit mutes, shouldn't we just use clusterHasFeature("health.supports_health_report_api)" in the if statement?

Yeah, I think that should work. (It actually crossed my mind, I don't remember why I went this route.) I'll give it a go.

@PeteGillinElastic
Copy link
Member Author

Yeah, that works, and avoids hard-coding the version numbers in question. I've verified that the assertion does run and passes for :qa:rolling-upgrade:v8.7.0#bwcTest. PTAL.

Copy link
Contributor

@gmarouli gmarouli left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this!

@PeteGillinElastic PeteGillinElastic enabled auto-merge (squash) January 8, 2025 12:44
@PeteGillinElastic PeteGillinElastic merged commit c124f1b into elastic:8.x Jan 8, 2025
15 checks passed
@PeteGillinElastic PeteGillinElastic deleted the health-api-bwc-test-failures branch January 8, 2025 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Health Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v8.18.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants