Heartbeat autodiscover #8415

andrewvc · 2018-09-24T17:05:50Z

This is a complete PR for heartbeat automatic reload aside from tests. Once those are added it will be ready for a true review.

This shares the adapter used by metricbeat, and moves that to libbeat to keep things DRY.

exekias

This is looking great!

exekias · 2018-09-24T17:07:17Z

heartbeat/heartbeat.reference.yml

+#          docker.container.image: redis
+#      config:
+#      - type: tcp
+#        hosts: ["localhost:${data.port}"]


Probably "${data.host}:${data.port}" is better?

exekias · 2018-09-24T17:07:56Z

libbeat/autodiscover/factoryadapter.go

@@ -26,20 +26,20 @@ import (
 	"github.com/elastic/beats/libbeat/common/bus"
 )

-// AutodiscoverAdapter for Metricbeat modules
-type AutodiscoverAdapter struct {
+// FactoryAdapter for Metricbeat modules


you will need to update the docs here (Metricbeat references), probably explain a little bit how this takes a factory and give you an autodiscover adapter

exekias · 2018-09-24T17:13:35Z

heartbeat/beater/heartbeat.go

 	}

 	if err := bt.scheduler.Start(); err != nil {
 		return err
 	}
 	defer bt.scheduler.Stop()
+	defer func() {
+
+	}()


spurious defer

Oops, good catch.

andrewvc · 2018-09-24T19:04:20Z

Thanks for the review @exekias ! I'll ping you once I've had the chance to add tests.

ruflin

I'm really excited to see autodiscovery in Heartbeat.

ruflin · 2018-09-25T09:03:22Z

heartbeat/beater/heartbeat.go

@@ -21,6 +21,8 @@ import (
 	"fmt"
 	"time"

+	"github.com/elastic/beats/libbeat/autodiscover"


import mess :-)

ruflin · 2018-09-25T09:04:40Z

libbeat/autodiscover/factoryadapter.go

@@ -26,20 +26,20 @@ import (
 	"github.com/elastic/beats/libbeat/common/bus"
 )

-// AutodiscoverAdapter for Metricbeat modules
-type AutodiscoverAdapter struct {
+// FactoryAdapter is an adapter that works with any cfgfile.RunnerFactory.


Good to see we generalised it.

andrewvc · 2018-09-27T21:31:28Z

Tests added & rebased off master. Will move out of WIP once CI passes

andrewvc · 2018-09-27T22:20:55Z

I believe the test failures are unrelated, but there's one for metricbeat autodiscovery, so I'll run the suite again.

andrewvc · 2018-09-27T22:21:04Z

jenkins, retest this please

ruflin · 2018-09-28T18:27:29Z

On travis the metricbeat autodiscovery fails which I assume is related.

andrewvc · 2018-09-28T18:32:01Z

@ruflin Yep. I've been spending today tracking it down, the metricbeat changes should have been a noop, but clearly that is not the case.

Narrowing down the cause, at the moment it's looking like somehow the hint builder is not being registered somehow.

andrewvc · 2018-09-28T18:40:38Z

OK, just pushed d74b291 which I believe should fix it.

andrewvc · 2018-09-28T20:10:19Z

@ruflin this is basically done.

The lack of a docker-compose + Dockerfile was breaking this (and #8475 ). I opened #8491 to fix this as a singular PR, so that should be a blocker for this.

I just rebased this onto that PR. Techincally, #8491 is a blocker however.

ruflin

Left some comments for the testing setup. Please ping me if you have further questions.

ruflin · 2018-10-01T08:16:48Z

heartbeat/docker-compose.yml

+    environment:
+    - REDIS_HOST=redis
+    - REDIS_PORT=6379
+    - KIBANA_HOST=kibana


Remove kibana

Yeah, this are complicated now because they're split across this and #8491. This is already fixed there, let's close that one out and I'll rebase here.

ruflin · 2018-10-01T08:18:34Z

heartbeat/tests/system/test_autodiscovery.py

+        )
+
+        proc = self.start_beat()
+        docker_client.images.pull('redis:3.2.4')


If we set TEST_ENVIRONMENT to true, these containers should already be running.

There are currently two models of testing against containers we have:

Start everything at the beginning: Libbeat and others

Start on request and stop afterwards: Metricbeat modules

We seem to mix the two here. For simplicity I would start with setting TEST_ENVIRONMENT to true and not handle container start / stop in here.

ruflin · 2018-10-01T08:19:09Z

heartbeat/docker-compose.yml

+      kibana:        { condition: service_healthy }
+      redis:         { condition: service_healthy }
+
+  elasticsearch:


As the output in the system tests below is not Elasticsearch, do we need elasticsearch?

Yep, we can get rid of that as well. should happen in the #8491 merge.

andrewvc · 2018-10-01T15:01:58Z

@ruflin let's put this on pause till #8491 is merged, since there's overlap on the docker stuff.

andrewvc · 2018-10-02T20:05:38Z

Rebased off current master with #8491 merged in

ruflin

Code LGTM. Do we plan to backport this?

Changelog is missing.

@exekias Could you have a look?
@andrewvc There will be docs needed and I think also the reference config file should potentially be updated. Will you cover this in a follow up PR?

In general I'm surprised how little change was needed to enable autodiscovery 🎉

ruflin · 2018-10-03T11:33:08Z

heartbeat/tests/system/test_autodiscovery.py

+        import docker
+        docker_client = docker.from_env()
+
+        self.render_config_template(


For later: As we have ES running anyways, we could add some additional tests for http.

Discussed this on slack with @ruflin , agreed that adding a second test was not appropriate due to the fact that we're testing autodiscovery, not reloading.

andrewvc · 2018-10-03T21:18:03Z

Hmm, I borked the tests with some finagling of the AD tests. Not sure exactly how, but I think it's the looser condition I'm trying

andrewvc · 2018-10-03T21:19:37Z

jenkins, retest this please

ruflin · 2018-10-04T12:26:56Z

heartbeat/tests/system/config/heartbeat.yml.j2

@@ -1,3 +1,5 @@
+logging.level: debug


This should not have any impact as -e is used to capture the logs from stdout.

@andrewvc I would suggest to remove the line but don't want to break your tests (it shouldn't ;-) ). I worry that this might cause confusion in the future.

