Keep a mapping of autodiscover instances and deployed configs #8851

exekias · 2018-10-31T15:40:46Z

This changes stop behavior, sometimes not all metadata is available in
the stop event, this results in some configs not being stopped because
of that.

After this change, we only rely on instance id from the event to decide
what configs to delete, as we'll keep a map of instance id -> deployed
configs.

Fixes #8535

This changes stop behavior, sometimes not all metadata is available in the stop event, this results in some configs not being stopped because of that. After this change, we only rely on instance id from the event to decide what configs to delete, as we'll keep a map of instance id -> deployed configs.

ruflin

Is there a way we could test this change? Don't forget the changelog (I see it's still in progress ;-) ).

ruflin · 2018-11-01T08:16:50Z

libbeat/autodiscover/autodiscover.go

+	logp.Debug(debugK, "Got a stop event: %v", event)
+	eventID := getID(event)
+	if eventID == "" {
+		logp.Err("Event didn't provide instance id, ignoring it")


Can we log some information about the event to make it easier to debug without having to enable debug level?

jsoriano · 2018-11-01T11:29:29Z

libbeat/autodiscover/autodiscover.go

@@ -138,8 +138,10 @@ func (a *Autodiscover) worker() {
 			}

 			configs := make([]*reload.ConfigWithMeta, 0, len(a.configs))


Now there can be more than len(a.configs) configs in configs?

libbeat/autodiscover/autodiscover.go

libbeat/autodiscover/providers/jolokia/discovery.go

andrewvc

Love these changes. Left some thoughts on possible improvements.

andrewvc · 2018-11-01T14:50:44Z

libbeat/autodiscover/autodiscover.go

@@ -61,7 +61,7 @@ type Autodiscover struct {
 	defaultPipeline beat.Pipeline
 	adapter         Adapter
 	providers       []Provider
-	configs         map[uint64]*reload.ConfigWithMeta
+	configs         map[string]map[uint64]*reload.ConfigWithMeta


My reading of this is that these IDs must be globally unique regardless of provider. This could be an issue if two providers use the same key.

Maybe we could modify getId to prepend the provider name? That way providers would only be responsible for generating IDs unique to themselves.

Thinking about it further, it's probably best just to add another level of map, using a synthetic key based on the provider. You can have multiple instances of a provider, and it probably makes sense to have the keys scoped per instance.

Autodiscover doesn't know about the provider, as everything is get from the same bus, I'm inclined to leave this as it is, as I don't expect this kind of configurations? Even if the user configures several providers, stop events would still be consistent?

I'm worried because there's no explicit contract here that external providers would have IDs that would never clash. I don't think any of the providers we use today have keys that clash, but it seems like we're creating an extra failure mode by not doing this namespacing.

Why wouldn't we enforce that sort of thing where we can? It seems like tempting fate not to. I also think it would make the workings of this code more clear. Encoding our understanding of the domain into the datastructures increases code readability.

Just pushed the changes to include unique provider as part of the mapping 👍

The changes look good!

libbeat/autodiscover/autodiscover.go

andrewvc · 2018-11-27T18:42:35Z

CHANGELOG.asciidoc

@@ -33,6 +33,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha1...master[Check the HEAD d

 *Affecting all Beats*

+- Keep a mapping of autodiscover instances and deployed configs. {pull}8851[8851]


Maybe this could be a bit more clear? What effect will users notice? If none, we should mention that this is an internal refactor to add new features later.

andrewvc

Needs a better changelog , but other than that WFG LGTM. Great work!

exekias · 2018-11-28T15:36:29Z

Thanks for the reviews @andrewvc, I've updated the changelog, let me know wdyt

andrewvc · 2018-11-28T19:31:41Z

LGTM but WFG. It looks like there are flaky tests here potentially however. We should fix those if that's holding up a clean build.

exekias · 2018-11-28T22:37:44Z

merged master to this branch, it seems to fix flaky tests

…c#8851) * Keep a mapping of autodiscover instances and deployed configs This changes stop behavior, sometimes not all metadata is available in the stop event, this results in some configs not being stopped because of that. After this change, we only rely on instance id from the event to decide what configs to delete, as we'll keep a map of instance id -> deployed configs. (cherry picked from commit 571b008)

exekias · 2018-12-07T10:59:00Z

pinging @vjsamuel for awareness, not sure this change may affect you

…#9290) * Keep a mapping of autodiscover instances and deployed configs This changes stop behavior, sometimes not all metadata is available in the stop event, this results in some configs not being stopped because of that. After this change, we only rely on instance id from the event to decide what configs to delete, as we'll keep a map of instance id -> deployed configs. (cherry picked from commit 571b008)

exekias added bug in progress Pull request is currently in progress. libbeat needs tests needs_backport PR is waiting to be backported to other branches. containers Related to containers use case labels Oct 31, 2018

exekias mentioned this pull request Oct 31, 2018

Kubernetes autodiscover doesn't stop module for containers without IP #8535

Closed

ruflin requested a review from jsoriano November 1, 2018 08:17

ruflin reviewed Nov 1, 2018

View reviewed changes

exekias mentioned this pull request Nov 1, 2018

Autodiscover should not require identical metadata for start/stop events #8863

Closed

jsoriano reviewed Nov 1, 2018

View reviewed changes

andrewvc reviewed Nov 1, 2018

View reviewed changes

andrewvc mentioned this pull request Nov 1, 2018

[WIP] Beta AWS ELB Autodiscovery Provider #8680

Closed

exekias mentioned this pull request Nov 6, 2018

Start autodiscover consumers before producers #7926

Merged

Carlos Pérez-Aradros Herce added 2 commits November 13, 2018 13:37

Update jolokia provider

2a5a1b1

Apply review comments

7ef8bc6

exekias force-pushed the autodiscover-instance-id branch from 4de0214 to 7ef8bc6 Compare November 13, 2018 12:41

Carlos Pérez-Aradros Herce added 2 commits November 23, 2018 19:11

Add provider UUID to split configs by provider

144a6df

Merge branch 'master' into autodiscover-instance-id

69a40c1

exekias removed in progress Pull request is currently in progress. needs tests labels Nov 23, 2018

Fix tests

38c928d

exekias added the v6.6.0 label Nov 27, 2018

exekias force-pushed the autodiscover-instance-id branch from e7e6a0b to 939f271 Compare November 27, 2018 12:08

Update changelog

c44d1f4

exekias force-pushed the autodiscover-instance-id branch from 939f271 to c44d1f4 Compare November 27, 2018 12:22

ruflin added the Team:Integrations Label for the Integrations team label Nov 27, 2018

andrewvc reviewed Nov 27, 2018

View reviewed changes

andrewvc requested changes Nov 27, 2018

View reviewed changes

Update CHANGELOG.asciidoc

04ce1ee

jsoriano approved these changes Nov 28, 2018

View reviewed changes

andrewvc approved these changes Nov 28, 2018

View reviewed changes

Carlos Pérez-Aradros Herce added 2 commits November 28, 2018 22:03

Merge branch 'master' into autodiscover-instance-id

3bd7e30

Fix typo

8603139

exekias merged commit 571b008 into elastic:master Nov 29, 2018

exekias mentioned this pull request Nov 29, 2018

Cherry-pick #8851 to 6.x: Keep a mapping of autodiscover instances and deployed configs #9290

Merged

exekias removed the needs_backport PR is waiting to be backported to other branches. label Nov 29, 2018

exekias mentioned this pull request Jan 8, 2019

filebeat error and excessive memory usage with autodiscover #9078

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep a mapping of autodiscover instances and deployed configs #8851

Keep a mapping of autodiscover instances and deployed configs #8851

exekias commented Oct 31, 2018

ruflin left a comment

ruflin Nov 1, 2018

jsoriano Nov 1, 2018

andrewvc left a comment

andrewvc Nov 1, 2018

andrewvc Nov 6, 2018

exekias Nov 13, 2018

andrewvc Nov 13, 2018

exekias Nov 23, 2018

andrewvc Nov 26, 2018

andrewvc Nov 27, 2018

andrewvc left a comment

exekias commented Nov 28, 2018

andrewvc commented Nov 28, 2018

exekias commented Nov 28, 2018

exekias commented Dec 7, 2018

		@@ -138,8 +138,10 @@ func (a *Autodiscover) worker() {
		}

		configs := make([]*reload.ConfigWithMeta, 0, len(a.configs))

		@@ -33,6 +33,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha1...master[Check the HEAD d

		Affecting all Beats

		- Keep a mapping of autodiscover instances and deployed configs. {pull}8851[8851]

Keep a mapping of autodiscover instances and deployed configs #8851

Keep a mapping of autodiscover instances and deployed configs #8851

Conversation

exekias commented Oct 31, 2018

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc 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

andrewvc left a comment

Choose a reason for hiding this comment

exekias commented Nov 28, 2018

andrewvc commented Nov 28, 2018

exekias commented Nov 28, 2018

exekias commented Dec 7, 2018