Skip to content

Make watcher settings reloadable #31746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jul 13, 2018
Merged

Make watcher settings reloadable #31746

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f89207c
Make watcher plugin reloadable
hub-cap Jun 28, 2018
153a97b
rename to reload
hub-cap Jul 3, 2018
7be7b49
Use a list instead of a method and clean test
hub-cap Jul 3, 2018
b97f21b
Fix enabled status on reload
hub-cap Jul 3, 2018
ee19d3a
Merge remote-tracking branch 'upstream/master' into reload_watcher
hub-cap Jul 3, 2018
98d41ee
Remove unused code
hub-cap Jul 3, 2018
b49a645
Merge remote-tracking branch 'upstream/master' into reload_watcher
hub-cap Jul 10, 2018
cc09f7b
Fixing merge conflict properly
hub-cap Jul 10, 2018
0a1d9af
Remove setOnce
hub-cap Jul 11, 2018
38df695
Fix test up and cleanup reload method
hub-cap Jul 11, 2018
ad5d8a1
Change pkg private to protected
hub-cap Jul 11, 2018
68ef919
Merge remote-tracking branch 'upstream/master' into reload_watcher
hub-cap Jul 11, 2018
79f0b7e
fix checkstyle
hub-cap Jul 11, 2018
e956505
Minor nit changes
hub-cap Jul 11, 2018
a326338
Merge remote-tracking branch 'upstream/master' into reload_watcher
hub-cap Jul 11, 2018
0b90cc8
Merge remote-tracking branch 'upstream/master' into reload_watcher
hub-cap Jul 13, 2018
9218784
Merge remote-tracking branch 'upstream/master' into reload_watcher
hub-cap Jul 13, 2018
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
61 changes: 49 additions & 12 deletions x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.elasticsearch.xpack.watcher;

import org.apache.logging.log4j.Logger;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.bootstrap.BootstrapCheck;
Expand Down Expand Up @@ -38,6 +39,7 @@
import org.elasticsearch.node.Node;
import org.elasticsearch.plugins.ActionPlugin;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.ReloadablePlugin;
import org.elasticsearch.plugins.ScriptPlugin;
import org.elasticsearch.rest.RestController;
import org.elasticsearch.rest.RestHandler;
Expand Down Expand Up @@ -123,6 +125,7 @@
import org.elasticsearch.xpack.watcher.input.simple.SimpleInputFactory;
import org.elasticsearch.xpack.watcher.input.transform.TransformInput;
import org.elasticsearch.xpack.watcher.input.transform.TransformInputFactory;
import org.elasticsearch.xpack.watcher.notification.NotificationService;
import org.elasticsearch.xpack.watcher.notification.email.Account;
import org.elasticsearch.xpack.watcher.notification.email.EmailService;
import org.elasticsearch.xpack.watcher.notification.email.HtmlSanitizer;
Expand Down Expand Up @@ -194,7 +197,7 @@

import static java.util.Collections.emptyList;

