Skip to content
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.

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

[TEST] Add back necessary tests for deprecated settings that are replaced during applying inclusive naming #2825

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.hamcrest.Matchers;

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
Expand Down Expand Up @@ -144,6 +145,28 @@ public void testNodeCounts() {
}
}

// Validate assigning value "master" to setting "node.roles" can get correct count in Node Stats response after MASTER_ROLE deprecated.
// TODO: Remove the test after removing MASTER_ROLE.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick - I don't think we need these TODOs alongside all of the tests. When we remove DiscoveryNodeRole.MASTER_ROLE these tests would automatically break, so we would anyway resolve them then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @kartg Thanks for your review! 👍 I will remove the unnecessary TODOs.
The reason I added them is to giving a clear mind that it definitely can be removed. 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed all the TODO in this PR, and I believe the comment of every newly added test is enough to make people understand they are only used to test the deprecated "master" role setting.

public void testNodeCountsWithDeprecatedMasterRole() {
int total = 1;
Settings settings = Settings.builder()
.putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), Collections.singletonList(DiscoveryNodeRole.MASTER_ROLE.roleName()))
.build();
internalCluster().startNode(settings);
waitForNodes(total);

Map<String, Integer> expectedCounts = new HashMap<>();
expectedCounts.put(DiscoveryNodeRole.DATA_ROLE.roleName(), 0);
expectedCounts.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), 1);
expectedCounts.put(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), 1);
expectedCounts.put(DiscoveryNodeRole.INGEST_ROLE.roleName(), 0);
expectedCounts.put(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(), 0);
expectedCounts.put(ClusterStatsNodes.Counts.COORDINATING_ONLY, 0);

ClusterStatsResponse response = client().admin().cluster().prepareClusterStats().get();
assertCounts(response.getNodesStats().getCounts(), total, expectedCounts);
}

private static void incrementCountForRole(String role, Map<String, Integer> counts) {
Integer count = counts.get(role);
if (count == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,65 @@ public void testResolveAndCheckMaximum() {
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE);
}

// As of 2.0, MASTER_ROLE is deprecated to promote inclusive language.
// Validate node with MASTER_ROLE can be resolved by resolveVotingConfigExclusions() like before.
// TODO: Remove the test after removing MASTER_ROLE.
public void testResolveByNodeDescriptionWithDeprecatedMasterRole() {
final DiscoveryNode localNode = new DiscoveryNode(
"local",
"local",
buildNewFakeTransportAddress(),
emptyMap(),
singleton(DiscoveryNodeRole.MASTER_ROLE),
Version.CURRENT
);
final VotingConfigExclusion localNodeExclusion = new VotingConfigExclusion(localNode);

final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster"))
.nodes(new Builder().add(localNode).localNodeId(localNode.getId()))
.build();

assertThat(makeRequestWithNodeDescriptions("_local").resolveVotingConfigExclusions(clusterState), contains(localNodeExclusion));
assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE);
Copy link
Member

Choose a reason for hiding this comment

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

This assertion seems unnecessary for this test case - you can probably remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👏 You are so careful that notice such detail! Good catch, I will try to replace with exceptWarnings(), removing is not feasible, because any warnings can fail the test.

}

// TODO: Remove the test after removing MASTER_ROLE.
public void testResolveByNodeIdWithDeprecatedMasterRole() {
final DiscoveryNode node = new DiscoveryNode(
"nodeName",
"nodeId",
buildNewFakeTransportAddress(),
emptyMap(),
singleton(DiscoveryNodeRole.MASTER_ROLE),
Version.CURRENT
);
final VotingConfigExclusion nodeExclusion = new VotingConfigExclusion(node);

final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).nodes(new Builder().add(node)).build();

assertThat(
new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[] { "nodeId" }, Strings.EMPTY_ARRAY, TimeValue.ZERO)
.resolveVotingConfigExclusions(clusterState),
contains(nodeExclusion)
);
}

