Add stop functionality for output config changes #34049

fearful-symmetry · 2022-12-14T19:28:34Z

What does this PR do?

Fixes elastic/elastic-agent#1913

This adds a reloader component, set by a output_restart config flag, that will stop the beat when it receives a config change to the output.

This will require some other component changes before it works properly

The agent needs to understand that the beat will restart on output change. Currently this will just result in the beats being marked as FAILED.
For beats where we want this enabled, we need to add -E management.output_restart=true to the beat specfile.

Testing this is easy. Just add the aforementioned line to the specfile, run the beat, then change the config. If there's a change to the output, the beats will shut down, agent will restart with the updated config.

Why is it important?

We still want output changes to force a reload of the beat.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
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.

elasticmachine · 2022-12-14T19:28:37Z

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

cmacknz · 2022-12-14T19:57:04Z

The agent needs to understand that the beat will restart on output change. Currently this will just result in the beats being marked as FAILED.

Does it eventually recover from this? Or does it just stay in the failed state?

cmacknz · 2022-12-14T19:58:42Z

x-pack/libbeat/management/config.go

-	Blacklist ConfigBlacklistSettings `config:"blacklist" yaml:"blacklist"`
+	Enabled       bool                    `config:"enabled" yaml:"enabled"`
+	Blacklist     ConfigBlacklistSettings `config:"blacklist" yaml:"blacklist"`
+	OutputRestart bool                    `config:"output_restart" yaml:"output_restart"`


Suggested change

OutputRestart bool `config:"output_restart" yaml:"output_restart"`

RestartOnOutputChange bool `config:"restart_on_output_change" yaml:"output_restart"`

nit: Let's use the exact same name that was used in the 8.5 spec files so it is clear we just changed the implementation when comparing the two: https://github.com/elastic/elastic-agent/blob/f3d39629cc740ce7cf868a228da9e2dec1a83dec/internal/spec/filebeat.yml#L12

x-pack/libbeat/management/managerV2.go

cmacknz · 2022-12-14T20:10:53Z

x-pack/libbeat/management/managerV2.go

@@ -338,6 +346,16 @@ func (cm *BeatV2Manager) handleOutputReload(unit *client.Unit) {
 	}
 	cm.logger.Debugf("Got Output unit config '%s'", rawConfig.GetId())

+	// if needed, stop the beat, let agent restart


We are sure this only triggers when the actual output configuration changes? And it won't restart for changes in the unit that are not the actual output configuration, like the log level? https://github.com/elastic/elastic-agent-client/blob/15881a8e64ef95f32f688c933319cd926ff500b0/elastic-agent-client.proto#L280

Ah, didn't think of that! Should probably add some kind of check to diff the actual config.

elasticmachine · 2022-12-14T20:34:21Z

💚 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-12-15T22:21:57.641+0000
Duration: 60 min 27 sec

Test stats 🧪

Test	Results
Failed	0
Passed	7362
Skipped	338
Total	7700

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

belimawr

LGTM, but I believe we should implement the change suggested by @cmacknz, I believe it also makes the overall code a bit more readable.

Aside that I added minor suggestions.

belimawr · 2022-12-15T14:07:20Z

x-pack/libbeat/management/managerV2.go

