Dynamic configuration reloading for modules in Metricbeat #3281

ruflin · 2017-01-04T11:28:56Z

Currently if new module configs have to be added or changed, Metricbeat needs to be restarted. This change allows to define a configuration directory where new files can be added, removed or modified and updates will automatically processed by Metricbeat. New modules will started / stopped accordingly. This is especially useful in container environments where 1 container is used to monitor all services in other containers on the same host. New containers appear and disappear dynamically which requires changes to which modules are needed and which hosts must be monitored.

Configuration

The configuration in the main metricbeat.yml config file looks as following:

metricbeat.reload.modules:
  enabled: true
  path: configs/*.yml
  period: 10s

A path with a glob most be defined on which files should be checked for changes. A period is set on how often the files are checked for changes.

The configuration inside the files which are found by the glob look as following:

- module: system
  metricsets: ["cpu"]
  enabled: false
  perdiod: 1s

- module: system
  metricsets: ["network"]
  enabled: true
  period: 10s

Each file directly contains a list of modules. Each file can contain one or multiple module definitions.

How does it work

When Metricbeat is started a process is started that monitors the defined glob files for changes. If a change is detected, all config files are reloaded and module wrappers are created. A hash is created for the module wrapper to identify if it is a module that is already running or a new module. After loading all modules, all modules which are not in the list anymore will be stopped and removed. All new ones will be started.

The effect of the above is that if 2 identical module configuration exists, only 1 is started. It is not expected to have 2 modules with the exact same configuration.

The dynamic modules have no effect on the modules defined in the global configuration file. These modules will never change and keep running. Also if there is a dynamic module with the exact same config as a global module, these two will not be correlated and both will be run.

Changes

Introduce configuration variables for config reloading
Change metricbeat behaviour that if metricbeat.config.enabled: true, on startup also 0 modules can be defined. Also it is possible that at some point in time no modules are running.
Change shutdown to also shut down dynamic modules
Expvar stats for reloading added
Add common configuration files to metricbeat to have additional configs except from modules
Add basic system tests

urso · 2017-01-04T12:01:17Z

The dynamic modules have no effect on the modules defined in the global configuration file. These modules will never change and keep running.

What if a dynamic module resembles a global module?

Is it an issue to call Connect() for each new module?

No. This is what Connect is for, connecting multiple producers to the publisher pipeline. Just close the client once you're done.

The current hash only takes module level configs into account. Metricset specific options are no added to the hash. Any idea how we could fix this? The problem is that the config for the metricset is only unloaded when the metricset is created (which is good). It would be nice to be able to calculate a "correct" hash based on common.Config or similar.

Hmmm... One solution might be to unpack the full module config into a map[string]interface{} and compute the hash from the map.

urso · 2017-01-04T12:11:26Z

metricbeat/beater/metricbeat.go

+		go func() {
+			// Start Config monitoring
+			configReloader.Start()
+		}()


== go configReloader.Start()

urso · 2017-01-04T12:16:10Z

metricbeat/beater/reload.go

+	mutex sync.Mutex
+}
+
+func NewConfigReloader(config ReloaderConfig, p publisher.Publisher) ConfigReloader {


why no pointer? Using NewXXX implies a pointer being returned.

urso · 2017-01-04T12:17:47Z

metricbeat/beater/reload.go

+	}
+}
+
+func (r *ConfigReloader) Start() {


s/Start/Run? This method implements the run-loop itself. Using Start implies a go-routine being started, but returns immediately, Run implies the method blocks.

urso · 2017-01-04T12:19:25Z

metricbeat/beater/reload.go

+	close(r.done)
+	r.wg.Wait()
+	// Stop all running modules
+	r.StopModules(r.registry.CopyList())


alternatively StopModules can be done via defer in run loop.

agree. moved.

urso · 2017-01-04T12:20:52Z

metricbeat/beater/reload.go

+				continue
+			}
+
+			lastScan = lastScanTemp


why set lastScan only if any files have been changed?

Even thought that was lost during refactoring and initially the purpose was to only update lastScan if no error, I think it is still correct like this. If there are no file updates, it does not matter if the comparison of the files is to the most recent lastScan or one of the previous where no files were found. The resulting outcome should be the same and it keeps the code simpler. I will add a comment to the code.

ruflin · 2017-01-04T12:25:17Z

@urso Thanks for feedback:

dynamic modules resemblance. Both will be run. No correlation is done. I will update the PR description accordingly
Connect: Great
Hash: Will try if the above will work for comparison.

tsg · 2017-01-04T12:57:35Z

metricbeat/beater/reload.go

@@ -0,0 +1,252 @@
+package beater


I wonder if the code in this file could be generalized into a libbeat package. It seems to me that it contains relatively little Metricbeat specific logic, except for the StartModules and StopModules functions which can be easily injected.

I'm thinking that we'll want to make it easy to extend this type of reloading functionality to other beats.

Fully agree, that we probably should extend this to other beats, especially filebeat. Means this functionality will move to libbeat. I prefer to keep it currently in metricbeat to keep the changes as minimal as possible and do the abstraction in a second step when we add it to other beats.

monicasarbu · 2017-01-04T13:13:36Z

metricbeat/metricbeat.yml

@@ -10,41 +10,13 @@
 #==========================  Modules configuration ============================
 metricbeat.modules:


I think metricbeat.modules needs to be removed here.

@monicasarbu This one is WIP. I will restore the old metricbeat.yml as soon as I added automated tests. This file should not change.

andrewkroh · 2017-01-05T23:21:42Z

metricbeat/_meta/beat.full.yml

+metricbeat.reload:
+
+  # Glob pattern for configuration reloading
+  path: config/*.yml


I'd go with conf.d/*.yml. I think that name might provide a hint about the purpose of the directory.

I wonder if we could create this directory automatically in /etc/metricbeat/conf.d when we install the rpm/deb. And do the same for Filebeat.

Changed to conf.d. I like the idea of creating this automatically. Reminds me to check to what the path is relative. Should the default be ${path.conf}/conf.d/*.yml?

BTW: Creation of the directory I would suggest not to do in this PR and look into later.

andrewkroh

Nice work! My comments are all just minor issues.

andrewkroh · 2017-01-05T23:32:21Z

metricbeat/beater/config.go


 // Config is the root of the Metricbeat configuration hierarchy.
 type Config struct {
 	// Modules is a list of module specific configuration data.
-	Modules []*common.Config `config:"modules" validate:"required"`
+	Modules       []*common.Config    `config:"modules"`
+	ReloadModules ModulesReloadConfig `config:"reload.modules"`


To be consistent with other parts of the config that are disabled by default should this be a pointer along with the Enabled *bool? It's a subtle difference, but it means that setting

metricbeat.reload.modules.path: conf.d/*.yml

would implicitly enable the reloading without having to also specify

metricbeat.reload.modules.enabled: true

An example of this is the tls config in the outputs which have an IsEnabled helper method.

Changed it to the same logic as the output. +1 on consistency here.

Hm, that didn't work as I expected. Because I have a default config, which I already load on the metricbeat level, the config is never nil. I plan to extract the reload part anyways. Ok if I do it in a second step?

Yeah, that's fine. With using the pointer you'll need handle defaults when reading the individual values (haven't found a better way).

You'll need to make ReloadModules a pointer too. But with the default present it won't be nil.

andrewkroh · 2017-01-05T23:39:38Z

metricbeat/beater/glob_change_detector.go

+			continue
+		}
+
+		// Directories are symlinks are skipped


"Directories and symlinks"?

andrewkroh · 2017-01-05T23:46:39Z

metricbeat/beater/glob_change_detector.go

+		// Check if one of the files was changed recently
+		// File modification time can be in seconds. -1 is to cover for files which
+		// were created during this second.
+		if info.ModTime().Sub(gcd.lastScan) >= -1*time.Second {


Could you use if info.ModTime().After(gcd.lastScan) here? That seems more readable. Then to account for the one second resolution you could Truncate the last scan time using if info.ModTime().After(gcd.lastScan.Truncate(time.Second)).

What systems still store the modification time in seconds?

Changed to your suggestion. OS X seems to store mtime in seconds. At least I only see seconds when I use stat.

andrewkroh · 2017-01-05T23:53:06Z

metricbeat/beater/glob_change_detector.go

+
+	hash, err := hashstructure.Hash(files, nil)
+	if err != nil {
+		logp.Err("Error hashing files list: %s", err)


Should this method continue if hashing fails? I didn't look to see what can cause an error here, but if it does fail maybe it should return the error OR not return an error and base its decision solely on the updatedFails flag.

I initially ignored the error as I didn't see an option how we ever get here an error. But to be sure we catch it in case it happens, I added the log line.

If there is an error, the hash is 0. So in case hashing fails one time because of reason x, scan would still return changed which is the save way to go. I prefer to return changed once too many. If it continously fails, it will in the future depend on updatedFiles only. The not so nice part about this is that it does not detect removed files with updatedFiles.

Thinking about this again, probably the most consistent behaviour would be directly return files, true, error in case of an error, to make sure in an error case changed is true. Will change it to this.

andrewkroh · 2017-01-05T23:57:27Z

metricbeat/beater/glob_change_detector_test.go

+
+	// Create random temp directory
+	id := strconv.Itoa(rand.New(rand.NewSource(int64(time.Now().Nanosecond()))).Int())
+	dir := os.TempDir() + "/" + id


You can use ioutil.TempDir("", id) for this. https://golang.org/pkg/io/ioutil/#TempDir

And you should cleanup the dir with a defer os.RemoveAll(dir).

andrewkroh · 2017-01-06T01:17:03Z

metricbeat/beater/reload.go

+	wg       sync.WaitGroup
+}
+
+type RunningRegistry struct {


It looks like this is used internally only by the ConfigReloader. Can this be unexported?

yes, changed

andrewkroh · 2017-01-06T01:23:23Z

metricbeat/beater/reload.go

+
+type RunningRegistry struct {
+	List  map[uint64]ModuleRunner
+	mutex sync.Mutex


If the mutex is required to access List then they should have the same visibility. But I like embedding the mutex in the struct then you can call runningReg.Lock().

andrewkroh · 2017-01-06T01:26:58Z

metricbeat/beater/reload.go

+	return c, nil
+}
+
+func NewRunningRegistry() RunningRegistry {


Given that this struct contains a sync.Mutex, returning a *RunningRegistry would better indicate to the caller that this object needs to be passed by reference than by value (as you should not copy a sync.Mutex).

andrewkroh · 2017-01-06T01:33:28Z

metricbeat/beater/reload.go

+
+			debugr("Number of module wrappers created: %v", len(s))
+
+			startList := []*ModuleWrapper{}


Use var startList []*ModuleWrapper to avoid allocating an empty map if it doesn't get used. ref

andrewkroh · 2017-01-06T01:43:58Z

metricbeat/_meta/beat.full.yml

+metricbeat.reload:
+
+  # Glob pattern for configuration reloading
+  path: config/*.yml


I wonder if we could create this directory automatically in /etc/metricbeat/conf.d when we install the rpm/deb. And do the same for Filebeat.

ruflin · 2017-01-06T08:10:55Z

@andrewkroh I pushed 2 new commits. One for the general refactoring and one for the config changes to make it relative. Could you have a look?

monicasarbu · 2017-01-06T09:11:16Z

metricbeat/docs/reference/configuration/reload-configuration.asciidoc

+  period: 10s
+------------------------------------------------------------------------------
+
+A path with a glob most be defined on which files should be checked for changes. A period is set on how often


/s/most/must

monicasarbu · 2017-01-06T09:13:58Z

metricbeat/metricbeat.full.yml

+
+# Config reloading allows to dynamically load modules. Each file which is
+# monitored must contain one or multiple modules as a list.
+metricbeat.reload:


I think it should be metricbeat.reload.modules here.

Definitively, thanks for spotting. Fixed.

urso · 2017-01-06T13:20:33Z

metricbeat/_meta/beat.full.yml

+metricbeat.reload:
+
+  # Glob pattern for configuration reloading
+  path: conf.d/*.yml


how about setting this to:

path: '${path.config}/conf.d/*.yml'

urso · 2017-01-06T13:22:43Z

metricbeat/beater/glob_change_detector.go

+	"github.com/mitchellh/hashstructure"
+)
+
+type GlobChangeDetector struct {


GlobWatcher, Wacher, or FileWatcher ?

I like GlobWatcher. Changing ...

urso · 2017-01-06T13:24:21Z

metricbeat/beater/glob_change_detector.go

+		// Directories and symlinks are skipped
+		if !info.Mode().IsRegular() {
+			continue
+		}


hm... having symlink support would be nice to have. This way op can enable modules selectively via ln -s... (kinda common practice in init systems, even systemd).

Ok, enabled symlinks

ruflin · 2017-01-06T15:02:54Z

@andrewkroh @monicasarbu @urso I applied the changes. If ok for all of you I will squash and rebase to update the commit message.

andrewkroh

The changes look good.

andrewkroh · 2017-01-06T14:58:44Z

metricbeat/beater/config.go


 // Config is the root of the Metricbeat configuration hierarchy.
 type Config struct {
 	// Modules is a list of module specific configuration data.
-	Modules []*common.Config `config:"modules" validate:"required"`
+	Modules       []*common.Config    `config:"modules"`
+	ReloadModules ModulesReloadConfig `config:"reload.modules"`


You'll need to make ReloadModules a pointer too. But with the default present it won't be nil.

andrewkroh · 2017-01-06T15:03:03Z

metricbeat/beater/reload.go

+	"fmt"
+	"sync"
+	"time"
+


Extra line here

About Reload: Yeah, tried different versions, but didn't work. Extracting it and having a separate unpack should work (in a later PR). Not sure why I can't comment above -> Github fail.

Will remove the extra line when rebasing.

Currently if new module configs have to be added or changed, Metricbeat needs to be restarted. This change allows to define a configuration directory where new files can be added, removed or modified and updates will automatically processed by Metricbeat. New modules will started / stopped accordingly. This is especially useful in container environments where 1 container is used to monitor all services in other containers on the same host. New containers appear and disappear dynamically which requires changes to which modules are needed and which hosts must be monitored. **Configuration** The configuration in the main metricbeat.yml config file looks as following: ``` metricbeat.reload.modules: enabled: true path: configs/*.yml period: 10s ``` A path with a glob most be defined on which files should be checked for changes. A period is set on how often the files are checked for changes. The configuration inside the files which are found by the glob look as following: ``` - module: system metricsets: ["cpu"] enabled: false perdiod: 1s - module: system metricsets: ["network"] enabled: true period: 10s ``` Each file directly contains a list of modules. Each file can contain one or multiple module definitions. **How does it work** When Metricbeat is started a process is started that monitors the defined glob files for changes. If a change is detected, all config files are reloaded and module wrappers are created. A hash is created for the module wrapper to identify if it is a module that is already running or a new module. After loading all modules, all modules which are not in the list anymore will be stopped and removed. All new ones will be started. The effect of the above is that if 2 identical module configuration exists, only 1 is started. It is not expected to have 2 modules with the exact same configuration. The dynamic modules have no effect on the modules defined in the global configuration file. These modules will never change and keep running. Also if there is a dynamic module with the exact same config as a global module, these two will not be correlated and both will be run. **Changes** * Introduce configuration variables for config reloading * Change metricbeat behaviour that if `metricbeat.config.enabled: true`, on startup also 0 modules can be defined. Also it is possible that at some point in time no modules are running. * Change shutdown to also shut down dynamic modules * Expvar stats for reloading added * Add common configuration files to metricbeat to have additional configs except from modules * Add basic system tests

andrewkroh · 2017-01-06T16:51:20Z

jenkins, test it

ruflin added in progress Pull request is currently in progress. Metricbeat Metricbeat labels Jan 4, 2017

urso reviewed Jan 4, 2017

View reviewed changes

tsg reviewed Jan 4, 2017

View reviewed changes

monicasarbu reviewed Jan 4, 2017

View reviewed changes

ruflin force-pushed the module-reloading branch 5 times, most recently from 3cc2565 to 802860d Compare January 4, 2017 19:08

ruflin changed the title ~~Dynamic configuration reloading in Metricbeat~~ Dynamic configuration reloading for modules in Metricbeat Jan 4, 2017

ruflin force-pushed the module-reloading branch 2 times, most recently from e94bb57 to aad532c Compare January 5, 2017 09:04

ruflin added review and removed in progress Pull request is currently in progress. labels Jan 5, 2017

ruflin mentioned this pull request Jan 5, 2017

Live reload config files without restarting filebeat process #2043

Closed

andrewkroh reviewed Jan 5, 2017

View reviewed changes

andrewkroh approved these changes Jan 6, 2017

View reviewed changes

monicasarbu reviewed Jan 6, 2017

View reviewed changes

urso reviewed Jan 6, 2017

View reviewed changes

andrewkroh approved these changes Jan 6, 2017

View reviewed changes

ruflin force-pushed the module-reloading branch from c075357 to 8fed57c Compare January 6, 2017 15:52

andrewkroh merged commit 77cdb8f into elastic:master Jan 6, 2017

dedemorton mentioned this pull request Jan 10, 2017

Beats Doc Updates for 5.2 #3329

Closed

6 tasks

dedemorton mentioned this pull request Feb 14, 2017

Beats Doc Updates for 5.3 #3539

Closed

12 tasks

		@@ -10,41 +10,13 @@
		#========================== Modules configuration ============================
		metricbeat.modules:


		debugr("Number of module wrappers created: %v", len(s))

		startList := []*ModuleWrapper{}

Dynamic configuration reloading for modules in Metricbeat #3281

Dynamic configuration reloading for modules in Metricbeat #3281

Conversation

ruflin commented Jan 4, 2017 • edited Loading

urso commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Jan 6, 2017

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Jan 6, 2017

ruflin commented Jan 4, 2017 •

edited

Loading

ruflin Jan 6, 2017 •

edited

Loading