Skip to content

Remove rolling restart checks for x-pack #93843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Remove rolling restart checks for x-pack #93843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
577531b
init removal - remove key bits and fix compilation
jakelandis Feb 15, 2023
8635ddc
Merge remote-tracking branch 'upstream/main' into remove_xpack_rollin…
jakelandis Feb 21, 2023
5a9c7dd
minor merge fix
jakelandis Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/LicenseService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import org.elasticsearch.protocol.xpack.license.LicensesStatus;
import org.elasticsearch.protocol.xpack.license.PutLicenseResponse;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xpack.core.XPackPlugin;
import org.elasticsearch.xpack.core.XPackSettings;

import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -300,7 +299,6 @@ protected PutLicenseResponse newResponse(boolean acknowledged) {

@Override
public ClusterState execute(ClusterState currentState) throws Exception {
XPackPlugin.checkReadyForXPackCustomMetadata(currentState);
final Version oldestNodeVersion = currentState.nodes().getSmallestNonClientNodeVersion();
if (licenseIsCompatible(newLicense, oldestNodeVersion) == false) {
throw new IllegalStateException(
Expand Down Expand Up @@ -462,8 +460,7 @@ protected void doStart() throws ElasticsearchException {
if (clusterService.lifecycleState() == Lifecycle.State.STARTED) {
final ClusterState clusterState = clusterService.state();
if (clusterState.blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK) == false
&& clusterState.nodes().getMasterNode() != null
&& XPackPlugin.isReadyForXPackCustomMetadata(clusterState)) {
&& clusterState.nodes().getMasterNode() != null) {
final LicensesMetadata currentMetadata = clusterState.metadata().custom(LicensesMetadata.TYPE);
boolean noLicense = currentMetadata == null || currentMetadata.getLicense() == null;
if (clusterState.getNodes().isLocalNodeElectedMaster()
Expand Down Expand Up @@ -495,14 +492,6 @@ public void clusterChanged(ClusterChangedEvent event) {
final ClusterState previousClusterState = event.previousState();
final ClusterState currentClusterState = event.state();
if (currentClusterState.blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK) == false) {
if (XPackPlugin.isReadyForXPackCustomMetadata(currentClusterState) == false) {
logger.debug(
"cannot add license to cluster as the following nodes might not understand the license metadata: {}",
() -> XPackPlugin.nodesNotReadyForXPackCustomMetadata(currentClusterState)
);
return;
}

final LicensesMetadata prevLicensesMetadata = previousClusterState.getMetadata().custom(LicensesMetadata.TYPE);
final LicensesMetadata currentLicensesMetadata = currentClusterState.getMetadata().custom(LicensesMetadata.TYPE);
// notify all interested plugins
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/StartBasicClusterTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xpack.core.XPackPlugin;

import java.time.Clock;
import java.util.Map;
Expand Down Expand Up @@ -124,7 +123,6 @@ static class Executor implements ClusterStateTaskExecutor<StartBasicClusterTask>
@Override
public ClusterState execute(BatchExecutionContext<StartBasicClusterTask> batchExecutionContext) throws Exception {
final var initialState = batchExecutionContext.initialState();
XPackPlugin.checkReadyForXPackCustomMetadata(initialState);
final LicensesMetadata originalLicensesMetadata = initialState.metadata().custom(LicensesMetadata.TYPE);
var currentLicensesMetadata = originalLicensesMetadata;
for (final var taskContext : batchExecutionContext.taskContexts()) {
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/StartTrialClusterTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xpack.core.XPackPlugin;

import java.time.Clock;
import java.util.Collections;
Expand Down Expand Up @@ -111,7 +110,6 @@ static class Executor implements ClusterStateTaskExecutor<StartTrialClusterTask>
@Override
public ClusterState execute(BatchExecutionContext<StartTrialClusterTask> batchExecutionContext) throws Exception {
final var initialState = batchExecutionContext.initialState();
XPackPlugin.checkReadyForXPackCustomMetadata(initialState);
final LicensesMetadata originalLicensesMetadata = initialState.metadata().custom(LicensesMetadata.TYPE);
var currentLicensesMetadata = originalLicensesMetadata;
for (final var taskContext : batchExecutionContext.taskContexts()) {
Expand Down
2 changes: 0 additions & 2 deletions .../plugin/core/src/main/java/org/elasticsearch/license/StartupSelfGeneratedLicenseTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xpack.core.XPackPlugin;

import java.time.Clock;
import java.util.UUID;
Expand Down Expand Up @@ -51,7 +50,6 @@ public void clusterStateProcessed(ClusterState oldState, ClusterState newState)

@Override
public ClusterState execute(ClusterState currentState) throws Exception {
XPackPlugin.checkReadyForXPackCustomMetadata(currentState);
final Metadata metadata = currentState.metadata();
final LicensesMetadata currentLicensesMetadata = metadata.custom(LicensesMetadata.TYPE);
// do not generate a license if any license is present
Expand Down
71 changes: 0 additions & 71 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@
import org.elasticsearch.action.support.ActionFilter;
import org.elasticsearch.action.support.TransportAction;
import org.elasticsearch.client.internal.Client;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.cluster.routing.allocation.AllocationService;
Expand All @@ -33,7 +31,6 @@
import org.elasticsearch.common.settings.SettingsFilter;
import org.elasticsearch.common.ssl.SslConfiguration;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.core.Booleans;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.index.IndexSettingProvider;
Expand All @@ -42,7 +39,6 @@
import org.elasticsearch.index.mapper.MetadataFieldMapper;
import org.elasticsearch.indices.recovery.RecoverySettings;
import org.elasticsearch.license.LicenseService;
import org.elasticsearch.license.LicensesMetadata;
import org.elasticsearch.license.Licensing;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.plugins.ClusterPlugin;
Expand Down Expand Up @@ -78,18 +74,14 @@
import org.elasticsearch.xpack.core.action.XPackUsageResponse;
import org.elasticsearch.xpack.core.async.DeleteAsyncResultAction;
import org.elasticsearch.xpack.core.async.TransportDeleteAsyncResultAction;
import org.elasticsearch.xpack.core.ml.MlMetadata;
import org.elasticsearch.xpack.core.rest.action.RestReloadAnalyzersAction;
import org.elasticsearch.xpack.core.rest.action.RestXPackInfoAction;
import org.elasticsearch.xpack.core.rest.action.RestXPackUsageAction;
import org.elasticsearch.xpack.core.security.authc.TokenMetadata;
import org.elasticsearch.xpack.core.ssl.SSLConfigurationReloader;
import org.elasticsearch.xpack.core.ssl.SSLService;
import org.elasticsearch.xpack.core.termsenum.action.TermsEnumAction;
import org.elasticsearch.xpack.core.termsenum.action.TransportTermsEnumAction;
import org.elasticsearch.xpack.core.termsenum.rest.RestTermsEnumAction;
import org.elasticsearch.xpack.core.transform.TransformMetadata;
import org.elasticsearch.xpack.core.watcher.WatcherMetadata;

import java.nio.file.Files;
import java.nio.file.Path;
Expand All @@ -104,7 +96,6 @@
import java.util.Optional;
import java.util.function.LongSupplier;
import java.util.function.Supplier;
import java.util.stream.Collectors;

@SuppressWarnings("HiddenField")
public class XPackPlugin extends XPackClientPlugin
Expand All @@ -117,7 +108,6 @@ public class XPackPlugin extends XPackClientPlugin
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(XPackPlugin.class);

public static final String ASYNC_RESULTS_INDEX = ".async-search";
public static final String XPACK_INSTALLED_NODE_ATTR = "xpack.installed";

// TODO: clean up this library to not ask for write access to all system properties!
static {
Expand Down Expand Up @@ -227,72 +217,11 @@ public static LongSupplier getSharedEpochMillisSupplier() {
return epochMillisSupplier.get();
}

/**
* Checks if the cluster state allows this node to add x-pack metadata to the cluster state,
* and throws an exception otherwise.
* This check should be called before installing any x-pack metadata to the cluster state,
* to ensure that the other nodes that are part of the cluster will be able to deserialize
* that metadata. Note that if the cluster state already contains x-pack metadata, this
* check assumes that the nodes are already ready to receive additional x-pack metadata.
* Having this check properly in place everywhere allows to install x-pack into a cluster
* using a rolling restart.
*/
public static void checkReadyForXPackCustomMetadata(ClusterState clusterState) {
if (alreadyContainsXPackCustomMetadata(clusterState)) {
return;
}
List<DiscoveryNode> notReadyNodes = nodesNotReadyForXPackCustomMetadata(clusterState);
if (notReadyNodes.isEmpty() == false) {
throw new IllegalStateException("The following nodes are not ready yet for enabling x-pack custom metadata: " + notReadyNodes);
}
}

/**
* Checks if the cluster state allows this node to add x-pack metadata to the cluster state.
* See {@link #checkReadyForXPackCustomMetadata} for more details.
*/
public static boolean isReadyForXPackCustomMetadata(ClusterState clusterState) {
return alreadyContainsXPackCustomMetadata(clusterState) || nodesNotReadyForXPackCustomMetadata(clusterState).isEmpty();
}

/**
* Returns the list of nodes that won't allow this node from adding x-pack metadata to the cluster state.
* See {@link #checkReadyForXPackCustomMetadata} for more details.
*/
public static List<DiscoveryNode> nodesNotReadyForXPackCustomMetadata(ClusterState clusterState) {
// check that all nodes would be capable of deserializing newly added x-pack metadata
final List<DiscoveryNode> notReadyNodes = clusterState.nodes().stream().filter(node -> {
final String xpackInstalledAttr = node.getAttributes().getOrDefault(XPACK_INSTALLED_NODE_ATTR, "false");
return Booleans.parseBoolean(xpackInstalledAttr) == false;
}).collect(Collectors.toList());

return notReadyNodes;
}

private static boolean alreadyContainsXPackCustomMetadata(ClusterState clusterState) {
final Metadata metadata = clusterState.metadata();
return metadata.custom(LicensesMetadata.TYPE) != null
|| metadata.custom(MlMetadata.TYPE) != null
|| metadata.custom(WatcherMetadata.TYPE) != null
|| clusterState.custom(TokenMetadata.TYPE) != null
|| metadata.custom(TransformMetadata.TYPE) != null;
}

@Override
public Map<String, MetadataFieldMapper.TypeParser> getMetadataMappers() {
return Map.of(DataTierFieldMapper.NAME, DataTierFieldMapper.PARSER);
}

@Override
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();
}
Comment on lines -287 to -294
Copy link
Contributor

@DaveCTurner DaveCTurner Feb 24, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jakelandis jakelandis Feb 24, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@DaveCTurner DaveCTurner Feb 24, 2023

Choose a reason for hiding this comment

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

Yes 7.1/8.7 mixed clusters are not valid, but AIUI even 8.6 masters will require this attribute.

Copy link
Contributor Author

@jakelandis jakelandis Feb 27, 2023

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@DaveCTurner DaveCTurner Feb 28, 2023

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

@jakelandis jakelandis Feb 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@DaveCTurner DaveCTurner Feb 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jakelandis jakelandis Feb 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jakelandis jakelandis Mar 9, 2023

Choose a reason for hiding this comment

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

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)


@Override
public Collection<Object> createComponents(
Client client,
Expand Down
11 changes: 2 additions & 9 deletions ...k/plugin/core/src/test/java/org/elasticsearch/license/AbstractLicenseServiceTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,14 @@
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.threadpool.TestThreadPool;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xpack.core.XPackPlugin;
import org.elasticsearch.xpack.core.watcher.watch.ClockMock;
import org.junit.After;
import org.junit.Before;

import java.util.stream.Stream;

import static java.util.Collections.emptyMap;
import static java.util.Collections.emptySet;
import static java.util.Collections.singletonMap;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -84,13 +83,7 @@ protected void setInitialState(License license, XPackLicenseState licenseState,
}

protected DiscoveryNode getLocalNode() {
return new DiscoveryNode(
"b",
buildNewFakeTransportAddress(),
singletonMap(XPackPlugin.XPACK_INSTALLED_NODE_ATTR, "true"),
emptySet(),
Version.CURRENT
);
return new DiscoveryNode("b", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT);
}

@After
Expand Down
103 changes: 0 additions & 103 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/XPackPluginTests.java
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions ...llup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xpack.core.ClientHelper;
import org.elasticsearch.xpack.core.XPackPlugin;
import org.elasticsearch.xpack.core.rollup.RollupField;
import org.elasticsearch.xpack.core.rollup.action.PutRollupJobAction;
import org.elasticsearch.xpack.core.rollup.job.RollupJob;
Expand Down Expand Up @@ -100,7 +99,6 @@ protected void masterOperation(
ClusterState clusterState,
ActionListener<AcknowledgedResponse> listener
) {
XPackPlugin.checkReadyForXPackCustomMetadata(clusterState);
checkForDeprecatedTZ(request);

FieldCapabilitiesRequest fieldCapsRequest = new FieldCapabilitiesRequest().indices(request.indices())
Expand Down
Loading