Remove rolling restart checks for x-pack #93843
Conversation
|
@mark-vieira - Can you confirm my rationale is correct w.r.t. rolling restarts to 8.x will always come from a distribution that has x-pack installed ? @DaveCTurner - Can you check the removal ? It was too old to revert the commit so I just removed it manually. It was straight forward without any surprises. |
I believe this is a sound assumption. The only deviation from this would be the integ-test distribution but that does have xpack core and security installed so it may depend on what we mean when we say "x-pack". I think currently our rolling restart tests use the default distribution, but it'd be nice to be able to change that. |
elasticsearchmachine
added
the
needs:triage
jakelandis
added
:Distributed/Distributed
|
Pinging @elastic/es-distributed (Team:Distributed) |
| public Settings additionalSettings() { | ||
| final String xpackInstalledNodeAttrSetting = "node.attr." + XPACK_INSTALLED_NODE_ATTR; | ||
|
|
||
| if (settings.get(xpackInstalledNodeAttrSetting) != null) { | ||
| throw new IllegalArgumentException("Directly setting [" + xpackInstalledNodeAttrSetting + "] is not permitted"); | ||
| } | ||
| return Settings.builder().put(super.additionalSettings()).put(xpackInstalledNodeAttrSetting, "true").build(); | ||
| } |
There was a problem hiding this comment.
I think we still need this node attribute because we will sometimes be forming clusters with an older master node, and older versions still require this attribute.
There was a problem hiding this comment.
What is the scenario when an older master node (say 7.1) be running when trying to form/keep a cluster for new data nodes (say 8.7) ? Wouldn't that effectively be a rolling upgrade from from 7.1 OSS -> 8.7 which is not supported.
If there was some scenerio where you had a 7.1 master running with 8.7 nodes trying to join it would only be an issue for 7.1 OSS since if you had x-pack installed in 7.1 these checks are short circuited. I'm not sure exactly how the issue would manifest itself for a 7.1 OSS master but I don't think it would could brick a cluster state and would expect at worst some x-pack features to not work until you upgrade your master. If you have a specific scenario in mind I can execute a manual test.
There was a problem hiding this comment.
Yes 7.1/8.7 mixed clusters are not valid, but AIUI even 8.6 masters will require this attribute.
There was a problem hiding this comment.
Sorry if I am dense, but I still don't see how it is required. When a cluster is running in 8.6 it will install the x-pack bits (mostly likely the license bits) such that the node attribute check is not needed. So when you are on a non-mixed cluster with the default distribution (post 7.12), the attribute is never read again (for this purpose). However, if it is read it will (correctly) return as missing/false. For cluster formation and other apis (like _cat/nodeattrs), it is fine since we expect node attributes to come and go (unlike fixed settings). I think the only scenario is an 8.6 running a custom distribution that only includes the OSS build and then they are trying to upgrade our default distribution. (which is isn't supported and not catastrophic)
I did a manual rolling restart test and all went well and our other upgrade tests should also any catch issues (I hope).
There was a problem hiding this comment.
If you form a cluster with an 8.6.2 master and an 8.8.0-SNAPSHOT node running from this branch then various APIs do not work:
$ curl --silent -XPOST 'http://localhost:9200/_license/start_basic?pretty&error_trace'
{
"error" : {
"root_cause" : [
{
"type" : "illegal_state_exception",
"reason" : "The following nodes are not ready yet for enabling x-pack custom metadata: [{node-0}{RKegf5WzSniZIq_C4JfvSA}{ofwnUim4Q7mt2xo_qGDFtg}{node-0}{127.0.0.1}{127.0.0.1:9301}{dilmrt}{ml.allocated_processors=16, ml.allocated_processors_double=16.0, ml.machine_memory=68719476736, ml.max_jvm_size=1073741824}]",
"stack_trace" : "org.elasticsearch.ElasticsearchException$1: The following nodes are not ready yet for enabling x-pack custom metadata: [{node-0}{RKegf5WzSniZIq_C4JfvSA}{ofwnUim4Q7mt2xo_qGDFtg}{node-0}{127.0.0.1}{127.0.0.1:9301}{dilmrt}{ml.allocated_processors=16, ml.allocated_processors_double=16.0, ml.machine_memory=68719476736, ml.max_jvm_size=1073741824}]\n\tat [email protected]/org.elasticsearch.ElasticsearchException.guessRootCauses(ElasticsearchException.java:640)\n\tat [email protected]/org.elasticsearch.ElasticsearchException.generateFailureXContent(ElasticsearchException.java:568)\n\tat [email protected]/org.elasticsearch.rest.RestResponse.build(RestResponse.java:175)\n\tat [email protected]/org.elasticsearch.rest.RestResponse.<init>(RestResponse.java:123)\n\tat [email protected]/org.elasticsearch.rest.RestResponse.<init>(RestResponse.java:102)\n\tat [email protected]/org.elasticsearch.rest.action.RestActionListener.onFailure(RestActionListener.java:55)\n\tat [email protected]/org.elasticsearch.client.internal.node.NodeClient$SafelyWrappedActionListener.onFailure(NodeClient.java:170)\n\tat [email protected]/org.elasticsearch.tasks.TaskManager$1.onFailure(TaskManager.java:217)\n\tat [email protected]/org.elasticsearch.action.support.master.TransportMasterNodeAction$AsyncSingleAction.lambda$doStart$2(TransportMasterNodeAction.java:231)\n\tat [email protected]/org.elasticsearch.action.ActionListener$DelegatingActionListener.onFailure(ActionListener.java:218)\n\tat [email protected]/org.elasticsearch.license.StartBasicClusterTask.onFailure(StartBasicClusterTask.java:97)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService$Batcher$UpdateTask.onFailure(MasterService.java:199)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService.runTasks(MasterService.java:283)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService$Batcher.run(MasterService.java:170)\n\tat [email protected]/org.elasticsearch.cluster.service.TaskBatcher.runIfNotProcessed(TaskBatcher.java:110)\n\tat [email protected]/org.elasticsearch.cluster.service.TaskBatcher$BatchedTask.run(TaskBatcher.java:148)\n\tat [email protected]/org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:850)\n\tat [email protected]/org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedEsThreadPoolExecutor.java:257)\n\tat [email protected]/org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedEsThreadPoolExecutor.java:223)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)\n\tat java.base/java.lang.Thread.run(Thread.java:1589)\nCaused by: java.lang.IllegalStateException: The following nodes are not ready yet for enabling x-pack custom metadata: [{node-0}{RKegf5WzSniZIq_C4JfvSA}{ofwnUim4Q7mt2xo_qGDFtg}{node-0}{127.0.0.1}{127.0.0.1:9301}{dilmrt}{ml.allocated_processors=16, ml.allocated_processors_double=16.0, ml.machine_memory=68719476736, ml.max_jvm_size=1073741824}]\n\tat [email protected]/org.elasticsearch.xpack.core.XPackPlugin.checkReadyForXPackCustomMetadata(XPackPlugin.java:246)\n\tat [email protected]/org.elasticsearch.license.StartBasicClusterTask$Executor.execute(StartBasicClusterTask.java:127)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService.innerExecuteTasks(MasterService.java:1052)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService.executeTasks(MasterService.java:1017)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService.runTasks(MasterService.java:278)\n\t... 9 more\n"
}
],
"type" : "illegal_state_exception",
"reason" : "The following nodes are not ready yet for enabling x-pack custom metadata: [{node-0}{RKegf5WzSniZIq_C4JfvSA}{ofwnUim4Q7mt2xo_qGDFtg}{node-0}{127.0.0.1}{127.0.0.1:9301}{dilmrt}{ml.allocated_processors=16, ml.allocated_processors_double=16.0, ml.machine_memory=68719476736, ml.max_jvm_size=1073741824}]",
"stack_trace" : "java.lang.IllegalStateException: The following nodes are not ready yet for enabling x-pack custom metadata: [{node-0}{RKegf5WzSniZIq_C4JfvSA}{ofwnUim4Q7mt2xo_qGDFtg}{node-0}{127.0.0.1}{127.0.0.1:9301}{dilmrt}{ml.allocated_processors=16, ml.allocated_processors_double=16.0, ml.machine_memory=68719476736, ml.max_jvm_size=1073741824}]\n\tat [email protected]/org.elasticsearch.xpack.core.XPackPlugin.checkReadyForXPackCustomMetadata(XPackPlugin.java:246)\n\tat [email protected]/org.elasticsearch.license.StartBasicClusterTask$Executor.execute(StartBasicClusterTask.java:127)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService.innerExecuteTasks(MasterService.java:1052)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService.executeTasks(MasterService.java:1017)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService.runTasks(MasterService.java:278)\n\tat [email protected]/org.elasticsearch.cluster.service.MasterService$Batcher.run(MasterService.java:170)\n\tat [email protected]/org.elasticsearch.cluster.service.TaskBatcher.runIfNotProcessed(TaskBatcher.java:110)\n\tat [email protected]/org.elasticsearch.cluster.service.TaskBatcher$BatchedTask.run(TaskBatcher.java:148)\n\tat [email protected]/org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:850)\n\tat [email protected]/org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedEsThreadPoolExecutor.java:257)\n\tat [email protected]/org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedEsThreadPoolExecutor.java:223)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)\n\tat java.base/java.lang.Thread.run(Thread.java:1589)\n"
},
"status" : 500
}
There was a problem hiding this comment.
OK I see now ... the scenario is a brand new cluster but with an elder version for the master node. I didn't realize that was supported (or at least not-not-supported). I only tested forming a cluster with non-mixed versions then doing a rolling upgrade holding the master on the elder version (which works fine). That works because when the non-mixed cluster is started the self license gets installed along with the bits needed to short curcuit the check which avoids reading the attribute entirely for subsequent checks. For this scenario (new mixed cluster with elder master) the self license can't be installed due the lack of the attribute and subsequent checks are not short circuited. I will close out this PR ... the goal was just some tech debt clean up and don't want to introduce any risk.
There was a problem hiding this comment.
I think this PR is otherwise good, we just need to keep this attribute around until the next major. That would be true no matter when we make this change, so IMO we may as well do this now.
There was a problem hiding this comment.
gotcha ... I will re-open and adjust the PR to leave (only) the attribute behind so that for the scenario above the elder master can pass the checks.
There was a problem hiding this comment.
the changes here conflict with some other refactoring I am working on and I will come back to this PR once those other refactorings have landed. (it wont cause any functional changes just merge conflicts)
|
|
This commit removes the checks introduced in #30743.
Those checks were to help enable rolling restarts from the
OSS distribution to the (now) default distribution that includes
x-pack by default. These checks are no longer needed since
we no longer ship an OSS version #68797
that is the base (lowest) version during a rolling restart.
Any rolling restart to 8.x requires 7.17+ but the last official
OSS distribution that did not include x-pack was 7.11. This
change, when applied to 8.x, means that all supported rolling
restarts to 8.x will already have x-pack metadata and thus no
need for these checks.