Skip to content

Commit 50f8012

Browse files
prdoyleprdoyle
authored
Report GREEN status when FileSettingsService is stopped (#118182)
* Fix FileSettingsHealthIndicatorService on stop. When FileSettingsService is stopped, the health should go GREEN. * Update FileSettingsHealthIndicatorServiceTests * Spotless
1 parent ae9bb90 commit 50f8012

File tree

4 files changed

+71
-35
lines changed

4 files changed

+71
-35
lines changed

‎muted-tests.yml‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -171,9 +171,6 @@ tests:
171171
- class: org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT
172172
method: test {p0=synonyms/90_synonyms_reloading_for_synset/Reload analyzers for specific synonym set}
173173
issue: https://github.com/elastic/elasticsearch/issues/116777
174-
- class: org.elasticsearch.reservedstate.service.FileSettingsServiceTests
175-
method: testStopWorksInMiddleOfProcessing
176-
issue: https://github.com/elastic/elasticsearch/issues/117591
177174
- class: "org.elasticsearch.xpack.esql.qa.multi_node.EsqlSpecIT"
178175
method: "test {scoring.*}"
179176
issue: https://github.com/elastic/elasticsearch/issues/117641

‎server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java‎

Lines changed: 42 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,6 @@
3737
import java.util.List;
3838
import java.util.Map;
3939
import java.util.concurrent.ExecutionException;
40-
import java.util.concurrent.atomic.AtomicLong;
41-
import java.util.concurrent.atomic.AtomicReference;
4240

4341
import static org.elasticsearch.health.HealthStatus.GREEN;
4442
import static org.elasticsearch.health.HealthStatus.YELLOW;
@@ -122,6 +120,18 @@ public void handleSnapshotRestore(ClusterState clusterState, Metadata.Builder md
122120
}
123121
}
124122

