Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
6 changes: 6 additions & 0 deletions docs/changelog/125054.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 125054
summary: Truncate `step_info` and error reason in ILM execution state and history
area: ILM+SLM
type: enhancement
issues:
- 124181
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
package org.elasticsearch.cluster.metadata;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.Strings;

import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -42,6 +43,7 @@ public record LifecycleExecutionState(
) {

public static final String ILM_CUSTOM_METADATA_KEY = "ilm";
public static final int MAXIMUM_STEP_INFO_STRING_LENGTH = 1024;

private static final String PHASE = "phase";
private static final String ACTION = "action";
Expand Down Expand Up @@ -308,7 +310,7 @@ public Builder setFailedStep(String failedStep) {
}

public Builder setStepInfo(String stepInfo) {
this.stepInfo = stepInfo;
this.stepInfo = Strings.cleanTruncate(stepInfo, MAXIMUM_STEP_INFO_STRING_LENGTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be valuable to add some truncation information to the string, i.e. ... x chars more or ... x chars truncated. If someone is looking at these step infos (from the API or the history), I think it would be valuable to know for sure that the string was truncated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I added this!

return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import java.util.HashMap;
import java.util.Map;

import static org.hamcrest.Matchers.equalTo;

public class LifecycleExecutionStateTests extends ESTestCase {

public void testConversion() {
Expand All @@ -22,6 +24,15 @@ public void testConversion() {
assertEquals(customMetadata, parsed.asMap());
}

public void testTruncatingStepInfo() {
Map<String, String> custom = createCustomMetadata();
LifecycleExecutionState state = LifecycleExecutionState.fromCustomMetadata(custom);
assertThat(custom.get("step_info"), equalTo(state.stepInfo()));
String longStepInfo = randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 20);
LifecycleExecutionState newState = LifecycleExecutionState.builder(state).setStepInfo(longStepInfo).build();
assertThat(newState.stepInfo().length(), equalTo(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH));
}

public void testEmptyValuesAreNotSerialized() {
LifecycleExecutionState empty = LifecycleExecutionState.builder().build();
assertEquals(new HashMap<String, String>().entrySet(), empty.asMap().entrySet());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,15 @@ public static ILMHistoryItem failure(
Exception error
) {
Objects.requireNonNull(error, "ILM failures require an attached exception");
return new ILMHistoryItem(index, policyId, timestamp, indexAge, false, executionState, exceptionToString(error));
String fullErrorString = exceptionToString(error);
String truncatedErrorString = Strings.cleanTruncate(fullErrorString, LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH);
if (truncatedErrorString.equals(fullErrorString) == false) {
// Append a closing quote and closing brace to attempt to make it valid JSON.
// There is no requirement that it actually be valid JSON, so this is
// best-effort, but does not cause problems if it is still invalid.
truncatedErrorString += "\"}";
}
return new ILMHistoryItem(index, policyId, timestamp, indexAge, false, executionState, truncatedErrorString);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,13 @@
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentFactory;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xcontent.XContentParserConfiguration;
import org.elasticsearch.xcontent.XContentType;

import java.io.IOException;
import java.util.Map;

import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -111,4 +116,34 @@ public void testToXContent() throws IOException {
\\"stack_trace\\":\\"java.lang.IllegalArgumentException: failure""".replaceAll("\\s", "")));
}
}

public void testTruncateLongError() throws IOException {
String longError = randomAlphaOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 20);
ILMHistoryItem failure = ILMHistoryItem.failure(
"index",
"policy",
1234L,
100L,
LifecycleExecutionState.EMPTY_STATE,
new IllegalArgumentException(longError)
);

try (XContentBuilder builder = jsonBuilder()) {
failure.toXContent(builder, ToXContent.EMPTY_PARAMS);
String json = Strings.toString(builder);
try (XContentParser p = XContentFactory.xContent(XContentType.JSON).createParser(XContentParserConfiguration.EMPTY, json)) {
Map<String, Object> item = p.map();
assertThat(
item.get("error_details"),
equalTo(
"{\"type\":\"illegal_argument_exception\",\"reason\":\""
// We subtract a number of characters here due to the truncation being based
// on the length of the whole string, not just the "reason" part.
+ longError.substring(0, LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - 47)
+ "\"}"
)
);
}
}
}
}