Fix issue where its possible for a component to receive a unit without a config #2138

blakerouse · 2023-01-18T19:48:19Z

What does this PR do?

Fixes an issue where it is possible for a component to receive a unit without a configuration. This was occurring because the usage of the checkinExpected channel was incorrect.

This fixes the issue by ensures that channel is empty on a new Checkin RPC call and prevent the sending goroutine from reading messages from the channel until the initial checkin has been processed.

This also adds a very defensive path where it only takes the latest CheckinExpected message and sends it, but only after parsing all previous messages to ensure that the latest message does not have a missing configuration that was sent in a previous CheckinExpected.

This also adds more testing.

Why is it important?

It was possible for a component to restart with the a missing configuration for a unit, which prevents the component from working correctly and it reports itself as failed.

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/fragments using the changelog tool

Related issues

Closes Beats managed by agent can be left without a configured output #2086

…ion.

elasticmachine · 2023-01-18T19:51:58Z

💚 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: 2023-01-19T20:50:06.029+0000
Duration: 17 min 2 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4833
Skipped	13
Total	4846

💚 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.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-01-18T20:37:32Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.305% (`58/59`)	👍
Files	69.268% (`142/205`)	👍
Classes	69.231% (`270/390`)	👍
Methods	54.047% (`828/1532`)	👍
Lines	39.288% (`9026/22974`)	👍 0.024
Conditionals	100.0% (`0/0`)	💚

cmacknz

On first pass this looks good. Will continue tomorrow, I haven't run the tests or installed the agent yet to double check this works.

pkg/component/runtime/runtime_comm.go

pkg/component/fake/shipper/listener.go

pkg/component/runtime/manager_test.go

cmacknz · 2023-01-18T21:01:35Z

pkg/component/runtime/manager_test.go

