Fix issue where updating a single unit results in other units being turned off #34504

blakerouse · 2023-02-07T15:59:34Z

What does this PR do?

Fixes an issue in the managerV2 where it would not call reload with all inputs, resulting in inputs being turned off when they should stay running.

Why is it important?

Fixes an issue where if one input changes it turns off all the other inputs.

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.

Related issues

Closes [Fleet]: CPU and Memory fields changes to NA for Ubuntu agent on changing logging level to debug. elastic-agent#2232

mergify · 2023-02-07T16:00:09Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @blakerouse? 🙏.
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 · 2023-02-07T16:05:39Z

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

elasticmachine · 2023-02-07T16:10:37Z

💚 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-02-09T13:35:30.417+0000
Duration: 60 min 17 sec

Test stats 🧪

Test	Results
Failed	0
Passed	7771
Skipped	343
Total	8114

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

leehinman

LGTM

cmacknz

Needs a test case (and to fix the existing unit tests it seems). You will have to manually backport to the 8.7.0 branch if this doesn't merge by the end of today.

CHANGELOG.next.asciidoc

cmacknz · 2023-02-07T19:14:22Z

x-pack/libbeat/management/managerV2.go

+		}
+	}
+
+	if !changed {


We should add a regression unit test for this bug before merging.

blakerouse · 2023-02-07T20:38:43Z

@cmacknz The unit test is actually testing that path, that is why the test is failing. The code is actually not working for stopping, just noticed that in local testing as well. Working on a fix and will do some more testing.

Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

blakerouse · 2023-02-08T00:51:30Z

@cmacknz Local testing is looking good, would like to get an extra set of testing on this PR before merging. Could you give it a test as well?

cmacknz · 2023-02-08T01:34:16Z

The unit test is actually testing that path, that is why the test is failing. The code is actually not working for stopping, just noticed that in local testing as well. Working on a fix and will do some more testing.

@cmacknz Local testing is looking good, would like to get an extra set of testing on this PR before merging. Could you give it a test as well?

I don't see any new tests that would have caught the problem we are fixing here. I am concerned this bug might come back because there is nothing stopping that from happening. Can we write a test that ensures that a change that results in no unit config changes (like a log level change) preserves the set of input units that should have been running?

This needs to make the 8.6.2 feature freeze which is EOD Thursday. The 8.7 FF is today but since this is a bug fix we can backport it in after that. We have time to add an additional test I think.

blakerouse · 2023-02-09T13:36:09Z

@cmacknz I modified the current test to catch the bad case. I verified this by removing all the changes I had added in managerV2 and ensuring that the test failed. Then I placed my modified code back into the managerV2 and the test past, so catching this error is now covered by the unit test.

cmacknz · 2023-02-09T16:42:41Z

Thanks! Test LGTM.

…urned off (#34504) * Reload all inputs when only one changes. * Update changelog. * Fix stopping case. * Update CHANGELOG.next.asciidoc Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> * Adjust test to catch bad case. --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> (cherry picked from commit 9f15870)

…urned off (#34504) (#34546) * Reload all inputs when only one changes. * Update changelog. * Fix stopping case. * Update CHANGELOG.next.asciidoc Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> * Adjust test to catch bad case. --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> (cherry picked from commit 9f15870) Co-authored-by: Blake Rouse <blake.rouse@elastic.co>

…urned off (#34504) (#34545) * Reload all inputs when only one changes. * Update changelog. * Fix stopping case. * Update CHANGELOG.next.asciidoc Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> * Adjust test to catch bad case. --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> (cherry picked from commit 9f15870) Co-authored-by: Blake Rouse <blake.rouse@elastic.co>

…urned off (#34504) * Reload all inputs when only one changes. * Update changelog. * Fix stopping case. * Update CHANGELOG.next.asciidoc Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> * Adjust test to catch bad case. --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

Reload all inputs when only one changes.

91bfc3b

blakerouse added the Team:Elastic-Agent Label for the Agent team label Feb 7, 2023

blakerouse self-assigned this Feb 7, 2023

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 Feb 7, 2023

blakerouse added the backport-v8.6.0 Automated backport with mergify label Feb 7, 2023

blakerouse changed the title ~~ManageV2 reload all inputs when one changes~~ Fix issue where updating a single unit results in other units being turned off Feb 7, 2023

Update changelog.

789ca71

blakerouse marked this pull request as ready for review February 7, 2023 16:05

blakerouse requested a review from a team as a code owner February 7, 2023 16:05

blakerouse requested review from fearful-symmetry and faec and removed request for a team February 7, 2023 16:05

leehinman approved these changes Feb 7, 2023

View reviewed changes

cmacknz reviewed Feb 7, 2023

View reviewed changes

blakerouse and others added 2 commits February 7, 2023 15:46

Fix stopping case.

07dda79

Update CHANGELOG.next.asciidoc

af8f2b9

Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

cmacknz added the backport-v8.7.0 Automated backport with mergify label Feb 8, 2023

blakerouse added 2 commits February 9, 2023 08:34

Adjust test to catch bad case.

5bca6a5

Merge branch 'main' into fix-agent-2232

2779f84

blakerouse merged commit 9f15870 into elastic:main Feb 9, 2023

blakerouse deleted the fix-agent-2232 branch February 9, 2023 16:42

mergify bot mentioned this pull request Feb 9, 2023

[8.6](backport #34504) Fix issue where updating a single unit results in other units being turned off #34545

Merged

mergify bot mentioned this pull request Feb 9, 2023

[8.7](backport #34504) Fix issue where updating a single unit results in other units being turned off #34546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue where updating a single unit results in other units being turned off #34504

Fix issue where updating a single unit results in other units being turned off #34504

blakerouse commented Feb 7, 2023 •

edited

Loading

mergify bot commented Feb 7, 2023

elasticmachine commented Feb 7, 2023

elasticmachine commented Feb 7, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

leehinman left a comment

cmacknz left a comment

cmacknz Feb 7, 2023 •

edited

Loading

blakerouse commented Feb 7, 2023

blakerouse commented Feb 8, 2023

cmacknz commented Feb 8, 2023

blakerouse commented Feb 9, 2023

cmacknz commented Feb 9, 2023

Fix issue where updating a single unit results in other units being turned off #34504

Fix issue where updating a single unit results in other units being turned off #34504

Conversation

blakerouse commented Feb 7, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

mergify bot commented Feb 7, 2023

elasticmachine commented Feb 7, 2023

elasticmachine commented Feb 7, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

leehinman left a comment

Choose a reason for hiding this comment

cmacknz left a comment

Choose a reason for hiding this comment

cmacknz Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

blakerouse commented Feb 7, 2023

blakerouse commented Feb 8, 2023

cmacknz commented Feb 8, 2023

blakerouse commented Feb 9, 2023

cmacknz commented Feb 9, 2023

blakerouse commented Feb 7, 2023 •

edited

Loading

elasticmachine commented Feb 7, 2023 •

edited by jenkins-beats-ci bot

Loading

cmacknz Feb 7, 2023 •

edited

Loading