Deprecate minimum_master_nodes by DaveCTurner · Pull Request #37868 · elastic/elasticsearch

DaveCTurner · 2019-01-25T15:06:50Z

Today we pass discovery.zen.minimum_master_nodes to nodes started up in
tests, but for 7.x nodes this setting is not required as it has no effect.
This commit removes this setting so that nodes are started with more realistic
configurations, and deprecates it.

Today the `discovery.zen.minimum_master_nodes` setting is ignored in 7.0 but must still be permitted to live as a cluster-level setting for cases where 6.x nodes leave and rejoin the cluster. However there is no good reason for it to exist as a node-level setting on 7.0 nodes. This change prevents a 7.0 node from starting up with the `discovery.zen.minimum_master_nodes` setting set.

elasticmachine · 2019-01-25T15:06:51Z

Pinging @elastic/es-distributed

DaveCTurner · 2019-01-25T15:07:53Z

We are still debating whether we should actually forbid this setting like this. However even if we decide not to do so, this PR removes many places in which we depend on this setting.

DaveCTurner · 2019-01-27T21:41:24Z

@elasticmachine please run elasticsearch-ci/1

…ping

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy

ywelsch · 2019-01-28T18:39:44Z

server/src/test/java/org/elasticsearch/cluster/coordination/Zen1IT.java

                assertFalse(nodeName, clusterHealthResponse.isTimedOut());
-                return Coordinator.addZen1Attribute(false, Settings.builder().put(ZEN2_SETTINGS)).build();
+                return Coordinator.addZen1Attribute(false, Settings.builder().put(ZEN2_SETTINGS)
+                    .put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), Integer.MAX_VALUE)).build();


add a comment why this is set to MAX_VALUE? Typically you would expect this to be unconfigured. Or we could randomize between unconfigured, configured to num_nodes / 2 + 1 and MAX_VALUE (in case user forgot to remove setting in upgrade)

I replaced it with a more specific name in 9137ad2. The point is that there's no facility to remove a setting via this API, so we set it to an explicitly silly value and then assert that this is what we wanted when removing it here:

elasticsearch/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

Lines 993 to 1001 in 9137ad2

// simulating an upgrade from Zen1 to Zen2, but there's no way to remove a setting when restarting a node, so

// you have to set it to REMOVED_MINIMUM_MASTER_NODES (== Integer.MAX_VALUE) to indicate its removal:

assertTrue(USE_ZEN2.exists(finalSettings));

assertTrue(USE_ZEN2.get(finalSettings));

assertThat(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.get(finalSettings), equalTo(REMOVED_MINIMUM_MASTER_NODES));

final Builder builder = Settings.builder().put(finalSettings);

builder.remove(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey());

finalSettings = builder.build();

How about doing .putNull(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey()) here and then

assertThat(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.get(finalSettings), equalTo(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getDefault(finalSettings)));

in InternalTestCluster?

Setting it to null is a legitimate thing that a test might do, and I want to assert that this is not what has happened here.

DaveCTurner · 2019-01-29T19:54:31Z

@elasticmachine run elasticsearch-ci/1 run elasticsearch-ci/default-distro

DaveCTurner · 2019-01-30T10:19:17Z

@elasticmachine run elasticsearch-ci/1 run elasticsearch-ci/docs-check

DaveCTurner · 2019-01-30T16:41:02Z

@elasticmachine run elasticsearch-ci/2