@@ -87,12 +91,16 @@ func NewV2AgentManager(config *conf.C, registry *reload.Registry, _ uuid.UUID) (
 // NewV2AgentManagerWithClient actually creates the manager instance used by the rest of the beats.
 func NewV2AgentManagerWithClient(config *Config, registry *reload.Registry, agentClient client.V2) (lbmanagement.Manager, error) {
 	log := logp.NewLogger(lbmanagement.DebugK)
+	if config.OutputRestart {
+		log.Infof("Output reload is enabled, the beat will restart as needed on change of output config")


nit: No need to use Infof here, just Info as you're not formatting anything.

Suggested change

log.Infof("Output reload is enabled, the beat will restart as needed on change of output config")

log.Info("Output reload is enabled, the beat will restart as needed on change of output config")

belimawr · 2022-12-15T14:09:58Z

x-pack/libbeat/management/managerV2.go

@@ -338,6 +346,16 @@ func (cm *BeatV2Manager) handleOutputReload(unit *client.Unit) {
 	}
 	cm.logger.Debugf("Got Output unit config '%s'", rawConfig.GetId())

+	// if needed, stop the beat, let agent restart
+	if cm.stopOnOutputReload && cm.outputIsConfigured {
+		cm.logger.Infof("beat will now stop for output reload")


nit: same thing here

Suggested change

cm.logger.Infof("beat will now stop for output reload")

cm.logger.Info("beat will now stop for output reload")

sonarqubecloud · 2022-12-15T22:29:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

fearful-symmetry · 2022-12-20T00:47:12Z

for anyone reading: holding off on this until #34066 is merged.

cmacknz · 2022-12-20T19:45:26Z

The agent needs to understand that the beat will restart on output change. Currently this will just result in the beats being marked as FAILED.

Does it eventually recover from this? Or does it just stay in the failed state?

Spoke with Alex about this. The FAILED state lasts a second or two while the agent detects the process is stopped and restarts. If this happens right before the next checkin with fleet server we may have the agent reported unhealthy unnecessarily, both in that the behaviour causing the failed state is intentional and that the status won't clear until the next checkin up to 5 minutes later.

To avoid this we need to debounce the process state locally as described in elastic/elastic-agent#1946

cmacknz · 2022-12-21T01:57:22Z

These changes have been merged into #34066, closing.

add stop functionality for output config changes

17382e9

fearful-symmetry added Team:Elastic-Agent Label for the Agent team backport-v8.6.0 Automated backport with mergify labels Dec 14, 2022

fearful-symmetry self-assigned this Dec 14, 2022

fearful-symmetry requested a review from a team as a code owner December 14, 2022 19:28

fearful-symmetry requested review from belimawr and cmacknz and removed request for a team December 14, 2022 19:28

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Dec 14, 2022

cmacknz requested a review from blakerouse December 14, 2022 19:43

cmacknz reviewed Dec 14, 2022

View reviewed changes

belimawr requested changes Dec 15, 2022

View reviewed changes

cleanup

db53398

blakerouse mentioned this pull request Dec 20, 2022

Fix BeatV2Manager to configure inputs and set log level #34066

Merged

3 tasks

cmacknz closed this Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stop functionality for output config changes #34049

Add stop functionality for output config changes #34049

fearful-symmetry commented Dec 14, 2022 •

edited by jlind23

Loading

elasticmachine commented Dec 14, 2022

cmacknz commented Dec 14, 2022

cmacknz Dec 14, 2022

cmacknz Dec 14, 2022

fearful-symmetry Dec 15, 2022

elasticmachine commented Dec 14, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

belimawr left a comment

belimawr Dec 15, 2022

belimawr Dec 15, 2022

sonarqubecloud bot commented Dec 15, 2022

fearful-symmetry commented Dec 20, 2022

cmacknz commented Dec 20, 2022

cmacknz commented Dec 21, 2022

	OutputRestart bool `config:"output_restart" yaml:"output_restart"`
	RestartOnOutputChange bool `config:"restart_on_output_change" yaml:"output_restart"`

	log.Infof("Output reload is enabled, the beat will restart as needed on change of output config")
	log.Info("Output reload is enabled, the beat will restart as needed on change of output config")

	cm.logger.Infof("beat will now stop for output reload")
	cm.logger.Info("beat will now stop for output reload")

Add stop functionality for output config changes #34049

Add stop functionality for output config changes #34049

Conversation

fearful-symmetry commented Dec 14, 2022 • edited by jlind23 Loading

What does this PR do?

Why is it important?

Checklist

elasticmachine commented Dec 14, 2022

cmacknz commented Dec 14, 2022

cmacknz Dec 14, 2022

Choose a reason for hiding this comment

cmacknz Dec 14, 2022

Choose a reason for hiding this comment

fearful-symmetry Dec 15, 2022

Choose a reason for hiding this comment

elasticmachine commented Dec 14, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

belimawr left a comment

Choose a reason for hiding this comment

belimawr Dec 15, 2022

Choose a reason for hiding this comment

belimawr Dec 15, 2022

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 15, 2022

fearful-symmetry commented Dec 20, 2022

cmacknz commented Dec 20, 2022

cmacknz commented Dec 21, 2022

fearful-symmetry commented Dec 14, 2022 •

edited by jlind23

Loading

elasticmachine commented Dec 14, 2022 •

edited by jenkins-beats-ci bot

Loading