Clean up config checks / fix heartbeat NPE #11165

andrewvc · 2019-03-08T17:34:58Z

This fixes a bug where heartbeat would encounter an NPE while attempting to free resources during a config check and cleans up some internal interfaces.

This change lets beats choose whether their factories implement a special config check function. If not, it is assumed their creation function will return an error and that this can be used for checking the config. It is further assumed that creation with the factory is fully GC-able, which is not the case for heartbeat, hence the need for a custom CheckConfig method.

elasticmachine · 2019-03-08T17:35:00Z

Pinging @elastic/uptime

andrewvc · 2019-03-08T17:35:26Z

heartbeat/monitors/monitor_test.go

 	m1, m1Err := makeTestMon()
-	assert.NoError(t, m1Err)
+	require.NoError(t, m1Err)


These are now require because subsequent assertions make no sense if prior ones fail.

andrewvc · 2019-03-09T00:42:26Z

This also fixes a libbeat bug where the dynamic config reloading code would start the runners specified in the factory as part of the check, but never stop them. This uses the proper Check method which should allow cleanup.

I tried adding a test for this, but the code is quite hard to mock and stub. It probably needs a refactor beyond the scope of this PR to become more testable.

ruflin

I like the implementation with the Unsafe and Check config.

This will also need a changelog entry.

heartbeat/monitors/factory.go

heartbeat/monitors/monitor.go

libbeat/cfgfile/reload.go

andrewvc · 2019-03-18T21:14:17Z

@ruflin should be ready for another pass, I think I've addressed all your concerns.

ruflin

CI failure looks related.

ruflin · 2019-03-19T10:02:12Z

heartbeat/monitors/factory.go

