[Heartbeat] States reloader and eslegclient by emilioalvap · Pull Request #33405 · elastic/beats

emilioalvap · 2022-10-19T16:36:52Z

What does this PR do?

Requires andrewvc/integrations#1.
Supersedes #33357.
Fixes #33354, #33299 and #33244

This PR:

Introduces a DeferredStateLoader reloader for managed output section of heartbeat instances. It holds monitors from running state queries until the agent has had enough time to initialize it async or a default timeout is triggered.
Replaces the use of elasticsearch.Client with eslegclient.Connection, which supports all output options by default.
Fixes a bug where state would be retrieved even when found.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

To test eslegclient:

Checkout this PR and build HB locally.
Run ES locally with self-signed certificates and use that as HB default output with disabled ssl verification.
Run/ stop HB multiple times. Check in discover that states are reused.

To test output reloader:

Build Heartbeat and Elastic-agent locally.
Enroll a container in a private location policy multiple times and check that states are reused.

Screenshots

…ing in managed mode

mergify · 2022-10-19T16:37:34Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @emilioalvap? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2022-10-19T17:05:56Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-10-20T21:11:47.963+0000
Duration: 48 min 7 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1889
Skipped	25
Total	1914

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2022-10-20T18:29:02Z

Pinging @elastic/uptime (Team:Uptime)

emilioalvap · 2022-10-20T18:30:40Z

/test

andrewvc

Left an initial review based on viewing code alone, will do some manual testing next.

andrewvc · 2022-10-20T18:59:07Z

heartbeat/_meta/fields.common.yml

            - name: down
              type: integer
              description: total down checks run
+        - name: reason


We should nix this unless we actually populate it in this PR

andrewvc · 2022-10-20T18:59:28Z

heartbeat/monitors/wrappers/monitorstate/monitorstate.go

-	Ends            *State `json:"ends"`
+	Ends *State `json:"ends"`
+	// Cause of the transition to this state
+	Reason          string `json:"reason"`


Once again, worth nixing unless we use it

andrewvc · 2022-10-20T18:59:40Z

heartbeat/docs/fields.asciidoc


 --

+*`state.reason`*::


Will be deleted by a mage update

andrewvc · 2022-10-20T19:01:26Z

heartbeat/beater/heartbeat.go

-		if esc != nil {
-			stateLoader = monitorstate.MakeESLoader(esc, "synthetics-*,heartbeat-*", parsedConfig.RunFrom)
+	stateLoader, replaceStateLoader := monitorstate.AtomicStateLoader(monitorstate.NilStateLoader)
+	if b.Config.Output.Name() == "elasticsearch" && !b.Manager.Enabled() {


We should also add in the changes from https://github.com/elastic/beats/pull/33375/files#diff-3edc3a1c86f948ac04dc2aeef09be1c549a2e5cbedd80a76e49dde4d95fa6883R82 which adds a heartbeat.states.enabled config flag. The service won't have the manager enabled, so we'll have to enable that to have states work with the service.

This actually checks if heartbeat is in unmanaged mode (!b.Manager.Enabled()) to initialize a non-deferred states loader, so it should apply for the service as well.

IIRC, states flag was motivated by #33357. IMO, this PR supersedes the original problem since we can now load the config on both modes. Is there an scenario where we would still like to disable states?

andrewvc · 2022-10-20T19:06:52Z

heartbeat/monitors/wrappers/monitorstate/tracker.go

+			logp.L().Warn("Timeout trying to defer state loader")
+		}
+
+		defer cancel()


why defer these both if no statements follow them? Were they intended to go at the top of the function?

Probably a mix of an idiomatic declaration of intent and a sequential mindset when writing it 😄 I can move them to the top

andrewvc · 2022-10-20T19:10:22Z

heartbeat/monitors/wrappers/monitorstate/tracker.go

+	stateLoader, replaceStateLoader := AtomicStateLoader(inner)
+
+	wg := sync.WaitGroup{}
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)


Very idiomatic go concurrency!

andrewvc · 2022-10-20T19:10:38Z

heartbeat/monitors/wrappers/monitorstate/tracker_test.go

 	requireMSStatusCount(t, ms, StatusDown, 1)
 }
