Make watcher settings reloadable by hub-cap · Pull Request #31746 · elastic/elasticsearch

hub-cap · 2018-07-02T17:51:20Z

This commit allows for rebuilding watcher secure secrets via the
reload_secure_settings API call. The commit also renames a method in the
Notification Service to make it a bit more readable.

This commit allows for rebuilding watcher secure secrets via the reload_secure_settings API call. The commit also renames a method in the Notification Service to make it a bit more readable.

elasticmachine · 2018-07-02T17:51:22Z

Pinging @elastic/es-core-infra

spinscale

left a few comments

spinscale · 2018-07-02T21:38:05Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

    protected final boolean transportClient;
    protected final boolean enabled;
    protected final Environment env;
+    private SetOnce<EmailService> emailService = new SetOnce<>();


what about a List<NotificationService> that you fill in createComponents? This way you could get rid of the getReloadableServices() method entirely and loop through that list in the reload() method.

spinscale · 2018-07-02T21:39:13Z

.../watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java

    }

-    protected synchronized void setAccountSetting(Settings settings) {
+    public synchronized void loadSettings(Settings settings) {


how about naming this reload() as well just like in ReloadablePlugin plugin?

The only reason i did not name it reload is because it is used to init settings as well, but ill change it to reload.

spinscale · 2018-07-02T21:41:13Z

...r/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java

        clusterSettings.addAffixUpdateConsumer(SETTING_SECURE_SERVICE_API_KEY, (s, o) -> {}, (s, o) -> {});
        clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {});
-        setAccountSetting(settings);
+        loadSettings(settings);


is this PagerDutyService missing

clusterSettings.addSettingsUpdateConsumer(this::loadSettings, getSettings());

in its ctor?

In order to prevent this mistake, you could move

clusterSettings.addSettingsUpdateConsumer(this::loadSettings, getSettings());

into the NotificationService constructor and pass the getSettings() as arg to super()

I could also just have NotificationService impl a getSettings that the services overwrite. which is best?

ahh, nm, its static, it would be more work to change all that.

Im doing this in another commit, to ensure it does not muddy up this pull request. Ill edit this comment w/ the new PR once ive finished testing.

PR #31762

#31762 has been merged

spinscale · 2018-07-02T21:44:02Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/WatcherPluginTests.java

+
+    public void testReload() throws Exception {
+        Settings settings = Settings.builder()
+            .put("xpack.watcher.enabled", false)


watcher is not enabled, but reloading works? That could be a noop in this case, or?

alternatively you could mock a NotificationService and check with verify if the loadSettings method was called, no need to have an impl then

in terms of the enable, there is no check to see if the service is active, like there are in things like register components. But there should be, since the case involving a reload for a inactive plugin will attempt to register some things and likely fail miserably. Will add & add tests, and just mock them so i dont have to impl them.

spinscale · 2018-07-02T21:44:24Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/WatcherPluginTests.java

+
+    private class TestNotificationService extends NotificationService<Account> {
+
+        boolean calledCreateAccount = false;


how about isAccountCreated?

no longer needed with the mock.

oops forgot to remove it!

hub-cap · 2018-07-10T18:49:02Z

These comments have been addressed, pls have another review @spinscale

spinscale

some more cosmetic changes, almost there

spinscale · 2018-07-11T08:24:18Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

+        if (enabled == false || transportClient) {
+            return;
+        }
+        for (NotificationService service : reloadableServices) {


reloadableServices.forEach(NotificationService::reload)?

spinscale · 2018-07-11T08:25:00Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

    protected final boolean enabled;
    protected final Environment env;
+    private SetOnce<EmailService> emailService = new SetOnce<>();
+    private SetOnce<HipChatService> hipChatService = new SetOnce<>();


You can get rid of all the SetOnce instance definitions here again with the List<>, or?

spinscale · 2018-07-11T08:26:41Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/WatcherPluginTests.java

+
+        verify(mockService, times(0)).reload(settings);
+        watcher.reload(settings);
+        verify(mockService, times(0)).reload(settings);


there is a verifyNoInteractions() method, that makes this test a bit easier to read

spinscale · 2018-07-11T08:32:57Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/WatcherPluginTests.java

+            .build();
+        NotificationService mockService = mock(NotificationService.class);
+        Watcher watcher = new Watcher(settings);
+        watcher.reloadableServices.clear();


instead of exposing the reloadableServices via package private visibility, how about making it protected and do this (otherwise we have another different visibility in the watcher class)

Watcher watcher = new Watcher(settings) { @Override public Collection<Object> createComponents(Client client, ClusterService clusterService, ThreadPool threadPool, ResourceWatcherService resourceWatcherService, ScriptService scriptService, NamedXContentRegistry xContentRegistry, Environment environment, NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) { reloadableServices.add(mockService); return Collections.emptyList(); } };

Since createComponents is not called, i just created a class that extends Watcher and set this in its constructor.

spinscale

left two minor cleanup comments. LGTM once CI passes

spinscale · 2018-07-11T14:16:45Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

        // actions
        final Map<String, ActionFactory> actionFactoryMap = new HashMap<>();
-        actionFactoryMap.put(EmailAction.TYPE, new EmailActionFactory(settings, emailService, templateEngine, emailAttachmentsParser));
+        actionFactoryMap.put(EmailAction.TYPE, new EmailActionFactory(settings, emailService, templateEngine,


unneeded change

spinscale · 2018-07-11T14:16:51Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

        return Arrays.asList(registry, inputRegistry, historyStore, triggerService, triggeredWatchParser,
                watcherLifeCycleService, executionService, triggerEngineListener, watcherService, watchParser,
-                configuredTriggerEngine, triggeredWatchStore, watcherSearchTemplateService, slackService, pagerDutyService, hipChatService);
+                configuredTriggerEngine, triggeredWatchStore, watcherSearchTemplateService, slackService, pagerDutyService,


unneeded change

spinscale · 2018-07-11T14:18:04Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

+     *            retrievable, including the values stored in the node's keystore.
+     *            The setting values are the initial ones, from when the node has be
+     *            started, i.e. they don't follow dynamic updates.
+     */


this is already documented in ReloadablePlugin I suppose?

Yea intellij automagically placed it here... i assumed that was a good thing (tm) but ill gladly remove it.

spinscale · 2018-07-11T14:18:31Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/WatcherPluginTests.java

+
+        TestWatcher(Settings settings, NotificationService service) {
+            super(settings);
+            reloadableServices.add(service);


hub-cap · 2018-07-13T14:01:45Z

FUTURE ME, change the PR title to watcher:....

This commit allows for rebuilding watcher secure secrets via the reload_secure_settings API call. The commit also renames a method in the Notification Service to make it a bit more readable.

* 6.x: Watcher: Make settings reloadable (#31746) [Rollup] Histo group config should support scaled_floats (#32048) lazy snapshot repository initialization (#31606) Add secure setting for watcher email password (#31620) Watcher: cleanup ensureWatchExists use (#31926) Add second level of field collapsing (#31808) Added lenient flag for synonym token filter (#31484) (#31970) Test: Fix a second case of bad watch creation [Rollup] Use composite's missing_bucket (#31402) Docs: Restyled cloud link in getting started Docs: Change formatting of Cloud options Re-instate link in StringFunctionUtils javadocs Correct spelling of AnalysisPlugin#requriesAnalysisSettings (#32025) Fix problematic chars in javadoc [ML] Move open job failure explanation out of root cause (#31925) [ML] Switch ML native QA tests to use a 3 node cluster (#32011)

Make watcher plugin reloadable

f89207c

This commit allows for rebuilding watcher secure secrets via the reload_secure_settings API call. The commit also renames a method in the Notification Service to make it a bit more readable.

hub-cap added >feature v7.0.0 :Distributed/Watcher v6.4.0 labels Jul 2, 2018

hub-cap requested a review from spinscale July 2, 2018 17:51

hub-cap changed the title ~~Make watcher plugin reloadable~~ Make watcher settings reloadable Jul 2, 2018

spinscale requested changes Jul 2, 2018

View reviewed changes

hub-cap added 6 commits July 3, 2018 09:21

rename to reload

153a97b

Use a list instead of a method and clean test

7be7b49

Fix enabled status on reload

b97f21b

Merge remote-tracking branch 'upstream/master' into reload_watcher

ee19d3a

Remove unused code

98d41ee

Merge remote-tracking branch 'upstream/master' into reload_watcher

b49a645

Fixing merge conflict properly

cc09f7b

spinscale requested changes Jul 11, 2018

View reviewed changes

hub-cap added 5 commits July 11, 2018 08:37

Remove setOnce

0a1d9af

Fix test up and cleanup reload method

38df695

Change pkg private to protected

ad5d8a1

Merge remote-tracking branch 'upstream/master' into reload_watcher

68ef919

fix checkstyle

79f0b7e

spinscale approved these changes Jul 11, 2018

View reviewed changes

hub-cap added 2 commits July 11, 2018 09:48

Minor nit changes

e956505

Merge remote-tracking branch 'upstream/master' into reload_watcher

a326338

hub-cap added 2 commits July 13, 2018 09:02

Merge remote-tracking branch 'upstream/master' into reload_watcher

0b90cc8

Merge remote-tracking branch 'upstream/master' into reload_watcher

9218784

hub-cap merged commit 1f72afa into elastic:master Jul 13, 2018

hub-cap added the backport pending label Jul 13, 2018

hub-cap removed the backport pending label Jul 13, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

This was referenced Aug 12, 2020

Watcher: Support reloadable keystore #30481

Closed

[Doc] Document (in one place) which secure settings are reloadable #36112

Closed

lcawl mentioned this pull request Aug 13, 2020

[DOCS] Identify reloadable Watcher settings #61126

Closed

jrodewig mentioned this pull request Nov 12, 2020

[DOCS] Document reloadable Watcher settings #64998

Merged


		private class TestNotificationService extends NotificationService<Account> {

		boolean calledCreateAccount = false;

Conversation

hub-cap commented Jul 2, 2018

Uh oh!

elasticmachine commented Jul 2, 2018

Uh oh!

spinscale 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!

hub-cap Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hub-cap commented Jul 10, 2018

Uh oh!

spinscale 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!

spinscale 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!

hub-cap commented Jul 13, 2018

Uh oh!

Reviewers

Assignees

hub-cap Jul 3, 2018 •

edited

Loading