diff --git a/server/src/main/java/org/opensearch/cluster/coordination/DecommissionNodeAttributeClusterStateTaskExecutor.java b/server/src/main/java/org/opensearch/cluster/coordination/DecommissionNodeAttributeClusterStateTaskExecutor.java index 3832c5a8cb26e..eda370087e987 100644 --- a/server/src/main/java/org/opensearch/cluster/coordination/DecommissionNodeAttributeClusterStateTaskExecutor.java +++ b/server/src/main/java/org/opensearch/cluster/coordination/DecommissionNodeAttributeClusterStateTaskExecutor.java @@ -104,10 +104,8 @@ public ClusterTasksResult execute(ClusterState currentState, List ta } private boolean nodeHasDecommissionedAttribute(DiscoveryNode discoveryNode, Task task) { - // TODO - Validation for key present - return task.decommissionAttribute().attributeValues().contains( - discoveryNode.getAttributes().get(task.decommissionAttribute().attributeName()) - ); + String discoveryNodeAttributeValue = discoveryNode.getAttributes().get(task.decommissionAttribute().attributeName()); + return discoveryNodeAttributeValue != null && task.decommissionAttribute().attributeValues().contains(discoveryNodeAttributeValue); } // visible for testing diff --git a/server/src/test/java/org/opensearch/cluster/coordination/DecommissionNodeAttributeClusterStateTaskExecutorTests.java b/server/src/test/java/org/opensearch/cluster/coordination/DecommissionNodeAttributeClusterStateTaskExecutorTests.java index 1e124a29726cb..01680a9166121 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/DecommissionNodeAttributeClusterStateTaskExecutorTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/DecommissionNodeAttributeClusterStateTaskExecutorTests.java @@ -33,6 +33,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -93,8 +94,60 @@ protected ClusterState remainingNodesClusterState(ClusterState currentState, Dis } } - // Test when attribute value is not presrnt in any node - // Test when attribute name is not present in any node + public void testSameClusterStateAfterExecutionForUnknownAttributeNameAndValue() throws Exception{ + final AllocationService allocationService = mock(AllocationService.class); + when(allocationService.disassociateDeadNodes(any(ClusterState.class), eq(true), any(String.class))).thenAnswer( + im -> im.getArguments()[0] + ); + final AtomicReference remainingNodesClusterState = new AtomicReference<>(); + ClusterState clusterState = ClusterState.builder(new ClusterName("test")).build(); + + logger.info("--> adding five nodes on same zone_1"); + clusterState = addNodes(clusterState, "zone_1", "node1", "node2", "node3", "node4", "node5"); + + logger.info("--> adding five nodes on same zone_2"); + clusterState = addNodes(clusterState, "zone_2", "node6", "node7", "node8", "node9", "node10"); + + logger.info("--> adding five nodes on same zone_3"); + clusterState = addNodes(clusterState, "zone_3", "node11", "node12", "node13", "node14", "node15"); + + final DecommissionNodeAttributeClusterStateTaskExecutor executor = new DecommissionNodeAttributeClusterStateTaskExecutor(allocationService, logger) { + @Override + protected ClusterState remainingNodesClusterState(ClusterState currentState, DiscoveryNodes.Builder remainingNodesBuilder) { + remainingNodesClusterState.set(super.remainingNodesClusterState(currentState, remainingNodesBuilder)); + return remainingNodesClusterState.get(); + } + }; + + final List tasks = new ArrayList<>(); + // Task 1 with unknown attribute name + tasks.add(new DecommissionNodeAttributeClusterStateTaskExecutor.Task( + new DecommissionAttribute("unknown_zone_name", Collections.singletonList("unknown_zone_value")), + "unit test zone decommission executor") + ); + // Task 2 with unknown attribute value + tasks.add(new DecommissionNodeAttributeClusterStateTaskExecutor.Task( + new DecommissionAttribute("zone", Collections.singletonList("unknown_zone_value")), + "unit test zone decommission executor") + ); + + final ClusterStateTaskExecutor.ClusterTasksResult result = executor.execute( + clusterState, + tasks + ); + + ClusterState expectedClusterState = remainingNodesClusterState.get(); + ClusterState actualClusterState = result.resultingState; + + // assert that disassociate dead node tasks is never executed + verify(allocationService, never()).disassociateDeadNodes(eq(expectedClusterState), eq(true), any(String.class)); + + // assert that cluster state remains same + assertEquals(clusterState, actualClusterState); + + // Verify all 15 nodes present in the cluster after decommissioning unknown attribute name + assertEquals(actualClusterState.nodes().getNodes().size(), 15); + } private ClusterState addNodes(ClusterState clusterState, String zone, String... nodeIds) { DiscoveryNodes.Builder nodeBuilder = DiscoveryNodes.builder(clusterState.nodes()); @@ -113,7 +166,7 @@ private DiscoveryNode newNode(String nodeId, Map attributes) { ); } - private static Set CLUSTER_MANAGER_DATA_ROLES = Collections.unmodifiableSet( + final private static Set CLUSTER_MANAGER_DATA_ROLES = Collections.unmodifiableSet( new HashSet<>(Arrays.asList(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE, DiscoveryNodeRole.DATA_ROLE)) ); }