* elastic/master: ILM setPriority corrections for a 0 value (elastic#38001) Temporarily disable BWC for retention lease stats (elastic#38049) Skip Shrink when numberOfShards not changed (elastic#37953) Add dispatching to `HandledTransportAction` (elastic#38050) Update httpclient for JDK 11 TLS engine (elastic#37994) Reduce flaxiness of ccr recovery timeouts test (elastic#38035) Fix ILM status to allow unknown fields (elastic#38043) Fix ILM Lifecycle Policy to allow unknown fields (elastic#38041) Update verify repository to allow unknown fields (elastic#37619) [ML] Datafeed deprecation checks (elastic#38026) Deprecate minimum_master_nodes (elastic#37868) Remove types from watcher docs (elastic#38002) Add test coverage for Painless general casting of boolean and Boolean (elastic#37780) Fixed test bug, lastFollowTime is null if there are no follower indices. Add ECS schema for user-agent ingest processor (elastic#37727) (elastic#37984) Extract TransportRequestDeduplication from ShardStateAction (elastic#37870)

…r-primary-term * elastic/master: Mute failing date index name processor test Reenable BWC testing after retention lease stats (elastic#38062) Move watcher to use seq# and primary term for concurrency control (elastic#37977) ILM setPriority corrections for a 0 value (elastic#38001) Temporarily disable BWC for retention lease stats (elastic#38049) Skip Shrink when numberOfShards not changed (elastic#37953) Add dispatching to `HandledTransportAction` (elastic#38050) Update httpclient for JDK 11 TLS engine (elastic#37994) Reduce flaxiness of ccr recovery timeouts test (elastic#38035) Fix ILM status to allow unknown fields (elastic#38043) Fix ILM Lifecycle Policy to allow unknown fields (elastic#38041) Update verify repository to allow unknown fields (elastic#37619) [ML] Datafeed deprecation checks (elastic#38026) Deprecate minimum_master_nodes (elastic#37868)

Zen discovery and its associated settings were deprecated in 7.0. This commit adds an entry to the release notes calling this out. Relates elastic#38289 elastic#38333 elastic#38350 elastic#37868

Zen discovery and its associated settings were deprecated in 7.0. This commit adds an entry to the release notes calling this out. Relates #38289 #38333 #38350 #37868

DaveCTurner added >enhancement >breaking v7.0.0 :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Jan 25, 2019

DaveCTurner requested a review from ywelsch January 25, 2019 15:06

DaveCTurner added 2 commits January 25, 2019 15:10

startNode(Settings.EMPTY) is just startNode()

7aed19d

startNode(Settings.EMPTY) is just startNode()

9a952da

DaveCTurner added the WIP label Jan 25, 2019

DaveCTurner added 6 commits January 25, 2019 16:53

Fix REST tests too

6dc78b1

Add NOCOMMIT because we don't want to be so strict

186a16d

Gotta catch them all

05af9a7

Merge branch 'master' into 2019-01-25-forbid-minimum-master-nodes

ba246f5

Fix docs

b8f9e45

Always set discovery.initial_state_timeout: 0s

8585d63

DaveCTurner removed the >breaking label Jan 27, 2019

DaveCTurner changed the title ~~Forbid minimum_master_nodes in node settings~~ Remove minimum_master_nodes from settings in tests Jan 27, 2019

DaveCTurner removed the WIP label Jan 27, 2019

Revert no-op changes to GatewayService

38332f4

DaveCTurner added 6 commits January 28, 2019 07:29

Remove exception and mark property as deprecated

4871b7e

Validate cluster fully formed before restart, preventing re-bootstrap…

dd30a2e

…ping

Don't use mmn in REST test regarding settings

b9a64f5

Remove MMN from comment

79f7f41

Merge branch 'master' into 2019-01-25-forbid-minimum-master-nodes

7c58475

Expect warnings in tests

c2674ab

ywelsch reviewed Jan 28, 2019

View reviewed changes

Merge branch 'master' into 2019-01-25-forbid-minimum-master-nodes

7da96d4

Merge branch 'master' into 2019-01-25-forbid-minimum-master-nodes

f9fa2d2

DaveCTurner added 3 commits January 30, 2019 08:20

Reducing the recovery timeout changes the expected log message

140dfce

Merge branch 'master' into 2019-01-25-forbid-minimum-master-nodes

52fdcba

Merge branch 'master' into 2019-01-25-forbid-minimum-master-nodes

9f737b0

Merge branch 'master' into 2019-01-25-forbid-minimum-master-nodes

89bb05f

DaveCTurner merged commit 81c443c into elastic:master Jan 30, 2019

DaveCTurner deleted the 2019-01-25-forbid-minimum-master-nodes branch January 30, 2019 20:09

DaveCTurner changed the title ~~Remove minimum_master_nodes from settings in tests~~ Deprecate minimum_master_nodes Jan 30, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

ywelsch mentioned this pull request Mar 8, 2019

[CI] InternalTestClusterTests.testDifferentRolesMaintainPathOnRestart failure in master #37462

Closed

DaveCTurner mentioned this pull request Mar 12, 2019

Make node.master a dynamic setting #10793

Closed

DaveCTurner mentioned this pull request Oct 7, 2021

Document that discovery.zen.* settings are deprecated #78824

Merged

rursprung mentioned this pull request Dec 10, 2021

[BUG][opensearch] dead "minimum_master_nodes" setting still listed opensearch-project/helm-charts#167

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate minimum_master_nodes#37868

Deprecate minimum_master_nodes#37868
DaveCTurner merged 25 commits intoelastic:masterfrom
DaveCTurner:2019-01-25-forbid-minimum-master-nodes

DaveCTurner commented Jan 25, 2019 •

edited

Loading

Uh oh!

elasticmachine commented Jan 25, 2019

Uh oh!

DaveCTurner commented Jan 25, 2019

Uh oh!

DaveCTurner commented Jan 27, 2019

Uh oh!

Uh oh!

Uh oh!

ywelsch Jan 28, 2019

Uh oh!

DaveCTurner Jan 29, 2019 •

edited

Loading

Uh oh!

ywelsch Jan 29, 2019

Uh oh!

DaveCTurner Jan 29, 2019

Uh oh!

DaveCTurner commented Jan 29, 2019

Uh oh!

DaveCTurner commented Jan 30, 2019

Uh oh!

DaveCTurner commented Jan 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	// simulating an upgrade from Zen1 to Zen2, but there's no way to remove a setting when restarting a node, so
	// you have to set it to REMOVED_MINIMUM_MASTER_NODES (== Integer.MAX_VALUE) to indicate its removal:
	assertTrue(USE_ZEN2.exists(finalSettings));
	assertTrue(USE_ZEN2.get(finalSettings));
	assertThat(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.get(finalSettings), equalTo(REMOVED_MINIMUM_MASTER_NODES));

	final Builder builder = Settings.builder().put(finalSettings);
	builder.remove(DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey());
	finalSettings = builder.build();

Conversation

DaveCTurner commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jan 25, 2019

Uh oh!

DaveCTurner commented Jan 25, 2019

Uh oh!

DaveCTurner commented Jan 27, 2019

Uh oh!

Uh oh!

Uh oh!

ywelsch Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 29, 2019 • 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!

ywelsch Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Jan 29, 2019

Uh oh!

DaveCTurner commented Jan 30, 2019

Uh oh!

DaveCTurner commented Jan 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DaveCTurner commented Jan 25, 2019 •

edited

Loading

DaveCTurner Jan 29, 2019 •

edited

Loading