public void testResolveByNodeNameWithDeprecatedMasterRole() {
final DiscoveryNode node = new DiscoveryNode(
"nodeName",
"nodeId",
buildNewFakeTransportAddress(),
emptyMap(),
singleton(DiscoveryNodeRole.MASTER_ROLE),
Version.CURRENT
);
final VotingConfigExclusion nodeExclusion = new VotingConfigExclusion(node);

final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).nodes(new Builder().add(node)).build();

assertThat(new AddVotingConfigExclusionsRequest("nodeName").resolveVotingConfigExclusions(clusterState), contains(nodeExclusion));
}

private static AddVotingConfigExclusionsRequest makeRequestWithNodeDescriptions(String... nodeDescriptions) {
return new AddVotingConfigExclusionsRequest(
nodeDescriptions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,42 @@ public void testShrinkIndexSettings() {
assertEquals(request.waitForActiveShards(), activeShardCount);
}

// Validate the Action works correctly on a node with deprecated 'master' role
// TODO: Remove the test after removing MASTER_ROLE.
public void testPassNumRoutingShardsOnNodeWithDeprecatedMasterRole() {
final Set<DiscoveryNodeRole> roles = Set.of(DiscoveryNodeRole.MASTER_ROLE, DiscoveryNodeRole.DATA_ROLE);

ClusterState clusterState = ClusterState.builder(
createClusterState("source", 1, 0, Settings.builder().put("index.blocks.write", true).build())
)
.nodes(
DiscoveryNodes.builder().add(new DiscoveryNode("node1", buildNewFakeTransportAddress(), emptyMap(), roles, Version.CURRENT))
)
.build();
AllocationService service = new AllocationService(
new AllocationDeciders(Collections.singleton(new MaxRetryAllocationDecider())),
new TestGatewayAllocator(),
new BalancedShardsAllocator(Settings.EMPTY),
EmptyClusterInfoService.INSTANCE,
EmptySnapshotsInfoService.INSTANCE
);

RoutingTable routingTable = service.reroute(clusterState, "reroute").routingTable();
clusterState = ClusterState.builder(clusterState).routingTable(routingTable).build();
// now we start the shard
routingTable = OpenSearchAllocationTestCase.startInitializingShardsAndReroute(service, clusterState, "source").routingTable();
clusterState = ClusterState.builder(clusterState).routingTable(routingTable).build();

ResizeRequest resizeRequest = new ResizeRequest("target", "source");
resizeRequest.setResizeType(ResizeType.SPLIT);
resizeRequest.getTargetIndexRequest().settings(Settings.builder().put("index.number_of_shards", 2).build());
TransportResizeAction.prepareCreateIndexRequest(resizeRequest, clusterState, null, "source", "target");

resizeRequest.getTargetIndexRequest()
.settings(Settings.builder().put("index.number_of_routing_shards", 2).put("index.number_of_shards", 2).build());
TransportResizeAction.prepareCreateIndexRequest(resizeRequest, clusterState, null, "source", "target");
}

private DiscoveryNode newNode(String nodeId) {
final Set<DiscoveryNodeRole> roles = Collections.unmodifiableSet(
new HashSet<>(Arrays.asList(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE, DiscoveryNodeRole.DATA_ROLE))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,4 +286,31 @@ public void testDefaultMaxConcurrentSearches() {
assertThat(result, equalTo(1));
}

// Validate the Action works correctly on a node with deprecated 'master' role
// TODO: Remove the test after removing MASTER_ROLE.
public void testDefaultMaxConcurrentSearchesOnNodeWithDeprecatedMasterRole() {
DiscoveryNodes.Builder builder = DiscoveryNodes.builder();
builder.add(
new DiscoveryNode(
"master",
buildNewFakeTransportAddress(),
Collections.emptyMap(),
Collections.singleton(DiscoveryNodeRole.MASTER_ROLE),
Version.CURRENT
)
);
builder.add(
new DiscoveryNode(
"data",
buildNewFakeTransportAddress(),
Collections.emptyMap(),
Collections.singleton(DiscoveryNodeRole.DATA_ROLE),
Version.CURRENT
)
);

ClusterState state = ClusterState.builder(new ClusterName("_name")).nodes(builder).build();
int result = TransportMultiSearchAction.defaultMaxConcurrentSearches(10, state);
assertThat(result, equalTo(10));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -525,4 +525,41 @@ protected void masterOperation(Request request, ClusterState state, ActionListen
assertTrue(listener.isDone());
assertThat(listener.get(), equalTo(response));
}

// Validate TransportMasterNodeAction.testDelegateToMaster() works correctly on node with the deprecated MASTER_ROLE.
// TODO: Remove the test after removing MASTER_ROLE.
public void testDelegateToMasterOnNodeWithDeprecatedMasterRole() throws ExecutionException, InterruptedException {
DiscoveryNode localNode = new DiscoveryNode(
"local_node",
buildNewFakeTransportAddress(),
Collections.emptyMap(),
Collections.singleton(DiscoveryNodeRole.MASTER_ROLE),
Version.CURRENT
);
DiscoveryNode remoteNode = new DiscoveryNode(
"remote_node",
buildNewFakeTransportAddress(),
Collections.emptyMap(),
Collections.singleton(DiscoveryNodeRole.MASTER_ROLE),
Version.CURRENT
);
DiscoveryNode[] allNodes = new DiscoveryNode[] { localNode, remoteNode };

Request request = new Request();
setState(clusterService, ClusterStateCreationUtils.state(localNode, remoteNode, allNodes));

PlainActionFuture<Response> listener = new PlainActionFuture<>();
new Action("internal:testAction", transportService, clusterService, threadPool).execute(request, listener);

assertThat(transport.capturedRequests().length, equalTo(1));
CapturingTransport.CapturedRequest capturedRequest = transport.capturedRequests()[0];
assertTrue(capturedRequest.node.isMasterNode());
assertThat(capturedRequest.request, equalTo(request));
assertThat(capturedRequest.action, equalTo("internal:testAction"));

Response response = new Response();
transport.handleResponse(capturedRequest.requestId, response);
assertTrue(listener.isDone());
assertThat(listener.get(), equalTo(response));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,36 @@ public void testChangedCustomMetadataSet() {
assertTrue(changedCustomMetadataTypeSet.contains(customMetadata1.getWriteableName()));
}

// Validate the above test case testLocalNodeIsMaster() passes when the deprecated 'master' role is assigned to the local node.
// TODO: Remove the test after removing MASTER_ROLE.
public void testLocalNodeIsMasterWithDeprecatedMasterRole() {
final DiscoveryNodes.Builder builderLocalIsMaster = DiscoveryNodes.builder();
final DiscoveryNode node0 = newNode("node_0", Set.of(DiscoveryNodeRole.MASTER_ROLE));
final DiscoveryNode node1 = newNode("node_1", Set.of(DiscoveryNodeRole.DATA_ROLE));
builderLocalIsMaster.add(node0).add(node1).masterNodeId(node0.getId()).localNodeId(node0.getId());

final DiscoveryNodes.Builder builderLocalNotMaster = DiscoveryNodes.builder();
builderLocalNotMaster.add(node0).add(node1).masterNodeId(node0.getId()).localNodeId(node1.getId());

ClusterState previousState = createSimpleClusterState();
final Metadata metadata = createMetadata(initialIndices);
ClusterState newState = ClusterState.builder(TEST_CLUSTER_NAME)
.nodes(builderLocalIsMaster.build())
.metadata(metadata)
.routingTable(createRoutingTable(1, metadata))
.build();
ClusterChangedEvent event = new ClusterChangedEvent("_na_", newState, previousState);
assertTrue("local node should be master", event.localNodeMaster());

newState = ClusterState.builder(TEST_CLUSTER_NAME)
.nodes(builderLocalNotMaster.build())
.metadata(metadata)
.routingTable(createRoutingTable(1, metadata))
.build();
event = new ClusterChangedEvent("_na_", newState, previousState);
assertFalse("local node should not be master", event.localNodeMaster());
}

private static class CustomMetadata2 extends TestCustomMetadata {
protected CustomMetadata2(String data) {
super(data);
Expand Down
Loading