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

Fix to load indices status in batches so that the URL used to call OpenSearch does not exceed maximum length #21208

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions changelog/unreleased/pr-21208.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
type = "f"
message = "batching request for index block status if the combined length of the indices exceed the max possible URL length "

pulls = ["21208"]
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ public class IndicesAdapterES7 implements IndicesAdapter {
private final ClusterStateApi clusterStateApi;
private final IndexTemplateAdapter indexTemplateAdapter;

// this is the maximum amount of bytes that the index list is supposed to fill in a request,
// it assumes that these don't need url encoding. If we exceed the maximum, we request settings for all indices
// and filter after wards
private final int MAX_INDICES_URL_LENGTH = 3000;

@Inject
public IndicesAdapterES7(ElasticsearchClient client,
StatsApi statsApi,
Expand Down Expand Up @@ -435,15 +440,24 @@ public IndicesBlockStatus getIndicesBlocksStatus(final List<String> indices) {
if (indices == null || indices.isEmpty()) {
throw new IllegalArgumentException("Expecting list of indices with at least one index present.");
}
final GetSettingsRequest getSettingsRequest = new GetSettingsRequest()
.indices(indices.toArray(new String[]{}))
.indicesOptions(IndicesOptions.fromOptions(false, true, true, true))
.names(new String[]{});

return client.execute((c, requestOptions) -> {
final GetSettingsResponse settingsResponse = c.indices().getSettings(getSettingsRequest, requestOptions);
return BlockSettingsParser.parseBlockSettings(settingsResponse);
});
if(String.join(",", indices).length() > MAX_INDICES_URL_LENGTH) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should safe some buffer here, as there will also be the path + additional query parameters (+ potential url encoding overhead?). Let's play it safe and reduce MAX_INDICES_URL_LENGTH to 2500?

final GetSettingsRequest getSettingsRequest = new GetSettingsRequest()
.indicesOptions(IndicesOptions.fromOptions(false, true, true, true))
.names(new String[]{});
final GetSettingsResponse settingsResponse = client.execute((c, requestOptions) -> c.indices().getSettings(getSettingsRequest, requestOptions));
return BlockSettingsParser.parseBlockSettings(settingsResponse, Optional.of(indices));
} else {
final GetSettingsRequest getSettingsRequest = new GetSettingsRequest()
.indices(indices.toArray(new String[]{}))
.indicesOptions(IndicesOptions.fromOptions(false, true, true, true))
.names(new String[]{});

return client.execute((c, requestOptions) -> {
final GetSettingsResponse settingsResponse = c.indices().getSettings(getSettingsRequest, requestOptions);
return BlockSettingsParser.parseBlockSettings(settingsResponse);
});
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
import org.graylog.shaded.elasticsearch7.org.elasticsearch.common.settings.Settings;
import org.graylog2.indexer.indices.blocks.IndicesBlockStatus;

import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;

Expand All @@ -29,23 +32,31 @@ public class BlockSettingsParser {
static final String BLOCK_SETTINGS_PREFIX = "index.blocks.";

public static IndicesBlockStatus parseBlockSettings(final GetSettingsResponse settingsResponse) {
IndicesBlockStatus result = new IndicesBlockStatus();
return parseBlockSettings(settingsResponse, Optional.empty());
}

public static IndicesBlockStatus parseBlockSettings(final GetSettingsResponse settingsResponse, final Optional<List<String>> indices) {
final IndicesBlockStatus result = new IndicesBlockStatus();
final ImmutableOpenMap<String, Settings> indexToSettingsMap = settingsResponse.getIndexToSettings();
final String[] indicesInResponse = indexToSettingsMap.keys().toArray(String.class);
for (String index : indicesInResponse) {
final Settings blockSettings = indexToSettingsMap.get(index).getByPrefix(BLOCK_SETTINGS_PREFIX);

if (!blockSettings.isEmpty()) {
final Set<String> blockSettingsNames = blockSettings.names();
final Set<String> blockSettingsSetToTrue = blockSettingsNames.stream()
.filter(s -> blockSettings.getAsBoolean(s, false))
.map(s -> BLOCK_SETTINGS_PREFIX + s)
.collect(Collectors.toSet());
if (!blockSettingsSetToTrue.isEmpty()) {
result.addIndexBlocks(index, blockSettingsSetToTrue);

indices.orElse(Arrays.stream(indicesInResponse).toList()).forEach(index -> {
final var settings = indexToSettingsMap.get(index);
if(settings != null) {
final Settings blockSettings = settings.getByPrefix(BLOCK_SETTINGS_PREFIX);

if (!blockSettings.isEmpty()) {
final Set<String> blockSettingsNames = blockSettings.names();
final Set<String> blockSettingsSetToTrue = blockSettingsNames.stream()
.filter(s -> blockSettings.getAsBoolean(s, false))
.map(s -> BLOCK_SETTINGS_PREFIX + s)
.collect(Collectors.toSet());
if (!blockSettingsSetToTrue.isEmpty()) {
result.addIndexBlocks(index, blockSettingsSetToTrue);
}
}
}
}
});

return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -97,6 +98,7 @@
import java.util.function.Consumer;
import java.util.stream.Collectors;

import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
import static org.graylog.storage.opensearch2.OpenSearchClient.withTimeout;

Expand All @@ -109,6 +111,11 @@ public class IndicesAdapterOS2 implements IndicesAdapter {
private final ClusterStateApi clusterStateApi;
private final IndexTemplateAdapter indexTemplateAdapter;

// this is the maximum amount of bytes that the index list is supposed to fill in a request,
// it assumes that these don't need url encoding. If we exceed the maximum, we request settings for all indices
// and filter after wards
private final int MAX_INDICES_URL_LENGTH = 3000;

@Inject
public IndicesAdapterOS2(OpenSearchClient client,
StatsApi statsApi,
Expand Down Expand Up @@ -431,20 +438,30 @@ public List<ShardsInfo> getShardsInfo(String indexName) {
return catApi.getShardsInfo(indexName);
}


@Override
public IndicesBlockStatus getIndicesBlocksStatus(final List<String> indices) {
if (indices == null || indices.isEmpty()) {
throw new IllegalArgumentException("Expecting list of indices with at least one index present.");
}
final GetSettingsRequest getSettingsRequest = new GetSettingsRequest()
.indices(indices.toArray(new String[]{}))
.indicesOptions(IndicesOptions.fromOptions(false, true, true, true))
.names(new String[]{});

return client.execute((c, requestOptions) -> {
final GetSettingsResponse settingsResponse = c.indices().getSettings(getSettingsRequest, requestOptions);
return BlockSettingsParser.parseBlockSettings(settingsResponse);
});
if(String.join(",", indices).length() > MAX_INDICES_URL_LENGTH) {
final GetSettingsRequest getSettingsRequest = new GetSettingsRequest()
.indicesOptions(IndicesOptions.fromOptions(false, true, true, true))
Copy link
Member

Choose a reason for hiding this comment

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

Can we use .names("index.blocks.read", ...) to get only the blocks here?

.names(new String[]{});
final GetSettingsResponse settingsResponse = client.execute((c, requestOptions) -> c.indices().getSettings(getSettingsRequest, requestOptions));
return BlockSettingsParser.parseBlockSettings(settingsResponse, Optional.of(indices));
} else {
final GetSettingsRequest getSettingsRequest = new GetSettingsRequest()
.indices(indices.toArray(new String[]{}))
.indicesOptions(IndicesOptions.fromOptions(false, true, true, true))
.names(new String[]{});

return client.execute((c, requestOptions) -> {
final GetSettingsResponse settingsResponse = c.indices().getSettings(getSettingsRequest, requestOptions);
return BlockSettingsParser.parseBlockSettings(settingsResponse);
});
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import org.graylog.shaded.opensearch2.org.opensearch.action.admin.indices.settings.get.GetSettingsResponse;
import org.graylog.shaded.opensearch2.org.opensearch.common.settings.Settings;

import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;

Expand All @@ -28,23 +30,30 @@ public class BlockSettingsParser {
static final String BLOCK_SETTINGS_PREFIX = "index.blocks.";

public static IndicesBlockStatus parseBlockSettings(final GetSettingsResponse settingsResponse) {
IndicesBlockStatus result = new IndicesBlockStatus();
return parseBlockSettings(settingsResponse, Optional.empty());
}

public static IndicesBlockStatus parseBlockSettings(final GetSettingsResponse settingsResponse, final Optional<List<String>> indices) {
final IndicesBlockStatus result = new IndicesBlockStatus();
final var indexToSettingsMap = settingsResponse.getIndexToSettings();
final String[] indicesInResponse = indexToSettingsMap.keySet().toArray(new String[0]);
for (String index : indicesInResponse) {
final Settings blockSettings = indexToSettingsMap.get(index).getByPrefix(BLOCK_SETTINGS_PREFIX);

if (!blockSettings.isEmpty()) {
final Set<String> blockSettingsNames = blockSettings.names();
final Set<String> blockSettingsSetToTrue = blockSettingsNames.stream()
.filter(s -> blockSettings.getAsBoolean(s, false))
.map(s -> BLOCK_SETTINGS_PREFIX + s)
.collect(Collectors.toSet());
if (!blockSettingsSetToTrue.isEmpty()) {
result.addIndexBlocks(index, blockSettingsSetToTrue);

indices.orElse(indexToSettingsMap.keySet().stream().toList()).forEach(index -> {
final var settings = indexToSettingsMap.get(index);
if(settings != null) {
final Settings blockSettings = settings.getByPrefix(BLOCK_SETTINGS_PREFIX);

if (!blockSettings.isEmpty()) {
final Set<String> blockSettingsNames = blockSettings.names();
final Set<String> blockSettingsSetToTrue = blockSettingsNames.stream()
.filter(s -> blockSettings.getAsBoolean(s, false))
.map(s -> BLOCK_SETTINGS_PREFIX + s)
.collect(Collectors.toSet());
if (!blockSettingsSetToTrue.isEmpty()) {
result.addIndexBlocks(index, blockSettingsSetToTrue);
}
}
}
}
});

return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ public class BlockSettingsParserTest {
@Test
public void noBlockedIndicesIdentifiedIfEmptyResponseParsed() {
GetSettingsResponse emptyResponse = new GetSettingsResponse(Map.of(), Map.of());
final IndicesBlockStatus indicesBlockStatus = BlockSettingsParser.parseBlockSettings(emptyResponse);
final IndicesBlockStatus indicesBlockStatus = new IndicesBlockStatus();
BlockSettingsParser.parseBlockSettings(indicesBlockStatus, emptyResponse);
assertNotNull(indicesBlockStatus);
assertEquals(0, indicesBlockStatus.countBlockedIndices());
}
Expand All @@ -44,7 +45,8 @@ public void noBlockedIndicesIdentifiedIfEmptyResponseParsed() {
public void noBlockedIndicesIdentifiedIfEmptySettingsPresent() {
var settingsBuilder = Map.of("index_0", Settings.builder().build());
GetSettingsResponse emptySettingsResponse = new GetSettingsResponse(settingsBuilder, Map.of());
final IndicesBlockStatus indicesBlockStatus = BlockSettingsParser.parseBlockSettings(emptySettingsResponse);
final IndicesBlockStatus indicesBlockStatus = new IndicesBlockStatus();
BlockSettingsParser.parseBlockSettings(indicesBlockStatus, emptySettingsResponse);
assertNotNull(indicesBlockStatus);
assertEquals(0, indicesBlockStatus.countBlockedIndices());
}
Expand All @@ -64,7 +66,8 @@ public void parserProperlyResponseWithMultipleIndicesWithDifferentBlockSettings(
.put("index.blocks.read_only_allow_delete", true)
.build());
GetSettingsResponse settingsResponse = new GetSettingsResponse(settingsBuilder, Map.of());
final IndicesBlockStatus indicesBlockStatus = BlockSettingsParser.parseBlockSettings(settingsResponse);
final IndicesBlockStatus indicesBlockStatus = new IndicesBlockStatus();
BlockSettingsParser.parseBlockSettings(indicesBlockStatus, settingsResponse);
assertNotNull(indicesBlockStatus);
assertEquals(3, indicesBlockStatus.countBlockedIndices());
final Set<String> blockedIndices = indicesBlockStatus.getBlockedIndices();
Expand Down
Loading