+		}
+	}()
+
+	if err != nil {


Use just return err, no additional check needed.

libbeat/cfgfile/reload.go

andrewvc · 2019-03-28T17:09:11Z

@ruflin implemented a totally new approach in 7501885 . This wipes out much of the old work, except what was in the heartbeat folder. We now make the implementation of CheckConfig optional. Only heartbeat implements it at the moment.

ruflin · 2019-04-08T10:06:01Z

I like this change as it is it removes the non implemented methods and allows Heartbeat to move forward with the interface.

ruflin

Overall change LGTM. You will need to rebase, and can you also add a changelog entry?

andrewvc · 2019-05-01T16:02:50Z

@ruflin fixed merge conflicts, updated the issue description / title (so that it will make a better commit message) and added a changelog entry. Can I get a final LGTM?

ruflin · 2019-05-03T06:54:29Z

jenkins, test this

ruflin

LGTM. I like the solution that was found here and I hope we didn't miss any side effects.

There are 3 files in vendor directory which changed in this PR which I think shouldn't ...

ruflin · 2019-05-03T06:56:30Z

vendor/github.com/docker/go-connections/tlsconfig/certpool_other.go

@@ -4,7 +4,6 @@ package tlsconfig

 import (
 	"crypto/x509"


How did these vendor changes slip in here? Also see below.

Fixed via a new squashed commit. Something weird was going on with git, but squashing fixed it. I couldn't remove them in subsequent commits weirdly.

ruflin · 2019-05-06T12:33:56Z

@andrewvc The travis build was failing for Heartbeat, but I don't think related at all. I restarted it .

andrewvc · 2019-05-06T16:36:03Z

Merging. Failures are unrelated. Metricbeat tests keep failing for different reasons. Out of three tries, two for package management servers being down.

This fixes a bug where heartbeat would encounter an NPE while attempting to free resources during a config check and cleans up some internal interfaces. This change lets beats choose whether their factories implement a special config check function. If not, it is assumed their creation function will return an error and that this can be used for checking the config. It is further assumed that creation with the factory is fully GC-able, which is not the case for heartbeat, hence the need for a custom CheckConfig method. (cherry picked from commit dcce078)

andrewvc added bug needs_backport PR is waiting to be backported to other branches. Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Mar 8, 2019

andrewvc self-assigned this Mar 8, 2019

andrewvc requested a review from ruflin March 8, 2019 17:34

andrewvc requested a review from a team as a code owner March 8, 2019 17:34

andrewvc commented Mar 8, 2019

View reviewed changes

andrewvc force-pushed the fix-bad-check-config branch from 7d0bc02 to c62d376 Compare March 8, 2019 17:36

andrewvc requested a review from a team as a code owner March 9, 2019 00:41

ruflin requested changes Mar 12, 2019

View reviewed changes

heartbeat/monitors/factory.go Outdated Show resolved Hide resolved

heartbeat/monitors/monitor.go Outdated Show resolved Hide resolved

libbeat/cfgfile/reload.go Outdated Show resolved Hide resolved

libbeat/cfgfile/reload.go Outdated Show resolved Hide resolved

ruflin requested changes Mar 19, 2019

View reviewed changes

andrewvc mentioned this pull request Mar 19, 2019

Resources leaks when checking configs #11313

Closed

ruflin reviewed Mar 20, 2019

View reviewed changes

libbeat/cfgfile/reload.go Outdated Show resolved Hide resolved

andrewvc mentioned this pull request Mar 28, 2019

Fix memory leaks on RunnerFactory and Heartbeat NPEs #11514

Closed

andrewvc added Filebeat Filebeat libbeat Metricbeat Metricbeat labels Mar 28, 2019

andrewvc force-pushed the fix-bad-check-config branch from ab19dd9 to b277a43 Compare March 28, 2019 17:07

houndci-bot reviewed Mar 28, 2019

View reviewed changes

libbeat/cfgfile/reload.go Show resolved Hide resolved

andrewvc force-pushed the fix-bad-check-config branch from b277a43 to 409af72 Compare March 28, 2019 17:08

andrewvc mentioned this pull request Mar 28, 2019

Fix RunnerFactory/Runner Interface #11383

Closed

ruflin approved these changes Apr 8, 2019

View reviewed changes

andrewvc changed the title ~~[Heartbeat] Fix NPE on config check~~ Clean up config checks / fix heartbeat NPE May 1, 2019

ruflin approved these changes May 3, 2019

View reviewed changes

Fix bad check conf

a93aa27

andrewvc force-pushed the fix-bad-check-config branch from 2da47d0 to a93aa27 Compare May 3, 2019 19:34

ruflin approved these changes May 6, 2019

View reviewed changes

andrewvc merged commit dcce078 into elastic:master May 6, 2019

andrewvc deleted the fix-bad-check-config branch May 6, 2019 16:36

andrewvc mentioned this pull request May 6, 2019

Cherry-pick #11165 to 7.0: Clean up config checks / fix heartbeat NPE #12071

Closed

andrewvc added v7.0.2 and removed needs_backport PR is waiting to be backported to other branches. labels May 6, 2019

andrewvc mentioned this pull request May 9, 2019

Cherry-pick #11165 to 7.1: Clean up config checks / fix heartbeat NPE #12117

Closed

andrewvc added the v7.1.0 label May 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up config checks / fix heartbeat NPE #11165

Clean up config checks / fix heartbeat NPE #11165

andrewvc commented Mar 8, 2019 •

edited

Loading

elasticmachine commented Mar 8, 2019

andrewvc Mar 8, 2019

andrewvc commented Mar 9, 2019

ruflin left a comment

andrewvc commented Mar 18, 2019

ruflin left a comment

ruflin Mar 19, 2019

andrewvc commented Mar 28, 2019

ruflin commented Apr 8, 2019

ruflin left a comment

andrewvc commented May 1, 2019

ruflin commented May 3, 2019

ruflin left a comment

ruflin May 3, 2019

andrewvc May 3, 2019

ruflin commented May 6, 2019

andrewvc commented May 6, 2019

Clean up config checks / fix heartbeat NPE #11165

Clean up config checks / fix heartbeat NPE #11165

Conversation

andrewvc commented Mar 8, 2019 • edited Loading

elasticmachine commented Mar 8, 2019

andrewvc Mar 8, 2019

Choose a reason for hiding this comment

andrewvc commented Mar 9, 2019

ruflin left a comment

Choose a reason for hiding this comment

andrewvc commented Mar 18, 2019

ruflin left a comment

Choose a reason for hiding this comment

ruflin Mar 19, 2019

Choose a reason for hiding this comment

andrewvc commented Mar 28, 2019

ruflin commented Apr 8, 2019

ruflin left a comment

Choose a reason for hiding this comment

andrewvc commented May 1, 2019

ruflin commented May 3, 2019

ruflin left a comment

Choose a reason for hiding this comment

ruflin May 3, 2019

Choose a reason for hiding this comment

andrewvc May 3, 2019

Choose a reason for hiding this comment

ruflin commented May 6, 2019

andrewvc commented May 6, 2019

andrewvc commented Mar 8, 2019 •

edited

Loading