+	require.NoError(t, err)
+}
+
+func TestManager_FakeInput_KeepsRestarting(t *testing.T) {


Did this or the other test ever reproduce the problem? What about if you reverted my ClearPendingCheckin change that didn't completely fix the problem? Does it catch it then or not at all?

Do the tests catch the issue with no fixes applied at all? (is what I was trying to ask above)

I just tried it with your change removed and sadly these did not show the symptoms either. I wanted to keep the tests in the change because overall I thought they still exercised the code path well.

Agreed I like them too, maybe if we ran them for long enough they'd catch it.

pkg/component/runtime/runtime_comm.go

cmacknz

Ran the tests and confirmed the agent can install and enroll with Fleet.

It is difficult to tell if the issue is completely gone (I thought it was the first time) but these changes look like they should prevent it.

blakerouse · 2023-01-19T19:58:01Z

Using the latest changes in this branch I added the following change and ran the TestManager_FakeInput_Restarts test. It causes the exact same issue that we are seeing in production where the Config is nil.

index b30a3721a..7538b4065 100644
--- a/pkg/component/runtime/runtime_comm.go
+++ b/pkg/component/runtime/runtime_comm.go
@@ -268,6 +268,18 @@ func (c *runtimeComm) checkin(server proto.ElasticAgent_CheckinV2Server, init *p
        // the channel as well as prevent the sender channel from reading from `c.checkinExpected` until
        // we have sent the message on `c.checkinObserved`.
        c.latestCheckinExpected(nil)
+
+       c.checkinExpected <- &proto.CheckinExpected{
+               Units: []*proto.UnitExpected{
+                       {
+                               Type:           proto.UnitType_INPUT,
+                               State:          proto.State_HEALTHY,
+                               ConfigStateIdx: 1,
+                               Config:         nil,
+                       },
+               },
+       }
+
        c.checkinObserved <- init
        close(waitExp)

So my change still does not 100% fix the issue, because it is possible that the goroutine reads from the channel and what is on the channel is invalid for a restart component.

I am working more on this PR to have a full proof solution. I want to ensure there is no window of possibility that this happens. I am also looking at adding some defensive code on the elastic-agent-client side to ensure that even if it does happen it handles the case correctly.

cmacknz · 2023-01-19T20:04:48Z

I completely removed all of the fixes including the first attempt at a fix that this PR replaces and ran the two unit tests here continuously for ~1 hour without reproducing a failure.

This appears particularly challenging to reproduce in an isolated way, I'm glad we managed to recreate it artificially at least.

…served message.

pkg/component/runtime/runtime_comm.go

cmacknz · 2023-01-20T02:11:08Z

pkg/component/runtime/runtime_comm.go

+	if initObserved != nil {
+		// the next call to `CheckinExpected` must be from the initial `CheckinObserved` message
+		if observed != initObserved {
+			// not the initial observed message; we don't send it


Is there any scenario where the initial observed message could be lost or altered before we get here? Unlikely but if it this were possible we'd be stuck unable to configure anything.

Is there a client side timeout on the Checkin request from the component? If there is that would guard against this by giving it a way to make a second request and reset the initCheckinObserved message.

If the lack of an expected message being sent to the client would cause it to miss subsequent checkins that would also get us out of this theoretical state via the eventual restart of the process.

Yes it will result in an eventual restart of the process by the command runtime. So this situation is self healing, but extremely unlikely.

cmacknz · 2023-01-20T02:15:19Z

The latest round of changes looks good to me, although I would feel better with some targeted tests to ensure this definitely fixes the bug, or that we can't accidentally deadlock the checkin process waiting for an initial message that doesn't match what we expect.

cmacknz · 2023-01-23T14:51:14Z

Installed locally and changed the output configuration a few times without issue.

…t a config (#2138) * Fix issue where checkinExpected channel might have out dated information. * Run mage fmt. * Add changelog entry. * Increase rate lime for failure in test for slow CI runners. * Cleanups from code review. * Refactor the design of ensuring an initial expected comes from the observed message. (cherry picked from commit fefe64f)

…t a config (#2138) (#2151) * Fix issue where checkinExpected channel might have out dated information. * Run mage fmt. * Add changelog entry. * Increase rate lime for failure in test for slow CI runners. * Cleanups from code review. * Refactor the design of ensuring an initial expected comes from the observed message. (cherry picked from commit fefe64f) Co-authored-by: Blake Rouse <blake.rouse@elastic.co>

blakerouse added 2 commits January 18, 2023 14:42

Fix issue where checkinExpected channel might have out dated informat…

3d7900c

…ion.

Run mage fmt.

160b074

blakerouse added Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent Label for the Agent team backport-v8.6.0 Automated backport with mergify labels Jan 18, 2023

blakerouse self-assigned this Jan 18, 2023

blakerouse requested a review from a team as a code owner January 18, 2023 19:48

blakerouse requested review from michalpristas and michel-laterman and removed request for a team January 18, 2023 19:48

Add changelog entry.

cb917d9

Increase rate lime for failure in test for slow CI runners.

5b62c03

cmacknz requested a review from AndersonQ January 18, 2023 20:33

cmacknz reviewed Jan 18, 2023

View reviewed changes

Cleanups from code review.

d6018da

cmacknz reviewed Jan 18, 2023

View reviewed changes

pkg/component/runtime/runtime_comm.go Show resolved Hide resolved

cmacknz approved these changes Jan 19, 2023

View reviewed changes

Refactor the design of ensuring an initial expected comes from the ob…

dab5a58

…served message.

cmacknz reviewed Jan 20, 2023

View reviewed changes

pkg/component/runtime/runtime_comm.go Show resolved Hide resolved

cmacknz reviewed Jan 20, 2023

View reviewed changes

blakerouse merged commit fefe64f into elastic:main Jan 23, 2023

blakerouse deleted the defensive-checkin-expected branch January 23, 2023 15:00

mergify bot mentioned this pull request Jan 23, 2023

[8.6](backport #2138) Fix issue where its possible for a component to receive a unit without a config #2151

Merged

dedemorton mentioned this pull request Jan 25, 2023

Add Fleet/Agent 8.6.1 relnotes elastic/observability-docs#2544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue where its possible for a component to receive a unit without a config #2138

Fix issue where its possible for a component to receive a unit without a config #2138

blakerouse commented Jan 18, 2023 •

edited

Loading

elasticmachine commented Jan 18, 2023 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Jan 18, 2023 •

edited

Loading

cmacknz left a comment

cmacknz Jan 18, 2023

cmacknz Jan 18, 2023

blakerouse Jan 18, 2023

cmacknz Jan 18, 2023

cmacknz left a comment

blakerouse commented Jan 19, 2023 •

edited

Loading

cmacknz commented Jan 19, 2023

cmacknz Jan 20, 2023

cmacknz Jan 20, 2023

blakerouse Jan 23, 2023

cmacknz commented Jan 20, 2023

cmacknz commented Jan 23, 2023

Fix issue where its possible for a component to receive a unit without a config #2138

Fix issue where its possible for a component to receive a unit without a config #2138

Conversation

blakerouse commented Jan 18, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Jan 18, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Jan 18, 2023 • edited Loading

🌐 Coverage report

cmacknz left a comment

Choose a reason for hiding this comment

cmacknz Jan 18, 2023

Choose a reason for hiding this comment

cmacknz Jan 18, 2023

Choose a reason for hiding this comment

blakerouse Jan 18, 2023

Choose a reason for hiding this comment

cmacknz Jan 18, 2023

Choose a reason for hiding this comment

cmacknz left a comment

Choose a reason for hiding this comment

blakerouse commented Jan 19, 2023 • edited Loading

cmacknz commented Jan 19, 2023

cmacknz Jan 20, 2023

Choose a reason for hiding this comment

cmacknz Jan 20, 2023

Choose a reason for hiding this comment

blakerouse Jan 23, 2023

Choose a reason for hiding this comment

cmacknz commented Jan 20, 2023

cmacknz commented Jan 23, 2023

blakerouse commented Jan 18, 2023 •

edited

Loading

elasticmachine commented Jan 18, 2023 •

edited

Loading

elasticmachine commented Jan 18, 2023 •

edited

Loading

blakerouse commented Jan 19, 2023 •

edited

Loading