123+
@Override
124+
protected void doStart() {
125+
healthIndicatorService.startOccurred();
126+
super.doStart();
127+
}
128+
129+
@Override
130+
protected void doStop() {
131+
super.doStop();
132+
healthIndicatorService.stopOccurred();
133+
}
134+
125135
/**
126136
* If the file settings metadata version is set to zero, then we have restored from
127137
* a snapshot and must reprocess the file.
@@ -211,6 +221,7 @@ protected void processInitialFileMissing() throws ExecutionException, Interrupte
211221

212222
public static class FileSettingsHealthIndicatorService implements HealthIndicatorService {
213223
static final String NAME = "file_settings";
224+
static final String INACTIVE_SYMPTOM = "File-based settings are inactive";
214225
static final String NO_CHANGES_SYMPTOM = "No file-based setting changes have occurred";
215226
static final String SUCCESS_SYMPTOM = "The most recent file-based settings were applied successfully";
216227
static final String FAILURE_SYMPTOM = "The most recent file-based settings encountered an error";
@@ -225,21 +236,33 @@ public static class FileSettingsHealthIndicatorService implements HealthIndicato
225236
)
226237
);
227238

228-
private final AtomicLong changeCount = new AtomicLong(0);
229-
private final AtomicLong failureStreak = new AtomicLong(0);
230-
private final AtomicReference<String> mostRecentFailure = new AtomicReference<>();
239+
private boolean isActive = false;
240+
private long changeCount = 0;
241+
private long failureStreak = 0;
242+
private String mostRecentFailure = null;
231243

232-
public void changeOccurred() {
233-
changeCount.incrementAndGet();
244+
public synchronized void startOccurred() {
245+
isActive = true;
246+
failureStreak = 0;
234247
}
235248

236-
public void successOccurred() {
237-
failureStreak.set(0);
249+
public synchronized void stopOccurred() {
250+
isActive = false;
251+
mostRecentFailure = null;
238252
}
239253

240-
public void failureOccurred(String description) {
241-
failureStreak.incrementAndGet();
242-
mostRecentFailure.set(description);
254+
public synchronized void changeOccurred() {
255+
++changeCount;
256+
}
257+
258+
public synchronized void successOccurred() {
259+
failureStreak = 0;
260+
mostRecentFailure = null;
261+
}
262+
263+
public synchronized void failureOccurred(String description) {
264+
++failureStreak;
265+
mostRecentFailure = description;
243266
}
244267

245268
@Override
@@ -248,18 +271,20 @@ public String name() {
248271
}
249272

250273
@Override
251-
public HealthIndicatorResult calculate(boolean verbose, int maxAffectedResourcesCount, HealthInfo healthInfo) {
252-
if (0 == changeCount.get()) {
274+
public synchronized HealthIndicatorResult calculate(boolean verbose, int maxAffectedResourcesCount, HealthInfo healthInfo) {
275+
if (isActive == false) {
276+
return createIndicator(GREEN, INACTIVE_SYMPTOM, HealthIndicatorDetails.EMPTY, List.of(), List.of());
277+
}
278+
if (0 == changeCount) {
253279
return createIndicator(GREEN, NO_CHANGES_SYMPTOM, HealthIndicatorDetails.EMPTY, List.of(), List.of());
254280
}
255-
long numFailures = failureStreak.get();
256-
if (0 == numFailures) {
281+
if (0 == failureStreak) {
257282
return createIndicator(GREEN, SUCCESS_SYMPTOM, HealthIndicatorDetails.EMPTY, List.of(), List.of());
258283
} else {
259284
return createIndicator(
260285
YELLOW,
261286
FAILURE_SYMPTOM,
262-
new SimpleHealthIndicatorDetails(Map.of("failure_streak", numFailures, "most_recent_failure", mostRecentFailure.get())),
287+
new SimpleHealthIndicatorDetails(Map.of("failure_streak", failureStreak, "most_recent_failure", mostRecentFailure)),
263288
STALE_SETTINGS_IMPACT,
264289
List.of()
265290
);

‎server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsHealthIndicatorServiceTests.java‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import static org.elasticsearch.health.HealthStatus.GREEN;
2323
import static org.elasticsearch.health.HealthStatus.YELLOW;
2424
import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.FAILURE_SYMPTOM;
25+
import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.INACTIVE_SYMPTOM;
2526
import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.NO_CHANGES_SYMPTOM;
2627
import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.STALE_SETTINGS_IMPACT;
2728
import static org.elasticsearch.reservedstate.service.FileSettingsService.FileSettingsHealthIndicatorService.SUCCESS_SYMPTOM;
@@ -39,14 +40,27 @@ public void initialize() {
3940
healthIndicatorService = new FileSettingsHealthIndicatorService();
4041
}
4142

42-
public void testInitiallyGreen() {
43+
public void testInitiallyGreen() {}
44+
45+
public void testStartAndStop() {
46+
assertEquals(
47+
new HealthIndicatorResult("file_settings", GREEN, INACTIVE_SYMPTOM, HealthIndicatorDetails.EMPTY, List.of(), List.of()),
48+
healthIndicatorService.calculate(false, null)
49+
);
50+
healthIndicatorService.startOccurred();
4351
assertEquals(
4452
new HealthIndicatorResult("file_settings", GREEN, NO_CHANGES_SYMPTOM, HealthIndicatorDetails.EMPTY, List.of(), List.of()),
4553
healthIndicatorService.calculate(false, null)
4654
);
55+
healthIndicatorService.stopOccurred();
56+
assertEquals(
57+
new HealthIndicatorResult("file_settings", GREEN, INACTIVE_SYMPTOM, HealthIndicatorDetails.EMPTY, List.of(), List.of()),
58+
healthIndicatorService.calculate(false, null)
59+
);
4760
}
4861

4962
public void testGreenYellowYellowGreen() {
63+
healthIndicatorService.startOccurred();
5064
healthIndicatorService.changeOccurred();
5165
// This is a strange case: a change occurred, but neither success nor failure have been reported yet.
5266
// While the change is still in progress, we don't change the status.

‎server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
import org.elasticsearch.cluster.ClusterName;
1818
import org.elasticsearch.cluster.ClusterState;
1919
import org.elasticsearch.cluster.NodeConnectionsService;
20-
import org.elasticsearch.cluster.coordination.FailedToCommitClusterStateException;
2120
import org.elasticsearch.cluster.metadata.Metadata;
2221
import org.elasticsearch.cluster.metadata.ReservedStateMetadata;
2322
import org.elasticsearch.cluster.node.DiscoveryNode;
@@ -69,6 +68,8 @@
6968

7069
import static java.nio.file.StandardCopyOption.ATOMIC_MOVE;
7170
import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
71+
import static org.elasticsearch.health.HealthStatus.GREEN;
72+
import static org.elasticsearch.health.HealthStatus.YELLOW;
7273
import static org.elasticsearch.node.Node.NODE_NAME_SETTING;
7374
import static org.hamcrest.Matchers.anEmptyMap;
7475
import static org.hamcrest.Matchers.hasEntry;
@@ -82,8 +83,6 @@
8283
import static org.mockito.Mockito.spy;
8384
import static org.mockito.Mockito.times;
8485
import static org.mockito.Mockito.verify;
85-
import static org.mockito.Mockito.verifyNoInteractions;
86-
import static org.mockito.Mockito.verifyNoMoreInteractions;
8786

8887
public class FileSettingsServiceTests extends ESTestCase {
8988
private static final Logger logger = LogManager.getLogger(FileSettingsServiceTests.class);
@@ -138,7 +137,7 @@ public void setUp() throws Exception {
138137
List.of(new ReservedClusterSettingsAction(clusterSettings))
139138
)
140139
);
141-
healthIndicatorService = mock(FileSettingsHealthIndicatorService.class);
140+
healthIndicatorService = spy(new FileSettingsHealthIndicatorService());
142141
fileSettingsService = spy(new FileSettingsService(clusterService, controller, env, healthIndicatorService));
143142
}
144143

@@ -170,7 +169,8 @@ public void testStartStop() {
170169
assertTrue(fileSettingsService.watching());
171170
fileSettingsService.stop();
172171
assertFalse(fileSettingsService.watching());
173-
verifyNoInteractions(healthIndicatorService);
172+
verify(healthIndicatorService, times(1)).startOccurred();
173+
verify(healthIndicatorService, times(1)).stopOccurred();
174174
}
175175

176176
public void testOperatorDirName() {
@@ -218,9 +218,9 @@ public void testInitialFileError() throws Exception {
218218
// assert we never notified any listeners of successful application of file based settings
219219
assertFalse(settingsChanged.get());
220220

221+
assertEquals(YELLOW, healthIndicatorService.calculate(false, null).status());
221222
verify(healthIndicatorService, times(1)).changeOccurred();
222223
verify(healthIndicatorService, times(1)).failureOccurred(argThat(s -> s.startsWith(IllegalStateException.class.getName())));
223-
verifyNoMoreInteractions(healthIndicatorService);
224224
}
225225

226226
@SuppressWarnings("unchecked")
@@ -246,9 +246,9 @@ public void testInitialFileWorks() throws Exception {
246246
verify(fileSettingsService, times(1)).processFileOnServiceStart();
247247
verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_OR_SAME_VERSION), any());
248248

249+
assertEquals(GREEN, healthIndicatorService.calculate(false, null).status());
249250
verify(healthIndicatorService, times(1)).changeOccurred();
250251
verify(healthIndicatorService, times(1)).successOccurred();
251-
verifyNoMoreInteractions(healthIndicatorService);
252252
}
253253

254254
@SuppressWarnings("unchecked")
@@ -285,9 +285,9 @@ public void testProcessFileChanges() throws Exception {
285285
verify(fileSettingsService, times(1)).processFileChanges();
286286
verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_VERSION_ONLY), any());
287287

288+
assertEquals(GREEN, healthIndicatorService.calculate(false, null).status());
288289
verify(healthIndicatorService, times(2)).changeOccurred();
289290
verify(healthIndicatorService, times(2)).successOccurred();
290-
verifyNoMoreInteractions(healthIndicatorService);
291291
}
292292

293293
public void testInvalidJSON() throws Exception {
@@ -323,6 +323,7 @@ public void testInvalidJSON() throws Exception {
323323
// referring to fileSettingsService.start(). Rather, it is referring to the initialization
324324
// of the watcher thread itself, which occurs asynchronously when clusterChanged is first called.
325325

326+
assertEquals(YELLOW, healthIndicatorService.calculate(false, null).status());
326327
verify(healthIndicatorService).failureOccurred(contains(XContentParseException.class.getName()));
327328
}
328329

@@ -388,14 +389,13 @@ public void testStopWorksInMiddleOfProcessing() throws Exception {
388389
fileSettingsService.stop();
389390
assertFalse(fileSettingsService.watching());
390391
fileSettingsService.close();
392+
393+
// When the service is stopped, the health indicator should be green
394+
assertEquals(GREEN, healthIndicatorService.calculate(false, null).status());
395+
verify(healthIndicatorService).stopOccurred();
396+
391397
// let the deadlocked thread end, so we can cleanly exit the test
392398
deadThreadLatch.countDown();
393-
394-
verify(healthIndicatorService, times(1)).changeOccurred();
395-
verify(healthIndicatorService, times(1)).failureOccurred(
396-
argThat(s -> s.startsWith(FailedToCommitClusterStateException.class.getName()))
397-
);
398-
verifyNoMoreInteractions(healthIndicatorService);
399399
}
400400

401401
public void testHandleSnapshotRestoreClearsMetadata() throws Exception {

0 commit comments

Comments
 (0)