+
+func TestAtomicStateLoader(t *testing.T) {


Can we add a test for the new DeferredStateLoader as well?

andrewvc

I've tested this with heartbeat's local monitor reloading, it worked fine. I think the CFT cluster I used to test this was missing elastic/kibana#143162, but I'm LGTM given what I've seen here.

cmacknz · 2022-10-27T14:06:59Z

Drive by comment: this should ensure your ES client keeps working when we merge the agent V2 support for beats in the next few days. I have this PR listed as a "blocker" for our merge of the v2 agent changes here for this reason, to ensure your second ES client keeps working after we merge those changes.

emilioalvap · 2022-10-31T17:15:14Z

@cmacknz My initial impression was that libbeat output reloader is not affected by V2 changes. To make sure, I've gone back and tested it (partially) on #33157 & https://github.com/elastic/beats/tree/feature-arch-v2. The reload callback is still being fired as expected:

{"file.name":"beater/heartbeat.go","file.line":194},"message":"RELOAD OUTPUT: %!w
(config.Namespace={elasticsearch 0xc000628210})","service.name":"heartbeat","ecs.version":"1.6.0"}

IMO, the scope of elastic/elastic-agent#1476 changes with this approach. Do you think it would be possible to have a similar mechanism with the shipper?

cmacknz · 2022-10-31T19:40:16Z

IMO, the scope of elastic/elastic-agent#1476 changes with this approach. Do you think it would be possible to have a similar mechanism with the shipper?

Yes, ideally this should be the only change you need to make. When the shipper is introduced, we just need to keep providing the Elasticsearch output configuration to Heartbeat and this code should keep working.

* Better loading / reloading of state tracker * Refactor states loader to use eslegclient and reload config when running in managed mode * Add deferred state loader for managed instances, fix some logs * Fix integration tests * Review feedback, deferred loader unit test and changelog Co-authored-by: Andrew Cholakian <andrewvc@elastic.co> (cherry picked from commit 5c1d1af)

* Better loading / reloading of state tracker * Refactor states loader to use eslegclient and reload config when running in managed mode * Add deferred state loader for managed instances, fix some logs * Fix integration tests * Review feedback, deferred loader unit test and changelog Co-authored-by: Andrew Cholakian <andrewvc@elastic.co> (cherry picked from commit 5c1d1af) Co-authored-by: Emilio Alvarez Piñeiro <95703246+emilioalvap@users.noreply.github.com>

* Better loading / reloading of state tracker * Refactor states loader to use eslegclient and reload config when running in managed mode * Add deferred state loader for managed instances, fix some logs * Fix integration tests * Review feedback, deferred loader unit test and changelog Co-authored-by: Andrew Cholakian <andrewvc@elastic.co>

andrewvc and others added 4 commits October 17, 2022 15:34

Better loading / reloading of state tracker

6958df7

Checkpoint

4b07bde

Checkpoint

51b32fe

Refactor states loader to use eslegclient and reload config when runn…

c3ac248

…ing in managed mode

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 19, 2022

mergify bot assigned emilioalvap Oct 19, 2022

andrewvc changed the title ~~[Draft][Heartbeat] States reloadder and eslegclient~~ [Draft][Heartbeat] States reloader and eslegclient Oct 19, 2022

emilioalvap added 5 commits October 19, 2022 21:47

Add deferred state loader for managed instances, fix some logs

e063101

Fix naming

f4a9720

Fix linting

cd25e6a

Fix docs

2c5c3c1

Fix docs

08bdd5a

emilioalvap mentioned this pull request Oct 20, 2022

Add read index privilege to synth integration andrewvc/integrations#1

Merged

Fix integration tests

5f6e6ba

emilioalvap added Team:obs-ds-hosted-services Label for the Observability Hosted Services team v8.6.0 bug and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 20, 2022

emilioalvap marked this pull request as ready for review October 20, 2022 18:28

emilioalvap requested a review from a team as a code owner October 20, 2022 18:29

emilioalvap changed the title ~~[Draft][Heartbeat] States reloader and eslegclient~~ [Heartbeat] States reloader and eslegclient Oct 20, 2022

emilioalvap mentioned this pull request Oct 20, 2022

[Meta][Heartbeat] HB support for elastic-agent V2 #33314

Closed

6 tasks

andrewvc reviewed Oct 20, 2022

View reviewed changes

Review feedback, deferred loader unit test and changelog

f242e6b

andrewvc approved these changes Oct 27, 2022

View reviewed changes

emilioalvap added the backport-v8.5.0 Automated backport with mergify label Nov 2, 2022

emilioalvap merged commit 5c1d1af into elastic:main Nov 2, 2022

mergify bot mentioned this pull request Nov 2, 2022

[8.5](backport #33405) [Heartbeat] States reloader and eslegclient #33561

Merged

emilioalvap mentioned this pull request Nov 2, 2022

[Heartbeat] Better loading / reloading of state tracker #33375

Closed

6 tasks

cmacknz mentioned this pull request Nov 4, 2022

[Meta] V2 Feature Architecture (8.5/8.6) elastic/elastic-agent#836

Closed

52 tasks

This was referenced Nov 7, 2022

[Heartbeat] States ES connection should reset on fleet reload (mandatory for 8.6) #33244

Closed

[Heartbeat] States loader should only be enabled in narrow situations #33357

Closed

emilioalvap mentioned this pull request Nov 22, 2022

[Heartbeat] New state created immediately after each monitor edit #33299

Closed

emilioalvap mentioned this pull request Jan 9, 2023

[Synthetics] Errors - browser monitors report new error on every down check elastic/kibana#144372

Closed


		--

		`state.reason`::

Conversation

emilioalvap commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

How to test this PR locally

Screenshots

Uh oh!

mergify bot commented Oct 19, 2022

Uh oh!

elasticmachine commented Oct 19, 2022 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

elasticmachine commented Oct 20, 2022

Uh oh!

emilioalvap commented Oct 20, 2022

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

cmacknz commented Oct 27, 2022

Uh oh!

emilioalvap commented Oct 31, 2022

Uh oh!

cmacknz commented Oct 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emilioalvap commented Oct 19, 2022 •

edited

Loading

elasticmachine commented Oct 19, 2022 •

edited by jenkins-beats-ci bot

Loading