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.

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
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,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 +124,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 +196,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 +223,7 @@ public class Watcher extends Plugin implements ActionPlugin, ScriptPlugin {
protected final boolean transportClient;
protected final boolean enabled;
protected final Environment env;
protected List<NotificationService> reloadableServices = new ArrayList<>();

public Watcher(final Settings settings) {
this.settings = settings;
Expand Down Expand Up @@ -275,6 +278,12 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
SlackService slackService = new SlackService(settings, httpClient, clusterService.getClusterSettings());
PagerDutyService pagerDutyService = new PagerDutyService(settings, httpClient, clusterService.getClusterSettings());

reloadableServices.add(emailService);
reloadableServices.add(hipChatService);
reloadableServices.add(jiraService);
reloadableServices.add(slackService);
reloadableServices.add(pagerDutyService);

TextTemplateEngine templateEngine = new TextTemplateEngine(settings, scriptService);
Map<String, EmailAttachmentParser> emailAttachmentParsers = new HashMap<>();
emailAttachmentParsers.put(HttpEmailAttachementParser.TYPE, new HttpEmailAttachementParser(httpClient, httpTemplateParser,
Expand All @@ -300,7 +309,8 @@ 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, templateEngine,
Copy link
Contributor

Choose a reason for hiding this comment

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

unneeded change

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));
Expand Down Expand Up @@ -367,7 +377,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, pagerDutyService,
Copy link
Contributor

Choose a reason for hiding this comment

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

unneeded change

hipChatService);
}

protected TriggerEngine getTriggerEngine(Clock clock, ScheduleRegistry scheduleRegistry) {
Expand Down Expand Up @@ -613,4 +624,21 @@ 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.
*/
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;
}
reloadableServices.forEach(s -> s.reload(settings));
}
}
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
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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,18 @@
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 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;
import static org.mockito.Mockito.verifyNoMoreInteractions;

public class WatcherPluginTests extends ESTestCase {

Expand Down Expand Up @@ -97,4 +102,36 @@ 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 TestWatcher(settings, mockService);

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.

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

watcher.reload(settings);
verifyNoMoreInteractions(mockService);
}

private class TestWatcher extends Watcher {

TestWatcher(Settings settings, NotificationService service) {
super(settings);
reloadableServices.add(service);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}
}
}
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