ruflin · 2018-10-04T12:29:26Z

heartbeat/tests/system/test_autodiscovery.py

+        self.wait_until(lambda: self.log_contains(re.compile('autodiscover.+Got a start event:', re.I)))
+
+        self.wait_until(lambda: self.output_count(lambda x: x >= 1))
+        container.stop()


This seems the line it fails. As redis is started outside, no need to stop the container here. I assume varialbe is not initialised?

andrewvc · 2018-10-04T23:59:50Z

Fixed some bugs, but this exposed a flimsy way of enumerating over docker hosts in the autodiscover tests in python in the tests. This needs to apply the same filter there.

andrewvc · 2018-10-05T02:59:46Z

hmmm, these pass locally but not remotely... will take another look tomorrow

andrewvc · 2018-10-05T17:24:12Z

OK, phew tests finally passed, the issue was that locally the dict with docker network attrs had the IP duplicated to an extra field. network_settings['Networks'].values()[0]['IPAddress'] is the right magic to get it.

Autodiscover support for Heartbeat + tests (cherry picked from commit cff3e40)

andrewvc added enhancement in progress Pull request is currently in progress. Heartbeat labels Sep 24, 2018

andrewvc requested a review from exekias September 24, 2018 17:05

exekias reviewed Sep 24, 2018

View reviewed changes

andrewvc force-pushed the autodiscover branch from 013def8 to ede6ed5 Compare September 24, 2018 18:58

ruflin reviewed Sep 25, 2018

View reviewed changes

andrewvc force-pushed the autodiscover branch 2 times, most recently from 3e0724b to ac43c34 Compare September 27, 2018 21:31

andrewvc mentioned this pull request Sep 28, 2018

(Backport to 6.x) Make heartbeat GA! 🎉 (#8392) #8475

Merged

andrewvc changed the title ~~WIP Heartbeat autodiscover~~ Heartbeat autodiscover Sep 28, 2018

andrewvc added review and removed in progress Pull request is currently in progress. labels Sep 28, 2018

andrewvc force-pushed the autodiscover branch from ae95acb to f0b6912 Compare September 28, 2018 20:09

ruflin reviewed Oct 1, 2018

View reviewed changes

andrewvc force-pushed the autodiscover branch from c741b75 to 5323137 Compare October 2, 2018 20:04

ruflin suggested changes Oct 3, 2018

View reviewed changes

exekias approved these changes Oct 3, 2018

View reviewed changes

andrewvc added 2 commits October 3, 2018 10:32

WIP Heartbeat autodiscover

1f5ce46

Incorporate PR feedback

69b14b4

make update

fb7d269

ruflin reviewed Oct 4, 2018

View reviewed changes

andrewvc added 3 commits October 4, 2018 11:56

Fix failing test

02a652f

Revert test specificity

4453a71

Stop watching for stop events in heartbeat autodiscover

64c7bcb

andrewvc added 2 commits October 4, 2018 19:14

Filter docker instances for redis

5474cf0

Search not match

7cee5a9

andrewvc added 2 commits October 5, 2018 08:03

Add debug for ci tests

805a907

Another test

187dfdf

andrewvc force-pushed the autodiscover branch 4 times, most recently from df9584d to 92407f0 Compare October 5, 2018 17:03

more debug

2b8a711

andrewvc force-pushed the autodiscover branch from 92407f0 to 2b8a711 Compare October 5, 2018 17:23

ruflin approved these changes Oct 5, 2018

View reviewed changes

andrewvc merged commit cff3e40 into elastic:master Oct 5, 2018

andrewvc mentioned this pull request Oct 5, 2018

Cherry-pick #8415 to 6.x: Heartbeat autodiscover #8593

Merged

andrewvc added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Oct 5, 2018

andrewvc added a commit to andrewvc/beats that referenced this pull request Oct 16, 2018

Heartbeat autodiscover (elastic#8415)

7522b1c

Autodiscover support for Heartbeat + tests (cherry picked from commit cff3e40)

andrewvc added a commit that referenced this pull request Oct 16, 2018

Heartbeat autodiscover (#8415) (#8593)

09e26ad

Autodiscover support for Heartbeat + tests (cherry picked from commit cff3e40)

andrewvc mentioned this pull request Oct 23, 2018

Cherry-pick #8557 to 6.x: Autodiscover docs #8712

Merged

Heartbeat autodiscover #8415

Heartbeat autodiscover #8415

Conversation

andrewvc commented Sep 24, 2018 • edited Loading

exekias 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

andrewvc commented Sep 24, 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 commented Sep 27, 2018

andrewvc commented Sep 27, 2018

andrewvc commented Sep 27, 2018

ruflin commented Sep 28, 2018

andrewvc commented Sep 28, 2018

andrewvc commented Sep 28, 2018

andrewvc commented Sep 28, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Oct 1, 2018

andrewvc commented Oct 2, 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 commented Oct 3, 2018

andrewvc commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Oct 4, 2018

andrewvc commented Oct 5, 2018

andrewvc commented Oct 5, 2018

andrewvc commented Sep 24, 2018 •

edited

Loading

andrewvc commented Sep 28, 2018 •

edited

Loading