From 1a3269860c89544b800215b994dcface001fbfb8 Mon Sep 17 00:00:00 2001 From: Patrick Doyle Date: Thu, 30 Jan 2025 15:44:20 -0500 Subject: [PATCH 1/3] Add length limit FileSettingsHealthIndicatorService.description --- .../elasticsearch/node/NodeConstruction.java | 2 +- .../service/FileSettingsService.java | 27 ++++++++++++++++-- .../ingest/ReservedPipelineActionTests.java | 2 +- ...leSettingsHealthIndicatorServiceTests.java | 28 ++++++++++++++++++- .../service/FileSettingsServiceTests.java | 2 +- 5 files changed, 55 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/node/NodeConstruction.java b/server/src/main/java/org/elasticsearch/node/NodeConstruction.java index 61ac8bbbfc69a..beeb1c3c86a44 100644 --- a/server/src/main/java/org/elasticsearch/node/NodeConstruction.java +++ b/server/src/main/java/org/elasticsearch/node/NodeConstruction.java @@ -1067,7 +1067,7 @@ public Map searchFields() { actionModule.getReservedClusterStateService().installStateHandler(new ReservedRepositoryAction(repositoriesService)); actionModule.getReservedClusterStateService().installStateHandler(new ReservedPipelineAction()); - FileSettingsHealthIndicatorService fileSettingsHealthIndicatorService = new FileSettingsHealthIndicatorService(); + FileSettingsHealthIndicatorService fileSettingsHealthIndicatorService = new FileSettingsHealthIndicatorService(settings); FileSettingsService fileSettingsService = new FileSettingsService( clusterService, actionModule.getReservedClusterStateService(), diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index e36604f9a58c8..71233d30bfbab 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -21,6 +21,8 @@ import org.elasticsearch.cluster.metadata.ReservedStateMetadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.file.MasterNodeFileWatchingService; +import org.elasticsearch.common.settings.Setting; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.health.HealthIndicatorDetails; import org.elasticsearch.health.HealthIndicatorImpact; @@ -212,7 +214,7 @@ protected void onProcessFileChangesException(Exception e) { } @Override - protected void processInitialFileMissing() throws ExecutionException, InterruptedException, IOException { + protected void processInitialFileMissing() throws ExecutionException, InterruptedException { PlainActionFuture completion = new PlainActionFuture<>(); logger.info("setting file [{}] not found, initializing [{}] as empty", watchedFile(), NAMESPACE); stateService.initEmpty(NAMESPACE, completion); @@ -236,11 +238,23 @@ public static class FileSettingsHealthIndicatorService implements HealthIndicato ) ); + public static final String DESCRIPTION_LENGTH_LIMIT_KEY = "fileSettings.descriptionLengthLimit"; + static final Setting DESCRIPTION_LENGTH_LIMIT = Setting.intSetting( + DESCRIPTION_LENGTH_LIMIT_KEY, + 100, + 1, // Need room for the ellipsis + Setting.Property.OperatorDynamic); + + private final Settings settings; private boolean isActive = false; private long changeCount = 0; private long failureStreak = 0; private String mostRecentFailure = null; + public FileSettingsHealthIndicatorService(Settings settings) { + this.settings = settings; + } + public synchronized void startOccurred() { isActive = true; failureStreak = 0; @@ -262,7 +276,16 @@ public synchronized void successOccurred() { public synchronized void failureOccurred(String description) { ++failureStreak; - mostRecentFailure = description; + mostRecentFailure = limitLength(description); + } + + private String limitLength(String description) { + int descriptionLengthLimit = DESCRIPTION_LENGTH_LIMIT.get(settings); + if (description.length() > descriptionLengthLimit) { + return description.substring(0, descriptionLengthLimit - 1) + "…"; + } else { + return description; + } } @Override diff --git a/server/src/test/java/org/elasticsearch/action/ingest/ReservedPipelineActionTests.java b/server/src/test/java/org/elasticsearch/action/ingest/ReservedPipelineActionTests.java index dc1698e3459ec..41a5919060095 100644 --- a/server/src/test/java/org/elasticsearch/action/ingest/ReservedPipelineActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/ingest/ReservedPipelineActionTests.java @@ -138,7 +138,7 @@ public void setup() { clusterService, mock(ReservedClusterStateService.class), newEnvironment(Settings.EMPTY), - new FileSettingsService.FileSettingsHealthIndicatorService() + new FileSettingsService.FileSettingsHealthIndicatorService(Settings.EMPTY) ) ); } diff --git a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsHealthIndicatorServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsHealthIndicatorServiceTests.java index 20ea43910e68d..e973073efb184 100644 --- a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsHealthIndicatorServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsHealthIndicatorServiceTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.reservedstate.service; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.health.HealthIndicatorDetails; import org.elasticsearch.health.HealthIndicatorResult; import org.elasticsearch.health.SimpleHealthIndicatorDetails; @@ -21,6 +22,7 @@ import static org.elasticsearch.health.HealthStatus.GREEN; import static org.elasticsearch.health.HealthStatus.YELLOW; +import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.DESCRIPTION_LENGTH_LIMIT_KEY; import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.FAILURE_SYMPTOM; import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.INACTIVE_SYMPTOM; import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.NO_CHANGES_SYMPTOM; @@ -37,7 +39,7 @@ public class FileSettingsHealthIndicatorServiceTests extends ESTestCase { @Before public void initialize() { - healthIndicatorService = new FileSettingsHealthIndicatorService(); + healthIndicatorService = new FileSettingsHealthIndicatorService(Settings.EMPTY); } public void testInitiallyGreen() {} @@ -101,4 +103,28 @@ public void testGreenYellowYellowGreen() { healthIndicatorService.calculate(false, null) ); } + + public void testDescriptionIsTruncated() { + checkTruncatedDescription(9, "123456789", "123456789"); + checkTruncatedDescription(8, "123456789", "1234567…"); + checkTruncatedDescription(1, "12", "…"); + } + + private void checkTruncatedDescription(int lengthLimit, String description, String expectedTruncatedDescription) { + var service = new FileSettingsHealthIndicatorService(Settings.builder().put(DESCRIPTION_LENGTH_LIMIT_KEY, lengthLimit).build()); + service.startOccurred(); + service.changeOccurred(); + service.failureOccurred(description); + assertEquals( + new HealthIndicatorResult( + "file_settings", + YELLOW, + FAILURE_SYMPTOM, + new SimpleHealthIndicatorDetails(Map.of("failure_streak", 1L, "most_recent_failure", expectedTruncatedDescription)), + STALE_SETTINGS_IMPACT, + List.of() + ), + service.calculate(false, null) + ); + } } diff --git a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java index 7cb12c1b316e8..78d57d62e21e1 100644 --- a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java @@ -138,7 +138,7 @@ public void setUp() throws Exception { List.of(new ReservedClusterSettingsAction(clusterSettings)) ) ); - healthIndicatorService = spy(new FileSettingsHealthIndicatorService()); + healthIndicatorService = spy(new FileSettingsHealthIndicatorService(Settings.EMPTY)); fileSettingsService = spy(new FileSettingsService(clusterService, controller, env, healthIndicatorService)); } From 3383e28c590e25025b152162add6aae596f5d43f Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Thu, 30 Jan 2025 21:12:45 +0000 Subject: [PATCH 2/3] [CI] Auto commit changes from spotless --- .../reservedstate/service/FileSettingsService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 71233d30bfbab..75dda5c0da8f3 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -243,7 +243,8 @@ public static class FileSettingsHealthIndicatorService implements HealthIndicato DESCRIPTION_LENGTH_LIMIT_KEY, 100, 1, // Need room for the ellipsis - Setting.Property.OperatorDynamic); + Setting.Property.OperatorDynamic + ); private final Settings settings; private boolean isActive = false; From cbc74a08eb1b7d2a82949e53dd79e69578f2117e Mon Sep 17 00:00:00 2001 From: Patrick Doyle Date: Fri, 31 Jan 2025 11:29:09 -0500 Subject: [PATCH 3/3] Add javadocs explaining `fileSettings.descriptionLengthLimit` setting --- .../reservedstate/service/FileSettingsService.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 71233d30bfbab..ad8ed52648aa8 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -238,6 +238,11 @@ public static class FileSettingsHealthIndicatorService implements HealthIndicato ) ); + /** + * We want a length limit so we don't blow past the indexing limit in the case of a long description string. + * This is an {@code OperatorDynamic} setting so that if the truncation hampers troubleshooting efforts, + * the operator could override it and retry the operation without necessarily restarting the cluster. + */ public static final String DESCRIPTION_LENGTH_LIMIT_KEY = "fileSettings.descriptionLengthLimit"; static final Setting DESCRIPTION_LENGTH_LIMIT = Setting.intSetting( DESCRIPTION_LENGTH_LIMIT_KEY,