File Settings Service #88329

grcevski · 2022-07-06T21:08:58Z

Watches a directory under Elasticsearch config for changes to a file called settings.json. This is a JSON file describing the settings that the operator wants to set that will be immutable through the REST API. Any changes to these settings will only be allowed through modifying the file. As configured by default we watch for a path operator/settings.json under the config directory. The name of the directory operator can be changed via settings.

The File Settings Service will notice updates to the file and perform an update. The operator directory doesn't have to exist, the file setting service will notice when it's created. Recommended way to change operator file based settings is through modifying the settings.json file in a brand new location and then performing a symlink under the Elasticsearch config directory.

Relates to #86224

Implement operator handlers for cluster state and ILM

elasticsearchmachine · 2022-07-06T21:09:23Z

Hi @grcevski, I've created a changelog YAML for you.

…arch into operator/file_service

grcevski · 2022-07-20T21:39:33Z

@elasticsearchmachine run elasticsearch-ci/part-2

Also, make sure we check error state version on all error paths, before we update the error state in the cluster state.

elasticsearchmachine · 2022-07-21T00:59:36Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst

Looks pretty good! I have several comments, mostly nits

rjernst · 2022-07-21T02:48:13Z

server/src/internalClusterTest/java/org/elasticsearch/threadpool/SimpleThreadPoolIT.java

                || threadName.contains("JVMCI-native") // GraalVM Compiler Thread
