Skip to content

YAML test framework: simplify version node_selector#103954

Merged
ldematte merged 9 commits intoelastic:mainfrom
ldematte:limited-node-selector
Mar 5, 2024
Merged

YAML test framework: simplify version node_selector#103954
ldematte merged 9 commits intoelastic:mainfrom
ldematte:limited-node-selector

Conversation

@ldematte
Copy link
Copy Markdown
Contributor

@ldematte ldematte commented Jan 5, 2024

Remove ranges and allow just current/original.

Node selectors with version ranges are used today just for mixed cluster/rolling upgrade tests, to filter out (or select) the new upgraded nodes that exhibit a change behaviour.
Therefore it is not needed to specify explicitly a version range: selecting nodes that have been upgraded (current) or not (original) is sufficient.

Today just current exists; this PR adds original (with one example usage) and removes the ability to select nodes based on version.

(Follows #103889, where we removed all version-based node_selectors but this one)

@ldematte ldematte added >test Issues or PRs that are addressing/adding tests :Core/Infra/Core Core issues without another label labels Jan 5, 2024
@ldematte ldematte requested a review from thecoop January 5, 2024 08:36
@mosche
Copy link
Copy Markdown
Contributor

mosche commented Jan 8, 2024

This makes sense to me @ldematte 👍 But please gather some more opinions :)
As a suggestion, how about using previous instead of the negated non_current?

@ldematte
Copy link
Copy Markdown
Contributor Author

ldematte commented Jan 8, 2024

Naming is hard :)
In code, we current have "original" and "old" to describe the "not current" cluster. "Previous" can be confused with some -1 version (previous minor like 8.12 or previous major like 7). Maybe if we want to avoid the negation, original is a better choice.

@ldematte ldematte marked this pull request as ready for review January 29, 2024 08:48
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 29, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@ldematte
Copy link
Copy Markdown
Contributor Author

As a suggestion, how about using previous instead of the negated non_current?

I will change that to original after merging #103889 - it makes more sense in the context of upgraded/mixed clusters. Wdyt @mosche?

@mosche
Copy link
Copy Markdown
Contributor

mosche commented Jan 31, 2024

Is this only for mixed cluster tests, @ldematte? Initially I was struggling to bridge the gap between original and current, but the more I think about it the better I like it 👍
Even better would be something like original (or also initial) and upgraded instead of current. That way both terms are not about a specific version, but more about the state in terms of upgrading.

@ldematte
Copy link
Copy Markdown
Contributor Author

AFAIK, it has a meaning only in mixed cluster and rolling upgrade tests. I like the change to upgraded too - will have to be careful with that/do it in 2 phases as I think it is used in (and was introduced for) serverless.

Copy link
Copy Markdown
Contributor

@williamrandolph williamrandolph left a comment

Choose a reason for hiding this comment

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

Just one question, but LGTM otherwise.

@ldematte ldematte added test-update-serverless test-full-bwc Trigger full BWC version matrix tests labels Feb 22, 2024
@ldematte
Copy link
Copy Markdown
Contributor Author

I had to re-introduce the version matching for restCompat tests. Those test are a bit peculiar: they checkout elastic/7.17 to get the yaml files/specs from that branch, but run elastic/main test code.
So I added a check to see if we are in a restCompat tests, and only if we are, we do go down the legacy parser that support versions. This would prevent new usages of version-based selectors to sneak in, and also made removal once we switch to 9 (I have decorated the relevant methods with @UpdateForV9)

@ldematte
Copy link
Copy Markdown
Contributor Author

Serverless tests fail for an unrelated, known issue: https://github.com/elastic/elasticsearch-serverless/issues/1461

@ldematte ldematte requested a review from williamrandolph March 4, 2024 07:39
Copy link
Copy Markdown
Contributor

@williamrandolph williamrandolph left a comment

Choose a reason for hiding this comment

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

LGTM!

@ldematte ldematte merged commit 1c03489 into elastic:main Mar 5, 2024
@ldematte ldematte deleted the limited-node-selector branch March 5, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests test-full-bwc Trigger full BWC version matrix tests test-update-serverless v8.14.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants