Infrastructure for assuming cluster features in the next major version by thecoop · Pull Request #118143 · elastic/elasticsearch

thecoop · 2024-12-06T11:58:04Z

Add support for node features to be labelled as 'assumed in the next major version', allowing them to be removed.

This is targetted at v9 and v8.18, as the feature removals need to be done on both versions to maintain continued compatibility. Once the infrastructure is merged and backported, the procedure for removing features is as follows:

Mark the feature as assumed on v9, and backport to v8.18
Wait for that commit to be fully deployed to serverless
Remove the feature from main and elide all checks for that feature

elasticsearchmachine · 2024-12-06T11:58:28Z

Hi @thecoop, I've created a changelog YAML for you.

thecoop · 2024-12-06T12:06:56Z

This uses DiscoveryNode.getVersion() extensively to get the major version of a node. As this is due to be removed, I've created #118255 to introduce a 'major version' concept to BuildVersion

elasticsearchmachine · 2024-12-09T11:52:54Z

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

elasticsearchmachine · 2024-12-09T11:52:54Z

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

server/src/main/java/org/elasticsearch/cluster/ClusterFeatures.java

thecoop · 2024-12-09T14:40:41Z

server/src/test/java/org/elasticsearch/cluster/coordination/NodeJoinExecutorTests.java

+    }
+
+    private static Version nextMajor() {
+        return Version.fromId((Version.CURRENT.major + 1) * 1_000_000 + 99);


Until the assert in VersionInformation constructor is removed, we need to continue to use Version here

thecoop · 2024-12-09T16:24:54Z

server/src/main/java/org/elasticsearch/features/NodeFeature.java

+ *              and so should be assumed to be true for all nodes after that boundary.
 */
-public record NodeFeature(String id) {
+public record NodeFeature(String id, boolean assumedAfterNextCompatibilityBoundary) {


Naming might need a bit of work

I struggle coming up with something better... assumePresenceAfterNextCompatibilityBoundary, but that's even harder to read.... assumedOnNextMajor maybe?

Though next major is not correct in the context of serverless...

Yeah, exactly, it was assumedOnNextMajor, but it's always true for serverless, so...

server/src/main/java/org/elasticsearch/cluster/ClusterFeatures.java

server/src/main/java/org/elasticsearch/cluster/coordination/NodeJoinExecutor.java

mosche

A few more nits / suggestions on naming, but LGTM 👍

mosche · 2024-12-12T15:37:37Z

server/src/main/java/org/elasticsearch/cluster/ClusterFeatures.java

+     * @see org.elasticsearch.env.BuildVersion#canRemoveAssumedFeatures
+     */
+    public static boolean featuresCanBeAssumedForNode(DiscoveryNode node) {
+        return node.getBuildVersion().canRemoveAssumedFeatures();


Stumbled over remove in canRemoveAssumedFeatures, that was confusing to me and I kept looking for where we remove those. Maybe turning it around makes it easier to understand: canAssumeRemovedFeatures

That's not quite right though, its not that all features that are missing can be assumed, it's that the features that this ES has marked as assumed can be taken as being met in a node with this version, regardless of whether they're published by the node or not.

Yes, you're right 👍
If we can find something better than remove, it'd be great, but certainly not worth delaying this further.
canInferAssumedFeatures, canExpectAssumedFeatures, canSkipAssumedFeatures
Or more targeting the versioning side of this: hasPassedNextCompatiblityBoundary or similar.

mosche · 2024-12-12T15:50:34Z

server/src/main/java/org/elasticsearch/features/NodeFeature.java

+ *              and so should be assumed to be true for all nodes after that boundary.
 */
-public record NodeFeature(String id) {
+public record NodeFeature(String id, boolean assumedAfterNextCompatibilityBoundary) {


I struggle coming up with something better... assumePresenceAfterNextCompatibilityBoundary, but that's even harder to read.... assumedOnNextMajor maybe?

thecoop · 2024-12-12T16:01:58Z

@elasticmachine rerun elasticsearch-ci/part-1

elasticsearchmachine · 2024-12-17T13:20:04Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 118143

elastic#118143) Allow features to be marked as 'assumed', allowing them to be removed in the next major version.

Backport #118143 to 8.18

Allow node features to be assumed in the next major version

49f359d

Update docs/changelog/118143.yaml

885e8de

thecoop force-pushed the cluster-feature-majors branch from 654b6de to 885e8de Compare December 6, 2024 12:01

thecoop added 3 commits December 6, 2024 12:52

Serverless can also assume features

efad7f3

Update forbidden apis

5544bd4

So we still need this check...

b604511

thecoop force-pushed the cluster-feature-majors branch from cee97fa to b604511 Compare December 6, 2024 14:49

thecoop added 3 commits December 6, 2024 17:21

Test fixes

0930466

Merge branch 'main' into cluster-feature-majors

733f2dd

Add tests

7a1dc02

thecoop marked this pull request as ready for review December 9, 2024 11:52

thecoop requested review from a team as code owners December 9, 2024 11:52

elasticsearchmachine added Team:Core/Infra Meta label for core/infra team Team:Distributed Coordination (obsolete) Meta label for Distributed Coordination team. Obsolete. Please do not use. labels Dec 9, 2024

thecoop mentioned this pull request Dec 9, 2024

Add a method to get the major version number from BuildVersion #118255

Closed

Test update

812c280

rjernst reviewed Dec 9, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/ClusterFeatures.java Outdated Show resolved Hide resolved

Use BuildVersion to determine if an assumed feature can be removed

3889d4e

thecoop force-pushed the cluster-feature-majors branch from 3becca3 to 3889d4e Compare December 9, 2024 14:31

thecoop commented Dec 9, 2024

View reviewed changes

splotless

86e0171

thecoop requested review from a team and rjernst December 9, 2024 16:06

thecoop commented Dec 9, 2024

View reviewed changes

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Dec 9, 2024

thecoop added 3 commits December 9, 2024 17:29

Update comments

a2c71a7

Fix javadoc

4b94b1a

Merge remote-tracking branch 'upstream/main' into cluster-feature-majors

0939c46

prdoyle reviewed Dec 11, 2024

View reviewed changes

thecoop added 2 commits December 12, 2024 15:36

PR comments

c84b2e6

Merge remote-tracking branch 'upstream/main' into cluster-feature-majors

1ea0d67

thecoop requested review from mosche and prdoyle December 12, 2024 15:50

mosche approved these changes Dec 12, 2024

View reviewed changes

thecoop added 2 commits December 13, 2024 09:51

Merge branch 'main' into cluster-feature-majors

7a12300

Merge branch 'main' into cluster-feature-majors

9d19288

prdoyle approved these changes Dec 16, 2024

View reviewed changes

thecoop merged commit f5712e4 into elastic:main Dec 17, 2024

thecoop deleted the cluster-feature-majors branch December 17, 2024 13:18

elasticsearchmachine added the backport pending label Dec 17, 2024

thecoop added a commit to thecoop/elasticsearch that referenced this pull request Dec 17, 2024

Infrastructure for assuming cluster features in the next major version (

bdb571e

elastic#118143) Allow features to be marked as 'assumed', allowing them to be removed in the next major version.

thecoop mentioned this pull request Dec 17, 2024

[8.18] Assumed cluster features infrastructure #118848

Merged

thecoop removed the backport pending label Dec 17, 2024

thecoop added a commit that referenced this pull request Dec 18, 2024

[8.18] Assumed cluster features infrastructure (#118143) (#118848)

51fd70d

Backport #118143 to 8.18

nielsbauman mentioned this pull request Dec 23, 2024

Unmute IngestCommonClientYamlTestSuiteIT #119188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infrastructure for assuming cluster features in the next major version#118143

Infrastructure for assuming cluster features in the next major version#118143
thecoop merged 18 commits intoelastic:mainfrom
thecoop:cluster-feature-majors

thecoop commented Dec 6, 2024 •

edited

Loading

elasticsearchmachine commented Dec 6, 2024

thecoop commented Dec 6, 2024 •

edited

Loading

elasticsearchmachine commented Dec 9, 2024

elasticsearchmachine commented Dec 9, 2024

Uh oh!

thecoop Dec 9, 2024

thecoop Dec 9, 2024

mosche Dec 12, 2024

mosche Dec 12, 2024

thecoop Dec 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mosche left a comment

mosche Dec 12, 2024

thecoop Dec 12, 2024 •

edited

Loading

mosche Dec 13, 2024

mosche Dec 12, 2024

thecoop commented Dec 12, 2024

elasticsearchmachine commented Dec 17, 2024

Labels

5 participants

Conversation

thecoop commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Dec 6, 2024

thecoop commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Dec 9, 2024

elasticsearchmachine commented Dec 9, 2024

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mosche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecoop Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecoop commented Dec 12, 2024

elasticsearchmachine commented Dec 17, 2024

💔 Backport failed

Labels

5 participants

thecoop commented Dec 6, 2024 •

edited

Loading

thecoop commented Dec 6, 2024 •

edited

Loading

thecoop Dec 12, 2024 •

edited

Loading