Add / report persistent policy error metadata in Coordinator #3076

faec · 2023-07-13T18:46:35Z

Update Coordinator's error tracking and aggregate state reporting:

Add standalone CoordinatorState / CoordinatorMessage fields to coordinator.State ~~and proto.StateResponse~~, so Coordinator has its own state instead of overloading the global Agent state.
Add new error fields to Coordinator tracking the three types of errors a policy update can encounter (initial parsing, component model generation, and runtime update).
Update (*Coordinator).generateReportableState to aggregate the new error/state fields into the global State/Message reported by Fleet.

Fixes #2852.

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~~
~~I have added an integration test or an E2E test~~

elasticmachine · 2023-07-13T19:07:39Z

💚 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-07-24T14:58:08.008+0000
Duration: 23 min 21 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6099
Skipped	27
Total	6126

💚 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!)

blakerouse · 2023-07-17T14:44:48Z

control_v2.proto

+  // State + message for the Agent Coordinator, reporting manager-level errors
+  // like failed policy updates.
+  State coordinatorState = 7;
+  string coordinatorMessage = 8;


I do not like the idea of exposing an internal detail like the coordinator to the status output.

Well, I expect it wouldn't be user-facing unless we change the UI, and it might be nice to have a way to check it when debugging... but I wouldn't mind being conservative about what we add to the proto in this pass. The most important part of Coordinator state was reporting manager errors and similar, which this PR replaces with explicit aggregation.

What if we break the implicit sync between coordinator state and the proto? Leave the proto unchanged, but keep the coordinator state/message in the internal struct? That way it can still be persisted / aggregated into the reported value, but without any explicit new fields over the wire?

elasticmachine · 2023-07-17T20:46:37Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

elasticmachine · 2023-07-17T20:50:34Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.718% (`77/78`)	👍
Files	68.657% (`184/268`)	👍
Classes	67.606% (`336/497`)	👍
Methods	54.222% (`1053/1942`)	👍 0.094
Lines	40.342% (`12048/29865`)	👍 0.137
Conditionals	100.0% (`0/0`)	💚

faec · 2023-07-17T22:47:17Z

buildkite test this

michalpristas

small comments here and there
tested this scenario locally:

install agent with system integration logs/metrics enabled
unplug cable
check fleet status is FAILED
reconnect
check status is back to green
removed log collection from system integration
rename filebeat executable
add logs back and check status is failed
rename filebeat back to filebeat
check it's green

michalpristas · 2023-07-20T07:31:31Z

internal/pkg/agent/application/coordinator/coordinator.go

@@ -214,11 +214,26 @@ type Coordinator struct {
 	// into the reported state before broadcasting -- State() will report
 	// agentclient.Failed if one of these is set, even if the underlying
 	// coordinator state is agentclient.Healthy.
-	runtimeMgrErr error
+	runtimeMgrErr error // Currently unused


why do we keep it then?

Fair question -- I asked @blakerouse a while ago and he said it was so the runtime manager satisfies the Runner interface. A more immediate answer is that it will probably be used soon: I have a PR in progress to centralize more of the runtime manager logic in its own run loop. For that, it's useful to decouple runtime manager Update calls from the error result they produce, to avoid accessing runtime internals from the Coordinator goroutine. In that PR I've started using the previously-idle error channel to report component model update failures, and those end up being stored in this field.

michalpristas · 2023-07-20T07:37:11Z

internal/pkg/agent/application/coordinator/coordinator.go

+		return nil
+	}
+
+	span, ctx := apm.StartSpan(ctx, "refreshComponentModel", "app.internal")


can this be considered breaking? renaming span name

being that APM tracing inside of the Elastic Agent itself is for development or debugging only I would think not

michalpristas · 2023-07-20T07:38:42Z

internal/pkg/agent/application/coordinator/coordinator_state.go

