Skip to content

Fix NPE in Desired Balance API #97775

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

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Fix NPE in Desired Balance API #97775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
572cfd1
Fix NPE in Desired Balance API
DaveCTurner Jul 19, 2023
7760830
Update docs/changelog/97775.yaml
DaveCTurner Jul 19, 2023
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/97775.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 97775
summary: Fix NPE in Desired Balance API
area: Allocation
type: bug
issues: []
12 changes: 11 additions & 1 deletion ...c/main/java/org/elasticsearch/action/admin/cluster/allocation/DesiredBalanceResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,18 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws

public record ShardAssignmentView(Set<String> nodeIds, int total, int unassigned, int ignored) implements Writeable, ToXContentObject {

public static final ShardAssignmentView EMPTY = new ShardAssignmentView(Set.of(), 0, 0, 0);

public static ShardAssignmentView from(StreamInput in) throws IOException {
return new ShardAssignmentView(in.readSet(StreamInput::readString), in.readVInt(), in.readVInt(), in.readVInt());
final var nodeIds = in.readSet(StreamInput::readString);
final var total = in.readVInt();
final var unassigned = in.readVInt();
final var ignored = in.readVInt();
if (nodeIds.isEmpty() && total == 0 && unassigned == 0 && ignored == 0) {
return EMPTY;
} else {
return new ShardAssignmentView(nodeIds, total, unassigned, ignored);
}
}

@Override
Expand Down
6 changes: 3 additions & 3 deletions ...a/org/elasticsearch/action/admin/cluster/allocation/TransportGetDesiredBalanceAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,14 @@ private Map<String, Map<Integer, DesiredBalanceResponse.DesiredShards>> createRo
shardId,
new DesiredBalanceResponse.DesiredShards(
shardViews,
shardAssignment != null
? new DesiredBalanceResponse.ShardAssignmentView(
shardAssignment == null
? DesiredBalanceResponse.ShardAssignmentView.EMPTY
: new DesiredBalanceResponse.ShardAssignmentView(
shardAssignment.nodeIds(),
shardAssignment.total(),
shardAssignment.unassigned(),
shardAssignment.ignored()
)
: null
)
);
}
Expand Down
69 changes: 40 additions & 29 deletions .../elasticsearch/action/admin/cluster/allocation/TransportGetDesiredBalanceActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@

import org.elasticsearch.ResourceNotFoundException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.support.ActionFilters;
import org.elasticsearch.action.support.PlainActionFuture;
import org.elasticsearch.cluster.ClusterInfo;
import org.elasticsearch.cluster.ClusterInfoService;
import org.elasticsearch.cluster.ClusterName;
Expand Down Expand Up @@ -40,15 +40,17 @@
import org.elasticsearch.index.shard.ShardId;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.tasks.TaskId;
import org.elasticsearch.test.AbstractChunkedSerializingTestCase;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService;
import org.mockito.ArgumentCaptor;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import static org.elasticsearch.cluster.ClusterModule.BALANCED_ALLOCATOR;
Expand All @@ -57,7 +59,6 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

public class TransportGetDesiredBalanceActionTests extends ESAllocationTestCase {
Expand All @@ -74,13 +75,28 @@ public class TransportGetDesiredBalanceActionTests extends ESAllocationTestCase
clusterInfoService,
TEST_WRITE_LOAD_FORECASTER
);
@SuppressWarnings("unchecked")
private final ActionListener<DesiredBalanceResponse> listener = mock(ActionListener.class);

private static DesiredBalanceResponse execute(TransportGetDesiredBalanceAction action, ClusterState clusterState) throws Exception {
return PlainActionFuture.get(
future -> action.masterOperation(
new Task(1, "test", GetDesiredBalanceAction.NAME, "", TaskId.EMPTY_TASK_ID, Map.of()),
new DesiredBalanceRequest(),
clusterState,
future
),
10,
TimeUnit.SECONDS
);
}

private DesiredBalanceResponse executeAction(ClusterState clusterState) throws Exception {
return execute(transportGetDesiredBalanceAction, clusterState);
}

public void testReturnsErrorIfAllocatorIsNotDesiredBalanced() throws Exception {
var clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadataWithConfiguredAllocator(BALANCED_ALLOCATOR)).build();