-                || threadName.contains("readiness-service")) {
+                || threadName.contains("file-settings-watcher")
+                || threadName.contains("FileSystemWatchService")) {


We have many other threadpools, but we've never needed to modify this test for each of them. Can we utilize the existing ThreadPools class to manage these threads, or do something so that we don't need to modify this for every system thread that is added?

I agree that's this is tedious to modify each time. We need to explicitly add each thread that's not in any threadpool. When I worked on the readiness-service, I was advised not to use the thread pools for threads that will constantly be running, so I followed the same approach here. FileSystemWatchService comes from the JDK, so there's no avoiding adding that one.

Is it OK if I followed up with a new PR to tackle this problem, rather than extend this PR further? I think I should be able to ensure all standalone Elasticsearch threads are registered in a way that this list doesn't have to be manually modified.

A followup is great

rjernst · 2022-07-21T02:52:35Z

server/src/main/java/org/elasticsearch/node/Node.java

@@ -1019,6 +1036,11 @@ protected Node(
                modules.add(b -> b.bind(ReadinessService.class).toInstance(new ReadinessService(clusterService, environment)));
            }

+            modules.add(
+                b -> b.bind(FileSettingsService.class)
+                    .toInstance(new FileSettingsService(clusterService, actionModule.getReservedClusterStateService(), environment))


This can be created much higher up and then added to the existing large bind. I know it's ugly. I don't feel strongly, since this method is an enormous mess, but I think it would be nice to follow the existing pattern for now, rather than creating another one (where we start calling modules.add all over again).

rjernst · 2022-07-21T02:59:02Z

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

+    // package private for testing
+    Path operatorSettingsDir() {
+        String dirPath = OPERATOR_DIRECTORY.get(environment.settings());
+        return environment.configFile().toAbsolutePath().resolve(dirPath);


Why not extract this into a member variable on construction? Then the environment member is not needed.

rjernst · 2022-07-21T03:04:35Z

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

+
+        Path settingsDir = operatorSettingsDir();
+
+        /**


trivial: not a javadoc, so no need for the extra *

rjernst · 2022-07-21T03:08:35Z

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

+                }
+                enableSettingsWatcher(settingsDir);
+            } else {
+                logger.info("operator settings directory [{}] not found, will watch for its creation...", settingsDir);


nit: This seems more like a debug level message?

rjernst · 2022-07-21T03:29:29Z

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

+                }
+            });
+        } catch (Exception e) {
+            logger.error("Error processing operator settings json file", e);


process doesn't throw anything checked. So why is this guarded? There is already a catch outside of this method that should handled logging the exception?

rjernst · 2022-07-21T03:31:08Z

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

+                if (logger.isDebugEnabled()) {
+                    logger.debug("encountered exception watching", e);
+                }
+                logger.info("shutting down watcher thread");


This would only happen when an exception occurs, so is this useful as an info message? Why not log the exception at error level, stating the watcher thread is shutting down? Isn't this always an exception case (not how the thread would be shutdown)?

I didn't want to pollute the logs with an exception trace when the watching is interrupted on the take() method call. It's how we interrupt the watcher when we stop watching, e.g. we are no longer the master node. So what I'll do here is log info when ClosedWatchServiceException is thrown and log error on any other exception.

We could have a separate catch for InterruptedException so that any unknown errors are logged at the appropriate level?

rjernst · 2022-07-21T03:32:50Z

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

+                    watcherThreadLatch.await();
+                }
+            } catch (IOException | InterruptedException e) {
+                logger.info("encountered exception while closing watch service", e);


why is this info instead of error or warn?

I'll switch to warn.

rjernst · 2022-07-21T03:35:15Z

server/src/test/java/org/elasticsearch/action/ActionModuleTests.java

-            null
+            null,
+            mock(ClusterService.class),
+            Collections.emptyList()


trivial: all these Collections.emptyList() can be List.of()

rjernst · 2022-07-21T03:38:28Z

test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java

@@ -76,4 +80,43 @@ public PluginsAndModules info() {
            List.of()
        );
    }
+
+    @Override
+    public <T> List<? extends T> loadServiceProviders(Class<T> service) {


Can this be moved out to a separate PR, with a unit test?

Sounds good, I made a new PR here #88690. Once that's merged I'll rebase and remove this change.

grcevski · 2022-07-23T02:01:39Z

I think I addressed all of the feedback and I added an integration test to ensure it all is wired up together and it's working.

grcevski · 2022-07-23T02:02:35Z

server/src/internalClusterTest/java/org/elasticsearch/threadpool/SimpleThreadPoolIT.java

                || threadName.contains("JVMCI-native") // GraalVM Compiler Thread
-                || threadName.contains("readiness-service")) {
+                || threadName.contains("file-settings-watcher")


I will tackle cleaning up this manual registration in a separate PR.

rjernst

A few more comments. LGTM, no need for anymore review if you agree with them.

rjernst · 2022-07-25T17:30:07Z

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

+                if (watcherThreadLatch != null) {
+                    watcherThreadLatch.await();
+                }
+            } catch (IOException | InterruptedException e) {


Stop happens on another ES thread, not one controlled here. So the interrupt should be restored for InterruptedException. (perhaps capture InterruptedException in another catch block). Note I think it is ok to not restore above since the thread is owned by this service and the thread will end after the interrupted exception is logged there.

rjernst · 2022-07-25T17:38:54Z

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

+                                processFileSettings(path, false);
+                            }
+                        } catch (Exception e) {
+                            logger.warn("error processing operator settings file", e);


How is this possible given processFileSettings logs any errors? It seems like the only possible error here is IOException? Could this be narrowed to that exception, and the warning message be distinguished from that on line 307? They are very similarly worded, yet this one, I don't think is specific to processing the file, it's just some IO problem happened while trying read it.

rjernst · 2022-07-25T17:42:28Z

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

+     * Error subclass that is thrown when we encounter a fatal error while applying
+     * the operator cluster state at Elasticsearch boot time.
+     */
+    public static class OperatorConfigurationError extends Error {


We shouldn't be extending Error (and should be renamed to avoid configuration, perhaps mentioning this is a startup problem in the name). This can extend RuntimeException so it can be propagated easily.

rjernst · 2022-07-25T17:44:10Z

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

+                    if (e != null) {
+                        // If we encountered an exception trying to apply the operator state at
+                        // startup time, we throw an error to force Elasticsearch to exit.
+                        if (onStartup) {


The logic here is a little difficult to reason about since it depends on this boolean (but the error here is then not possible outside of startup). Perhaps a more direct approach would be to pass a Consumer so the two callers can decide how to handle it.

rjernst · 2022-07-25T17:46:55Z

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

+    private volatile boolean active = false;
+    private volatile boolean initialState = true;
+
+    public static final Setting<String> OPERATOR_DIRECTORY = Setting.simpleString(


Why is this configurable? I think we could start with it hardcoded? I much prefer it is always the same, so that we know it will always be directly under the config dir.

Sure thing, I'll fix it.

rjernst · 2022-07-25T17:50:12Z

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

+         *  - any changes to files inside the operator directory if it exists, filtering for settings.json
+         */
+        try {
+            this.watchService = PathUtils.getDefaultFileSystem().newWatchService();


Rather than grab the default filesystem, it is probably better to grab it from the config dir?

Nikola Grcevski added 17 commits June 27, 2022 16:39

Implement few operator handlers

df61848

Implement operator handlers for cluster state and ILM

Add few ILM operator handler tests

407fd72

Merge branch 'master' into operator/state_ilm_handlers

b78d73f

Merge master

334f39c

Rename operator to immutablestate

2836ff7

Merge branch 'master' into operator/state_ilm_handlers

70f7202

Address PR review feedback

2c11e74

Add ImmutableClusterStateController

c636026

Merge master

da2654c

Merge branch 'operator/state_ilm_handlers' into operator/controller

49a1809

Merge master

3a66114

Fix test to run the transforms

4f54393

Merge branch 'master' into operator/controller

fcf8ef7

Refactor to split out parsing in handlers

16c3272

Fix test JSON parsing

d9273f4

Merge branch 'master' into operator/controller

d36a968

Add file settings service

81703a7

grcevski added >feature :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.4.0 labels Jul 6, 2022

Update docs/changelog/88329.yaml

dac7a6a

Nikola Grcevski added 7 commits July 6, 2022 18:29

Fix tests to reflect more debugging info

f58b7a8

Merge branch 'operator/file_service' of github.com:grcevski/elasticse…

30648a2

…arch into operator/file_service

Merge master

1f0ee7a

Update to latest version

f09a658

Remove unused helper

9357c28

Change package

089fe78

Fix MockNode

ecf8834

grcevski marked this pull request as ready for review July 20, 2022 17:05

grcevski mentioned this pull request Jul 20, 2022

Immutable cluster state controller #88224

Closed

Fix tests

7de8408

Clean-up duplicate error reporting

31e0df1

Also, make sure we check error state version on all error paths, before we update the error state in the cluster state.

rjernst reviewed Jul 21, 2022

View reviewed changes

Nikola Grcevski added 5 commits July 21, 2022 12:10

Apply some PR review comments

7877236

Address feedback

a702f9a

Update tests to handle more debug info

837919a

Merge master

e966002

Add integration test

bdee9c9

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:05

grcevski commented Jul 23, 2022

View reviewed changes

Remove unused method

2fe7fe5

grcevski requested review from rjernst and ChrisHegarty July 23, 2022 02:07

Spotless

8e6f84e

rjernst approved these changes Jul 25, 2022

View reviewed changes

Nikola Grcevski added 5 commits July 25, 2022 16:35

Fix tests for Windows.

dce6e17

Address further feedback

610b63b

Fix issue with missing config dir in tests

6552723

Merge main

f70cdfc

Fix merge issue

50e2fe1

grcevski merged commit f8a8df4 into elastic:main Jul 26, 2022

grcevski deleted the operator/file_service branch July 26, 2022 02:07

grcevski mentioned this pull request Aug 8, 2022

File Based Settings for Elasticsearch #89183

Closed

10 tasks

File Settings Service #88329

File Settings Service #88329

Uh oh!

Conversation

grcevski commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 6, 2022

Uh oh!

grcevski commented Jul 20, 2022

Uh oh!

elasticsearchmachine commented Jul 21, 2022

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grcevski commented Jul 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grcevski commented Jul 6, 2022 •

edited

Loading