@@ -124,53 +158,66 @@ func (c *Coordinator) applyComponentState(state runtime.ComponentComponentState)
 // components and units are also healthy (or in ephemeral non-error states).
 // Must be called on the main Coordinator goroutine.
 func (c *Coordinator) generateReportableState() (s State) {
-	s.State = c.state.State
-	s.Message = c.state.Message
+	//s.State = c.state.State


please remove or explain why this is commented out in additional comment
but considering logic below i think it can be removed

Ah yes, leftover from the earlier draft, thank you!

mergify · 2023-07-20T13:26:22Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b coord-reporting upstream/coord-reporting
git merge upstream/main
git push upstream coord-reporting

blakerouse

Looks really good. Testing looks to cover the additions.

blakerouse · 2023-07-20T14:03:23Z

internal/pkg/agent/application/coordinator/coordinator.go

+		return nil
+	}
+
+	span, ctx := apm.StartSpan(ctx, "refreshComponentModel", "app.internal")


being that APM tracing inside of the Elastic Agent itself is for development or debugging only I would think not

faec · 2023-07-20T19:12:54Z

jenkins test this

faec · 2023-07-24T01:03:12Z

jenkins test this

…porting

faec added 3 commits July 13, 2023 11:00

adding new state fields

bf3e634

plug in most error reporting

c98ac6e

Update error state aggregation

6252de3

faec added bug Something isn't working Team:Elastic-Agent Label for the Agent team backport-skip skip-changelog labels Jul 13, 2023

faec self-assigned this Jul 13, 2023

blakerouse reviewed Jul 17, 2023

View reviewed changes

update / add tests

eb9a295

faec marked this pull request as ready for review July 17, 2023 20:46

faec requested a review from a team as a code owner July 17, 2023 20:46

faec requested review from michalpristas and blakerouse July 17, 2023 20:46

revert new proto fields

2cb093d

michalpristas approved these changes Jul 20, 2023

View reviewed changes

blakerouse approved these changes Jul 20, 2023

View reviewed changes

faec added 2 commits July 20, 2023 10:22

remove commented-out code

06900f2

Merge remote-tracking branch 'upstream/main' into coord-reporting

0b1303d

Merge branch 'main' of github.com:elastic/elastic-agent into coord-re…

ed1c3a0

…porting

faec merged commit 4269472 into elastic:main Jul 24, 2023
9 checks passed

faec deleted the coord-reporting branch July 24, 2023 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add / report persistent policy error metadata in Coordinator #3076

Add / report persistent policy error metadata in Coordinator #3076

faec commented Jul 13, 2023 •

edited

Loading

elasticmachine commented Jul 13, 2023 •

edited

Loading

Build stats

Test stats 🧪

blakerouse Jul 17, 2023

faec Jul 17, 2023

elasticmachine commented Jul 17, 2023

elasticmachine commented Jul 17, 2023 •

edited

Loading

faec commented Jul 17, 2023

michalpristas left a comment •

edited

Loading

michalpristas Jul 20, 2023

faec Jul 20, 2023

michalpristas Jul 20, 2023

blakerouse Jul 20, 2023

michalpristas Jul 20, 2023

faec Jul 20, 2023

mergify bot commented Jul 20, 2023

blakerouse left a comment

blakerouse Jul 20, 2023

faec commented Jul 20, 2023

faec commented Jul 24, 2023

Add / report persistent policy error metadata in Coordinator #3076

Add / report persistent policy error metadata in Coordinator #3076

Conversation

faec commented Jul 13, 2023 • edited Loading

Checklist

elasticmachine commented Jul 13, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 17, 2023

elasticmachine commented Jul 17, 2023 • edited Loading

🌐 Coverage report

faec commented Jul 17, 2023

michalpristas left a comment • edited Loading

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

mergify bot commented Jul 20, 2023

blakerouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faec commented Jul 20, 2023

faec commented Jul 24, 2023

faec commented Jul 13, 2023 •

edited

Loading

elasticmachine commented Jul 13, 2023 •

edited

Loading

elasticmachine commented Jul 17, 2023 •

edited

Loading

michalpristas left a comment •

edited

Loading