new TransportGetDesiredBalanceAction(
final var action = new TransportGetDesiredBalanceAction(
mock(TransportService.class),
mock(ClusterService.class),
mock(ThreadPool.class),
Expand All @@ -89,12 +105,9 @@ public void testReturnsErrorIfAllocatorIsNotDesiredBalanced() throws Exception {
mock(ShardsAllocator.class),
mock(ClusterInfoService.class),
mock(WriteLoadForecaster.class)
).masterOperation(mock(Task.class), mock(DesiredBalanceRequest.class), clusterState, listener);

ArgumentCaptor<ResourceNotFoundException> exceptionArgumentCaptor = ArgumentCaptor.forClass(ResourceNotFoundException.class);
verify(listener).onFailure(exceptionArgumentCaptor.capture());
);

final var exception = exceptionArgumentCaptor.getValue();
final var exception = expectThrows(ResourceNotFoundException.class, () -> execute(action, clusterState));
assertEquals("Desired balance allocator is not in use, no desired balance found", exception.getMessage());
assertThat(exception.status(), equalTo(RestStatus.NOT_FOUND));
}
Expand All @@ -104,12 +117,10 @@ public void testReturnsErrorIfDesiredBalanceIsNotAvailable() throws Exception {
.metadata(metadataWithConfiguredAllocator(DESIRED_BALANCE_ALLOCATOR))
.build();

transportGetDesiredBalanceAction.masterOperation(mock(Task.class), mock(DesiredBalanceRequest.class), clusterState, listener);

ArgumentCaptor<ResourceNotFoundException> exceptionArgumentCaptor = ArgumentCaptor.forClass(ResourceNotFoundException.class);
verify(listener).onFailure(exceptionArgumentCaptor.capture());

assertEquals("Desired balance is not computed yet", exceptionArgumentCaptor.getValue().getMessage());
assertEquals(
"Desired balance is not computed yet",
expectThrows(ResourceNotFoundException.class, () -> executeAction(clusterState)).getMessage()
);
}

public void testGetDesiredBalance() throws Exception {
Expand Down Expand Up @@ -220,15 +231,15 @@ public void testGetDesiredBalance() throws Exception {
.routingTable(routingTable)
.build();

transportGetDesiredBalanceAction.masterOperation(mock(Task.class), mock(DesiredBalanceRequest.class), clusterState, listener);

ArgumentCaptor<DesiredBalanceResponse> desiredBalanceResponseCaptor = ArgumentCaptor.forClass(DesiredBalanceResponse.class);
verify(listener).onResponse(desiredBalanceResponseCaptor.capture());
DesiredBalanceResponse desiredBalanceResponse = desiredBalanceResponseCaptor.getValue();
final var desiredBalanceResponse = executeAction(clusterState);
assertThat(desiredBalanceResponse.getStats(), equalTo(desiredBalanceStats));
assertThat(desiredBalanceResponse.getClusterBalanceStats(), notNullValue());
assertThat(desiredBalanceResponse.getClusterInfo(), equalTo(clusterInfo));
assertEquals(indexShards.keySet(), desiredBalanceResponse.getRoutingTable().keySet());

assertEquals(desiredBalanceResponse, copyWriteable(desiredBalanceResponse, writableRegistry(), DesiredBalanceResponse::from));
AbstractChunkedSerializingTestCase.assertChunkCount(desiredBalanceResponse, r -> 2 + r.getRoutingTable().size());

for (var e : desiredBalanceResponse.getRoutingTable().entrySet()) {
String index = e.getKey();
Map<Integer, DesiredBalanceResponse.DesiredShards> shardsMap = e.getValue();
Expand Down Expand Up @@ -267,14 +278,14 @@ public void testGetDesiredBalance() throws Exception {
);
assertEquals(indexMetadata.getTierPreference(), shardView.tierPreference());
}
Optional<ShardAssignment> shardAssignment = Optional.ofNullable(shardAssignments.get(indexShardRoutingTable.shardId()));
if (shardAssignment.isPresent()) {
assertEquals(shardAssignment.get().nodeIds(), desiredShard.desired().nodeIds());
assertEquals(shardAssignment.get().total(), desiredShard.desired().total());
assertEquals(shardAssignment.get().unassigned(), desiredShard.desired().unassigned());
assertEquals(shardAssignment.get().ignored(), desiredShard.desired().ignored());
final var shardAssignment = shardAssignments.get(indexShardRoutingTable.shardId());
if (shardAssignment == null) {
Copy link
Contributor

@idegtiarenko idegtiarenko Jul 19, 2023

Choose a reason for hiding this comment

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

Is this test generating null assignments?
Copy link
Contributor Author

@DaveCTurner DaveCTurner Jul 19, 2023

Choose a reason for hiding this comment

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

Yes it only assigns some random subset of the shards.
assertSame(desiredShard.desired(), DesiredBalanceResponse.ShardAssignmentView.EMPTY);
} else {
assertNull(desiredShard.desired());
assertEquals(shardAssignment.nodeIds(), desiredShard.desired().nodeIds());
assertEquals(shardAssignment.total(), desiredShard.desired().total());
assertEquals(shardAssignment.unassigned(), desiredShard.desired().unassigned());
assertEquals(shardAssignment.ignored(), desiredShard.desired().ignored());
}
}
}
Expand Down