public class Watcher extends Plugin implements ActionPlugin, ScriptPlugin {
public class Watcher extends Plugin implements ActionPlugin, ScriptPlugin, ReloadablePlugin {

// This setting is only here for backward compatibility reasons as 6.x indices made use of it. It can be removed in 8.x.
@Deprecated
Expand All @@ -221,6 +224,12 @@ public class Watcher extends Plugin implements ActionPlugin, ScriptPlugin {
protected final boolean transportClient;
protected final boolean enabled;
protected final Environment env;
private SetOnce<EmailService> emailService = new SetOnce<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

7be7b49

private SetOnce<HipChatService> hipChatService = new SetOnce<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

private SetOnce<JiraService> jiraService = new SetOnce<>();
private SetOnce<SlackService> slackService = new SetOnce<>();
private SetOnce<PagerDutyService> pagerDutyService = new SetOnce<>();
List<NotificationService> reloadableServices = new ArrayList<>();

public Watcher(final Settings settings) {
this.settings = settings;
Expand Down Expand Up @@ -269,11 +278,17 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
httpClient = new HttpClient(settings, httpAuthRegistry, getSslService());

// notification
EmailService emailService = new EmailService(settings, cryptoService, clusterService.getClusterSettings());
HipChatService hipChatService = new HipChatService(settings, httpClient, clusterService.getClusterSettings());
JiraService jiraService = new JiraService(settings, httpClient, clusterService.getClusterSettings());
SlackService slackService = new SlackService(settings, httpClient, clusterService.getClusterSettings());
PagerDutyService pagerDutyService = new PagerDutyService(settings, httpClient, clusterService.getClusterSettings());
emailService.set(new EmailService(settings, cryptoService, clusterService.getClusterSettings()));
reloadableServices.add(emailService.get());
hipChatService.set(new HipChatService(settings, httpClient, clusterService.getClusterSettings()));
reloadableServices.add(hipChatService.get());
jiraService.set(new JiraService(settings, httpClient, clusterService.getClusterSettings()));
reloadableServices.add(jiraService.get());
slackService.set(new SlackService(settings, httpClient, clusterService.getClusterSettings()));
reloadableServices.add(slackService.get());
pagerDutyService.set(new PagerDutyService(settings, httpClient, clusterService.getClusterSettings()));
reloadableServices.add(pagerDutyService.get());


TextTemplateEngine templateEngine = new TextTemplateEngine(settings, scriptService);
Map<String, EmailAttachmentParser> emailAttachmentParsers = new HashMap<>();
Expand All @@ -300,14 +315,15 @@ public Collection<Object> createComponents(Client client, ClusterService cluster

// 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.get(), templateEngine,
emailAttachmentsParser));
actionFactoryMap.put(WebhookAction.TYPE, new WebhookActionFactory(settings, httpClient, httpTemplateParser, templateEngine));
actionFactoryMap.put(IndexAction.TYPE, new IndexActionFactory(settings, client));
actionFactoryMap.put(LoggingAction.TYPE, new LoggingActionFactory(settings, templateEngine));
actionFactoryMap.put(HipChatAction.TYPE, new HipChatActionFactory(settings, templateEngine, hipChatService));
actionFactoryMap.put(JiraAction.TYPE, new JiraActionFactory(settings, templateEngine, jiraService));
actionFactoryMap.put(SlackAction.TYPE, new SlackActionFactory(settings, templateEngine, slackService));
actionFactoryMap.put(PagerDutyAction.TYPE, new PagerDutyActionFactory(settings, templateEngine, pagerDutyService));
actionFactoryMap.put(HipChatAction.TYPE, new HipChatActionFactory(settings, templateEngine, hipChatService.get()));
actionFactoryMap.put(JiraAction.TYPE, new JiraActionFactory(settings, templateEngine, jiraService.get()));
actionFactoryMap.put(SlackAction.TYPE, new SlackActionFactory(settings, templateEngine, slackService.get()));
actionFactoryMap.put(PagerDutyAction.TYPE, new PagerDutyActionFactory(settings, templateEngine, pagerDutyService.get()));
final ActionRegistry registry = new ActionRegistry(actionFactoryMap, conditionRegistry, transformRegistry, getClock(),
getLicenseState());

Expand Down Expand Up @@ -367,7 +383,8 @@ public Collection<Object> createComponents(Client client, ClusterService cluster

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

protected TriggerEngine getTriggerEngine(Clock clock, ScheduleRegistry scheduleRegistry) {
Expand Down Expand Up @@ -613,4 +630,24 @@ public List<ScriptContext> getContexts() {
public void close() throws IOException {
IOUtils.closeWhileHandlingException(httpClient);
}


/**
* Reloads all reloadable services' settings.
* @param settings
* Settings used while reloading the plugin. All values are
* 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.
* @throws Exception
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

this is already documented in ReloadablePlugin I suppose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Override
public void reload(Settings settings) {
if (enabled == false || transportClient) {
return;
}
for (NotificationService service : reloadableServices) {
Copy link
Contributor

Choose a reason for hiding this comment

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

reloadableServices.forEach(NotificationService::reload)?

service.reload(settings);
}
}
}
4 changes: 2 additions & 2 deletions ...tcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public abstract class NotificationService<Account> extends AbstractComponent {
public NotificationService(Settings settings, String type,
ClusterSettings clusterSettings, List<Setting<?>> pluginSettings) {
this(settings, type);
clusterSettings.addSettingsUpdateConsumer(this::setAccountSetting, pluginSettings);
clusterSettings.addSettingsUpdateConsumer(this::reload, pluginSettings);
}

// Used for testing only
Expand All @@ -40,7 +40,7 @@ public NotificationService(Settings settings, String type,
this.type = type;
}

protected synchronized void setAccountSetting(Settings settings) {
public synchronized void reload(Settings settings) {
Tuple<Map<String, Account>, Account> accounts = buildAccounts(settings, this::createAccount);
this.accounts = Collections.unmodifiableMap(accounts.v1());
this.defaultAccount = accounts.v2();
Expand Down
2 changes: 1 addition & 1 deletion ...atcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public EmailService(Settings settings, @Nullable CryptoService cryptoService, Cl
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_SEND_PARTIAL, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_WAIT_ON_QUIT, (s, o) -> {}, (s, o) -> {});
// do an initial load
setAccountSetting(settings);
reload(settings);
}

@Override
Expand Down
6 changes: 3 additions & 3 deletions ...er/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,13 @@ public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings
clusterSettings.addAffixUpdateConsumer(SETTING_PORT, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_MESSAGE_DEFAULTS, (s, o) -> {}, (s, o) -> {});

setAccountSetting(settings);
reload(settings);
}

@Override
protected synchronized void setAccountSetting(Settings settings) {
public synchronized void reload(Settings settings) {
defaultServer = new HipChatServer(settings.getByPrefix("xpack.notification.hipchat."));
super.setAccountSetting(settings);
super.reload(settings);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion .../watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public JiraService(Settings settings, HttpClient httpClient, ClusterSettings clu
clusterSettings.addAffixUpdateConsumer(SETTING_SECURE_PASSWORD, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {});
// do an initial load
setAccountSetting(settings);
reload(settings);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...rc/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public PagerDutyService(Settings settings, HttpClient httpClient, ClusterSetting
clusterSettings.addAffixUpdateConsumer(SETTING_SERVICE_API_KEY, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_SECURE_SERVICE_API_KEY, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {});
setAccountSetting(settings);
reload(settings);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...atcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public SlackService(Settings settings, HttpClient httpClient, ClusterSettings cl
clusterSettings.addAffixUpdateConsumer(SETTING_URL, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_URL_SECURE, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {});
setAccountSetting(settings);
reload(settings);
}

@Override
Expand Down
34 changes: 34 additions & 0 deletions x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/WatcherPluginTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,19 @@
import org.elasticsearch.test.IndexSettingsModule;
import org.elasticsearch.threadpool.ExecutorBuilder;
import org.elasticsearch.xpack.core.watcher.watch.Watch;
import org.elasticsearch.xpack.watcher.notification.NotificationService;
import org.elasticsearch.xpack.watcher.notification.email.Account;

import java.util.Collections;
import java.util.List;

import static java.util.Collections.emptyMap;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

public class WatcherPluginTests extends ESTestCase {

Expand Down Expand Up @@ -97,4 +103,32 @@ public void testThreadPoolSize() {
.build();
assertThat(Watcher.getWatcherThreadPoolSize(noDataNodeSettings), is(1));
}

public void testReload() {
Settings settings = Settings.builder()
.put("xpack.watcher.enabled", true)
.put("path.home", createTempDir())
.build();
NotificationService mockService = mock(NotificationService.class);
Watcher watcher = new Watcher(settings);
watcher.reloadableServices.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

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();
            }
        };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

watcher.reloadableServices.add(mockService);

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

public void testReloadDisabled() {
Settings settings = Settings.builder()
.put("xpack.watcher.enabled", false)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

b97f21b

.put("path.home", createTempDir())
.build();
NotificationService mockService = mock(NotificationService.class);
Watcher watcher = new Watcher(settings);

verify(mockService, times(0)).reload(settings);
watcher.reload(settings);
verify(mockService, times(0)).reload(settings);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}
2 changes: 1 addition & 1 deletion .../src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ private static class TestNotificationService extends NotificationService<String>

TestNotificationService(Settings settings) {
super(settings, "test");
setAccountSetting(settings);
reload(settings);
}

@Override
Expand Down