Skip to content
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

Make watcher settings reloadable #31746

Merged
merged 17 commits into from
Jul 13, 2018
Merged

Make watcher settings reloadable #31746

Show file tree
Hide file tree
Changes from 1 commit
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
63 changes: 51 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,11 @@ 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<>();

public Watcher(final Settings settings) {
this.settings = settings;
Expand Down Expand Up @@ -269,11 +277,11 @@ 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()));
hipChatService.set(new HipChatService(settings, httpClient, clusterService.getClusterSettings()));
jiraService.set(new JiraService(settings, httpClient, clusterService.getClusterSettings()));
slackService.set(new SlackService(settings, httpClient, clusterService.getClusterSettings()));
pagerDutyService.set(new PagerDutyService(settings, httpClient, clusterService.getClusterSettings()));

TextTemplateEngine templateEngine = new TextTemplateEngine(settings, scriptService);
Map<String, EmailAttachmentParser> emailAttachmentParsers = new HashMap<>();
Expand All @@ -300,14 +308,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 +376,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 +623,33 @@ public List<ScriptContext> getContexts() {
public void close() throws IOException {
IOUtils.closeWhileHandlingException(httpClient);
}

/**
* Used to detect which {@link NotificationService} get reloaded. Also useful for testing the reload in tests.
* @return
*/
protected List<NotificationService> getReloadableServices() {
return Arrays.asList(
emailService.get(),
hipChatService.get(),
jiraService.get(),
slackService.get(),
pagerDutyService.get());
}

/**
* 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) throws Exception {
for (NotificationService service : getReloadableServices()) {
service.loadSettings(settings);
}
}
}
2 changes: 1 addition & 1 deletion ...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 @@ -30,7 +30,7 @@ public NotificationService(Settings settings, String type) {
this.type = type;
}

protected synchronized void setAccountSetting(Settings settings) {
public synchronized void loadSettings(Settings settings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

153a97b

Tuple<Map<String, Account>, Account> accounts = buildAccounts(settings, this::createAccount);
this.accounts = Collections.unmodifiableMap(accounts.v1());
this.defaultAccount = accounts.v2();
Expand Down
4 changes: 2 additions & 2 deletions ...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 @@ -96,7 +96,7 @@ public class EmailService extends NotificationService<Account> {
public EmailService(Settings settings, @Nullable CryptoService cryptoService, ClusterSettings clusterSettings) {
super(settings, "email");
this.cryptoService = cryptoService;
clusterSettings.addSettingsUpdateConsumer(this::setAccountSetting, getSettings());
clusterSettings.addSettingsUpdateConsumer(this::loadSettings, getSettings());
// ensure logging of setting changes
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_PROFILE, (s, o) -> {}, (s, o) -> {});
Expand All @@ -116,7 +116,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);
loadSettings(settings);
}

@Override
Expand Down
8 changes: 4 additions & 4 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 @@ -67,7 +67,7 @@ public class HipChatService extends NotificationService<HipChatAccount> {
public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) {
super(settings, "hipchat");
this.httpClient = httpClient;
clusterSettings.addSettingsUpdateConsumer(this::setAccountSetting, getSettings());
clusterSettings.addSettingsUpdateConsumer(this::loadSettings, getSettings());
// ensure logging of setting changes
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {});
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_HOST, (s) -> {});
Expand All @@ -80,13 +80,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);
loadSettings(settings);
}

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

@Override
Expand Down
4 changes: 2 additions & 2 deletions .../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 @@ -62,7 +62,7 @@ public class JiraService extends NotificationService<JiraAccount> {
public JiraService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) {
super(settings, "jira");
this.httpClient = httpClient;
clusterSettings.addSettingsUpdateConsumer(this::setAccountSetting, getSettings());
clusterSettings.addSettingsUpdateConsumer(this::loadSettings, getSettings());
// ensure logging of setting changes
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_ALLOW_HTTP, (s, o) -> {}, (s, o) -> {});
Expand All @@ -74,7 +74,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);
loadSettings(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 @@ -45,7 +45,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);
loadSettings(settings);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this PagerDutyService missing

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

in its ctor?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@hub-cap hub-cap Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#31762 has been merged

}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...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 @@ -41,12 +41,12 @@ public class SlackService extends NotificationService<SlackAccount> {
public SlackService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) {
super(settings, "slack");
this.httpClient = httpClient;
clusterSettings.addSettingsUpdateConsumer(this::setAccountSetting, getSettings());
clusterSettings.addSettingsUpdateConsumer(this::loadSettings, getSettings());
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {});
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);
loadSettings(settings);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions .../watcher/src/test/java/org/elasticsearch/xpack/notification/NotificationServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,12 @@ private static class TestNotificationService extends NotificationService<String>

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

@Override
protected String createAccount(String name, Settings accountSettings) {
return name;
}
}
}
}
42 changes: 42 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,7 +15,10 @@
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;
Expand Down Expand Up @@ -97,4 +100,43 @@ public void testThreadPoolSize() {
.build();
assertThat(Watcher.getWatcherThreadPoolSize(noDataNodeSettings), is(1));
}

private class TestNotificationService extends NotificationService<Account> {

boolean calledCreateAccount = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

how about isAccountCreated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no longer needed with the mock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops forgot to remove it!


TestNotificationService(Settings settings, String type) {
super(settings, type);
}

@Override
protected Account createAccount(String name, Settings accountSettings) {
return null;
}

@Override
public synchronized void loadSettings(Settings settings) {
calledCreateAccount = true;
super.loadSettings(settings);
}
}

public void testReload() throws Exception {
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();
TestNotificationService service = new TestNotificationService(settings, "test");
Watcher watcher = new Watcher(settings) {
@Override
protected List<NotificationService> getReloadableServices() {
return Collections.singletonList(service);
}
};

assertFalse(service.calledCreateAccount);
watcher.reload(settings);
assertTrue(service.calledCreateAccount);

}
}