[tests][integration] fix monitoring test cases

[VihasMakwana]

• Bug

What does this PR do?

• With introduction of status reporter for metricbeat, integration test cases were failing
• From diagnostics, it was observed that the status transitioned from HEALTHY to DEGRADED (momentarily) and again to HEALTHY.
• Previous behaviour:
  • During the above transition, runner.agentFixture.ExecStatus(ctx) returned an error because of the momentarily DEGRADED state and it caused entire test to fail (even though it returned to a HEALTHY state, its just how go tests function).
• This PR does the following:
  • Remove the check on error inside require.Eventually.
    • If all the components return to a healthy state, the test would pass eventually.
    • If it remains in a unhealthy state, the test would fail.

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

closes #5209

[mergify]

This pull request does not have a backport label. Could you fix it @VihasMakwana? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

[elasticmachine]

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

[blakerouse]

Maybe it would be better to change -

elastic-agent/pkg/testing/fixture.go

Line 717 in 2676a3f

func (f *Fixture) ExecStatus(ctx context.Context, opts ...process.CmdOption) (AgentStatusOutput, error) {

instead of the actual test.

Reading the docstring on ExecStatus it says it can return an output even on exit of 1. Maybe we should only return an error when we are unable to read the output, otherwise as long as some output is returned it should not be an error.

That would probably fix more tests and not just these specific tests.

[cmacknz]

Agree with Blake, the ExecStatus should return nil if we were able to get the output.

It feels like it should probably return three values: the command output, a boolean for whether agent is healthy, and an error that is only not-nil if we couldn't get or parse the output of the command.

If we were to keep it the way it is, the actual bug is that there is a require.NoError inside of a require.Eventually loop to immediately fail the test. The require.Error in the case below should have been if err != nil { return false } to let the eventually retry.

elastic-agent/testing/integration/agent_long_running_leak_test.go

Line 219 in 03b5d31

require.NoError(runner.T(), err)

[VihasMakwana]

@blakerouse @cmacknz Agree with you guys. Thanks for your inputs.

i've updated the ExecStatus and we return nil if we can parse the output.

[blakerouse]

Looks good, thanks for changing this around.

[VihasMakwana]

@blakerouse thanks for approving. So you know, I made a small change to combine uerr and err(just in case if it's not empty) a second after your approval 😅

elastic-agent/pkg/testing/fixture.go

Lines 719 to 722 in 9e77ba4

	if uerr := json.Unmarshal(out, &status); uerr != nil {
	return AgentStatusOutput{},
	fmt.Errorf("could not unmarshal agent status output: %w", errors.Join(uerr, err))
	} else if status.IsZero() {

[pierrehilbert]

Looks like we now have failing tests that may be related to your change.

[VihasMakwana]

@blakerouse @cmacknz from the logs of failing tests, I can see that output is valid and not empty but status.IsZero() returns false because Info.ID field is not set somehow. How it looks in json:

{
    "info": {
        "id": "",
        "version": "8.16.0",
        "commit": "48bf9adae418cb043d20c18fe13cae1e6c5d05f9",
        "build_time": "2024-07-30 09:58:04 +0000 UTC",
        "snapshot": true,
        "pid": 19922,
        "unprivileged": true
    },
    "state": 2,
    "message": "Running",
    "components": [],
    "FleetState": 6,
    "FleetMessage": "Not enrolled into Fleet"
}

I think we should check also check for Version, Message, and ID.
So, something like:

func IsZero() bool {
     return status.Message == "" && status.Info.Version == "" && status.Info.ID == ""
}

---

Edit:

PTAL! I've updated the condtions.

[elastic-sonarqube]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

[blakerouse]

Looks good. Thanks for the fixes.

[blakerouse]

This makes more sense actually. As IsZero should be if nothing is outputted at all.