Skip to content

Conversation

@bleskes
Copy link
Contributor

@bleskes bleskes commented Jun 29, 2016

Node IDs are currently randomly generated during node startup. That means they change every time the node is restarted. While this doesn't matter for ES proper, it makes it hard for external services to track nodes. Another, more minor, side effect is that indexing the output of, say, the node stats API results in creating new fields due to node ID being used as keys.

The first approach I considered was to use the node's published address as the base for the id. We already treat nodes with the same address as the same so this is a simple change (see here). While this is simple and it works for probably most cases, it is not perfect. For example, if after a node restart, the node is not able to bind to the same port (because it's not yet freed by the OS), it will cause the node to still change identity. Also in environments where the host IP can change due to a host restart, identity will not be the same.

Due to those limitation, I opted to go with a different approach where the node id will be persisted in the node's data folder. This has the upside of connecting the id to the nodes data. It also means that the host can be adapted in any way (replace network cards, attach storage to a new VM). I

It does however also have downsides - we now run the risk of two nodes having the same id, if someone copies clones a data folder from one node to another. To mitigate this I changed the semantics of the protection against multiple nodes with the same address to be stricter - it will now reject the incoming join if a node exists with the same id but a different address. Note that if the existing node doesn't respond to pings (i.e., it's not alive) it will be removed and the new node will be accepted when it tries another join.

Last, and most importantly, this change requires that all nodes persist data to disk. This is a change from current behavior where only data & master nodes store local files. This is the main reason for marking this PR as breaking.

Other less important notes:

  • DummyTransportAddress is removed as we need a unique network address per node. Use LocalTransportAddress.buildUnique() instead.
  • I renamed node.add_lid_to_custom_path to node.add_lock_id_to_custom_path to avoid confusion with the node ID which is now part of the NodeEnvironment logic.
  • I removed the version paramater from MetaDataStateFormat#write , it wasn't really used and was just in the way :)
  • TribeNodes are special in the sense that they do start multiple sub-nodes (previously known as client nodes). Those sub-nodes do not store local files but derive their ID from the parent node id, so they are generated consistently.

This PR supersedes #17811, and changes it by adding an ephimeralID field to DiscoveryNode that maintains the current id semantics, i.e., it changes with each node restart. This allows to keep the same semantics and use it for node equality.

*
* @param node of the node which existence should be verified
* @return <code>true</code> if the node exists. Otherwise <code>false</code>
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I see two usages of nodeExists(String nodeId) that could use this one instead. (LocalDiscovery and IndicesClusterStateServiceRandomUpdatesTests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catches. fixed.

@ywelsch
Copy link
Contributor

ywelsch commented Jun 30, 2016

@bleskes Left some comments on the PR but looks good overall. Can you add documentation as well? (breaking change). I'm ok if you want to address tribe nodes as well in this PR (I don't think it's too big of a change, see #17987).

@ywelsch ywelsch mentioned this pull request Jun 30, 2016
@bleskes
Copy link
Contributor Author

bleskes commented Jul 3, 2016

@ywelsch Thanks. I pushed a commit addressing your comments. I'm not sure about the tribe nodes - will reach out to discuss more.

@bleskes
Copy link
Contributor Author

bleskes commented Jul 4, 2016

@ywelsch I pushed another commit with the tribe node change we discussed.

@ywelsch
Copy link
Contributor

ywelsch commented Jul 4, 2016

LGTM. Can you also add something to the migration docs? (see e.g. https://github.com/elastic/elasticsearch/pull/17987/files#diff-2d50fb3821a6a54aedf97e29135d711aR13 )

@bleskes bleskes merged commit 6861d35 into elastic:master Jul 4, 2016
@bleskes bleskes deleted the node_persistent_id branch July 4, 2016 19:09
@bleskes bleskes mentioned this pull request Jul 15, 2016
bleskes added a commit that referenced this pull request Jul 23, 2016
With #19140 we started persisting the node ID across node restarts. Now that we have a "stable" anchor, we can use it to generate a stable default node name and make it easier to track nodes over a restarts. Sadly, this means we will not have those random fun Marvel characters but we feel this is the right tradeoff.

On the implementation side, this requires a bit of juggling because we now need to read the node id from disk before we can log as the node node is part of each log message. The PR move the initialization of NodeEnvironment as high up in the starting sequence as possible, with only one logging message before it to indicate we are initializing. Things look now like this:

```
[2016-07-15 19:38:39,742][INFO ][node                     ] [_unset_] initializing ...
[2016-07-15 19:38:39,826][INFO ][node                     ] [aAmiW40] node name set to [aAmiW40] by default. set the [node.name] settings to change it
[2016-07-15 19:38:39,829][INFO ][env                      ] [aAmiW40] using [1] data paths, mounts [[ /(/dev/disk1)]], net usable_space [5.5gb], net total_space [232.6gb], spins? [unknown], types [hfs]
[2016-07-15 19:38:39,830][INFO ][env                      ] [aAmiW40] heap size [1.9gb], compressed ordinary object pointers [true]
[2016-07-15 19:38:39,837][INFO ][node                     ] [aAmiW40] version[5.0.0-alpha5-SNAPSHOT], pid[46048], build[473d3c0/2016-07-15T17:38:06.771Z], OS[Mac OS X/10.11.5/x86_64], JVM[Oracle Corporation/Java HotSpot(TM) 64-Bit Server VM/1.8.0_51/25.51-b03]
[2016-07-15 19:38:40,980][INFO ][plugins                  ] [aAmiW40] modules [percolator, lang-mustache, lang-painless, reindex, aggs-matrix-stats, lang-expression, ingest-common, lang-groovy, transport-netty], plugins []
[2016-07-15 19:38:43,218][INFO ][node                     ] [aAmiW40] initialized
```

Needless to say, settings `node.name` explicitly still works as before.

The commit also contains some clean ups to the relationship between Environment, Settings and Plugins. The previous code suggested the path related settings could be changed after the initial Environment was changed. This did not have any effect as the security manager already locked things down.
bleskes added a commit that referenced this pull request Aug 5, 2016
…ster state (#19743)

When we introduces [persistent node ids](#19140) we were concerned that people may copy data folders from one to another resulting in two nodes competing for the same id in the cluster. To solve this we elected to not allow an incoming join if a different with same id already exists in the cluster, or if some other node already has the same transport address as the incoming join. The rationeel there was that it is better to prefer existing nodes and that we can rely on node fault detection to remove any node from the cluster that isn't correct any more, making room for the node that wants to join (and will keep trying).

Sadly there were two problems with this:
1) One minor and easy to fix - we didn't allow for the case where the existing node can have the same network address as the incoming one, but have a different ephemeral id (after node restart). This confused the logic in `AllocationService`, in this rare cases. The cluster is good enough to detect this and recover later on, but it's not clean.
2) The assumption that Node Fault Detection will clean up is *wrong* when the node just won an election (it wasn't master before) and needs to process the incoming joins in order to commit the cluster state and assume it's mastership. In those cases, the Node Fault Detection isn't active. 

This PR fixes these two and prefers incoming nodes to existing node when finishing an election. 
On top of the, on request by @ywelsch , `AllocationService` synchronization between the nodes of the cluster and it's routing table is now explicit rather than something we do all the time. The same goes for promotion of replicas to primaries.
@clintongormley clintongormley added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. and removed :Cluster labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>breaking :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. >enhancement v5.0.0-alpha5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants