Fix deadlock in error handling in OTelManager

[blakerouse]

What does this PR do?

Fixes an issue where a deadlock would cause the the OTelManager to get stuck.

This also seems to be the cause of the flaky test TestOTelManager_Run. I have ran this over 100 times and hit no more errors with this PR.

Why is it important?

Deadlock causes the running internal OTel collector from getting any more updates or actions.

Checklist

• I have read and understood the pull request guidelines of this project.
• 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

Disruptive User Impact

None

How to test this PR locally

go test -race -v -count=100 github.com/elastic/elastic-agent/internal/pkg/otel/manager

Related issues

• Closes [Flaky Test]: TestOTelManager_Run – otel collector never got healthy #6119

[mergify]

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

• backport-./d./d is the label that automatically backports to the 8./d branch. /d is the digit
• backport-active-all is the label that automatically backports to all active branches.
• backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
• backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

[elasticmachine]

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

[swiatekm]

I think we should do the same thing with status reporting.

[blakerouse]

@swiatekm We could do that and it could result in less chance of deadlock. But we do not do that with the runtime manager for a reason, and I think the same reason applies here. The reason we do not do that is for logging and StateWatch over the control protocol. If we don't ensure that every state change is sent over the channel, then logging or watching of that state could be missed. At the moment the runtime manager doesn't do that for that exact reason.

[swiatekm]

@swiatekm We could do that and it could result in less chance of deadlock. But we do not do that with the runtime manager for a reason, and I think the same reason applies here. The reason we do not do that is for logging and StateWatch over the control protocol. If we don't ensure that every state change is sent over the channel, then logging or watching of that state could be missed. At the moment the runtime manager doesn't do that for that exact reason.

I'm fine merging this PR as is, but I also think we should ensure we don't create the same problem with status updates. If all status updates need to be delivered, maybe it's worth having a reasonably large buffered channel for them? I don't think we're really concerned about the Coordinator just not ever consuming the updates, more about unlucky streaks of select choices in the Coordinator loop.

What I really do not want to do is leave another potential race condition in this code just because it's much less probable (right now).

[swiatekm]

The changes themselves look good to me. I would feel better about them if we added a simple test to verify that it's possible to, say, submit two empty config changes to the Otel manager without blocking. Especially since it's a bit undertested in general.

[blakerouse]

@swiatekm I added to the existing TestOTelManager_ConfigError test to send an invalid configuration multiple times to ensure that a deadlock is not caused.

[elastic-sonarqube]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
66.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube