Deprecation check for discovery configuration #36666
Conversation
Adds a check for missing discovery configuration, which is now required.
|
This might falsely give a warning if you run the check against a node that does not have |
|
Pinging @elastic/es-core-features |
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
I think we should check this slightly differently, but otherwise looks good.
|
|
||
| static DeprecationIssue discoveryConfigurationCheck(List<NodeInfo> nodeInfos, List<NodeStats> nodeStats) { | ||
| if (nodeInfos.size() == 1 | ||
| && nodeInfos.stream().anyMatch(nodeInfo -> "single-node".equals(nodeInfo.getSettings().get("discovery.type")))) { |
There was a problem hiding this comment.
Technically this check only applies if discovery.type is zen2:
Of course zen2 isn't a thing in 6.x so we can't check for that; I think we should check for usage of the default discovery type. We have already changed the default to zen2 in master and we hope to deprecate zen as a valid discovery type, which will I guess need another deprecation check.
FYI the comparison with single-node discovery applies to all bootstrap checks:
In practice I think we expect single-node and zen2 to be the only options here, rendering these two statements practically equivalent, but that's still to be confirmed and may change in future.
Also please could you use DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey() instead of the literal setting key. It makes it that bit easier to find usages of a setting in the IDE, and that bit less likely that we miss this usage in some future work.
| // This only checks for `ping.unicast.hosts` and `hosts_provider` because `cluster.initial_master_nodes` does not exist in 6.x | ||
| List<String> nodesFound = nodeInfos.stream() | ||
| .filter(nodeInfo -> nodeInfo.getSettings().hasValue("discovery.zen.ping.unicast.hosts") == false) | ||
| .filter(nodeInfo -> nodeInfo.getSettings().hasValue("discovery.zen.hosts_provider") == false) |
There was a problem hiding this comment.
Please could you use SettingsBasedHostsProvider.DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING.getKey() and DiscoveryModule.DISCOVERY_HOSTS_PROVIDER_SETTING.getKey() rather than the literal setting keys?
| Settings settings = Settings.builder() | ||
| .put("cluster.name", "elasticsearch") | ||
| .put("node.name", "node_check") | ||
| .put("discovery.type", "single-node") // Needed due to NodeDeprecationChecks#discoveryConfigurationCheck |
There was a problem hiding this comment.
Same request for setting keys rather than literals here please.
| null, null, null, null)); | ||
| Settings baseSettings = Settings.builder() | ||
| .put("cluster.name", "elasticsearch") | ||
| .put("node.name", "node_check") |
|
@DaveCTurner Thanks for the review! I've cleaned up the strings into setting references, and I think the check now matches what's actually required - it only checks nodes that do not have a discovery type explicitly set as otherwise Zen2 will not be used after upgrade. Please correct me if I misunderstood, though! |
|
@elasticmachine run gradle build tests 2 |
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM thanks @gwbrown
3 participants
Adds a check for missing discovery configuration, which is now
required.
Relates to #36024 and #36215