Add a cluster listener to fix missing cluster features after upgrade #110710

thecoop · 2024-07-10T12:15:05Z

The cluster listener spots the instances when the master has been upgraded to the most recent code version, along with non-master nodes, but some nodes are missing cluster features

Fixes #109254

thecoop · 2024-07-10T12:16:49Z

This doesn't have an integration test attached, as our existing upgrade tests don't have non-master nodes in them.

thecoop · 2024-07-10T12:18:12Z

server/src/main/java/org/elasticsearch/cluster/features/NodeFeaturesFixListener.java

+                .nodes()
+                .stream()
+                .map(DiscoveryNode::getId)
+                .filter(n -> nodeVersions.get(n).transportVersion().onOrAfter(TransportVersions.NODE_FEATURES_QUERY_ACTION))


This check is not actually reliable for upgrading from versions before 8.8.0, due to the transport version being inferred to start with. This would require this listener to run strictly after TransportVersionsFixupListener has completed, but I don't think it's possible to add a dependency in this way.

So our options here are:

Combine this and TransportVersionFixupListener into a complicated multi-step async listener thing that does the upgrade operations that are required in strict order (whilst dealing with failures, retries, etc)

Use Version instead of TransportVersion. This is actually ok, because this case will never be hit on Serverless (nodes will always have features), so the action that may or may not be there will never get called. This call will only happen on on-prem installs, where the Version is applicable.

Using Version (option 2.) seems ok to me as well in this case.
Alternatively we could listen for the fix implemented by NodeFeaturesFixListener listener in addition to make sure the fix has been applied.

elasticsearchmachine · 2024-07-11T09:21:46Z

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

elasticsearchmachine · 2024-07-11T09:21:47Z

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

…xup-listener

Update changelog

prdoyle

I'm afraid my review is fairly superficial.

This PR seems like a great one to bookmark for the next person who wants to add a transport action.

server/src/main/java/org/elasticsearch/cluster/features/NodeFeaturesFixupListener.java

ldematte

Looks good, I have a couple of questions

ldematte · 2024-07-13T06:20:54Z

.../java/org/elasticsearch/action/admin/cluster/node/features/TransportNodesFeaturesAction.java

+import java.util.List;
+
+@UpdateForV9
+// @UpdateForV10 // this can be removed in v10. It may be called by v8 nodes to v9 nodes.


Unrelated to this PR, but should we already create @UpdateForV10?

Probably - or just create it as part of the v9 migration

ldematte · 2024-07-13T06:30:24Z

server/src/main/java/org/elasticsearch/cluster/features/NodeFeaturesFixupListener.java

+            for (var c : context.taskContexts()) {
+                for (var e : c.getTask().results().entrySet()) {
+                    // double check there are still no features for the node
+                    if (existingFeatures.getOrDefault(e.getKey(), Set.of()).isEmpty()) {


Why is this needed?
Should not we merge the maps in any case?

This is more being extra-safe - in the case the node has left and re-joined in the meantime it will have the features in there already, which may be different to those that were queried before the node was shut down.

server/src/main/java/org/elasticsearch/cluster/features/NodeFeaturesFixupListener.java

…xup-listener

ldematte

LGTM

…lastic#110710) Non-master-eligible nodes that are already part of a cluster when the master is upgraded don't re-join the cluster, so their cluster features never get updated. This adds a cluster listener that spots this occurring, and manually gets the node's features with a new transport action and updates the cluster state after the fact.

elasticsearchmachine · 2024-07-16T13:37:09Z

💚 Backport successful

Status	Branch	Result
✅	8.15

…110710) (#110924) Non-master-eligible nodes that are already part of a cluster when the master is upgraded don't re-join the cluster, so their cluster features never get updated. This adds a cluster listener that spots this occurring, and manually gets the node's features with a new transport action and updates the cluster state after the fact.

…er upgrade (#115771) This PR modifies `TransportVersionsFixupListener` to include all of compatibility versions (not only TransportVersion) in the fixup. `TransportVersionsFixupListener` spots the instances when the master has been upgraded to the most recent code version, along with non-master nodes, but some nodes are missing a "proper" (non-inferred) Transport version. This PR adds another check to also ensure that we have real (non-empty) system index mapping versions. To do so, it modifies NodeInfo so it carries all of CompatibilityVersions (TransportVersion + SystemIndexDescriptor.MappingVersions). This was initially done via a separate fixup listener + ad-hoc transport action, but the 2 listeners "raced" to update ClusterState on the same CompatibilityVersions structure; it just made sense to do it at the same time. The fixup is very similar to #110710, which does the same for cluster features; plus, it adds a CI test to cover the bug raised in #112694 Closes #112694

thecoop added 3 commits July 9, 2024 15:47

Add a transport action to get the features of a node

2521628

Tweaks to the transport action

5355c90

Add a cluster listener to fix node features after an upgrade

df1be5d

thecoop added >bug WIP :Core/Infra/Core Core issues without another label labels Jul 10, 2024

thecoop requested a review from a team as a code owner July 10, 2024 12:15

thecoop mentioned this pull request Jul 10, 2024

Add a transport action to get the features of a node #110645

Closed

elasticsearchmachine added the v8.16.0 label Jul 10, 2024

thecoop commented Jul 10, 2024

View reviewed changes

thecoop added 2 commits July 10, 2024 14:12

Change to use Version rather than TransportVersion

54a7322

Add some unit tests

f935067

thecoop added auto-backport Automatically create backport pull requests when merged v8.15.0 and removed WIP labels Jul 11, 2024

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 11, 2024

Update docs/changelog/110710.yaml

bc39abd

thecoop added 3 commits July 11, 2024 10:22

Merge remote-tracking branch 'upstream/main' into cluster-features-fi…

bf3bb2a

…xup-listener

Don't need to export the transport type

cba58a5

Update changelog

Add to known transport actions list

61b14d5

prdoyle reviewed Jul 12, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/features/NodeFeaturesFixupListener.java Show resolved Hide resolved

server/src/main/java/org/elasticsearch/cluster/features/NodeFeaturesFixupListener.java Show resolved Hide resolved

ldematte reviewed Jul 13, 2024

View reviewed changes

Add test for overlapping operations

2fc134e

thecoop requested a review from a team July 15, 2024 09:22

Merge remote-tracking branch 'upstream/main' into cluster-features-fi…

551aa40

…xup-listener

ldematte approved these changes Jul 16, 2024

View reviewed changes

thecoop merged commit e64aab1 into elastic:main Jul 16, 2024
15 checks passed

thecoop deleted the cluster-features-fixup-listener branch July 16, 2024 13:33

thecoop mentioned this pull request Jul 16, 2024

[8.15] Add a cluster listener to fix missing cluster features after upgrade (#110710) #110924

Merged

lkts mentioned this pull request Aug 13, 2024

Fix references to logsdb index mode in release highlights lkts/elasticsearch#1

Closed

ldematte mentioned this pull request Oct 28, 2024

[8.x] Add a cluster listener to fix missing system index mappings after upgrade #115771

Merged

Add a cluster listener to fix missing cluster features after upgrade #110710

Add a cluster listener to fix missing cluster features after upgrade #110710

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

thecoop Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosche Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jul 11, 2024

Uh oh!

elasticsearchmachine commented Jul 11, 2024

Uh oh!

prdoyle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

ldematte Jul 13, 2024

Choose a reason for hiding this comment

Uh oh!

thecoop Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

ldematte Jul 13, 2024

Choose a reason for hiding this comment

Uh oh!

thecoop Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 16, 2024

💚 Backport successful

Uh oh!

Uh oh!

thecoop commented Jul 10, 2024 •

edited

Loading

thecoop commented Jul 10, 2024 •

edited

Loading

thecoop Jul 10, 2024 •

edited

Loading