[cmd/supervisor] Fix config merge and opamp server start order

[douglascamata]

Description

This PR fixes a bug that happens because of broken logic in the start up process when there is no explicit OpAMP server in the Supervisor's agent config. Currently it works like this:

1. The bootstrap happens, which starts the opamp server in the Supervisor using a random port. This leaves the port saved in Supervisor:

opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/supervisor.go

Lines 432 to 435 in a0044fb

	func (s *Supervisor) getBootstrapInfo() (err error) {
	_, span := s.getTracer().Start(context.Background(), "GetBootstrapInfo")
	defer span.End()
	s.opampServerPort, err = s.getSupervisorOpAMPServerPort()

2. The initial merged configuration is written to a file. It uses the opamp server port already saved in the Supervisor.

3. The "real" opamp server is started using a different random port, leading the Collector to be constantly restarted by the Supervisor because it never connects back via OpAMP.

opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/supervisor.go

Lines 713 to 720 in 929656d

	func (s *Supervisor) startOpAMPServer() error {
	s.opampServer = server.New(newLoggerFromZap(s.telemetrySettings.Logger, "opamp-server"))
	var err error
	s.opampServerPort, err = s.getSupervisorOpAMPServerPort()
	if err != nil {
	return err
	}

Testing

Added an e2e to cover this.

[evan-bradley]

Thank you for digging into this. The issue you point out and your fix both seem valid to me, but I'm not understanding why this doesn't happen when I run the Supervisor against the example server, or in any of the other E2E tests. What conditions trigger this error?

[douglascamata]

Thank you for digging into this. The issue you point out and your fix both seem valid to me, but I'm not understanding why this doesn't happen when I run the Supervisor against the example server, or in any of the other E2E tests. What conditions trigger this error?

@evan-bradley I think what triggers this is the presence of a local configuration that is enough for the Supervisor to start the Collector on the first attempt, inside Supervisor.Start. This scenario wasn't covered before by any other e2e test. Even if it was covered, we didn't have any check confirming that the Collector could connect back to the Supervisor.

This bug doesn't happen in the scenario where no local configuration is present because the Supervisor would have to receive some remote configuration first and this path correctly composes and writes the effective configuration to the file before starting the Collector process (see lines 1342 and 1343 below):

opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/supervisor.go

Lines 1330 to 1343 in c2c11e6

	case <-s.hasNewConfig:
	s.lastHealthFromClient = nil
	if !configApplyTimeoutTimer.Stop() {
	select {
	case <-configApplyTimeoutTimer.C: // Try to drain the channel
	default:
	}
	}
	configApplyTimeoutTimer.Reset(s.config.Agent.ConfigApplyTimeout)
	s.telemetrySettings.Logger.Debug("Restarting agent due to new config")
	restartTimer.Stop()
	s.stopAgentApplyConfig()
	status, err := s.startAgent()

[douglascamata]

Hey @evan-bradley, could you give this another look, please? 🙏

[evan-bradley]

Thanks @douglascamata. I was able to reproduce this when running the Supervisor locally by restricting the example server from sending any kind of reply that would cause the Supervisor to restart the Collector. Normally an additional message is sent that masks this bug.